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
