Re: [RFC PATCH v3 12/58] perf: core/x86: Register a new vector for KVM GUEST PMI

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 9/10/2024 6:11 AM, Colton Lewis wrote:
> Hello,
>
> Mingwei Zhang <mizhang@google.com> writes:
>
>> From: Xiong Zhang <xiong.y.zhang@linux.intel.com>
>> Create a new vector in the host IDT for kvm guest PMI handling within
>> mediated passthrough vPMU. In addition, guest PMI handler registration
>> is added into x86_set_kvm_irq_handler().
>> This is the preparation work to support mediated passthrough vPMU to
>> handle kvm guest PMIs without interference from PMI handler of the host
>> PMU.
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Signed-off-by: Xiong Zhang <xiong.y.zhang@linux.intel.com>
>> Tested-by: Yongwei Ma <yongwei.ma@intel.com>
>> Signed-off-by: Mingwei Zhang <mizhang@google.com>
>> ---
>>   arch/x86/include/asm/hardirq.h                |  1 +
>>   arch/x86/include/asm/idtentry.h               |  1 +
>>   arch/x86/include/asm/irq_vectors.h            |  5 ++++-
>>   arch/x86/kernel/idt.c                         |  1 +
>>   arch/x86/kernel/irq.c                         | 21 +++++++++++++++++++
>>   .../beauty/arch/x86/include/asm/irq_vectors.h |  5 ++++-
>>   6 files changed, 32 insertions(+), 2 deletions(-)
>> diff --git a/arch/x86/include/asm/hardirq.h  
>> b/arch/x86/include/asm/hardirq.h
>> index c67fa6ad098a..42a396763c8d 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_guest_pmis;
>>   #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 d4f24499b256..7b1e3e542b1d 100644
>> --- a/arch/x86/include/asm/idtentry.h
>> +++ b/arch/x86/include/asm/idtentry.h
>> @@ -745,6 +745,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_GUEST_PMI_VECTOR,	         
>> sysvec_kvm_guest_pmi_handler);
>>   #else
>>   # define fred_sysvec_kvm_posted_intr_ipi		NULL
>>   # define fred_sysvec_kvm_posted_intr_wakeup_ipi		NULL
>> diff --git a/arch/x86/include/asm/irq_vectors.h  
>> b/arch/x86/include/asm/irq_vectors.h
>> index 13aea8fc3d45..ada270e6f5cb 100644
>> --- a/arch/x86/include/asm/irq_vectors.h
>> +++ b/arch/x86/include/asm/irq_vectors.h
>> @@ -77,7 +77,10 @@
>>    */
>>   #define IRQ_WORK_VECTOR			0xf6
>> -/* 0xf5 - unused, was UV_BAU_MESSAGE */
>> +#if IS_ENABLED(CONFIG_KVM)
>> +#define KVM_GUEST_PMI_VECTOR		0xf5
>> +#endif
>> +
>>   #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 f445bec516a0..0bec4c7e2308 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_GUEST_PMI_VECTOR,		asm_sysvec_kvm_guest_pmi_handler),
>
> There is a subtle inconsistency in that this code is guarded on
> CONFIG_HAVE_KVM when the #define KVM_GUEST_PMI_VECTOR is guarded on
> CONFIG_KVM. Beats me why there are two different flags that are so
> similar, but it is possible they have different values leading to
> compilation errors.
>
> I happened to hit this compilation error because make defconfig will
> define CONFIG_HAVE_KVM but not CONFIG_KVM. CONFIG_KVM is the more strict
> of the two because it depends on CONFIG_HAVE_KVM.
>
> This code should also be guarded on CONFIG_KVM.

Hi Colton,

 what's you kernel base? In latest code base, the CONFIG_HAVE_KVM has been
dropped and this part of code has been guarded by CONFIG_KVM.

Please see commit dcf0926e9b89 "x86: replace CONFIG_HAVE_KVM with
IS_ENABLED(CONFIG_KVM)". Thanks.


>
>>   # endif
>>   # ifdef CONFIG_IRQ_WORK
>>   	INTG(IRQ_WORK_VECTOR,			asm_sysvec_irq_work),
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index 18cd418fe106..b29714e23fc4 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -183,6 +183,12 @@ int arch_show_interrupts(struct seq_file *p, int  
>> prec)
>>   		seq_printf(p, "%10u ",
>>   			   irq_stats(j)->kvm_posted_intr_wakeup_ipis);
>>   	seq_puts(p, "  Posted-interrupt wakeup event\n");
>> +
>> +	seq_printf(p, "%*s: ", prec, "VPMU");
>> +	for_each_online_cpu(j)
>> +		seq_printf(p, "%10u ",
>> +			   irq_stats(j)->kvm_guest_pmis);
>> +	seq_puts(p, " KVM GUEST PMI\n");
>>   #endif
>>   #ifdef CONFIG_X86_POSTED_MSI
>>   	seq_printf(p, "%*s: ", prec, "PMN");
>> @@ -311,6 +317,7 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_x86_platform_ipi)
>>   #if IS_ENABLED(CONFIG_KVM)
>>   static void dummy_handler(void) {}
>>   static void (*kvm_posted_intr_wakeup_handler)(void) = dummy_handler;
>> +static void (*kvm_guest_pmi_handler)(void) = dummy_handler;
>>   void x86_set_kvm_irq_handler(u8 vector, void (*handler)(void))
>>   {
>> @@ -321,6 +328,10 @@ void x86_set_kvm_irq_handler(u8 vector, void  
>> (*handler)(void))
>>   	    (handler == dummy_handler ||
>>   	     kvm_posted_intr_wakeup_handler == dummy_handler))
>>   		kvm_posted_intr_wakeup_handler = handler;
>> +	else if (vector == KVM_GUEST_PMI_VECTOR &&
>> +		 (handler == dummy_handler ||
>> +		  kvm_guest_pmi_handler == dummy_handler))
>> +		kvm_guest_pmi_handler = handler;
>>   	else
>>   		WARN_ON_ONCE(1);
>> @@ -356,6 +367,16 @@  
>> DEFINE_IDTENTRY_SYSVEC_SIMPLE(sysvec_kvm_posted_intr_nested_ipi)
>>   	apic_eoi();
>>   	inc_irq_stat(kvm_posted_intr_nested_ipis);
>>   }
>> +
>> +/*
>> + * Handler for KVM_GUEST_PMI_VECTOR.
>> + */
>> +DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_guest_pmi_handler)
>> +{
>> +	apic_eoi();
>> +	inc_irq_stat(kvm_guest_pmis);
>> +	kvm_guest_pmi_handler();
>> +}
>>   #endif
>>   #ifdef CONFIG_X86_POSTED_MSI
>> diff --git a/tools/perf/trace/beauty/arch/x86/include/asm/irq_vectors.h  
>> b/tools/perf/trace/beauty/arch/x86/include/asm/irq_vectors.h
>> index 13aea8fc3d45..670dcee46631 100644
>> --- a/tools/perf/trace/beauty/arch/x86/include/asm/irq_vectors.h
>> +++ b/tools/perf/trace/beauty/arch/x86/include/asm/irq_vectors.h
>> @@ -77,7 +77,10 @@
>>    */
>>   #define IRQ_WORK_VECTOR			0xf6
>> -/* 0xf5 - unused, was UV_BAU_MESSAGE */
>> +#if IS_ENABLED(CONFIG_KVM)
>> +#define KVM_GUEST_PMI_VECTOR           0xf5
>> +#endif
>> +
>>   #define DEFERRED_ERROR_VECTOR		0xf4
>>   /* Vector on which hypervisor callbacks will be delivered */