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 18:11, Jan Beulich wrote:
> On 05.05.2026 16:52, Orzel, Michal wrote:
>>
>>
>> On 05-May-26 15:05, Jan Beulich wrote:
>>> On 30.04.2026 14:51, Michal Orzel wrote:
>>>> --- a/xen/include/xen/pdx.h
>>>> +++ b/xen/include/xen/pdx.h
>>>> @@ -132,8 +132,9 @@ void set_pdx_range(unsigned long smfn, unsigned long 
>>>> emfn);
>>>>   */
>>>>  bool __mfn_valid(unsigned long mfn);
>>>>  
>>>> -#define page_to_pdx(pg)  ((pg) - frame_table)
>>>> -#define pdx_to_page(pdx) gcc11_wrap(frame_table + (pdx))
>>>> +#define page_to_pdx(pg) \
>>>> +    ((unsigned long)((pg) - frame_table) + frametable_base_pdx)
>>>> +#define pdx_to_page(pdx) gcc11_wrap(frame_table + ((pdx) - 
>>>> frametable_base_pdx))
>>>
>>> If you alter these, ...
>>>
>>>>  #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
>>>>  #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
>>>
>>> ... how come these can remain unaltered? Maybe you have some special
>>> arrangements in Arm code, but surely in generic code transformations done
>>> should be uniform. After all
>>>
>>>     ASSERT(page_to_pdx(pg) == mfn_to_pdx(page_to_mfn(pg)));
>>>
>>> (and alike) ought to be universally true for valid inputs.
>> The invariant holds. There are two transformations on different
>> boundaries:
>>
>>   - PFN <-> PDX: the compression scheme — lives in mfn_to_pdx /
>>     pdx_to_mfn.
>>   - PDX <-> frame-table index: +/- frametable_base_pdx — lives in
>>     page_to_pdx / pdx_to_page (and Arm's page_to_mfn / mfn_to_page).
>>
>> On x86 the second is the identity (frametable_base_pdx == 0), so it's
>> invisible. On Arm it isn't, so it has to appear in the macros that
>> cross that boundary. Pushing it into mfn_to_pdx as well would mix the
>> two boundaries and double-apply on Arm (page_to_mfn already adds it).
> 
> That's yet more odd. These transformations should equally apply to
> MFN <-> page (i.e. frame table index) and MFN <-> PDX translations.
> PDX really is meant to be the frame table index, and at the same
> time (scaled by PAGE_SHIFT) the direct map index. Both (generally
> huge) tables equally benefit from whatever compression is in use,
> and hence also ought to equally benefit from that
> frametable_base_pdx-only sub-form of offset compression. The
> anomaly of shrinking only one of the two pretty clearly shouldn't
> be extended past Arm, and ideally would be addressed there at some
> point.
Fair enough. I'll drop this patch and update the second with a local change (as
in v1 but this time with a comment explaining why) not to extend this behavior
past Arm and we can try to make things generic next release.

~Michal