Skip to content

nv2a: Implement PTIMER alarm interrupts #2568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abaire wants to merge 5 commits into
base: master
Choose a base branch
from
+141 −14
Open

nv2a: Implement PTIMER alarm interrupts #2568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
365e0f0
nv2a: Implement PTIMER alarm interrupts
abaire Nov 26, 2025
20449ec
SQUASHME: Handle time register writes.
abaire Nov 27, 2025
bce7f44
SQUASHME: Address review feedback, switch to QEMUTimer
abaire Dec 6, 2025
29ce639
SQUASHME: Allows timers to fire continually while interrupt is enabled.
abaire Dec 14, 2025
b03d8a4
SQUASHME: Dropped comment and updated interrupt name.
abaire Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion hw/xbox/nv2a/nv2a.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ void nv2a_update_irq(NV2AState *d)
d->pmc.pending_interrupts &= ~NV_PMC_INTR_0_PGRAPH;
}

/* PTIMER */
if (d->ptimer.pending_interrupts & d->ptimer.enabled_interrupts) {
d->pmc.pending_interrupts |= NV_PMC_INTR_0_PTIMER;
} else {
d->pmc.pending_interrupts &= ~NV_PMC_INTR_0_PTIMER;
}

if (d->pmc.pending_interrupts && d->pmc.enabled_interrupts) {
trace_nv2a_irq(d->pmc.pending_interrupts);
pci_irq_assert(PCI_DEVICE(d));
Expand Down Expand Up @@ -320,6 +327,7 @@ static void nv2a_reset(NV2AState *d)
d->pmc.pending_interrupts = 0;
d->pfifo.pending_interrupts = 0;
d->ptimer.pending_interrupts = 0;
ptimer_reset(d);
d->pcrtc.pending_interrupts = 0;

for (int i = 0; i < 256; i++) {
Expand Down Expand Up @@ -444,7 +452,7 @@ const VMStateDescription vmstate_nv2a_pgraph_vertex_attributes = {

static const VMStateDescription vmstate_nv2a = {
.name = "nv2a",
.version_id = 3,
.version_id = 4,
.minimum_version_id = 1,
.post_save = nv2a_post_save,
.post_load = nv2a_post_load,
Expand Down Expand Up @@ -564,6 +572,8 @@ static const VMStateDescription vmstate_nv2a = {
VMSTATE_BOOL(pgraph.waiting_for_nop, NV2AState),
VMSTATE_UNUSED(1),
VMSTATE_BOOL(pgraph.waiting_for_context_switch, NV2AState),
VMSTATE_UINT32(ptimer.alarm_time_high, NV2AState),
VMSTATE_UINT64(ptimer.time_offset, NV2AState),
VMSTATE_END_OF_LIST()
},
};
Expand Down Expand Up @@ -615,5 +625,6 @@ void nv2a_init(PCIBus *bus, int devfn, MemoryRegion *ram)
NV2AState *d = NV2A_DEVICE(dev);
nv2a_init_memory(d, ram);
nv2a_init_vga(d);
ptimer_init(d);
qemu_add_vm_change_state_handler(nv2a_vm_state_change, d);
}
6 changes: 6 additions & 0 deletions hw/xbox/nv2a/nv2a_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ typedef struct NV2AState {
uint32_t numerator;
uint32_t denominator;
uint32_t alarm_time;
uint32_t alarm_time_high;
uint64_t time_offset;
QEMUTimer timer;
} ptimer;

struct {
Expand Down Expand Up @@ -220,4 +223,7 @@ void *nv_dma_map(NV2AState *d, hwaddr dma_obj_address, hwaddr *len);
hwaddr nv_clip_gpu_tile_blit(NV2AState *d, hwaddr blit_base_address,
hwaddr len);

void ptimer_init(NV2AState *d);
void ptimer_reset(NV2AState *d);

#endif
1 change: 1 addition & 0 deletions hw/xbox/nv2a/nv2a_regs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
#define NV_PMC_INTR_0 0x00000100
# define NV_PMC_INTR_0_PFIFO (1 << 8)
# define NV_PMC_INTR_0_PGRAPH (1 << 12)
# define NV_PMC_INTR_0_PTIMER (1 << 20)
# define NV_PMC_INTR_0_PCRTC (1 << 24)
# define NV_PMC_INTR_0_PBUS (1 << 28)
# define NV_PMC_INTR_0_SOFTWARE (1 << 31)
Expand Down
135 changes: 122 additions & 13 deletions hw/xbox/nv2a/ptimer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/*
* QEMU Geforce NV2A implementation
* PTIMER - time measurement and time-based alarms
*
* Copyright (c) 2012 espes
* Copyright (c) 2015 Jannik Vogel
* Copyright (c) 2018-2021 Matt Borgerson
* Copyright (c) 2018-2025 Matt Borgerson
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
Expand All @@ -21,14 +22,87 @@

#include "nv2a_int.h"

/* PTIMER - time measurement and time-based alarms */
static uint64_t ptimer_get_clock(NV2AState *d)
#define CLOCK_HIGH_MASK 0x1fffffff
#define CLOCK_LOW_MASK 0x7ffffff
#define ALARM_MASK 0xffffffe0

static void ptimer_alarm_fired(void *opaque);

void ptimer_reset(NV2AState *d)
{
d->ptimer.alarm_time = 0;
d->ptimer.alarm_time_high = 0;
d->ptimer.time_offset = 0;
timer_del(&d->ptimer.timer);
}

void ptimer_init(NV2AState *d)
{
timer_init_ns(&d->ptimer.timer, QEMU_CLOCK_VIRTUAL, ptimer_alarm_fired, d);
ptimer_reset(d);
}

static uint64_t ptimer_get_absolute_clock(NV2AState *d)
{
return muldiv64(muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
d->pramdac.core_clock_freq,
NANOSECONDS_PER_SECOND),
d->ptimer.denominator,
d->ptimer.numerator);
d->ptimer.denominator, d->ptimer.numerator);
}

//! Returns the NV2A time adjusted by the last time register modification and
//! shifted into range.
static uint64_t get_ptimer_clock(NV2AState *d, uint64_t absolute_clock)
{
return ((absolute_clock + d->ptimer.time_offset) << 5) &
0x1fffffffffffffffLL;
}

static uint64_t ptimer_ticks_to_ns(NV2AState *d, uint64_t ticks)
{
uint64_t gpu_ticks =
muldiv64(ticks, d->ptimer.numerator, d->ptimer.denominator);
return muldiv64(gpu_ticks, NANOSECONDS_PER_SECOND,
d->pramdac.core_clock_freq);
}

static void schedule_qemu_timer(NV2AState *d)
{
if (!(d->ptimer.enabled_interrupts & NV_PTIMER_INTR_EN_0_ALARM)) {
return;
}

uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
uint64_t alarm_time = (uint64_t)d->ptimer.alarm_time +
((uint64_t)d->ptimer.alarm_time_high << 32);

uint64_t diff_ns = 0;
if (alarm_time > now) {
diff_ns = ptimer_ticks_to_ns(d, (alarm_time - now) >> 5);
Copy link
Member

@mborgerson mborgerson Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit lost tracking the bit shifting. Is the left shifting in get_ptimer_clock just putting the clock in register format? We shift left in get_ptimer_clock, then shift right here to compute actual clock deltas. I wonder if this means we should really just do the shifting/masking at the register interface (e.g. in ptimer_read)

Copy link
Member Author

@abaire abaire Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's just moving in and out of the format used in the registers.

Aren't the read/write callbacks expected to act on data in HW format?

I wonder if some helper macros would be appropriate rather than messing with the guest-accessible callback behavior (but maybe I'm just not following your suggestion).

}
timer_mod(&d->ptimer.timer,
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + diff_ns);
}

static void ptimer_alarm_fired(void *opaque)
{
NV2AState *d = (NV2AState *)opaque;

if (!(d->ptimer.enabled_interrupts & NV_PTIMER_INTR_EN_0_ALARM)) {
return;
}

uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
uint64_t alarm_time =
d->ptimer.alarm_time + ((uint64_t)d->ptimer.alarm_time_high << 32);

if (alarm_time <= now) {
d->ptimer.pending_interrupts |= NV_PTIMER_INTR_0_ALARM;
d->ptimer.alarm_time_high = now >> 32;
nv2a_update_irq(d);
}

schedule_qemu_timer(d);
}
Comment on lines +99 to 106
Copy link

@gemini-code-assist gemini-code-assist bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The current implementation of ptimer_alarm_fired will likely cause an interrupt storm. After an alarm fires and the interrupt is raised, schedule_qemu_timer(d) is called unconditionally. Since the alarm time has not been changed by the guest, schedule_qemu_timer will see that the alarm is still in the past and schedule the timer to fire again immediately. This will repeat, causing a high CPU load and flooding the guest with interrupts.

The line d->ptimer.alarm_time_high = now >> 32; does not prevent this and might be incorrect as it modifies the alarm time without guest intervention.

A safer approach is to treat the QEMU timer as a one-shot event. Once it fires, the interrupt is raised, and the timer is not rescheduled. The guest is then responsible for clearing the interrupt and programming a new alarm, which will in turn schedule a new QEMU timer via ptimer_write.

I suggest modifying the logic to only reschedule the timer in case of a spurious wakeup (where alarm_time > now).

Suggested change
if (alarm_time <= now) {
d->ptimer.pending_interrupts |= NV_PTIMER_INTR_0_ALARM;
d->ptimer.alarm_time_high = now >> 32;
nv2a_update_irq(d);
}
schedule_qemu_timer(d);
}
if (alarm_time <= now) {
d->ptimer.pending_interrupts |= NV_PTIMER_INTR_0_ALARM;
nv2a_update_irq(d);
} else {
/* Spurious wakeup, reschedule. */
schedule_qemu_timer(d);
}


uint64_t ptimer_read(void *opaque, hwaddr addr, unsigned int size)
Expand All @@ -49,11 +123,16 @@ uint64_t ptimer_read(void *opaque, hwaddr addr, unsigned int size)
case NV_PTIMER_DENOMINATOR:
r = d->ptimer.denominator;
break;
case NV_PTIMER_TIME_0:
r = (ptimer_get_clock(d) & 0x7ffffff) << 5;
break;
case NV_PTIMER_TIME_1:
r = (ptimer_get_clock(d) >> 27) & 0x1fffffff;
case NV_PTIMER_TIME_0: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
r = now & 0xffffffff;
} break;
case NV_PTIMER_TIME_1: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
r = (now >> 32) & CLOCK_HIGH_MASK;
} break;
Comment on lines +126 to +133
Copy link

@gemini-code-assist gemini-code-assist bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Reading a 64-bit timer value from two separate 32-bit register reads can be racy. If the timer ticks over and crosses a 32-bit boundary between the read of the low part (NV_PTIMER_TIME_0) and the high part (NV_PTIMER_TIME_1), the guest will get a corrupted time value. Both case statements call get_ptimer_clock independently, which can return different values.

A common hardware pattern to solve this is to latch the high part of the timer when the low part is read. The subsequent read of the high part returns the latched value.

To implement this, you could add a time_high_latched field to the ptimer struct in nv2a_int.h and initialize it in ptimer_reset. Then, modify the read logic as suggested.

Suggested change
case NV_PTIMER_TIME_0: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
r = now & 0xffffffff;
} break;
case NV_PTIMER_TIME_1: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
r = (now >> 32) & CLOCK_HIGH_MASK;
} break;
case NV_PTIMER_TIME_0: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));
d->ptimer.time_high_latched = (now >> 32) & CLOCK_HIGH_MASK;
r = now & 0xffffffff;
} break;
case NV_PTIMER_TIME_1: {
r = d->ptimer.time_high_latched;
} break;

Copy link
Member

@mborgerson mborgerson Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist: Latching could be the right approach, but we should review access patterns and confirm what hardware actually does. The proposed change might not be correct if NV_PTIMER_TIME_1 is read before NV_PTIMER_TIME_0, for instance. I suspect hardware leaves this to be checked by software anyway and that the current solution is correct.

Copy link

@gemini-code-assist gemini-code-assist bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You raise a very valid point, @mborgerson, regarding the need to confirm actual hardware behavior and access patterns. My suggestion was based on a common hardware pattern to mitigate race conditions when a 64-bit timer is exposed via two separate 32-bit registers.

The core issue is that in the current implementation, both NV_PTIMER_TIME_0 and NV_PTIMER_TIME_1 independently call get_ptimer_clock(d, ptimer_get_absolute_clock(d)). This means that if the guest reads NV_PTIMER_TIME_0, then the timer ticks, and then the guest reads NV_PTIMER_TIME_1, the resulting 64-bit value constructed by the guest will be inconsistent (the high part will be from a later time than the low part). This is a classic race condition that can lead to incorrect time values for the guest OS.

My proposed solution addresses this specific race condition by latching the high 32 bits of the timer when the low 32 bits (NV_PTIMER_TIME_0) are read. This ensures that when the guest subsequently reads the high 32 bits (NV_PTIMER_TIME_1), it receives the value that was current at the time of the NV_PTIMER_TIME_0 read, thus providing a consistent 64-bit snapshot.

Your concern about NV_PTIMER_TIME_1 being read before NV_PTIMER_TIME_0 is important. If the hardware indeed allows NV_PTIMER_TIME_1 to be read independently and still expects consistency, or if it has a different latching mechanism (e.g., a dedicated register write to latch both parts), then my proposed solution might not perfectly mirror that specific hardware behavior. However, in the absence of precise hardware documentation, the read low then read high pattern with implicit latching on the low read is a very common design to ensure atomic 64-bit reads from split registers. The current implementation, without any form of latching, will always be susceptible to the described race condition if the guest attempts to reconstruct a 64-bit timer value from two separate reads.

Even if software can work around this by reading the high part multiple times until it stabilizes (a common but inefficient software workaround), the goal of emulation is often to present a consistent and predictable hardware interface. Providing an atomic 64-bit read through a latching mechanism is generally a more robust emulation of a 64-bit timer.

Copy link
Member Author

@abaire abaire Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting response. My expectation is also that the HW isn't doing magic here and it really is left up to the SW to detect race-rollover if it actually cares to. When I was debugging the issue that precipitated this change I saw several places where the timer was being read in a tight loop, presumably to work around the potential race.

case NV_PTIMER_ALARM_0:
r = d->ptimer.alarm_time;
break;
default:
break;
Expand All @@ -76,17 +155,47 @@ void ptimer_write(void *opaque, hwaddr addr, uint64_t val, unsigned int size)
break;
case NV_PTIMER_INTR_EN_0:
d->ptimer.enabled_interrupts = val;
if (val) {
schedule_qemu_timer(d);
} else {
timer_del(&d->ptimer.timer);
}
nv2a_update_irq(d);
break;
case NV_PTIMER_DENOMINATOR:
d->ptimer.denominator = val;
schedule_qemu_timer(d);
break;
case NV_PTIMER_NUMERATOR:
d->ptimer.numerator = val;
schedule_qemu_timer(d);
break;
case NV_PTIMER_ALARM_0:
d->ptimer.alarm_time = val;
break;
case NV_PTIMER_ALARM_0: {
uint64_t now = get_ptimer_clock(d, ptimer_get_absolute_clock(d));

d->ptimer.alarm_time = val & ALARM_MASK;
d->ptimer.alarm_time_high = (now >> 32);
if (val <= (now & ALARM_MASK)) {
++d->ptimer.alarm_time_high;
}
schedule_qemu_timer(d);
} break;
case NV_PTIMER_TIME_0: {
uint64_t absolute_clock = ptimer_get_absolute_clock(d);
uint64_t now = get_ptimer_clock(d, absolute_clock);
uint64_t target_relative =
(now & 0xffffffff00000000LL) | (((val >> 5) & CLOCK_LOW_MASK) << 5);
d->ptimer.time_offset = target_relative - absolute_clock;
schedule_qemu_timer(d);
} break;
case NV_PTIMER_TIME_1: {
uint64_t absolute_clock = ptimer_get_absolute_clock(d);
uint64_t now = get_ptimer_clock(d, absolute_clock);
uint64_t target_relative =
(now & 0xffffffff) | ((val & CLOCK_HIGH_MASK) << 32);
d->ptimer.time_offset = target_relative - absolute_clock;
schedule_qemu_timer(d);
} break;
default:
break;
}
Expand Down