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

From: Julien Grall <julien@xen.org>
To: paul@xen.org, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Paul Durrant <pdurrant@amazon.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH v6 5/5] domain: use PGC_extra domheap page for shared_info
Date: Tue, 24 Mar 2020 14:22:33 +0000	[thread overview]
Message-ID: <65cb0423-09c3-209a-85dc-63b07085d7ac@xen.org> (raw)
In-Reply-To: <20200310174917.1514-6-paul@xen.org>

Hi Paul,

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