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
