Re: [Xen-devel] [PATCH v6 5/5] domain: use PGC_extra domheap page for shared_info

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
[snip]
> > Currently shared_info is a shared xenheap page but shared xenheap pages
> > complicate future plans for live-update of Xen so it is desirable to,
> > where possible, not use them [1]. This patch therefore converts shared_info
> > into a PGC_extra domheap page. This does entail freeing shared_info during
> > domain_relinquish_resources() rather than domain_destroy() so care is
> > needed to avoid de-referencing a NULL shared_info pointer hence some
> > extra checks of 'is_dying' are needed.
> >
> > NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is
> >        left in place since it is idempotent and called in the error path for
> >        arch_domain_create().
> 
> The approach looks good to me. I have one comment below.
> 

Thanks.

> [...]
> 
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 4ef0d3b21e..4f3266454f 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1651,24 +1651,44 @@ int continue_hypercall_on_cpu(
> >
> >   int alloc_shared_info(struct domain *d, unsigned int memflags)
> >   {
> > -    if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL )
> > +    struct page_info *pg;
> > +
> > +    pg = alloc_domheap_page(d, MEMF_no_refcount | memflags);
> > +    if ( !pg )
> >           return -ENOMEM;
> >
> > -    d->shared_info.mfn = virt_to_mfn(d->shared_info.virt);
> > +    if ( !get_page_and_type(pg, d, PGT_writable_page) )
> > +    {
> > +        /*
> > +         * The domain should not be running at this point so there is
> > +         * no way we should reach this error path.
> > +         */
> > +        ASSERT_UNREACHABLE();
> > +        return -ENODATA;
> > +    }
> > +
> > +    d->shared_info.mfn = page_to_mfn(pg);
> > +    d->shared_info.virt = __map_domain_page_global(pg);
> 
> __map_domain_page_global() is not guaranteed to succeed. For instance,
> on Arm32 this will be a call to vmap().
> 
> So you want to check the return.
> 

Ok, I'll add a check.

As Jan discovered, I messed up the version numbering so there is already a v7 
series where I dropped this patch (until I can group it with conversion of 
other shared xenheap pages).

  Paul

> Cheers,
> 
> --
> Julien Grall