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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/01/2025 9:15 am, Jan Beulich wrote:
> On 31.12.2024 19:27, 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
>> does not work on 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>
> As iirc Andrew had indicated when suggesting this, what you say for PPC is
> true for x86 as well. Yet still we get around with unsigned int. Hence I
> think the change needs describing differently.

x86 is currently unsigned int, and with some reasonably expensive
packing/unpacking under the hood.

x86 ought to become unsigned long, now we don't have the 32bit build to
worry about.  (back of the envelope calculation says up/down: 400/-3868
(-3468) but I've not checked that this boots).

>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -70,6 +70,10 @@
>>  #include <xen/perfc.h>
>>  #include <public/memory.h>
>>  
>> +#ifndef CONFIG_HAS_PTE_ATTR_T
>> +typedef unsigned int pte_attr_t;
>> +#endif
> This imo makes the Kconfig control a misnomer: We'll always have pte_attr_t.
> I wonder whether this actually needs a Kconfig setting in the first place:
> It's hardly the end of the world for all architectures to specify the type
> (later: the underlying type, for the real type to become type-safe)
> explicitly.

All architectures strictly need this.  It's not optional, so doesn't
warrant a Kconfig option.

Either, arch/mm.h is required to provide the typedef, or we could have a
common one as so:

#ifndef pte_attr_t
typedef unsigned int pte_attr_t;
#endif

which architectures can influence with:

typedef unsigned long pte_attr_t;
#define pte_attr_t pte_attr_t

in the usual way.


One thing however does occur to me.  ARM and RISC-V have systems with
MPU protections rather than MMU, with Xen already starting to support
this on ARM.

Therefore we might want to reconsider the name pte_attr_t to be
something slightly less pagetable specific.  Then again, I'm not sure
how much overlap there is between the map* functions and MPUs, where
mapping is of course not possible.


Finally, this wants to be at least 2 patches.  One introducing
pte_attr_t, and one changing PPC to be unsigned long.

~Andrew