Re: [PATCH v3 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 May 2024, Julien Grall wrote:
> Hi Henry,
> 
> + Juergen as the Xenstore maintainers. I'd like his opinion on the approach.
> The documentation of the new logic is in:
> 
> https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@xxxxxxx/
> 
> FWIW I am happy in principle with the logic (this is what we discussed on the
> call last week). Some comments below.
> 
> On 17/05/2024 04:21, Henry Wang wrote:
> > There are use cases (for example using the PV driver) in Dom0less
> > setup that require Dom0less DomUs start immediately with Dom0, but
> > initialize XenStore later after Dom0's successful boot and call to
> > the init-dom0less application.
> > 
> > An error message can seen from the init-dom0less application on
> > 1:1 direct-mapped domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> > 
> > The "magic page" is a terminology used in the toolstack as reserved
> > pages for the VM to have access to virtual platform capabilities.
> > Currently the magic pages for Dom0less DomUs are populated by the
> > init-dom0less app through populate_physmap(), and populate_physmap()
> > automatically assumes gfn == mfn for 1:1 direct mapped domains. This
> > cannot be true for the magic pages that are allocated later from the
> > init-dom0less application executed in Dom0. For domain using statically
> > allocated memory but not 1:1 direct-mapped, similar error "failed to
> > retrieve a reserved page" can be seen as the reserved memory list is
> > empty at that time.
> > 
> > Since for init-dom0less, the magic page region is only for XenStore.
> > To solve above issue, this commit allocates the XenStore page for
> > Dom0less DomUs at the domain construction time. The PFN will be
> > noted and communicated to the init-dom0less application executed
> > from Dom0. To keep the XenStore late init protocol, set the connection
> > status to XENSTORE_RECONNECT.
> 
> So this commit is allocating the page, but it will not be used by
> init-dom0less until the next patch. But Linux could use it. So would this
> break bisection? If so, then I think patch #3 needs to be folded in this
> patch.

I think that's fine, I'll leave that with you on commit. I'll resend as
is addressing the other comments.


> > 
> > Reported-by: Alec Kwapis <alec.kwapis@xxxxxxxxxxxxx>
> > Suggested-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> > ---
> > v3:
> > - Only allocate XenStore page. (Julien)
> > - Set HVM_PARAM_STORE_PFN and the XenStore connection status directly
> >    from hypervisor. (Stefano)
> > v2:
> > - Reword the commit msg to explain what is "magic page" and use generic
> >    terminology "hypervisor reserved pages" in commit msg. (Daniel)
> > - Also move the offset definition of magic pages. (Michal)
> > - Extract the magic page allocation logic to a function. (Michal)
> > ---
> >   xen/arch/arm/dom0less-build.c | 44 ++++++++++++++++++++++++++++++++++-
> >   1 file changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 74f053c242..95c4fd1a2d 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -1,5 +1,6 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> >   #include <xen/device_tree.h>
> > +#include <xen/domain_page.h>
> >   #include <xen/err.h>
> >   #include <xen/event.h>
> >   #include <xen/grant_table.h>
> > @@ -10,6 +11,8 @@
> >   #include <xen/sizes.h>
> >   #include <xen/vmap.h>
> >   +#include <public/io/xs_wire.h>
> > +
> >   #include <asm/arm64/sve.h>
> >   #include <asm/dom0less-build.h>
> >   #include <asm/domain_build.h>
> > @@ -739,6 +742,42 @@ static int __init alloc_xenstore_evtchn(struct domain
> > *d)
> >       return 0;
> >   }
> >   +#define XENSTORE_PFN_OFFSET 1
> > +static int __init alloc_xenstore_page(struct domain *d)
> > +{
> > +    struct page_info *xenstore_pg;
> > +    struct xenstore_domain_interface *interface;
> > +    mfn_t mfn;
> > +    gfn_t gfn;
> > +    int rc;
> > +
> > +    d->max_pages += 1;
> 
> Sorry I should have spotted it earlier. But you want to check d->max_pages is
> not overflowing. You can look at acquire_shared_memory_bank() for how to do
> it.
> 
> Also, maybe we want an helper to do it so it is not open-coded in multiple
> places.

Makes sense. I open-coded it because I wasn't sure if you preferred a
static inline or a macro and where to add the implementation.


> > +    xenstore_pg = alloc_domheap_page(d, 0);
> 
> I think we may want to restrict where the page is allocated. For instance,
> 32-bit domain using short page-tables will not be able to address all the
> physical memory.
> 
> I would consider to try to allocate the page below 32-bit (using
> MEMF-bits(32). And then fallback to above 32-bit only 64-bit for domain.

done


> Also, just to note that in theory alloc_domheap_page() could return MFN 0. In
> practice we have excluded MFN 0 because it breaks the page allocator so far.
> 
> But I would still prefer if we add a check on the MFN. This will make easier
> to spot any issue if we ever give MFN 0 to the allocator.

Good idea, but for simplicity I added a check on MFN and if the result
is zero return an error. At least we have explicit check.


> A possible implementation would be to call alloc_domphea_page() a second time
> and then free the first one (e.g. MFN 0).
>
> > +    if ( xenstore_pg == NULL )
> > +        return -ENOMEM;
> > +
> > +    mfn = page_to_mfn(xenstore_pg);
> > +    if ( !is_domain_direct_mapped(d) )
> > +        gfn = gaddr_to_gfn(GUEST_MAGIC_BASE +
> > +                           (XENSTORE_PFN_OFFSET << PAGE_SHIFT));
> > +    else
> > +        gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
> > +
> > +    rc = guest_physmap_add_page(d, gfn, mfn, 0); > +    if ( rc )
> > +    {
> > +        free_domheap_page(xenstore_pg);
> > +        return rc;
> > +    }
> > +
> > +    d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn);
> > +    interface = (struct xenstore_domain_interface *)map_domain_page(mfn);
> 
> map_domain_page() will return a void *. So why do you need to an explicit
> case?

Changed


> > +    interface->connection = XENSTORE_RECONNECT;
> > +    unmap_domain_page(interface);
> > +
> > +    return 0;
> > +}
> > +
> >   static int __init construct_domU(struct domain *d,
> >                                    const struct dt_device_node *node)
> >   {
> > @@ -839,7 +878,10 @@ static int __init construct_domU(struct domain *d,
> >           rc = alloc_xenstore_evtchn(d);
> >           if ( rc < 0 )
> >               return rc;
> > -        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
> > +
> > +        rc = alloc_xenstore_page(d);
> > +        if ( rc < 0 )
> > +            return rc;
> >       }
> >         return rc;
> 
> Cheers,
> 
> -- 
> Julien Grall
>