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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 15/11/2022 02:52, Penny Zheng wrote:
> @@ -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) )
I am a bit confused how this can help differentiating 
becaPGC_state_inuse is 0. So effectively, you are checking that 
count_info is equal to PGC_static.
But as I wrote in a previous patch, I don't think you should convert 
{xen,dom}heap pages to a static pages.
[...]

> +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;
> +    }
Looking at this change and...

> +
>       /*
>        * 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;
> +    }
... this one. The code to deal with a bank is exactly the same. But you 
need the duplication because you special case "one bank".
As I wrote in a previous patch, I don't think we should special case it. 
If the concern is memory usage, then we should look at reworking meminfo 
instead (or using a different structure).
>   
>       return 0;
>   
>    fail:
>       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;
>   }
>   
Cheers,

--
Julien Grall