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

[Huang, Kai]

On Mon, 2023-05-15 at 10:49 -0700, Sean Christopherson wrote:
> 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 meant it may make sense to allow Qemu to configure it.  Anyway thanks!

> 
> > 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.

+Isaku,

Presumably the only case is assigning GPU to TDX VMs, but again not sure we need
to consider this as AFAICT TDX *officially* doesn't support untrusted device
passthrough.  Isaku may have more information.

And Iskau,

Do you have any comments here, especially considering TDX 2.0 support for TEE-
IO?  We probably need a solution that is future extensible.

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