Re: [PATCH v3 3/3] xen/x86: move d->arch.physaddr_bitsize field handling to pv32

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 08.12.25 14:44, Jan Beulich wrote:
> On 28.11.2025 16:22, Grygorii Strashko wrote:
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -619,9 +619,12 @@ void __iomem *ioremap_wc(paddr_t pa, size_t len);
> >   
> >   extern int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm);
> >   
> > -void domain_set_alloc_bitsize(struct domain *d);
> > -unsigned int domain_clamp_alloc_bitsize(struct domain *d, unsigned int bits);
> > -#define domain_clamp_alloc_bitsize(d, bits) domain_clamp_alloc_bitsize(d, bits)
> > +#ifdef CONFIG_PV32
> > +#define domain_clamp_alloc_bitsize(d, bits)                                    
> > \
> > +    (((d) && (d)->arch.pv.physaddr_bitsize)                                    
> > \
> > +         ? min_t(uint32_t, (d)->arch.pv.physaddr_bitsize, (bits))              
> > \
> > +         : (bits))
> I'm not quite sure if converting to a macro was a good idea. But now that it's
> done this way, so be it. Just that a couple of issues may want / need sorting:
> - d is now evaluated up to 3 times,
> - indentation is wrong,
> - the use of uint32_t is against ./CODING_STYLE (I dislike the use of min_t()
>    anyway, but what do you do when a macro was asked for; of course we could
>    [easily] arrange for BITS_PER_LONG to be of type "unsigned int"),
> - the use of "bits" in min_t() doesn't really need parentheses.
I'll update it.

> > --- a/xen/arch/x86/pv/domain.c
> > +++ b/xen/arch/x86/pv/domain.c
> > @@ -254,7 +254,11 @@ int switch_compat(struct domain *d)
> >               goto undo_and_fail;
> >       }
> >   
> > -    domain_set_alloc_bitsize(d);
> > +    if ( MACH2PHYS_COMPAT_NR_ENTRIES(d) < max_page )
> You mention the change in condition in the revlog (but not in the description),
The updated chunk was based on snippet from Andrew [1], which
used incorrect condition - I've changed it and noted in change log

[1] https://patchwork.kernel.org/comment/26680551/

> and I'm having trouble to follow why ...
>
> > --- a/xen/arch/x86/x86_64/mm.c
> > +++ b/xen/arch/x86/x86_64/mm.c
> > @@ -1119,26 +1119,6 @@ unmap:
> >       return ret;
> >   }
> >   
> > -void domain_set_alloc_bitsize(struct domain *d)
> > -{
The domain_set_alloc_bitsize() inlined in  switch_compat() and x86 PV domain
always created as 64bit domain.

At the beginning of switch_compat() there is:

 ( is_pv_32bit_domain(d) )
        return 0;
[2]
above ensures that switch_compat() can be actually called only once (at least 
it can reach
point [2] only once, because there is no way to reset PV domain state 
32bit->64bit

this is original condition which says:
> > -    if ( !is_pv_32bit_domain(d) ||
do nothing if !is_pv_32bit_domain(d)
 - for inlined code is_pv_32bit_domain(d) == true, so is_pv_32bit_domain(d) can 
be ignored

> > -         (MACH2PHYS_COMPAT_NR_ENTRIES(d) >= max_page) ||
do nothing if (MACH2PHYS_COMPAT_NR_ENTRIES(d) >= max_page)
  - inlinded code should proceed if this condition is opposite
    (MACH2PHYS_COMPAT_NR_ENTRIES(d) < max_page)

> > -         d->arch.physaddr_bitsize > 0 )
do nothing if d->arch.physaddr_bitsize > 0 (already set)
  - inlined code will be executed only once, so (d->arch.physaddr_bitsize ==/!= 
0)
    can be ignored

> ... this 3rd part is going away.
Another observation: MACH2PHYS_COMPAT_NR_ENTRIES(d) is looks like a const, as
(d)->arch.hv_compat_vstart is always 0.

grep -rw hv_compat_vstart ./*
./arch/x86/include/asm/config.h:#define HYPERVISOR_COMPAT_VIRT_START(d) 
((d)->arch.hv_compat_vstart)
./arch/x86/include/asm/domain.h:    unsigned int hv_compat_vstart;


--
Best regards,
-grygorii