Received-SPF: pass (google.com: domain of linux-kernel+bounces-143487-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <71f037d5-38bb-4493-878f-19adc02af2df@linux.intel.com>
Date: Sat, 13 Apr 2024 09:17:07 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 04/41] perf: core/x86: Add support to register a new
 vector for PMI handling
To: "Zhang, Xiong Y" <xiong.y.zhang@linux.intel.com>,
 Sean Christopherson <seanjc@google.com>
Cc: pbonzini@redhat.com, peterz@infradead.org, mizhang@google.com,
 kan.liang@intel.com, zhenyuw@linux.intel.com, jmattson@google.com,
 kvm@vger.kernel.org, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org, zhiyuan.lv@intel.com, eranian@google.com,
 irogers@google.com, samantha.alt@intel.com, like.xu.linux@gmail.com,
 chao.gao@intel.com, Xiong Zhang <xiong.y.zhang@intel.com>
References: <20240126085444.324918-1-xiong.y.zhang@linux.intel.com>
 <20240126085444.324918-5-xiong.y.zhang@linux.intel.com>
 <ZhgZdqAB6LlvJLof@google.com>
 <3b4f03f8-146f-46ce-b729-046e9444d9e4@linux.intel.com>
Content-Language: en-US
From: "Mi, Dapeng" <dapeng1.mi@linux.intel.com>
In-Reply-To: <3b4f03f8-146f-46ce-b729-046e9444d9e4@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/12/2024 11:56 AM, Zhang, Xiong Y wrote:
>
> On 4/12/2024 1:10 AM, Sean Christopherson wrote:
>> On Fri, Jan 26, 2024, Xiong Zhang wrote:
>>> From: Xiong Zhang <xiong.y.zhang@intel.com>
>>>
>>> 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@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.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.

Yes, these interrupt stats are useful, e.g. when we analyze the VM-EXIT 
performance overhead, if the vm-exits are caused by external interrupt, 
we usually need to look at these interrupt stats and check which exact 
interrupt causes the vm-exits.

>>>   #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
>>>