Re: External USB disks not recognized with v6.1.8 when using Xen

From: Borislav Petkov <bp@alien8.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christian Kujau <lists@nerdbynature.de>,
	Juergen Gross <jgross@suse.com>,
	Michael Kelley <mikelley@microsoft.com>,
	linux-kernel@vger.kernel.org,
	Greg KH <gregkh@linuxfoundation.org>,
	Linux regressions mailing list <regressions@lists.linux.dev>
Subject: Re: External USB disks not recognized with v6.1.8 when using Xen
Date: Mon, 6 Feb 2023 10:43:05 +0100	[thread overview]
Message-ID: <Y+DLqV5MfuBJRnb6@zn.tnic> (raw)
In-Reply-To: <CAHk-=wgC_MEFnnzUGN4q9pmhxV+eFV1Oo=W2j1J69YhJF5EDtw@mail.gmail.com>

On Sun, Feb 05, 2023 at 12:21:42PM -0800, Linus Torvalds wrote:
> So this is the one I'd almost leave alone.
> 
> Because this is not a "there are no MTRR's" situation, this is a "I
> haven't enabled CONFIG_MTRR, so I don't _know_ if there are any MTRR's
> or not.
> 
> And returning MTRR_TYPE_UNCACHABLE will then disable things like
> largepages etc, so this change would effectively mean that if
> CONFIG_MTRR is off, it would turn off hugepage support too.

Right, if we wanted to be precise here, we would check whether the
underlying hw supports MTRRs - i.e., check CPUID bit - and if our
support for it is disabled, then we'd return UC because this is how the
MTRR-supporting hw behaves:

"If the MTRRs are disabled in implementations that support the MTRR
mechanism, the default memory type is set to uncacheable (UC)."

That's the AMD APM.

The Intel SDM has a similar wording:

"Following a hardware reset, the P6 and more recent processor families
disable all the fixed and variable MTRRs, which in effect makes all of
physical memory uncacheable."

So something like

	if (cpu_feature_enabled(X86_FEATURE_MTRR))
		return MTRR_TYPE_UNCACHABLE;
	else
		return MTRR_TYPE_INVALID;

> > @@ -721,8 +721,9 @@ 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 (mtrr != MTRR_TYPE_UNCACHABLE &&
> > +           mtrr != MTRR_TYPE_WRBACK &&
> > +           !uniform)
> >                 return 0;
> 
> Here you make up for it, but I don't actually understand why these
> checks exist at all.
> 
> I *think* that what the check should do is just check for uniformity.

Looka here:

6b6378355b92 ("x86, mm: support huge KVA mappings on x86")

Ack on the uniformity aspect. The WB is fine too because "has no affect on
the PAT memory types."

And then when MTRRs are disabled, then I guess it doesn't matter for the
large page mappings anyway. I would have said that we don't really care
about MTRRs being disabled but all those new confidential computing
things do disable MTRRs. Xen too.

Thx.

-- 
Regards/Gruss,
    Boris.

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