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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jun 12, 2025 at 11:03:21AM +0200, Jan Beulich wrote:
> On 11.06.2025 19:16, Roger Pau Monne wrote:
> > @@ -498,14 +474,15 @@ 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();
> > +         bits = flsl(pfn_to_paddr(pdx_to_pfn(
> > +             1UL << (sizeof(((struct page_info *)NULL)->v.inuse._domain) * 8))))
> > +             - 1;
> 
> While Andrew did point you at sizeof_field(), we can have this even less verbose
> by utilizing that frame_table is of the right type and (almost) globally in scope.
> 
> Further, why use pfn_to_paddr()?
> 
>          bits = flsl(pdx_to_pfn(1UL << 
>                                 (sizeof(frame_table->v.inuse._domain) * 8))) +
>                 PAGE_SHIFT - 1;

I've introduced and used pdx_to_paddr(), which I think it's more
natural.  We already had a paddr_to_pdx() which was missing it's
bidirectional equivalent.  It's now:

         bits = flsl(pdx_to_paddr(1UL << (sizeof_field(struct page_info,
                                                       v.inuse._domain) * 8)))
                - 1;

> However, it further feels like this was off by one; we had similar issues over
> time in several places. There potentially being a gap between one less than
> the PDX used here and that very PDX, don't we need to calculate based on the
> "one less" value here? Hmm, there being a gap means no allocation would
> succeed for the precise value of "bits" (in the mask-compression scheme), so
> functionally all would be fine. Yet just to avoid setting a bad precedent I
> think we'd still be better off using
> 
>          bits = flsl(pdx_to_pfn((1UL << 
>                                  (sizeof(frame_table->v.inuse._domain) * 8)) -
>                                 1)) + PAGE_SHIFT;
> 
> If one would log the value of bits, the result would then also be less
> confusing in (at least) the mask-compression scheme.


Is the above correct tough?

Take for example the hypothetical case where pdx_to_pfn() returns
0x10.  Then flsl() will return 5 (let's leave the PAGE_SHIFT
adjustment out for the example here).  The allocation bit width would
be off-by-one, because allocating using a bit width of 5 would also
allow 0x11 to be allocated, and that won't be correct.

I think we need to get the bit width of the next pdx (so the
non-inclusive end of the range), and then subtract 1 from it,
otherwise the allocation bit width is possibly off-by-one.

Thanks, Roger.