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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2025 13:44, Andrew Cooper wrote:
> On 09/12/2025 12:24 pm, Grygorii Strashko wrote:
>> 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 changed it in 02e78311cdc6
> 
>>
>> I'll update description.
>>
>> Or can add it back as !d->arch.physaddr_bitsize to be safe and avoid
>> confusions?
> 
> Please update the description.  The function really is singleshot now.

+1

Jan