Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR operations for TDX

[Binbin Wu]

On 4/5/2024 7:42 AM, Isaku Yamahata wrote:
> On Wed, Apr 03, 2024 at 08:14:04AM -0700,
> Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> > On Mon, Feb 26, 2024, isaku.yamahata@xxxxxxxxx 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;

Shoud return true here, right?
> > 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;
> > > +	}
> > > +}
> > > +