Re: [PATCH v2 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated

From: Stefano Stabellini <sstabellini@kernel.org>
To: Penny Zheng <Penny.Zheng@arm.com>
Cc: xen-devel@lists.xenproject.org, wei.chen@arm.com,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	 Bertrand Marquis <bertrand.marquis@arm.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated
Date: Fri, 6 May 2022 18:09:05 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2205061748560.43560@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <20220506072502.2177828-6-Penny.Zheng@arm.com>

On Fri, 6 May 2022, Penny Zheng wrote:
> Borrower domain will fail to get a page ref using the owner domain
> during allocation, when the owner is created after borrower.
> 
> So here, we decide to get and add the right amount of reference, which
> is the number of borrowers, when the owner is allocated.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>
> ---
> v2 change:
> - new commit
> ---
>  xen/arch/arm/domain_build.c | 62 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f43378227a..b3ba0c501d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -792,6 +792,34 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>  
>  }
>  
> +static int __init acquire_nr_borrower_domain(struct domain *d,
> +                                             paddr_t pbase, paddr_t psize,
> +                                             unsigned long *nr_borrowers)
> +{
> +    unsigned long bank;
> +
> +    /* Iterate reserved memory to find requested shm bank. */
> +    for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> +    {
> +        paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> +        paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> +
> +        if ( pbase == bank_start && psize == bank_size )
> +            break;
> +    }
> +
> +    if ( bank == bootinfo.reserved_mem.nr_banks )
> +        return -ENOENT;
> +
> +    if ( d == dom_io )
> +        *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain;
> +    else
> +        /* Exclude the owner domain itself. */
> +        *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1;
> +
> +    return 0;
> +}
> +
>  /*
>   * Func allocate_shared_memory is supposed to be only called
>   * from the owner.
> @@ -803,6 +831,8 @@ static int __init allocate_shared_memory(struct domain *d,
>  {
>      mfn_t smfn;
>      int ret = 0;
> +    unsigned long nr_pages, nr_borrowers, i;
> +    struct page_info *page;
>  
>      dprintk(XENLOG_INFO,
>              "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> @@ -817,6 +847,7 @@ static int __init allocate_shared_memory(struct domain *d,
>       * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
>       * It sees RAM 1:1 and we do not need to create P2M mapping for it.
>       */
> +    nr_pages = PFN_DOWN(psize);
>      if ( d != dom_io )
>      {
>          ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));
> @@ -828,6 +859,37 @@ static int __init allocate_shared_memory(struct domain *d,
>          }
>      }
>  
> +    /*
> +     * Get the right amount of references per page, which is the number of
> +     * borrow domains.
> +     */
> +    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> +    if ( ret )
> +        return ret;
> +
> +    /*
> +     * Instead of let borrower domain get a page ref, we add as many
> +     * additional reference as the number of borrowers when the owner
> +     * is allocated, since there is a chance that owner is created
> +     * after borrower.
> +     */
> +    page = mfn_to_page(smfn);
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        if ( !get_page_nr(page + i, d, nr_borrowers) )
> +        {
> +            dprintk(XENLOG_ERR,
> +                    "Failed to add %lu references to page %"PRI_mfn".\n",
> +                    nr_borrowers, mfn_x(smfn) + i);

dprintk only prints errors when DEBUG is selected. This is actually a
pretty serious error so I think it should be printed in all cases. A
normal printk would be fine.

> +            goto fail;
> +        }
> +    }
> +
> +    return 0;
> +
> + fail:
> +    while ( --i >= 0 )
> +        put_page_nr(page + i, nr_borrowers);
>      return ret;
>  }
>  
> -- 
> 2.25.1
> 