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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/12/2024 6:51 pm, Shawn Anastasio wrote:
> 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.

It is history and inertia most likely.

Original paging in x86 had 32bit PTEs with attributes in the low 12 bits
only.

PSE and PSE36 paging were next but not interesting in this context.

PAE paging changed to 64bit PTEs, and the NX bit was added in bit 63. 
This is where the upper 12 bits got introduced.

However, using uint64_t's in 32bit code is somewhat unergonomic and Xen
had a bunch of optimisations to use 32bit frames (== 44 bits addressable
memory).  To this day, we've still got this limitation with a __pdx_t
which dictates the size of the frametable (and CONFIG_BIGMEM to switch
to something larger).

x86_64 has a hard dependency on PAE paging, and added other bits into
the upper 12 range, but at least now a PTE isn't larger than the GPR size.

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

I was thinking of TYPE_SAFE() as a second step.  I'm certainly not
asking you to do make it happen in this patch/series.

But yes, the ergonomics aren't completely great given how many bit
operations we do.


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

Ok, in which case you want to start by introducing a per-arch typedef
defaulting to unsigned int.

Then rearrange the code necessary for PPC, and then change PPC to use
unsigned long.

This results no changes to other architectures until they take explicit
action to switch fully over to the new typedef.

~Andrew