Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Huang, Kai]

On Mon, 2023-03-20 at 14:47 +0100, Juergen Gross wrote:
> On 20.03.23 13:59, Huang, Kai wrote:
> > On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
> > > When running virtualized, MTRR access can be reduced (e.g. in Xen PV
> > > guests or when running as a SEV-SNP guest under Hyper-V). Typically
> > > the hypervisor will reset the MTRR feature in CPUID data, resulting
> > > in no MTRR memory type information being available for the kernel.
> > > 
> > > This has turned out to result in problems:
> > > 
> > > - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
> > > - Xen PV dom0 mapping memory as WB which should be UC- instead
> > > 
> > > Solve those problems by supporting to set a static MTRR state,
> > > overwriting the empty state used today. In case such a state has been
> > > set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
> > > will only be used by mtrr_type_lookup(), as in all other cases
> > > mtrr_enabled() is being checked, which will return false. Accept the
> > > overwrite call only for selected cases when running as a guest.
> > > Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
> > > just refusing them.
> > > 
> > > 
> > [...]
> > 
> > > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > > index ee09d359e08f..49b4cc923312 100644
> > > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > > @@ -8,10 +8,12 @@
> > >   #include <linux/init.h>
> > >   #include <linux/io.h>
> > >   #include <linux/mm.h>
> > > -
> > > +#include <linux/cc_platform.h>
> > >   #include <asm/processor-flags.h>
> > >   #include <asm/cacheinfo.h>
> > >   #include <asm/cpufeature.h>
> > > +#include <asm/hypervisor.h>
> > > +#include <asm/mshyperv.h>
> > 
> > Is <asm/mshyperv.h> needed here?
> 
> Yes, for hv_is_isolation_supported().
> 
> > 
> > >   #include <asm/tlbflush.h>
> > >   #include <asm/mtrr.h>
> > >   #include <asm/msr.h>
> > > @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > >   	return mtrr_state.def_type;
> > >   }
> > >   
> > > +/**
> > > + * mtrr_overwrite_state - set static MTRR state
> > > + *
> > > + * Used to set MTRR state via different means (e.g. with data obtained from
> > > + * a hypervisor).
> > 
> > +KVM list and KVM maintainers,
> > 
> > IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
> > hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
> > because they have mutual understanding?
> > 
> > What about the SNP guest running on other hypervisors such as KVM?
> > 
> > Since this code covers TDX guest too, I think eventually it makes sense for TDX
> > guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> > think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> > as I am not sure what's the point of making the TDX guest's MTRR behaviour
> > depending on specific hypervisor.
> 
> This series tries to fix the current fallout.
> 
> Boris Petkov asked for the hypervisor specific tests to be added, so I've
> added them after discussing the topic with him (he is the maintainer of
> this code after all).
> 
> > For now I don't see there's any use case for TDX guest to use non-WB memory type
> > (in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
> > the guest memory), so to me it seems it's OK to make a universal mutual
> > understanding that TDX guest will always have WB memory type for all memory.
> 
> I agree.
> 
> > But, I am not sure whether it's better to have a standard hypercall between
> > guest & hypervisor for this purpose so things can be more flexible?
> 
> Maybe. But for now we need to handle the current situation.
> 
> 

Agreed.  Thanks for explaining.