Re: [PATCH v2 1/2] xen/pdx: account for frametable_base_pdx in generic pdx_to_page/page_to_pdx

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 05, 2026 at 09:35:05AM +0200, Orzel, Michal wrote:
> 
> 
> On 05-May-26 09:13, Roger Pau Monné wrote:
> > On Tue, May 05, 2026 at 08:48:15AM +0200, Orzel, Michal wrote:
> >>
> >>
> >> On 04-May-26 17:28, Roger Pau Monné wrote:
> >>> On Thu, Apr 30, 2026 at 02:51:02PM +0200, Michal Orzel wrote:
> >>>> The generic pdx_to_page() and page_to_pdx() macros in xen/pdx.h assume
> >>>> the frame table starts at PDX 0, which is only true on x86. ARM
> >>>> uses a non-zero frametable_base_pdx to offset into the frame table (PPC 
> >>>> also
> >>>> defines it).
> >>>>
> >>>> Fix the generic macros to subtract/add frametable_base_pdx, defaulting
> >>>> to 0 when the arch does not define it. This makes the generic macros
> >>>> correct for all architectures, even though they are only used on x86
> >>>> today.
> >>>
> >>> Hm, I assume this offset was added because the original mask PDX
> >>> compression won't (usually) compress the gap between 0 and the start
> >>> of RAM.  However the newish offset PDX compression should be able to
> >>> compress from 0 to start of RAM, and hence you don't need to apply
> >>> an extra PDX offset there?
> >>>
> >>> If that's indeed the case it might be better to integrate
> >>> frametable_base_pdx into the mask compression algorithm itself, so
> >>> that on some arches it's a mask plus a decrease.
> >> The offset is needed regardless of whether compression is used. With
> >> CONFIG_PDX_NONE (no compression, PDX == MFN), if RAM starts at e.g.
> >> 0x80000000, the first valid PDX is 0x80000.
> > 
> > OK, so you are doing some (kind of) address space compression (removing
> > the leading empty range to the first RAM region) even when PDX is
> > disabled.
> > 
> >> Without frametable_base_pdx
> >> the frame table would have to be indexed from 0, wasting
> >> 0x80000 * sizeof(page_info) of memory just to cover the hole before RAM.
> > 
> > But you don't really "waste" memory, just address space?  Oh, maybe
> > not on ARM as it doesn't use pdx_group_valid?  And so you
> > unconditionally populate the frametable from PDX 0 to max PDX.
> With pdx_group_valid (which this series adds) we wouldn't waste
> physical memory for the leading gap. But we'd still waste virtual address
> space and the FRAMETABLE_NR check (max_pdx > FRAMETABLE_NR) becomes tighter
> because the full range from PDX 0 must fit. For example with RAM starting at 
> 5TB
> the virtual offset before the first usable entry would be ~70GB — more than 
> the
> entire 32GB FRAMETABLE_SIZE on ARM64.

Right, you need to use a PDX compression to fit.  My preference IMO
would be to add the leading offset into the PDX mask compression
algorithm if that's what ARM uses by default now.  The generic case
really means no compression, and that's a 1:1 map between physical
addresses and PDX.  Anything that's not an identity mapping between
those two address spaces implies some kind of compression.

> > 
> >> So frametable_base_pdx is really a frame table indexing offset, not
> >> something tied to the compression algorithm.
> > 
> > Right, it just seems odd to do that extra subtraction when using
> > offset compression, as in that case the compression logic itself
> > should remove that leading gap when RAM doesn't start at 0.
> > 
> > Instead of generalizing and expanding the usage of frametable_base_pdx
> > it might be better to implement support for pdx_group_valid when
> > populating the frame table, and switch by default to the offset
> > compression method that will already remove any leading unpopulated
> > spaces?
> Switching the compression method would be a bigger change, and with feature
> freeze on Friday I'd prefer not to get into that now. The current approach
> is minimal and self-contained and works with mask and no-pdx which is what we
> use nowadays: frametable_base_pdx already existed on ARM and PPC, we're just
> making the generic macros aware of it as Julien requested (in v1 I just
> overwrote the macro in local file). We can revisit the compression strategy 
> as a
> follow-up next release.

Right, I'm not going to oppose to this, but I also don't think it's
the right way to go.  This seems like a bodge on the side of PDX
compression, when it should have instead been integrated into it.  The
more that the offset compression logic will already do that removal of
the leading empty space up to the first RAM region, and hence such
adjustment of PDX values in that case is redundant at best.

Also, you only do this PDX adjustment for the frametable, but not the
direct map?

Thanks, Roger.