Re: [PATCH 4/4] x86/vPMU: don't statically reserve the interrupt vector

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/11/2025 10:51 am, Jan Beulich wrote:
> Move the setup to vPMU code, doing the allocation of a vector only when
> one is actually going to be needed. With that the handler function also
> doesn't need to be split across two places anymore.
>
> Add the freed up vector to the dynamically allocatable range.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> In case down the road we also want to have a build mode with vPMU code
> excluded, this may also simplify things a little there.
>
> --- 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 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.

> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -369,7 +373,7 @@ void vpmu_save(struct vcpu *v)
>  
>      vpmu_reset(vpmu, VPMU_CONTEXT_SAVE);
>  
> -    apic_write(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
> +    apic_write(APIC_LVTPC, pmu_apic_vector | APIC_LVT_MASKED);
>  }
>  
>  int vpmu_load(struct vcpu *v, bool from_guest)
> @@ -432,7 +436,7 @@ static int vpmu_arch_initialise(struct v
>          return ret;
>      }
>  
> -    vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
> +    vpmu->hw_lapic_lvtpc = pmu_apic_vector | APIC_LVT_MASKED;

Taking a step back, I'm confused as to why we have pmu_apic_vector at all.

LVTPC needs programming with NMIs in order to provide coherent information.

I think this might go a long way to explaining some of the complaints
we've had in the past about junk showing up.

~Andrew