Re: [PATCH] xen/mm: Update page APIs to use unsigned long flags

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

Hi Andrew,

On 12/20/24 12:23 PM, Andrew Cooper wrote:
> On 20/12/2024 5:53 pm, 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 update
>> these APIs to use unsigned long for flags instead.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> 
> Funnily enough, the same is true on x86 too.  NX and ProtKey bits are
> from bit 63 downwards.
> 
> x86 funnels all PTE flags through {get,put}_pte_flags() which compresses
> a 64bit PTE's flags (top and bottom 12 bits) into a 24 bit number.
> 
> This was allegedly for the benefit of 32bit builds of Xen with PAE
> paging.  I'm not convinced this claim was backed up evidence, even in
> the 32bit days, because the areas of code working on flags separately
> from a 64bit PTE are minimal.
>
> Nevertheless, it's been 11 years since we dropped x86_32, and it's
> definitely tech debt and unnecessary overhead in x86_64.

Interesting -- I wasn't aware that x86 was running into this issue too,
and that approach to solving it definitely seems a bit dubious from an
overhead standpoint.

> I firmly support making this adjustment.  It's been on my todo list for
> years, but never high enough to get started.
> 
> However, instead of just a plain unsigned long, perhaps we should have a
> concrete type, perhaps pte_attr_t ?
> 
> This lets different architectures handle things differently, and also
> lets us make it TYPE_SAFE() like mfn and friends.
>

I fully agree that introducing a TYPE_SAFE per-arch type for this is the
more robust solution. I went with this approach as it requires the least
invasive changes to other architectures, but if there's sufficient
buy-in from everyone then I think that this would be the better route.

One other consideration with that approach would be the ergonomics of
using the TYPE_SAFE macros for these flags which are often OR'd
together.  I know mfn addresses this by offering mfn_{add,max,min,eq}
helper functions, so introducing similar bitwise helpers for the new
pte_attr_t type could work. typesafe.h could even be expanded to include
macros to generate these helper functions for numeric types to reduce
duplication if you think that'd be reasonable.

> Most importantly though, it makes it less likely that we're going to end
> up with paths that accidentally have some long->int truncation.

In this patch some of those paths are explicitly present, for example
in arm's pt.c. The thinking was that these architectures already
obviously have no issue with 32-bit pte flags, so the truncation won't
cause any issues, but I agree it's not ideal. At the very least, it
presents a potential pitfall if architectures like x86 transition to
using the full 64-bit field in the future.