Re: [PATCH v5 04/15] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross <jgross@suse.com>]

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

On 11.04.23 15:20, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:41AM +0200, 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:
> 
> Let's add the links to those problems:
> 
>> - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
> 
> I can't find Michael's original report, do you have a link?

DYM 
https://lore.kernel.org/lkml/BYAPR21MB16883ABC186566BD4D2A1451D7FE9@BYAPR21MB1688.namprd21.prod.outlook.com/ 
?

> 
>> - Xen PV dom0 mapping memory as WB which should be UC- instead
> 
> Link: https://lore.kernel.org/all/4fe9541e-4d4c-2b2a-f8c8-2d34a7284930@nerdbynature.de/
> 
>>
>> Solve those problems by supporting to set a static MTRR state,
> 
> s/by supporting to set a/allowing an MTRR static state override/
> 
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> +			  mtrr_type def_type)
>> +{
>> +	unsigned int i;
>> +
>> +	/* Only allowed to be called once before mtrr_bp_init(). */
>> +	if (WARN_ON(mtrr_state_set))
> 
> WARN_ON_ONCE() is probably better.

If you like that better (I don't see the real benefit, as something would
be severely broken if this triggers more than once, but I don't really
mind).

> 
>> +		return;
>> +
>> +	/* Only allowed when running virtualized. */
>> +	if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
>> +		return;
>> +
>> +	/*
>> +	 * Only allowed for special virtualization cases:
>> +	 * - when running as SEV-SNP guest
>> +	 * - when running as Hyper-V isolated guest
> 
> 	when running as a SEV-SNP guest on a HyperV with vTOM enabled
> 
> that's a single condition.
> 
>> +	 * - when running as Xen PV guest
>> +	 * - when running as TSX guest
>> +	 */
>> +	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
>> +	    !hv_is_isolation_supported() &&
>> +	    !cpu_feature_enabled(X86_FEATURE_XENPV) &&
>> +	    !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> 
> IOW:
> 
> 	if (!(hv_is_isolation_supported() && cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) &&
> 	    !cpu_feature_enabled(X86_FEATURE_XENPV) &&
> 	    !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))

Okay.


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