On 09.12.25 14: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.
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.
Sorry, didn't get it - you wanna take ownership of the whole patch
or part of it?
In general, I'm ok.
In the later case I'll need to handle only domain_clamp_alloc_bitsize(), right?
--
Best regards,
-grygorii
