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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan