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

[Borislav Petkov <bp@alien8.de>]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette