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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 05-May-26 14:15, Roger Pau Monné wrote:
> 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).
I'd argue the PDX <-> PFN mapping itself is still identity here — with
PDX_NONE, pfn_to_pdx(x) == x and pdx_to_pfn(x) == x. frametable_base_pdx
is not a PDX-space transformation; it's an offset that's only applied
when *indexing into the frame table*

Conceptually it's closer to "skip the leading hole in the lookup arrays"
than to a compression of the PFN/PDX number space. It also sits on top
of (and is orthogonal to) whatever PDX scheme is selected.

> 
> It's an incomplete conversion IMO, as ARM applies it to the
> frametable, but not the direct map.
It is applied to the directmap on arm64 — see directmap_base_pdx.

So both the frametable and (on arm64) the directmap apply the
equivalent offset. They use separate variables (frametable_base_pdx vs.
directmap_base_pdx) but the principle is the same.

> 
> Thanks, Roger.

~Michal