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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 09.12.25 10:59, Jan Beulich wrote:
> On 08.12.2025 20:21, Grygorii Strashko wrote:
> > On 08.12.25 14:44, Jan Beulich wrote:
> > > On 28.11.2025 16:22, Grygorii Strashko wrote:
> > > > --- 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 is the crucial point: It being executed only once isn't spelled out
> anywhere in the description, and it's not entirely obvious why that would
> be. Yes, nowadays it is. Originally (iirc) one could switch the guest back
> to 64-bit mode, then again to 32-bit.
I'll update description.

Or can add it back as !d->arch.physaddr_bitsize to be safe and avoid
confusions?

> > > ... 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;
> This observation isn't directly related here, is it?
Yep. Just found it while investigated.

--
Best regards,
-grygorii