Re: [PATCH v2 1/8] x86/pdx: simplify calculation of domain struct allocation boundary

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 24, 2025 at 03:05:11PM +0200, Jan Beulich wrote:
> On 20.06.2025 13:11, Roger Pau Monne wrote:
> > When not using CONFIG_BIGMEM there are some restrictions in the address
> > width for allocations of the domain structure, as it's PDX truncated to 32
> > bits it's stashed into page_info structure for domain allocated pages.
> > 
> > The current logic to calculate this limit is based on the internals of the
> > PDX compression used, which is not strictly required.  Instead simplify the
> > logic to rely on the existing PDX to PFN conversion helpers used elsewhere.
> > 
> > This has the added benefit of allowing alternative PDX compression
> > algorithms to be implemented without requiring to change the calculation of
> > the domain structure allocation boundary.
> > 
> > As a side effect introduce pdx_to_paddr() conversion macro and use it.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> > @@ -498,14 +474,20 @@ struct domain *alloc_domain_struct(void)
> >       * On systems with CONFIG_BIGMEM there's no packing, and so there's no
> >       * such restriction.
> >       */
> > -#if defined(CONFIG_BIGMEM) || !defined(CONFIG_PDX_COMPRESSION)
> > -    const unsigned int bits = IS_ENABLED(CONFIG_BIGMEM) ? 0 :
> > -                                                          32 + PAGE_SHIFT;
> > +#if defined(CONFIG_BIGMEM)
> > +    const unsigned int bits = 0;
> >  #else
> > -    static unsigned int __read_mostly bits;
> > +    static unsigned int __ro_after_init bits;
> >  
> >      if ( unlikely(!bits) )
> > -         bits = _domain_struct_bits();
> > +         /*
> > +          * Get the width for the next pfn, and unconditionally subtract 
> > one
> > +          * from it to ensure the used width will not allocate past the PDX
> > +          * field limit.
> > +          */
> > +         bits = flsl(pdx_to_paddr(1UL << (sizeof_field(struct page_info,
> > +                                                       v.inuse._domain) * 
> > 8)))
> 
> You didn't like the slightly shorter sizeof(frame_table->v.inuse._domain) 
> then?

No strong opinion really, I have the impression however that using the
struct type itself would be less fragile, in case we ever change
frame_table variable name (which is very unlikely).

Roger.