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

[Dave Hansen <dave.hansen@intel.com>]

On 10/2/25 07:31, Brendan Jackman wrote:
> On Wed Oct 1, 2025 at 8:50 PM UTC, Dave Hansen wrote:
...
>> But let's say someone is doing something silly like:
>>
>> 	set_memory_np(addr, size);
>> 	set_memory_p(addr, size);
>>
>> Won't that end up in here and make the "unrestricted PGD" have
>> _PAGE_PRESENT==1 entries?
> 
> Er, yes, that's a bug, thanks for pointing this out. I guess this is
> actually broken under debug_pagealloc or something? I should check that.
> 
> This code should only mirror the bits that are irrelevant to ASI. 

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.

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