On Wed, Feb 04, 2026 at 03:20:09PM +0100, Andrew Cooper wrote:
> On 04/02/2026 12:25 pm, Roger Pau Monne wrote:
> > The limitation of shared_info being allocated below 4G to fit in the
> > start_info field only applies to 32bit PV guests. On 64bit PV guests the
> > start_info field is 64bits wide. HVM guests don't use start_info at all.
>
> All shared info? HVM does use it, but doesn't see the MFN.
HVM guest use shared_info, but not start_info. The issue is not with
shared_info itself, but the size of the field used to store
shared_info in start_info.
HVM doesn't use start_info, and hence doesn't care about the position
of shared_info in that regard.
> >
> > Drop the restriction in arch_domain_create() and instead free and
> > re-allocate the page from memory below 4G if needed in switch_compat(),
> > when the guest is set to run in 32bit PV mode.
> >
> > Fixes: 3cadc0469d5c ("x86_64: shared_info must be allocated below 4GB as it
> > is advertised to 32-bit guests via a 32-bit machine address field in
> > start_info.")
> > Signed-off-by: Roger Pau Monné <[email protected]>
> > ---
> > xen/arch/x86/domain.c | 7 ++++---
> > xen/arch/x86/pv/domain.c | 20 ++++++++++++++++++++
> > xen/common/domain.c | 2 +-
> > xen/include/xen/domain.h | 2 ++
> > 4 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index edb76366b596..3e701f2146c9 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -882,10 +882,11 @@ int arch_domain_create(struct domain *d,
> > goto fail;
> >
> > /*
> > - * The shared_info machine address must fit in a 32-bit field within a
> > - * 32-bit guest's start_info structure. Hence we specify MEMF_bits(32).
> > + * For 32bit PV guests the shared_info machine address must fit in a
> > 32-bit
> > + * field within the guest's start_info structure. We might need to
> > free
> > + * and allocate later if the guest turns out to be a 32bit PV one.
> > */
> > - if ( (d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32))) == NULL )
> > + if ( (d->shared_info = alloc_xenheap_page()) == NULL )
> > goto fail;
> >
>
> The comment is now out of place when the source is read naturally. I'd
> suggest dropping the comment entirely, and ...
>
> > clear_page(d->shared_info);
> > diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> > index 01499582d2d6..8ced3d70a52f 100644
> > --- a/xen/arch/x86/pv/domain.c
> > +++ b/xen/arch/x86/pv/domain.c
> > @@ -247,6 +247,26 @@ int switch_compat(struct domain *d)
> > d->arch.has_32bit_shinfo = 1;
> > d->arch.pv.is_32bit = true;
> >
> > + /* Check whether the shared_info page needs to be moved below 4G. */
>
> ... extending this one talking about the 32bit field.
Hm, yes, I've considered doing that. Unless Jan objects I will move
the comment then, seeing as you also think it's best.
> > + if ( virt_to_maddr(d->shared_info) >> 32 )
> > + {
> > + shared_info_t *prev = d->shared_info;
> > +
> > + d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
> > + if ( !d->shared_info )
> > + {
> > + d->shared_info = prev;
> > + rc = -ENOMEM;
> > + goto undo_and_fail;
> > + }
> > + put_page(virt_to_page(prev));
> > + clear_page(d->shared_info);
>
> I think copy_page() would be more appropriate. That way there are fewer
> implicit ordering dependencies.
Hm, I had a copy_page() initially, but I don't think it's appropriate
because (most of?) the fields will need translation. I don't think
translation is feasible, but I could at least call
update_domain_wallclock_time() which is what hvm_latch_shinfo_size()
does when changing bitness.
> > + share_xen_page_with_guest(virt_to_page(d->shared_info), d,
> > SHARE_rw);
> > + /* Ensure all references to the old shared_info page are dropped.
> > */
> > + for_each_vcpu( d, v )
> > + vcpu_info_reset(v);
>
> switch_compat() can only occur on a domain with no memory. How can we
> have outstanding references?
As Jan pointed out, it's not references, but stashed pointers to the
previous shared_info page. I've used the wrong wording here.
Thanks, Roger.
