On 05.02.23 21:21, Linus Torvalds wrote:
> On Sun, Feb 5, 2023 at 5:20 AM Borislav Petkov <bp@alien8.de> wrote:
>>
>> @@ -53,7 +53,8 @@ static inline u8 mtrr_type_lookup(u64 addr,
>>          /*
>>           * Return no-MTRRs:
>>           */
>> -       return MTRR_TYPE_INVALID;
>> +       *uniform = 1;
>> +       return MTRR_TYPE_UNCACHABLE;
> 
> 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.

Yes.

> 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.

Correct.

> 
> But maybe that was the only thing that cared, and we have:
> 
>> @@ -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.
> 
> Why would the largepage code otherwise care?

I agree. The reasoning in the comment above pud_set_huge() is nonsense, as
it is not specific to huge pages:

  * - MTRRs are enabled and the corresponding MTRR memory type is WB, which
  *   has no effect on the requested PAT memory type.

Any other MTRR memory type would interfere with the requested PAT memory
type in undesired ways, but this is still true when using small pages
only.

> Other MTRR types are explicitly fine, and I think things like the X
> server might even want to do write-combining with large pages etc.
> 
> So I think the hugepage code should only do
> 
>       if (!uniform)
>            return 0;
> 
> or there should be some explanation for why those types are special?

As written above: there is an explanation, but it doesn't make much sense
IMHO.

> 
>>> @@ -748,8 +749,9 @@ 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 (mtrr != MTRR_TYPE_UNCACHABLE &&
>> +           mtrr != MTRR_TYPE_WRBACK &&
>> +           !uniform) {
> 
> Same here.
> 
> Again, I *think* that the reason it used to do that "check two types"
> thing is simply because "uniform" wasn't set correctly.

This might very well be the reason, yes.

I still don't see why the original report of Christian is making sense:

According to the error message, the _requested_ memory type was UC-, but
the reverted patch only affects cases where the requested type is WB. So
why does a revert of 90b926e68f50 is helping to make this message go away?
The message was:

   ioremap error for 0xf2520000-0xf2530000, requested 0x2, got 0x0

Meanwhile I've found a system which is issuing such a message under Xen.
I'll investigate further _why_ a request of UC- ends up to get WB.


Juergen