Messages in this thread Patch in this message | | | Date | Wed, 3 Apr 2024 08:14:04 -0700 | Subject | Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR operations for TDX | From | Sean Christopherson <> |
| |
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; }
> + 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?
> + /* 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);
> 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.
| |