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'll update description.
Or can add it back as !d->arch.physaddr_bitsize to be safe and avoid
confusions?
... 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.
--
Best regards,
-grygorii
