Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

Hi Penny,

On 11/14/22 21:52, Penny Zheng wrote:
> With the introduction of new scenario where host address is not provided
> in "xen,shared-mem", the function "assign_shared_memory" shall be adapted
> to it too.
> 
> Shared memory will already be allocated from heap, when calling
> "assign_shared_memory" with unprovided host address.
> So in "assign_shared_memory", we just need to assign these static shared pages
> to its owner domain using function "assign_pages", and add as many
> additional reference as the number of borrowers.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 160 ++++++++++++++++++++++++++++++------
>  1 file changed, 133 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3de96882a5..faf0784bb0 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -817,8 +817,12 @@ static bool __init is_shm_allocated_to_domio(struct 
> shm_membank *shm_membank)
>  {
>      struct page_info *page;
>      struct domain *d;
> +    paddr_t pbase;
> 
> -    page = maddr_to_page(shm_membank->mem.bank->start);
> +    pbase = shm_membank->mem.banks.meminfo ?
> +            shm_membank->mem.banks.meminfo->bank[0].start :
> +            shm_membank->mem.bank->start;
> +    page = maddr_to_page(pbase);
>      d = page_get_owner_and_reference(page);
>      if ( d == NULL )
>          return false;
> @@ -907,6 +911,7 @@ static mfn_t __init acquire_shared_memory_bank(struct 
> domain *d,
>      mfn_t smfn;
>      unsigned long nr_pfns;
>      int res;
> +    struct page_info *page;
> 
>      /*
>       * Pages of statically shared memory shall be included
> @@ -922,33 +927,82 @@ static mfn_t __init acquire_shared_memory_bank(struct 
> domain *d,
>      d->max_pages += nr_pfns;
> 
>      smfn = maddr_to_mfn(pbase);
> -    res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> -    if ( res )
> +    page = mfn_to_page(smfn);
> +    /*
> +     * If page is allocated from heap as static shared memory, then we just
> +     * assign it to the owner domain
> +     */
> +    if ( page->count_info == (PGC_state_inuse | PGC_static) )
>      {
> -        printk(XENLOG_ERR
> -               "%pd: failed to acquire static memory: %d.\n", d, res);
> -        d->max_pages -= nr_pfns;
> -        return INVALID_MFN;
> +        res = assign_pages(page, nr_pfns, d, 0);
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR
> +                   "%pd: failed to assign static memory: %d.\n", d, res);
> +            return INVALID_MFN;
> +        }
> +    }
> +    else
> +    {
> +        res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR
> +                   "%pd: failed to acquire static memory: %d.\n", d, res);
> +                   d->max_pages -= nr_pfns;
> +            return INVALID_MFN;
> +        }
>      }
> 
>      return smfn;
>  }
> 
> -static int __init assign_shared_memory(struct domain *d,
> -                                       struct shm_membank *shm_membank,
> -                                       paddr_t gbase)
> +static void __init remove_shared_memory_ref(struct page_info *page,
> +                                            unsigned long nr_pages,
> +                                            unsigned long nr_borrowers)
>  {
> -    mfn_t smfn;
> -    int ret = 0;
> -    unsigned long nr_pages, nr_borrowers, i;
> -    struct page_info *page;
> -    paddr_t pbase, psize;
> +    while ( --nr_pages >= 0 )

arch/arm/domain_build.c: In function ‘remove_shared_memory_ref’:
arch/arm/domain_build.c:969:24: warning: comparison of unsigned expression in 
‘>= 0’ is always true [-Wtype-limits]
  969 |     while ( --nr_pages >= 0 )
      |                        ^~

> +         put_page_nr(page + nr_pages, nr_borrowers);
> +}
> 
> -    pbase = shm_membank->mem.bank->start;
> -    psize = shm_membank->mem.bank->size;
> +static int __init add_shared_memory_ref(struct domain *d, struct page_info 
> *page,
> +                                        unsigned long nr_pages,
> +                                        unsigned long nr_borrowers)
> +{
> +    unsigned int i;
> 
> -    printk("%pd: allocate static shared memory BANK 
> %#"PRIpaddr"-%#"PRIpaddr".\n",
> -           d, pbase, pbase + psize);
> +    /*
> +     * Instead of letting 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.
> +     * So if the borrower is created first, it will cause adding pages
> +     * in the P2M without reference.
> +     */
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        if ( !get_page_nr(page + i, d, nr_borrowers) )
> +        {
> +            printk(XENLOG_ERR
> +                   "Failed to add %lu references to page %"PRI_mfn".\n",
> +                   nr_borrowers, mfn_x(page_to_mfn(page)) + i);
> +            goto fail;
> +        }
> +    }
> +    return 0;
> +
> + fail:
> +    remove_shared_memory_ref(page, i, nr_borrowers);
> +    return -EINVAL;
> +}
> +
> +static int __init acquire_shared_memory(struct domain *d,
> +                                        paddr_t pbase, paddr_t psize,
> +                                        paddr_t gbase)
> +{
> +    mfn_t smfn;
> +    int ret = 0;
> +    unsigned long nr_pages;

When building with EXTRA_CFLAGS_XEN_CORE="-Wunused-but-set-variable 
-Wno-error=unused-but-set-variable"
arch/arm/domain_build.c: In function ‘acquire_shared_memory’:
arch/arm/domain_build.c:1010:19: warning: variable ‘nr_pages’ set but not used 
[-Wunused-but-set-variable]
 1010 |     unsigned long nr_pages;
      |                   ^~~~~~~~

> 
>      smfn = acquire_shared_memory_bank(d, pbase, psize);
>      if ( mfn_eq(smfn, INVALID_MFN) )
> @@ -970,6 +1024,44 @@ static int __init assign_shared_memory(struct domain *d,
>          }
>      }
> 
> +    return 0;
> +}
> +
> +static int __init assign_shared_memory(struct domain *d,
> +                                       struct shm_membank *shm_membank,
> +                                       paddr_t gbase)
> +{
> +    int ret = 0;
> +    unsigned long nr_pages, nr_borrowers;
> +    struct page_info *page;
> +    unsigned int i;
> +    struct meminfo *meminfo;
> +
> +    /* Host address is not provided in "xen,shared-mem" */
> +    if ( shm_membank->mem.banks.meminfo )
> +    {
> +        meminfo = shm_membank->mem.banks.meminfo;
> +        for ( i = 0; i < meminfo->nr_banks; i++ )
> +        {
> +            ret = acquire_shared_memory(d,
> +                                        meminfo->bank[i].start,
> +                                        meminfo->bank[i].size,
> +                                        gbase);
> +            if ( ret )
> +                return ret;
> +
> +            gbase += meminfo->bank[i].size;
> +        }
> +    }
> +    else
> +    {
> +        ret = acquire_shared_memory(d,
> +                                    shm_membank->mem.bank->start,
> +                                    shm_membank->mem.bank->size, gbase);
> +        if ( ret )
> +            return ret;
> +    }
> +
>      /*
>       * Get the right amount of references per page, which is the number of
>       * borrower domains.
> @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct domain 
> *d,
>       * So if the borrower is created first, it will cause adding pages
>       * in the P2M without reference.
>       */
> -    page = mfn_to_page(smfn);
> -    for ( i = 0; i < nr_pages; i++ )
> +    if ( shm_membank->mem.banks.meminfo )
>      {
> -        if ( !get_page_nr(page + i, d, nr_borrowers) )
> +        meminfo = shm_membank->mem.banks.meminfo;
> +        for ( i = 0; i < meminfo->nr_banks; i++ )
>          {
> -            printk(XENLOG_ERR
> -                   "Failed to add %lu references to page %"PRI_mfn".\n",
> -                   nr_borrowers, mfn_x(smfn) + i);
> -            goto fail;
> +            page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> +            nr_pages = PFN_DOWN(meminfo->bank[i].size);
> +            ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> +            if ( ret )
> +                goto fail;
>          }
>      }
> +    else
> +    {
> +        page = mfn_to_page(
> +                maddr_to_mfn(shm_membank->mem.bank->start));
> +        nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
> +        ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> +        if ( ret )
> +            return ret;
> +    }
> 
>      return 0;
> 
>   fail:
>      while ( --i >= 0 )

I'm aware that this line is unchanged, and I'm not trying to introduce scope 
creep, but I still wanted to point this out because (1) it is similar in nature 
to other occurrences introduced in this series and (2) the body of the loop is 
changed:
arch/arm/domain_build.c: In function ‘assign_shared_memory’:
arch/arm/domain_build.c:1109:17: warning: comparison of unsigned expression in 
‘>= 0’ is always true [-Wtype-limits]
 1109 |     while ( --i >= 0 )
      |                 ^~

> -        put_page_nr(page + i, nr_borrowers);
> +    {
> +        page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> +        nr_pages = PFN_DOWN(meminfo->bank[i].size);
> +        remove_shared_memory_ref(page, nr_pages, nr_borrowers);
> +    }
>      return ret;
>  }
> 
> --
> 2.25.1
> 
>