Messages in this thread | | | Date | Mon, 13 Feb 2023 07:35:29 +0100 | From | Juergen Gross <> | Subject | Re: [PATCH v2 7/8] x86/mm: only check uniform after calling mtrr_type_lookup() |
| |
On 13.02.23 02:08, Michael Kelley (LINUX) wrote: > From: Juergen Gross <jgross@suse.com> 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@linux-foundation.org> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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 > [ 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 don't see a flaw in your reasoning.
Rick mentioned a problem with this patch in a KVM guest. I'll try to reproduce his setup for checking whether fixing mtrr_type_lookup_variable() is enough, or if we need to keep the tests for WB in this patch.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |