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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> ... 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?

Jan