linux-kernel - Re: [RFC 3/3] x86: KVM: stats: Add a stat counter for GALog events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98493056-4a75-46ad-be79-eb6784034394@oracle.com>
Date: Tue, 9 Apr 2024 21:31:45 -0400
From: Alejandro Jimenez <alejandro.j.jimenez@...cle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: kvm@...r.kernel.org, seanjc@...gle.com, pbonzini@...hat.com,
        linux-kernel@...r.kernel.org, joao.m.martins@...cle.com,
        boris.ostrovsky@...cle.com, mark.kanda@...cle.com,
        suravee.suthikulpanit@....com, mlevitsk@...hat.com
Subject: Re: [RFC 3/3] x86: KVM: stats: Add a stat counter for GALog events

On 4/9/24 02:45, Chao Gao wrote:
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 4b74ea91f4e6..853cafe4a9af 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -165,8 +165,10 @@ int avic_ga_log_notifier(u32 ga_tag)
>> 	 * bit in the vAPIC backing page. So, we just need to schedule
>> 	 * in the vcpu.
>> 	 */
>> -	if (vcpu)
>> +	if (vcpu) {
>> 		kvm_vcpu_wake_up(vcpu);
>> +		++vcpu->stat.ga_log_event;
>> +	}
>>
> 
> I am not sure why this is added for SVM only.

I am mostly familiar with AVIC, and much less so with VMX's PI, so this is
why I am likely missing potential stats that could be useful to expose from
the VMX  side. I'll be glad to implement any other suggestions you have.

it looks to me GALog events are
> similar to Intel IOMMU's wakeup events. Can we have a general name? maybe
> iommu_wakeup_event

I believe that after:
d588bb9be1da ("KVM: VMX: enable IPI virtualization")

both the VT-d PI and the virtualized IPIs code paths will use POSTED_INTR_WAKEUP_VECTOR
for interrupts targeting a blocked vCPU. So on Intel hosts enabling IPI virtualization,
a counter incremented in pi_wakeup_handler() would record interrupts from both virtualized
IPIs and VT-d sources.

I don't think it is correct to generalize this counter since AMD's implementation is
different; when a blocked vCPU is targeted:

- by device interrupts, it uses the GA Log mechanism
- by an IPI, it generates an AVIC_INCOMPLETE_IPI #VMEXIT

If the reasoning above is correct, we can add a VMX specific counter (vmx_pi_wakeup_event?)
that is increased in pi_wakeup_handler() as you suggest, and document the difference
in behavior so that is not confused as equivalent with the ga_log_event counter.

An alternative if we'd like to have a common 'iommu_wakeup_event' is to add filtering on
pi_wakeup_handler() and only increment the common counter if IPI virtualization is not
enabled (i.e. !vmx_can_use_ipiv()), in which case  we'd only handle device interrupts
and it becomes the parallel case to GA Log events.

That leaves us with a VMX-specific counter (vmx_pi_wakeup_event) which provides no
definition between interrupt sources when IPI virtualization is enabled, or when disabled
we have a common/generic counter (iommu_wakeup_event) that applies to both vendors.

Please let me know if you agree with this approach or have other suggestions.

Thank you,
Alejandro

> 
> and increase the counter after the kvm_vcpu_wake_up() call in pi_wakeup_handler().