Received-SPF: pass (google.com: domain of linux-kernel+bounces-132270-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 4 Apr 2024 16:42:38 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR
 operations for TDX
Message-ID: <20240404234238.GW2444378@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <62f8890cb90e49a3e0b0d5946318c0267b80c540.1708933498.git.isaku.yamahata@intel.com>
 <Zg1yPIV6cVJrwGxX@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zg1yPIV6cVJrwGxX@google.com>

On Wed, Apr 03, 2024 at 08:14:04AM -0700,
Sean Christopherson <seanjc@google.com> wrote:

> On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 389bb95d2af0..c8f991b69720 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1877,6 +1877,76 @@ void tdx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason,
> >  	*error_code = 0;
> >  }
> >  
> > +static bool tdx_is_emulated_kvm_msr(u32 index, bool write)
> > +{
> > +	switch (index) {
> > +	case MSR_KVM_POLL_CONTROL:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +bool tdx_has_emulated_msr(u32 index, bool write)
> > +{
> > +	switch (index) {
> > +	case MSR_IA32_UCODE_REV:
> > +	case MSR_IA32_ARCH_CAPABILITIES:
> > +	case MSR_IA32_POWER_CTL:
> > +	case MSR_IA32_CR_PAT:
> > +	case MSR_IA32_TSC_DEADLINE:
> > +	case MSR_IA32_MISC_ENABLE:
> > +	case MSR_PLATFORM_INFO:
> > +	case MSR_MISC_FEATURES_ENABLES:
> > +	case MSR_IA32_MCG_CAP:
> > +	case MSR_IA32_MCG_STATUS:
> > +	case MSR_IA32_MCG_CTL:
> > +	case MSR_IA32_MCG_EXT_CTL:
> > +	case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> > +	case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
> > +		/* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
> > +		return true;
> > +	case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
> > +		/*
> > +		 * x2APIC registers that are virtualized by the CPU can't be
> > +		 * emulated, KVM doesn't have access to the virtual APIC page.
> > +		 */
> > +		switch (index) {
> > +		case X2APIC_MSR(APIC_TASKPRI):
> > +		case X2APIC_MSR(APIC_PROCPRI):
> > +		case X2APIC_MSR(APIC_EOI):
> > +		case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
> > +		case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
> > +		case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
> > +			return false;
> > +		default:
> > +			return true;
> > +		}
> > +	case MSR_IA32_APICBASE:
> > +	case MSR_EFER:
> > +		return !write;
> 
> Meh, for literally two MSRs, just open code them in tdx_set_msr() and drop the
> @write param.  Or alternatively add:
> 
> static bool tdx_is_read_only_msr(u32 msr){
> {
> 	return msr == MSR_IA32_APICBASE || msr == MSR_EFER;
> }

Sure will add.

> 
> > +	case 0x4b564d00 ... 0x4b564dff:
> 
> This is silly, just do
> 
> 	case MSR_KVM_POLL_CONTROL:
> 		return false;
> 
> and let everything else go through the default statement, no?

Now tdx_is_emulated_kvm_msr() is trivial, will open code it.


> > +		/* KVM custom MSRs */
> > +		return tdx_is_emulated_kvm_msr(index, write);
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +int tdx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> > +{
> > +	if (tdx_has_emulated_msr(msr->index, false))
> > +		return kvm_get_msr_common(vcpu, msr);
> > +	return 1;
> 
> Please invert these and make the happy path the not-taken path, i.e.
> 
> 	if (!tdx_has_emulated_msr(msr->index))
> 		return 1;
> 
> 	return kvm_get_msr_common(vcpu, msr);
> 
> The standard kernel pattern is
> 
> 	if (error)
> 		return <error thingie>
> 
> 	return <happy thingie>
> 
> > +}
> > +
> > +int tdx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> > +{
> > +	if (tdx_has_emulated_msr(msr->index, true))
> 
> As above:
> 
> 	if (tdx_is_read_only_msr(msr->index))
> 		return 1;
> 
> 	if (!tdx_has_emulated_msr(msr->index))
> 		return 1;
> 
> 	return kvm_set_msr_common(vcpu, msr);

Sure, will update them.


> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index d5b18cad9dcd..0e1d3853eeb4 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -90,7 +90,6 @@
> >  #include "trace.h"
> >  
> >  #define MAX_IO_MSRS 256
> > -#define KVM_MAX_MCE_BANKS 32
> >  
> >  struct kvm_caps kvm_caps __read_mostly = {
> >  	.supported_mce_cap = MCG_CTL_P | MCG_SER_P,
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index 4e40c23d66ed..c87b7a777b67 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -9,6 +9,8 @@
> >  #include "kvm_cache_regs.h"
> >  #include "kvm_emulate.h"
> >  
> > +#define KVM_MAX_MCE_BANKS 32
> 
> Split this to a separate.  Yes, it's trivial, but that's _exactly_ why it should
> be in a separate patch.  The more trivial refactoring you split out, the more we
> can apply _now_ and take off your hands.

Will split it.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>