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 04:44:16PM +0200, Orzel, Michal wrote:
> 
> 
> On 05-May-26 15:00, Jan Beulich wrote:
> > On 05.05.2026 13:46, 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.
> > 
> > And as also said before, this really is poor man's offset compression then. That
> > may be tolerable if you insist that's best for Arm, yet then I'd suggest to limit
> > that offset to just the "no compression" case. It's redundant with offset
> > compression, and it may be (possible to make) redundant with mask compression.
> > If the latter can't be arranged for, an offset may want introducing there as well.
> > But it shouldn't exist independent of the compression scheme used.
> Having a single per-scheme mechanism rather than an extra independent offset is
> cleaner. But I don't think we can limit frametable_base_pdx to PDX_NONE today:
> 
>  - Mask compression doesn't fold a leading [0, first_ram_pdx) zero
>    prefix into anything. So the PDX of
>    the first RAM frame stays at first_ram_pdx, and without the offset
>    the frame table virtual extent is max_pdx * sizeof(page_info)
>    rather than (max_pdx - first_ram_pdx) * sizeof(page_info).
> 
>    For systems with a high RAM base (the 5TB example I gave earlier
>    needs ~70GB just to skip the leading hole, vs. 32GB FRAMETABLE_SIZE
>    on arm64) the (max_pdx > FRAMETABLE_NR) check then fails and we
>    panic before mapping anything. pdx_group_valid (which patch 2/2
>    adds) avoids backing those leading groups with physical memory, but
>    it doesn't shrink the virtual extent — only the offset does.
> 
>  - With offset compression you're right that the leading hole could be
>    absorbed into the lookup table, making the extra offset redundant.
>    But Arm doesn't currently select offset compression, it's non-selectable,
> untested and switching
>    the default is a separate (and bigger) discussion that I don't think
>    should block this fix given the state of the release.
> 
> So as it stands, the offset is needed on Arm for both PDX_NONE and
> PDX_MASK_COMPRESSION. Folding it into the mask scheme (and dropping it
> for offset compression) is a reasonable cleanup, but it's a refactor
> of the compression layer itself, not something I'd like to mix into
> this series.

Right, then it's likely best to avoid generalizing frametable_base_pdx
and instead focus on integrating it with the PDX mask compression
algorithm at a possibly later time (after release)?

Thanks, Roger.