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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

I'd expect MC/DC coverage to complain at leaving the conditional in
place, not that this particular codepath is going to be on the top of
the list for coverage.

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

Don't be deceived by that.  pv's dom0_construct() has

    HYPERVISOR_COMPAT_VIRT_START(d) =
        max_t(unsigned int, m2p_compat_vstart, value);

which hides the write.  I've been meaning to fix this for ages.

In fact, I could also submit the inlining of domain_set_alloc_bitsize()
too with relevant history if you'd prefer.

~Andrew