On 20.11.2025 13:31, Jan Beulich wrote:
> On 20.11.2025 13:24, Andrew Cooper wrote:
>> On 19/11/2025 10:51 am, Jan Beulich wrote:
>>> --- a/xen/arch/x86/apic.c
>>> +++ b/xen/arch/x86/apic.c
>>> @@ -1313,16 +1313,6 @@ static void cf_check error_interrupt(voi
>>> entries[3], entries[2], entries[1], entries[0]);
>>> }
>>>
>>> -/*
>>> - * This interrupt handles performance counters interrupt
>>> - */
>>> -
>>> -static void cf_check pmu_interrupt(void)
>>> -{
>>> - ack_APIC_irq();
>>> - vpmu_do_interrupt();
>>> -}
>>> -
>>
>> I know you're only moving this, but it's likely-buggy before and after.
>> ack_APIC_irq() needs to be last, and Xen's habit for acking early is why
>> we have reentrancy problems.
>
> I was wondering, but was vaguely (but apparently wrongly) remembering that
> the PMU interrupt is self-disabling (i.e. requires re-enabling before it
> can fire again). Should have checked vpmu_do_interrupt() a little more
> closely, where from the various plain "return" it's pretty clear that isn't
> the case.
>
>> I think there wants to be a patch ahead of this one swapping the order
>> so the ack is at the end, so that this patch can retain that property
>> when merging the functions.
>>
>> Or, if you're absolutely certain it doesn't need backporting as a
>> bugfix, then merging into this patch is probably ok as long as it's
>> called out clearly in the commit message.
>
> No, I'll make this a separate, prereq patch.
It won't really need backporting, though: Direct-APIC-vector handlers are
called with IRQs off, and hence when to ack is benign as long as IRQs aren't
transiently turned on while handling. Nevertheless it probably makes sense
to switch things around, so I'll add that extra patch anyway.
Jan
