[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0c8f3f2d-11af-57b9-c922-94755001b973@oracle.com>
Date: Wed, 21 Feb 2024 08:50:04 -0800
From: Dongli Zhang <dongli.zhang@...cle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, pbonzini@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] KVM: VMX: simplify MSR interception enable/disable
On 2/21/24 07:43, Sean Christopherson wrote:
> On Tue, Feb 20, 2024, Dongli Zhang wrote:
>> Hi Sean,
>>
>> On 2/19/24 14:33, Sean Christopherson wrote:
>>> On Fri, Feb 16, 2024, Dongli Zhang wrote:
>>>> ---
>>>> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
>>>> 1 file changed, 28 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>> index 5a866d3c2bc8..76dff0e7d8bd 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
>>>> return -ENOENT;
>>>> }
>>>>
>>>> -static bool is_valid_passthrough_msr(u32 msr)
>>>> +#define VMX_POSSIBLE_PASSTHROUGH 1
>>>> +#define VMX_OTHER_PASSTHROUGH 2
>>>> +/*
>>>> + * Vefify if the msr is the passthrough MSRs.
>>>> + * Return the index in *possible_idx if it is a possible passthrough MSR.
>>>> + */
>>>> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
>>>
>>> There's no need for a custom tri-state return value or an out-param, just return
>>> the slot/-ENOENT. Not fully tested yet, but this should do the trick.
>>
>> The new patch looks good to me, from functionality's perspective.
>>
>> Just that the new patched function looks confusing. That's why I was adding the
>> out-param initially to differentiate from different cases.
>>
>> The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many
>> jobs together:
>>
>> 1. Get the possible passthrough msr slot index.
>>
>> 2. For x2APIC/PT/LBR msr, return -ENOENT.
>>
>> 3. For other msr, return the same -ENOENT, with a WARN.
>>
>> The semantics of the function look confusing.
>>
>> If the objective is to return passthrough msr slot, why return -ENOENT for
>> x2APIC/PT/LBR.
>
> Because there is no "slot" for them in vmx_possible_passthrough_msrs, and the
> main purpose of the helpers is to get that slot in order to efficiently update
> the MSR bitmaps in response to userspace MSR filter changes. The WARN is an extra
> sanity check to ensure that KVM doesn't start passing through an MSR without
> adding the MSR to vmx_possible_passthrough_msrs (or special casing it a la XAPIC,
> PT, and LBR MSRS).
>
>> Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other
>> MSRs may trigger WARN. (I know this is because the other MSRs do not belong to
>> any passthrough MSRs).
>
> The x2APIC/PT/LBR MSRs are given special treatment: KVM may pass them through to
> the guest, but unlike the "regular" passthrough MSRs, userspace is NOT allowed to
> override that behavior via MSR filters.
>
> And so as mentioned above, they don't have a slot in vmx_possible_passthrough_msrs.
Thank you very much for the explanation! This looks good to me.
Dongli Zhang
Powered by blists - more mailing lists