Messages in this thread | | | Date | Wed, 24 Jan 2024 15:24:10 -0800 | Subject | Re: [repost PATCH v2] x86/mm/ident_map: Use gbpages only where full GB page should be mapped. | From | Dave Hansen <> |
| |
On 1/24/24 09:36, Steve Wahl wrote: > Instead of using gbpages for all memory regions, which can include > vast areas outside what's actually been requested, use them only when > map creation requests include the full GB page of space; descend to > using smaller 2M pages when only portions of a GB page are included in > the request.
This kinda jumps immediately to the solution without stating the problem.
The problem is something like this:
Right now, ident_pud_init() will use 1GB pages to map an area as long as 1G pages are supported. It does not consider the size of the area being mapped. Mapping 1G? Use a 1GB mapping. Mapping 4k? Also use a 1GB mapping. On UV systems, this ends up mapping BIOS-reserved regions that will cause hardware to halt the system if accessed, even speculatively.
Right?
> + /* > + * To be eligible to use a gbpage: > + * - gbpages must be enabled > + * - addr must be gb aligned (start of region) > + * - next must be gb aligned (end of region) > + * - PUD must be empty (nothing already mapped in this region) > + */
.. this also needs to say _why_. As it stands, it kinda just rewrites the code in English which isn't super helpful. It's also awfully awkward to write a multi-line comment above a multi-line if().
Why not refactor it to do something like: bool can_use_gbpages = info->direct_gbpages;
/* Avoid using a gbpage when it would be too large: */ can_use_gbpages &= (addr & ~PUD_MASK) || (next & ~PUD_MASK);
/* Never overwrite existing mappings: */ can_use_gbpages &= !pud_present(*pud);
if (can_use_gbpages) { ...
| |