Re: [PATCH v2 2/5] mm: Factor out the pdx compression logic in ma/va converters

[Alejandro Vallejo <alejandro.vallejo@cloud.com>]

On Mon, Jul 31, 2023 at 05:15:19PM +0200, Jan Beulich wrote:
> On 28.07.2023 09:59, Alejandro Vallejo wrote:
> > --- a/xen/include/xen/pdx.h
> > +++ b/xen/include/xen/pdx.h
> > @@ -160,6 +160,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >  #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
> >  #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
> >  
> > +/**
> > + * Computes the offset into the direct map of an maddr
> > + *
> > + * @param ma Machine address
> > + * @return Offset on the direct map where that
> > + *         machine address can be accessed
> > + */
> > +static inline unsigned long maddr_to_directmapoff(uint64_t ma)
> 
> Was there prior agreement to use uint64_t here and ...
> 
> > +{
> > +    return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) |
> > +           (ma & ma_va_bottom_mask);
> > +}
> > +
> > +/**
> > + * Computes a machine address given a direct map offset
> > + *
> > + * @param offset Offset into the direct map
> > + * @return Corresponding machine address of that virtual location
> > + */
> > +static inline uint64_t directmapoff_to_maddr(unsigned long offset)
> 
> ... here, not paddr_t?
The whole file uses uint64_t rather than paddr_t so I added the new code
using the existing convention. I can just as well make it paddr_t, it's
not a problem.

> 
> Also you use unsigned long for the offset here, but size_t for
> maddr_to_directmapoff()'s return value in __maddr_to_virt().
> Would be nice if this was consistent within the patch.
That's fair. I can leave it as "unsigned long" (seeing that a previous nit was
preferring that type over size_t).

> Especially since the names of the helper functions are longish,
> I'm afraid I'm not fully convinced of the transformation.
In what sense? If it's just naming style I'm happy to consider other names,
but taking compression logic out of non-pdx code is essential to removing
compiling it out.

> But I'm also not meaning to stand in the way, if everyone else wants to
> move in that direction.
> 
> Jan

Thanks,
Alejandro