Re: [PATCH v2] xen/mm: Introduce per-arch pte_attr_t type for PTE flags

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

Hi Jan,

On 2/6/25 6:29 AM, Jan Beulich wrote:
> On 05.02.2025 22:02, Shawn Anastasio wrote:
>> Xen's memory management APIs map_pages_to_xen, modify_xen_mappings,
>> set_fixmap, ioremap_attr, and __vmap all use an unsigned int to
>> represent architecture-dependent page table entry flags. This assumption
>> is not well-suited for PPC/radix where some flags go past 32-bits, so
>> introduce the pte_attr_t type to allow architectures to opt in to larger
>> types to store PTE flags.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> Changes in v2:
>>   - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to
>>   opt-in to defining the type.
>>   - Move default pte_attr_definition to xen/types.h
> 
> I'm unconvinced of types.h being an appropriate place for something mm-
> related. mm-types.h maybe?
> 

This sounds reasonable (and re your follow-up -- for now the file should
only need to be included in xen/vmap.h, xen/mm.h and so on. Definitely
no need to include it in types.h).

>>   - Update commit message to reflect that this change isn't strictly
>>   necessary for arches w/ >32bit pte flags
> 
> I can't seem to be able to associate this with anything in the commit
> message. The comment here to me reads as if this was optional (but then
> for arches with <=32-bit PTE flags), yet in the description I can't spot
> anything to the same effect. Recall that it was said before that on x86
> we also have flags extending beyond bit 31, just that we pass them
> around in a compacted manner (which, as Andrew has been suggesting, may
> be undue extra overhead).
>

Admittedly the change was subtle, but I changed the wording in the
commit message as follows:

- This assumption does not work on PPC/radix where some flags go past
  32-bits, so [...]
+ This assumption is not well-suited for PPC/radix where some flags go
  past 32-bits, so [...]


The softening of "does not work" to "is not well-suited" was meant to
address your earlier comment and clarify that the change is not strictly
necessary. Though as you and Andrew pointed out x86_64 is able to make
do with the 32 bits, I would still argue that the hardcoded `unsigned
int` flags type is still not well-suited to that application.

> Jan

Thanks,
Shawn