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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 1/9/25 4:56 AM, Andrew Cooper wrote:
> Either, arch/mm.h is required to provide the typedef,

This approach seems fine to me. I wanted to avoid changes to other
architectures where possible, but if this is acceptable then this is
probably the route I'd want to go down (see below).

For the reason outlined below, this would also probably mean that the
typedef would go before other includes at the top of arch/mm.h, and I'm
not sure if that would be an issue from a style perspective.
Alternatively, I could have arch/type.h define it on every architecture
if that would be acceptable.

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

There seems to be a pretty messy header dependency tree with xen/mm.h
where it #includes arch/mm.h halfway through the file after defining
(among other things) the prototypes for the functions that would need
pte_attr_t. Defining pte_attr_t this late in the file won't work since
the definition also needs to be visible to xen/vmap.h which gets
transitively included by the arch headers.

The kconfig route I went with sidesteps this by allowing the common
definition to not depend on previous header inclusions in its #ifndef
guard, but I agree with yours and Jan's comments about it not being the
best approach.

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

Without too much familiarity with ARM/RISC-V's MPU situation, at first
glance it seems to me that it would warrant a separate type, even if it
happened to coincide with the flag type used by the MMU on those arches.

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

Gotcha -- not a problem.

> ~Andrew

Thanks,
Shawn