Re: [PATCH] x86/mtrr: Correct the returned MTRR type of mtrr_type_lookup.

[Huang, Ying-Tsun]

On Fri, 11 Dec 2020, Borislav Petkov wrote:

> [CAUTION: External Email]
> 
> On Mon, Dec 07, 2020 at 02:12:26PM +0800, Ying-Tsun Huang wrote:
> > In mtrr_type_lookup, if the input memory address region is not in the
> > MTRR, over 4GB, and not over the top of memory, write-back attribute
> > is returned. These condition checks are for ensuring the input memory
> > address region is mapped to the physical memory actually.
> >
> > However, if the end address is just aligned with the top of memory,
> > the condition check treats the address is over the top of memory, and
> > write-back attribute is not returned.
> 
> Oh fun. So to make sure I understand this correctly end ends up equal to
> TOM2?
> 

Yes, it's equal to TOM2.

> > There is a real case of NVDIMM. The nd_pmem module tries to map
> > NVDIMMs as cacheable memories when NVDIMMs are connected. If a NVDIMM
> > is the last of the DIMMs, the performance of this NVDIMM becomes very
> > low since it aligned with the top of memory and its memory type is
> > uncached-minus.
> >
> > To check the top of memory should use "<=" instead of "<" since both the
> > input end address and the value of top of memory are actually the start
> > of next region.
> 
> Right, so looking at that function, it calls
> mtrr_type_lookup_variable(), among others, and that does:
> 
>         /* Make end inclusive instead of exclusive */
>         end--;
> 
> which sounds to me like it expects ranges with exclusive end.
> 
> So maybe it would be better to do something like:
> 
>         /*
>          * Blurb about end address being == tom2, perhaps give your example
>          */
>         end--;
> 
> above the check so that it is absolutely obvious why this is done.
> 
> But but, looking at this more, the PPR says about TOM2:
> 
> "This value is normally placed above 4G. From 4G to TOM2 - 1 is DRAM;
> TOM2 and above is MMIO."
> 
> So the check is *actually* correct - TOM2 - 1 is DRAM so you need '<'.
> 
> Unless you do end-- before, which would make sense and suggest the end
> decrement to be the proper fix.
> 
> Hmm?
> 

I think your comment makes sense, "TOM2 - 1" is the last DRAM address, we 
should make the end be included in the DRAM range, and the condition check 
becomes more precise to use "<", even the both codes are with the same
result.

> > Fixes: b73522e0c1be ("x86/mm/mtrr: Enhance MTRR checks in kernel mapping
> > helpers")
> 
> I think you mean:
> 
> 35605a1027ac ("x86: enable PAT for amd k8 and fam10h")
> 
> which first added that check.
> 
> Btw, while doing git archeology, I saw that mtrr_type_lookup() used to do end--
> on function entry, see
> 
> 2e5d9c857d4e ("x86: PAT infrastructure patch")
> 

With your above comments, the bug should come from
0cc705f56e40 ("x86/mm/mtrr: Clean up mtrr_type_lookup()"

Before this commit, the "end--" is in __mtrr_type_lookup(), and the end 
address is not used in mtrr_type_lookup, so the condition check is 
correct.

After the commit, the end address is started to be used in 
mtrr_type_lookup(), but the "end--" is in the mtrr_type_lookup_variable(), 
it makes the check mtrr_type_lookup() become incorrect.

> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cying-tsun.huang%40amd.com%7C2e57de535baf40ff480f08d89dc2e51d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432807709717408%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tM5ww%2F96JirsAsDZyImHHtsR3L8TQyr%2B5xWeaGbdwH4%3D&amp;reserved=0
>