Re: [Xen-devel] [PATCH 08/14] xen/memory: Drop ARM put_gfn() stub

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 11/21/18 1:21 PM, Andrew Cooper wrote:
> On x86, get_gfn_*() and put_gfn() are reference counting pairs.  All the
> get_gfn_*() functions are called from within CONFIG_X86 sections, but
> put_gfn() is stubbed out on ARM.
>
> As a result, the common code reads as if ARM is dropping references it never
> acquired.
>
> Put all put_gfn() calls in common code inside CONFIG_X86 to make the code
> properly balanced, and drop the ARM stub.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Acked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>   xen/common/grant_table.c |  8 ++++++++
>   xen/common/memory.c      | 15 ++++++++++++++-
>   xen/include/asm-arm/mm.h |  2 --
>   3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fc41b65..f7860f6 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2110,7 +2110,9 @@ gnttab_transfer(
>           /* Check the passed page frame for basic validity. */
>           if ( unlikely(!mfn_valid(mfn)) )
>           {
> +#ifdef CONFIG_X86
>               put_gfn(d, gop.mfn);
> +#endif
>               gdprintk(XENLOG_INFO, "out-of-range %lx\n", (unsigned 
> long)gop.mfn);
>               gop.status = GNTST_bad_page;
>               goto copyback;
> @@ -2119,7 +2121,9 @@ gnttab_transfer(
>           page = mfn_to_page(mfn);
>           if ( (rc = steal_page(d, page, 0)) < 0 )
>           {
> +#ifdef CONFIG_X86
>               put_gfn(d, gop.mfn);
> +#endif
>               gop.status = rc == -EINVAL ? GNTST_bad_page : GNTST_general_error;
>               goto copyback;
>           }
> @@ -2149,7 +2153,9 @@ gnttab_transfer(
>           unlock_and_copyback:
>               rcu_unlock_domain(e);
>           put_gfn_and_copyback:
> +#ifdef CONFIG_X86
>               put_gfn(d, gop.mfn);
> +#endif
>               page->count_info &= ~(PGC_count_mask|PGC_allocated);
>               free_domheap_page(page);
>               goto copyback;
> @@ -2236,7 +2242,9 @@ gnttab_transfer(
>           page_set_owner(page, e);
>   
>           spin_unlock(&e->page_alloc_lock);
> +#ifdef CONFIG_X86
>           put_gfn(d, gop.mfn);
> +#endif
>   
>           TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
>   
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 58194b9..175bd62 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -358,7 +358,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>   #endif
>       if ( unlikely(!mfn_valid(mfn)) )
>       {
> +#ifdef CONFIG_X86
>           put_gfn(d, gmfn);
> +#endif
>           gdprintk(XENLOG_INFO, "Domain %u page number %lx invalid\n",
>                   d->domain_id, gmfn);
>   
> @@ -388,7 +390,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>       page = mfn_to_page(mfn);
>       if ( unlikely(!get_page(page, d)) )
>       {
> +#ifdef CONFIG_X86
>           put_gfn(d, gmfn);
> +#endif
>           gdprintk(XENLOG_INFO, "Bad page free for domain %u\n", d->domain_id);
>   
>           return -ENXIO;
> @@ -409,8 +413,11 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>           put_page(page);
>   
>       put_page(page);
> - out_put_gfn: __maybe_unused;
> +
> +#ifdef CONFIG_X86
> + out_put_gfn:
>       put_gfn(d, gmfn);
> +#endif
>   
>       /*
>        * Filter out -ENOENT return values that aren't a result of an empty p2m
> @@ -656,7 +663,9 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>   #endif
>                   if ( unlikely(!mfn_valid(mfn)) )
>                   {
> +#ifdef CONFIG_X86
>                       put_gfn(d, gmfn + k);
> +#endif
>                       rc = -EINVAL;
>                       goto fail;
>                   }
> @@ -666,12 +675,16 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>                   rc = steal_page(d, page, MEMF_no_refcount);
>                   if ( unlikely(rc) )
>                   {
> +#ifdef CONFIG_X86
>                       put_gfn(d, gmfn + k);
> +#endif
>                       goto fail;
>                   }
>   
>                   page_list_add(page, &in_chunk_list);
> +#ifdef CONFIG_X86
>                   put_gfn(d, gmfn + k);
> +#endif
>               }
>           }
>   
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 940b74b..b2f6104 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -313,8 +313,6 @@ static inline void *page_to_virt(const struct page_info *pg)
>   struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>                                       unsigned long flags);
>   
> -static inline void put_gfn(struct domain *d, unsigned long gfn) {}
> -
>   /*
>    * Arm does not have an M2P, but common code expects a handful of
>    * M2P-related defines and functions. Provide dummy versions of these.
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel