Re: [RFC PATCH 04/41] perf: core/x86: Add support to register a new vector for PMI handling
From: Zhang, Xiong Y
Date: Thu Apr 11 2024 - 23:56:57 EST
On 4/12/2024 1:10 AM, Sean Christopherson wrote:
> 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?
it is used in arch_show_interrupts() for /proc/interrupts.
>
>> #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.
yes, KVM_GUEST_PMI_VECTOR in your next reply is better.
>
>> #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?
yes, it should have #if IS_ENABLED(CONFIG_KVM)
>
>> #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...
I will change it to KVM_GUEST_PMI_VECTOR
>
>> + */
>> +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.
Get it. 0xf5 isn't aligned with 0xf2, and the above comment should be moved prior POSTED_INTR_VECTOR
thanks
>
>> #define POSTED_INTR_VECTOR 0xf2
>> #define POSTED_INTR_WAKEUP_VECTOR 0xf1
>> #define POSTED_INTR_NESTED_VECTOR 0xf0
>> --
>> 2.34.1
>>