Re: [PATCH v2 5/8] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Sean Christopherson <seanjc@google.com>]

On Mon, May 15, 2023, Kai Huang wrote:
> On Fri, 2023-05-12 at 09:35 -0700, Sean Christopherson wrote:
> > On Fri, May 12, 2023, Kai Huang wrote:
> > > On Thu, 2023-05-11 at 16:33 -0700, Sean Christopherson wrote:
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index e7f78fe79b32..8b356c9d8a81 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -3700,8 +3700,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > >  			return 1;
> > > >  		}
> > > >  		break;
> > > > -	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> > > > -	case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
> > > > +	case MSR_IA32_CR_PAT:
> > > > +	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
> > > > +	case MSR_MTRRdefType:
> > > >  		return kvm_mtrr_set_msr(vcpu, msr, data);
> > > >  	case MSR_IA32_APICBASE:
> > > >  		return kvm_set_apic_base(vcpu, msr_info);
> > > > @@ -4108,9 +4109,10 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > >  		msr_info->data = kvm_scale_tsc(rdtsc(), ratio) + offset;
> > > >  		break;
> > > >  	}
> > > > +	case MSR_IA32_CR_PAT:
> > > >  	case MSR_MTRRcap:
> > > 
> > > ... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
> > > kvm_set_msr_common()?
> > > 
> > > Looks there's no reason to put it before MSR_MTRRcap.
> > 
> > No, it's above MTRRcap for two reasons:
> > 
> >  1. When PAT is moved out of mtrr.c, PAT doesn't get bunded with the other MTRRs
> >     and so would just need to be hoisted back up.  The end code looks like:
> > 
> > 	case MSR_IA32_CR_PAT:
> > 		msr_info->data = vcpu->arch.pat;
> > 		break;
> > 	case MSR_MTRRcap:
> > 	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
> > 	case MSR_MTRRdefType:
> > 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> 
> Sorry I mistakenly thought MSR_MTRRcap wasn't handled in kvm_mtrr_get_msr(). 
> Yes looks good to me.
> 
> >  
> >  2. There is no MSR_MTRRcap case statement in kvm_set_msr_common() because it's
> >     a read-only MSR, i.e. the two can't be symmetric :-)
> 
> Do you know why it is a read-only MSR, which enables both FIXED ranges and
> (fixed number of) dynamic ranges?

MTTRcap doesn't "enable" anything, it's a capabilities MSR (MTRR Capability is
its given name in the SDM), similar to ARCH_CAPABILITIES, PERF_CAPABILITIES, etc.
They're all essentially CPUID leafs, but presumably are MSRs due to being relevant
only to CPL0.  Or maybe some higher ups at Intel just spin a wheel to determine
whether to use a CPUID leaf or an MSR.  :-)

> I am asking because there's a x86 series to fake a simple synthetic MTRR which
> neither has fixed nor dynamic ranges but only has a default MSR_MTRRdefType:
> 
> https://lore.kernel.org/lkml/20230509233641.GGZFrZCTDH7VwUMp5R@fat_crate.local/T/
> 
> The main use cases are Xen PV guests and SEV-SNP guests running under
> Hyper-V, but it appears TDX guest is also desired to have similar handling,
> because:�
> 
> 1) TDX module exposes MTRR in CPUID to guest, but handles nothing about MTRR
> MSRs but only injects #VE.
> 
> 2) TDX module always maps guest private memory as WB (and ignores guest's PAT
> IIUC);
> 
> 3) For shared memory, w/o non-coherent DMA guest's MTRRs are ignored by KVM
> anyway.  TDX doesn't officially support non-trusted device assignment AFAICT.
> Even we want to consider non-coherent DMA, it would only add confusion to honor
> guest's MTRRs since they can point to private memory, which is always mapped as
> WB.

Yeah, I think the best option is for KVM to disallow attaching non-coherent DMA
to TDX VMs.  AFAIK, there's no use case for such a setup.

> So basically looks there's no value to exposing FIXED and dynamic MTRR ranges to
> TDX guest.

Agreed.