Re: [PATCH 05/21] x86/mm/pat: mirror direct map changes to ASI

[Brendan Jackman <jackmanb@google.com>]

On Thu Oct 2, 2025 at 4:40 PM UTC, Dave Hansen wrote:
> On 10/2/25 07:31, Brendan Jackman wrote:
> It's actually anything that has _PAGE_PRESENT in cpa->mask_set. There
> are a number of those. Some of them are irrelevant like the execmem
> code, but there are quite a few more that look troublesome outside of
> debugging environments.
>
>>> Also, could we try and make the nomenclature consistent? We've got
>>> "unrestricted direct map" and "asi_nonsensitive_pgd" being used (at
>>> least). Could the terminology be made more consistent?
>> 
>> Hm. It is actually consistent: "unrestricted" is a property of the
>> address space / execution context. "nonsensitive" is a property of the
>> memory. Nonsensitive memory is mapped into the unrestricted address
>> space. asi_nonsensitive_pgd isn't an address space we enter it's just a
>> holding area (like if we never actually pointed CR3 at init_mm.pgd but
>> just useed it as a source to clone from).
>> 
>> However.. just because it's consistent doesn't mean it's not confusing.
>> Do you think we should just squash these two words and call the whole
>> thing "nonsensitive"? I don't know if "nonsensitive address space" makes
>> much sense... Is it possible I can fix this by just adding more
>> comments?
>
> It makes sense to me that a "nonsensitive address space" would not map
> any sensitive data and that a "asi_nonsensitive_pgd" is the root of that
> address space.

OK, then it probably just sounds wrong to me because I'm steeped in the
current jargon. For v2 I'll try just dropping "[un]restricted".

>>>>  static int __change_page_attr_set_clr(struct cpa_data *cpa, int primary)
>>>>  {
>>>>  	unsigned long numpages = cpa->numpages;
>>>> @@ -2007,6 +2033,8 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int primary)
>>>>  		if (!debug_pagealloc_enabled())
>>>>  			spin_lock(&cpa_lock);
>>>>  		ret = __change_page_attr(cpa, primary);
>>>> +		if (!ret)
>>>> +			ret = mirror_asi_direct_map(cpa, primary);
>>>>  		if (!debug_pagealloc_enabled())
>>>>  			spin_unlock(&cpa_lock);
>>>>  		if (ret)
>>>>
>>>
>>> Is cpa->pgd ever have any values other than NULL or init_mm->pgd? I
>>> didn't see anything in a quick grep.
>> 
>> It can also be efi_mm.pgd via sev_es_efi_map_ghcbs_cas().
>
> It would be _nice_ if the ASI exclusion wasn't so magic.
>
> Like, instead of hooking in to __change_page_attr_set_clr() and
> filtering on init_mm if we had the callers declare explicitly whether
> their changes get reflected into the ASI nonsensitive PGD.
>
> Maybe that looks like a new flag: CPA_DIRECT_MAP or something. Once you
> pass that flag in, the cpa code knows that you're working on init_mm.pgd
> and mirror_asi_direct_map() can look for *that* instead of init_mm.

Sounds good to me. If "The Direct Map" is a gonna be a special thing
then having it be a flag instead of a certain magic pgd_t * makes sense
to me. I'll try this out.