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&data=04%7C01%7Cying-tsun.huang%40amd.com%7C2e57de535baf40ff480f08d89dc2e51d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432807709717408%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tM5ww%2F96JirsAsDZyImHHtsR3L8TQyr%2B5xWeaGbdwH4%3D&reserved=0
>
