Re: [PATCH v2 7/8] x86/mm: only check uniform after calling mtrr_type_lookup()

From: Juergen Gross
Date: Wed Feb 15 2023 - 08:40:21 EST

On 13.02.23 02:08, Michael Kelley (LINUX) wrote:
From: Juergen Gross <jgross@xxxxxxxx> Sent: Wednesday, February 8, 2023 11:22 PM

Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
WB or INVALID after calling mtrr_type_lookup(). Those tests can be
dropped, as the only reason to not use a large mapping would be
uniform being 0. Any MTRR type can be accepted as long as it applies
to the whole memory range covered by the mapping, as the alternative
would only be to map the same region with smaller pages instead using
the same PAT type as for the large mapping.

Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
arch/x86/mm/pgtable.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index e4f499eb0f29..7b9c5443d176 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
u8 mtrr, uniform;

mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
- if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
- (mtrr != MTRR_TYPE_WRBACK))
+ if (!uniform)
return 0;

/* Bail out if we are we on a populated non-leaf entry: */
@@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
u8 mtrr, uniform;

mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
- if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
- (mtrr != MTRR_TYPE_WRBACK)) {
+ if (!uniform) {
pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
__func__, addr, addr + PMD_SIZE);

I'm seeing this warning trigger in a normal Hyper-V guest (i.e., *not* an
SEV-SNP Confidential VM). The original filtering here based on
MTRR_TYPE_WRBACK appears to be hiding a bug in mtrr_type_lookup_variable()
where it incorrectly thinks an address range matches two different variable
MTRRs, and hence clears "uniform".

Here are the variable MTRRs in the normal Hyper-V guest with 32 GiBytes
of memory:

[ 0.043592] MTRR variable ranges enabled:
[ 0.048308] 0 base 000000000000 mask FFFF00000000 write-back
[ 0.057450] 1 base 000100000000 mask FFF000000000 write-back

I've read the SDM chapter for MTRRs again. Doesn't #1 violate the requirements
for MTRR settings? The SDM says:

For ranges greater than 4 KBytes, each range must be of length 2^n and its
base address must be aligned on a 2^n boundary, where n is a value equal to
or greater than 12. The base-address alignment value cannot be less than its
length. For example, an 8-KByte range cannot be aligned on a 4-KByte boundary.
It must be aligned on at least an 8-KByte boundary.

This makes the reasoning below wrong.

[ 0.063972] 2 disabled
[ 0.066755] 3 disabled
[ 0.070024] 4 disabled
[ 0.072856] 5 disabled
[ 0.076112] 6 disabled
[ 0.078760] 7 disabled

Variable MTRR #0 covers addresses up to 4 GiByte, while #1 covers
4 GiByte to 64 GiByte. But in mtrr_type_lookup_variable(), address
range 0xF8000000 to 0xF81FFFFF is matching both MTRRs, when it
should be matching just #0.

The problem looks to be this code in mtrr_type_lookup_variable():

if ((start & mask) != (base & mask))
continue;

If the mask bits of start and base are different, then the
MTRR doesn't match, and the continue statement should be
executed. That's correct. But if the mask bits are the same,
that's not sufficient for the MTRR to match. If the end
address is less than base, the MTRR doesn't match, and
the continue statement should still be executed, which
isn't happening.

But somebody please check my thinking. :-)

I think you need to correct the hypervisor.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature