Re: [PATCH v2] x86/domain: adjust limitation on shared_info allocation below 4G

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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é <roger.pau@xxxxxxxxxx>
> > ---
> >  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.