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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5053 bytes --]

On 26.02.23 18:12, Michael Kelley (LINUX) wrote:
> From: Juergen Gross <jgross@suse.com> Sent: Thursday, February 23, 2023 1:33 AM
>>
>> 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.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>> V3:
>> - omit fixed MTRRs, as those are currently not needed
>> - disable X86_FEATURE_MTRR instead of testing it
>> - provide a stub for !CONFIG_MTRR (Michael Kelley)
>> - use cpu_feature_enabled() (Boris Petkov)
>> - add tests for mtrr_overwrite_state() being allowed (Boris Petkov)
>> ---
>>   arch/x86/include/asm/mtrr.h        |  8 ++++++
>>   arch/x86/kernel/cpu/mtrr/generic.c | 43 ++++++++++++++++++++++++++++++
>>   arch/x86/kernel/cpu/mtrr/mtrr.c    |  9 +++++++
>>   arch/x86/kernel/setup.c            |  2 ++
>>   4 files changed, 62 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
>> index f0eeaf6e5f5f..f1cb81330a64 100644
>> --- a/arch/x86/include/asm/mtrr.h
>> +++ b/arch/x86/include/asm/mtrr.h
>> @@ -31,6 +31,8 @@
>>    */
>>   # ifdef CONFIG_MTRR
>>   void mtrr_bp_init(void);
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> +			  mtrr_type def_type);
>>   extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
>>   extern void mtrr_save_fixed_ranges(void *);
>>   extern void mtrr_save_state(void);
>> @@ -48,6 +50,12 @@ void mtrr_disable(void);
>>   void mtrr_enable(void);
>>   void mtrr_generic_set_state(void);
>>   #  else
>> +static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
>> +					unsigned int num_var,
>> +					mtrr_type def_type)
>> +{
>> +}
>> +
>>   static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
>>   {
>>   	/*
>> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
>> index ee09d359e08f..40c59d522f57 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/tlbflush.h>
>>   #include <asm/mtrr.h>
>>   #include <asm/msr.h>
>> @@ -240,6 +242,47 @@ 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).
>> + * Is allowed only for special cases when running virtualized. Must be called
>> + * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
>> + */
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> +			  mtrr_type def_type)
>> +{
>> +	unsigned int i;
>> +
>> +	if (WARN_ON(mtrr_state_set ||
>> +		    hypervisor_is_type(X86_HYPER_NATIVE) ||
>> +		    !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
>> +		    (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
> 
> With current upstream code, this test doesn't allow Hyper-V SNP vTOM VMs
> to do the overwrite, as current upstream vTOM code doesn't participate in the
> cc_platform_has() mechanism.  That's being reworked in a separate patch
> set.  Can you add this test to cover the SNP vTOM case?
> 
> 		    !hv_is_isolation_supported() &&
> 
> This is the same test used in __set_memory_enc_dec(), for example.  You'll
> have to add #include <asm/mshyperv.h>.  There's already a stub that returns
> 'false' so that everything works when building with CONFIG_HYPERV=n.

Yes, this was an open question. In case nobody objects, I'll do the
modification.

> Once my other patch set is accepted, I'll revise this to remove use of
> hv_is_isolation_supported() outside of Hyper-V specific code, and use
> the newer cc_platform_has() instead.

Thanks,


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]