Re: [RFC PATCH 04/41] perf: core/x86: Add support to register a new vector for PMI handling

[Sean Christopherson]

On Fri, Jan 26, 2024, Xiong Zhang wrote:
> From: Xiong Zhang <xiong.y.zhang@xxxxxxxxx>
> 
> Create a new vector in the host IDT for PMI handling within a passthrough
> vPMU implementation. In addition, add a function to allow the registration
> of the handler and a function to switch the PMI handler.
> 
> This is the preparation work to support KVM passthrough vPMU to handle its
> own PMIs without interference from PMI handler of the host PMU.
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxx>
> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/hardirq.h           |  1 +
>  arch/x86/include/asm/idtentry.h          |  1 +
>  arch/x86/include/asm/irq.h               |  1 +
>  arch/x86/include/asm/irq_vectors.h       |  2 +-
>  arch/x86/kernel/idt.c                    |  1 +
>  arch/x86/kernel/irq.c                    | 29 ++++++++++++++++++++++++
>  tools/arch/x86/include/asm/irq_vectors.h |  1 +
>  7 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
> index 66837b8c67f1..c1e2c1a480bf 100644
> --- a/arch/x86/include/asm/hardirq.h
> +++ b/arch/x86/include/asm/hardirq.h
> @@ -19,6 +19,7 @@ typedef struct {
>  	unsigned int kvm_posted_intr_ipis;
>  	unsigned int kvm_posted_intr_wakeup_ipis;
>  	unsigned int kvm_posted_intr_nested_ipis;
> +	unsigned int kvm_vpmu_pmis;

Somewhat off topic, does anyone actually ever use these particular stats?  If the
desire is to track _all_ IRQs, why not have an array and bump the counts in common
code?

>  #endif
>  	unsigned int x86_platform_ipis;	/* arch dependent */
>  	unsigned int apic_perf_irqs;
> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> index 05fd175cec7d..d1b58366bc21 100644
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -675,6 +675,7 @@ DECLARE_IDTENTRY_SYSVEC(IRQ_WORK_VECTOR,		sysvec_irq_work);
>  DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_VECTOR,		sysvec_kvm_posted_intr_ipi);
>  DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_WAKEUP_VECTOR,	sysvec_kvm_posted_intr_wakeup_ipi);
>  DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_NESTED_VECTOR,	sysvec_kvm_posted_intr_nested_ipi);
> +DECLARE_IDTENTRY_SYSVEC(KVM_VPMU_VECTOR,	        sysvec_kvm_vpmu_handler);

I vote for KVM_VIRTUAL_PMI_VECTOR.  I don't see any reasy to abbreviate "virtual",
and the vector is a for a Performance Monitoring Interupt.

>  #endif
>  
>  #if IS_ENABLED(CONFIG_HYPERV)
> diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h
> index 836c170d3087..ee268f42d04a 100644
> --- a/arch/x86/include/asm/irq.h
> +++ b/arch/x86/include/asm/irq.h
> @@ -31,6 +31,7 @@ extern void fixup_irqs(void);
>  
>  #ifdef CONFIG_HAVE_KVM
>  extern void kvm_set_posted_intr_wakeup_handler(void (*handler)(void));
> +extern void kvm_set_vpmu_handler(void (*handler)(void));

virtual_pmi_handler()

>  #endif
>  
>  extern void (*x86_platform_ipi_callback)(void);
> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
> index 3a19904c2db6..120403572307 100644
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> @@ -77,7 +77,7 @@
>   */
>  #define IRQ_WORK_VECTOR			0xf6
>  
> -/* 0xf5 - unused, was UV_BAU_MESSAGE */
> +#define KVM_VPMU_VECTOR			0xf5

This should be inside

	#ifdef CONFIG_HAVE_KVM

no?

>  #define DEFERRED_ERROR_VECTOR		0xf4
>  
>  /* Vector on which hypervisor callbacks will be delivered */
> diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
> index 8857abc706e4..6944eec251f4 100644
> --- a/arch/x86/kernel/idt.c
> +++ b/arch/x86/kernel/idt.c
> @@ -157,6 +157,7 @@ static const __initconst struct idt_data apic_idts[] = {
>  	INTG(POSTED_INTR_VECTOR,		asm_sysvec_kvm_posted_intr_ipi),
>  	INTG(POSTED_INTR_WAKEUP_VECTOR,		asm_sysvec_kvm_posted_intr_wakeup_ipi),
>  	INTG(POSTED_INTR_NESTED_VECTOR,		asm_sysvec_kvm_posted_intr_nested_ipi),
> +	INTG(KVM_VPMU_VECTOR,		        asm_sysvec_kvm_vpmu_handler),

kvm_virtual_pmi_handler

> @@ -332,6 +351,16 @@ DEFINE_IDTENTRY_SYSVEC_SIMPLE(sysvec_kvm_posted_intr_nested_ipi)
>  	apic_eoi();
>  	inc_irq_stat(kvm_posted_intr_nested_ipis);
>  }
> +
> +/*
> + * Handler for KVM_PT_PMU_VECTOR.

Heh, not sure where the PT part came from...

> + */
> +DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_vpmu_handler)
> +{
> +	apic_eoi();
> +	inc_irq_stat(kvm_vpmu_pmis);
> +	kvm_vpmu_handler();
> +}
>  #endif
>  
>  
> diff --git a/tools/arch/x86/include/asm/irq_vectors.h b/tools/arch/x86/include/asm/irq_vectors.h
> index 3a19904c2db6..3773e60f1af8 100644
> --- a/tools/arch/x86/include/asm/irq_vectors.h
> +++ b/tools/arch/x86/include/asm/irq_vectors.h
> @@ -85,6 +85,7 @@
>  
>  /* Vector for KVM to deliver posted interrupt IPI */
>  #ifdef CONFIG_HAVE_KVM
> +#define KVM_VPMU_VECTOR			0xf5

Heh, and your copy+paste is out of date.

>  #define POSTED_INTR_VECTOR		0xf2
>  #define POSTED_INTR_WAKEUP_VECTOR	0xf1
>  #define POSTED_INTR_NESTED_VECTOR	0xf0
> -- 
> 2.34.1
>