[2/6] x86/pat: check for MTRRs enabled in memtype_reserve()

  Today memtype_reserve() bails out early if pat_enabled() returns false.
The same can be done in case MTRRs aren't enabled.

This will reinstate the behavior of memtype_reserve() before commit
72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
running on Xen"). There have been reports about that commit breaking
SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
but that again resulted in problems with Xen PV guests.

Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm/pat/memtype.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
  

* Juergen Gross <jgross@suse.com> wrote:

> Today memtype_reserve() bails out early if pat_enabled() returns false.
> The same can be done in case MTRRs aren't enabled.
> 
> This will reinstate the behavior of memtype_reserve() before commit
> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
> running on Xen"). There have been reports about that commit breaking
> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
> but that again resulted in problems with Xen PV guests.
> 
> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/mm/pat/memtype.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index fb4b1b5e0dea..18f612b43763 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
>  		return -EINVAL;
>  	}
>  
> -	if (!pat_enabled()) {
> -		/* This is identical to page table setting without PAT */
> +	/*
> +	 * PAT disabled or MTRRs disabled don't require any memory type
> +	 * tracking or type adjustments, as there can't be any conflicts
> +	 * between PAT and MTRRs with at least one of both being disabled.
> +	 */
> +	if (!pat_enabled() || !mtrr_enabled()) {
>  		if (new_type)
>  			*new_type = req_type;

Doesn't memtype_reserve() also check for overlapping ranges & type 
compatibility in memtype_check_conflict(), etc., which can occur even in a 
pure PAT setup? Ie. are we 100% sure that in the !MTRR case it would be a 
NOP?

But even if it's a functional NOP as you claim, we'd still be better off if 
the memtype tree was still intact - instead of just turning off the API.

Also, speling nit:

   s/one of both
    /one or both

Thanks,

	Ingo
  

On 07.02.23 09:49, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> Today memtype_reserve() bails out early if pat_enabled() returns false.
>> The same can be done in case MTRRs aren't enabled.
>>
>> This will reinstate the behavior of memtype_reserve() before commit
>> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
>> running on Xen"). There have been reports about that commit breaking
>> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
>> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
>> but that again resulted in problems with Xen PV guests.
>>
>> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
>> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/mm/pat/memtype.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
>> index fb4b1b5e0dea..18f612b43763 100644
>> --- a/arch/x86/mm/pat/memtype.c
>> +++ b/arch/x86/mm/pat/memtype.c
>> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (!pat_enabled()) {
>> -		/* This is identical to page table setting without PAT */
>> +	/*
>> +	 * PAT disabled or MTRRs disabled don't require any memory type
>> +	 * tracking or type adjustments, as there can't be any conflicts
>> +	 * between PAT and MTRRs with at least one of both being disabled.
>> +	 */
>> +	if (!pat_enabled() || !mtrr_enabled()) {
>>   		if (new_type)
>>   			*new_type = req_type;
> 
> Doesn't memtype_reserve() also check for overlapping ranges & type
> compatibility in memtype_check_conflict(), etc., which can occur even in a
> pure PAT setup? Ie. are we 100% sure that in the !MTRR case it would be a
> NOP?
> 
> But even if it's a functional NOP as you claim, we'd still be better off if
> the memtype tree was still intact - instead of just turning off the API.

Yes, that's basically the issue discussed in [patch 0/6].

It should still be better than the original case (PAT and MTRR off, but
the ability to use PAT nevertheless), though.

> 
> Also, speling nit:
> 
>     s/one of both
>      /one or both

Hmm, but only if I drop the "at least". I don't really mind either way.


Juergen
  

On Tue, Feb 07, 2023 at 08:28:58AM +0100, Juergen Gross wrote:
> Today memtype_reserve() bails out early if pat_enabled() returns false.
> The same can be done in case MTRRs aren't enabled.
> 
> This will reinstate the behavior of memtype_reserve() before commit
> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
> running on Xen"). There have been reports about that commit breaking
> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
> but that again resulted in problems with Xen PV guests.

No, no commit message text with references to other commits.

Considering how this is one of those "let's upset the universe" thing of
decoupling MTRRs from PAT and how it breaks in weird ways, if we ever
end up doing that, then we need to explain *exactly* why we're doing it.

And in detail.

Because otherwise, in the future, we'll end up scratching heads just
like we're doing now as to why the large page thing allowed those
certain types, and so on and so on.

> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/mm/pat/memtype.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index fb4b1b5e0dea..18f612b43763 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
>  		return -EINVAL;
>  	}
>  
> -	if (!pat_enabled()) {
> -		/* This is identical to page table setting without PAT */
> +	/*
> +	 * PAT disabled or MTRRs disabled don't require any memory type
> +	 * tracking or type adjustments, as there can't be any conflicts
> +	 * between PAT and MTRRs with at least one of both being disabled.
> +	 */
> +	if (!pat_enabled() || !mtrr_enabled()) {

Yah, looks straight-forward to me but I have said this before. And we
have broken shit so if anything, this needs to be tested on everything
before we go with it.

IMNSVHO.