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@citrix.com>]

On Tue, May 05, 2026 at 01:46:51PM +0200, Orzel, Michal wrote:
> 
> 
> On 05-May-26 12:49, Jan Beulich wrote:
> > On 05.05.2026 12:46, Orzel, Michal wrote:
> >> On 05-May-26 12:40, Jan Beulich wrote:
> >>> On 05.05.2026 09:35, 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.
> >>>
> >>> Yet still - this is exactly one of the situations offset compression means
> >>> to cover. I'm entirely with Roger as to it being undesirable to build a
> >>> special case variant of "offset compression" into "no compression".
> >> In this case, if you don't want to generalize the macros, how should we proceed
> >> on Arm if we still need the offset to cover the PDX_NONE variant that we also
> >> use? In v1 I just created a local override but Julien wanted to generalize the
> >> macros instead. The discussion about switching the default on Arm from mask to
> >> offset that is not even selectable on Arm needs to wait for the new release cycle.
> > 
> > I'm not convinced of that. If you need offset by default, why not enable it by
> > default (right now, and potentially even as a backport if there's any bug that
> > is being fixed)?
> As said before, we also need offset when using just PDX grouping and no compression.

But you don't really mean no compression?  The offset itself that you
subtract is a transformation, and hence a compression, as the physical
and PDX address spaces are no longer identity mapped?  Maybe those
systems should have never worked with PDX_NONE, and instead required
a PDX compression in place (one that would remove the offset from 0 to
the first RAM range).

It's an incomplete conversion IMO, as ARM applies it to the
frametable, but not the direct map.

Thanks, Roger.