Re: [PATCH v6 3/3] x86/mm: add set_direct_map_ro_noflush()

[David Hildenbrand (Arm)]

On 8/25/26 18:18, Dave Hansen wrote:
> On 7/30/26 02:06, Xueyuan Chen wrote:
>> +int set_direct_map_ro_noflush(const void *addr, unsigned long nr_pages)
>> +{
>> +	unsigned long tempaddr = (unsigned long)addr;
>> +	struct cpa_data cpa = {
>> +		.vaddr = &tempaddr,
>> +		.pgd = NULL,
>> +		.numpages = nr_pages,
>> +		.mask_set = __pgprot(0),
>> +		.mask_clr = __pgprot(_PAGE_RW | _PAGE_DIRTY),
>> +		.flags = CPA_NO_CHECK_ALIAS,
>> +	};
>> +
>> +	return __change_page_attr_set_clr(&cpa, 1);
>> +}
> 
> A couple of concerns here.
> 
> First, why the "_noflush"? Sure, the "this is a best effort hardening"
> function argument can be made, so it doesn't need to be correct. But the
> result is a function that's called once and also has some sharp corners
> and relatively high potential for misuse. Let's just do the flush.
> 
> Second, I see that the other set_direct_map*() callers use
> CPA_NO_CHECK_ALIAS. The reasoning behind it dates back to 2008 and I'm
> not 100% sure what it is referring to. On one hand, it would be nice to
> have all the set_direct_map*() callers be consistent. On the other hand,
> there shouldn't *be* any aliases of a 2M page that came out of the page
> allocator. We almost want a CPA_ASSERT_NO_ALIASES that goes out and
> checks for aliases more than we want to ignore them. (Note: I don't
> expect you to fix this, but a simple comment saying that no aliases are
> expected would be nice)
> 
> Third, what's with the 'tempaddr'? Are you working around the 'const'?
> Honestly, I'd rather have no const than have it and subvert it with
> casting trickery.
> 
> Last:
> 
> int set_direct_map_invalid_noflush(struct page *page)
> int set_direct_map_default_noflush(struct page *page)
> int set_direct_map_valid_noflush(struct page *page, unsigned nr, ...
> int set_direct_map_ro_noflush(const void *addr, unsigned long nr_pages)
> 
> Which one of these things is not like the other, despite being named
> just like them?
> 

That's called out in the cover letter:

"
This series adds set_direct_map_ro_noflush() so mm code can make a
direct-map range read-only, then uses it for the persistent huge zero
folio. The helper is direct-map specific, takes an address-based range as
discussed for set_direct_map* helpers[2], and leaves TLB invalidation to
the caller.
"

and patch #1

"
Use an address-based signature to match ongoing direct-map
helper work[2], where existing page-based helpers may move the same way.
The helper is direct-map specific and leaves TLB invalidation to its
caller. Architectures without direct-map permission support keep existing
behavior through the generic stub.
"

Currently it looks like this series would go in first, though, so it would be
better to keep the existing style.

-- 
Cheers,

David