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

[Julien Grall <julien@xxxxxxx>]

Hi Paul,

On 10/03/2020 17:49, paul@xxxxxxx wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
>
> 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.

[...]

> 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.

Cheers,

--
Julien Grall