Received-SPF: pass (google.com: domain of linux-kernel+bounces-75018-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 21 Feb 2024 07:43:30 -0800
In-Reply-To: <05571373-5072-b63b-4a79-f21037556cfa@oracle.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240217014513.7853-1-dongli.zhang@oracle.com>
 <20240217014513.7853-4-dongli.zhang@oracle.com> <ZdPXTfHj4uxfe0Ay@google.com> <05571373-5072-b63b-4a79-f21037556cfa@oracle.com>
Message-ID: <ZdYaIn7xwqTPdofq@google.com>
Subject: Re: [PATCH 3/3] KVM: VMX: simplify MSR interception enable/disable
From: Sean Christopherson <seanjc@google.com>
To: Dongli Zhang <dongli.zhang@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

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.