RE: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io

[Penny Zheng <Penny.Zheng@arm.com>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 2:22 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
> 
> Hi Penny,
> 
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commit introduces process_shm to cope with static shared memory
> > in domain construction.
> >
> > DOMID_IO will be the default owner of memory pre-shared among
> multiple
> > domains at boot time, when no explicit owner is specified.
> 
> The document in patch #1 suggest the page will be shared with dom_shared.
> But here you say "DOMID_IO".
> 
> Which one is correct?
> 

I’ll fix the documentation, DOM_IO is the last call.

> >
> > This commit only considers allocating static shared memory to dom_io
> > when owner domain is not explicitly defined in device tree, all the
> > left, including the "borrower" code path, the "explicit owner" code
> > path, shall be introduced later in the following patches.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v5 change:
> > - refine in-code comment
> > ---
> > v4 change:
> > - no changes
> > ---
> > v3 change:
> > - refine in-code comment
> > ---
> > v2 change:
> > - instead of introducing a new system domain, reuse the existing
> > dom_io
> > - make dom_io a non-auto-translated domain, then no need to create P2M
> > for it
> > - change dom_io definition and make it wider to support static shm
> > here too
> > - introduce is_shm_allocated_to_domio to check whether static shm is
> > allocated yet, instead of using shm_mask bitmap
> > - add in-code comment
> > ---
> >   xen/arch/arm/domain_build.c | 132
> +++++++++++++++++++++++++++++++++++-
> >   xen/common/domain.c         |   3 +
> >   2 files changed, 134 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7ddd16c26d..91a5ace851 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -527,6 +527,10 @@ static bool __init
> append_static_memory_to_bank(struct domain *d,
> >       return true;
> >   }
> >
> > +/*
> > + * If cell is NULL, pbase and psize should hold valid values.
> > + * Otherwise, cell will be populated together with pbase and psize.
> > + */
> >   static mfn_t __init acquire_static_memory_bank(struct domain *d,
> >                                                  const __be32 **cell,
> >                                                  u32 addr_cells, u32
> > size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> acquire_static_memory_bank(struct domain *d,
> >       mfn_t smfn;
> >       int res;
> >
> > -    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> > +    if ( cell )
> > +        device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> > + psize);
> 
> I think this is a bit of a hack. To me it sounds like this should be moved out to
> a separate helper. This will also make the interface of
> acquire_shared_memory_bank() less questionable (see below).
> 

Ok,  I'll try to not reuse acquire_static_memory_bank in
acquire_shared_memory_bank.

> As this is v5, I would be OK with a follow-up for this split. But this interface of
> acuiqre_shared_memory_bank() needs to change.
> 

I'll try to fix it in next version.

> >       ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> > PAGE_SIZE));
> 
> In the context of your series, who is checking that both psize and pbase are
> suitably aligned?
> 

Actually, the whole parsing process is redundant for the static shared memory.
I've already parsed it and checked it before in process_shm.

> >       if ( PFN_DOWN(*psize) > UINT_MAX )
> >       {
> > @@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct
> domain *d,
> >       panic("Failed to assign requested static memory for direct-map
> domain %pd.",
> >             d);
> >   }
> > +
> > +#ifdef CONFIG_STATIC_SHM
> > +/*
> > + * This function checks whether the static shared memory region is
> > + * already allocated to dom_io.
> > + */
> > +static bool __init is_shm_allocated_to_domio(paddr_t pbase) {
> > +    struct page_info *page;
> > +
> > +    page = maddr_to_page(pbase);
> > +    ASSERT(page);
> 
> maddr_to_page() can never return NULL. If you want to check a page will be
> valid, then you should use mfn_valid().
> 
> However, the ASSERT() implies that the address was suitably checked before.
> But I can't find such check.
> 
> > +
> > +    if ( page_get_owner(page) == NULL )
> > +        return false;
> > +
> > +    ASSERT(page_get_owner(page) == dom_io);
> Could this be hit because of a wrong device-tree? If yes, then this should not
> be an ASSERT() (they are not suitable to check user input).
> 

Yes, it can happen, I'll change it to if-array to output the error.

> > +    return true;
> > +}
> > +
> > +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> > +                                               u32 addr_cells, u32 size_cells,
> > +                                               paddr_t *pbase,
> > +paddr_t *psize)
> 
> There is something that doesn't add-up in this interface. The use of pointer
> implies that pbase and psize may be modified by the function. So...
> 

Just like you points out before, it's a bit hacky to use acquire_static_memory_bank,
and the pointer used here is also due to it. Internal parsing process of acquire_static_memory_bank
needs pointer to deliver the result.

I’ll rewrite acquire_shared_memory, and it will be like:
"
static mfn_t __init acquire_shared_memory_bank(struct domain *d,
                                               paddr_t pbase, paddr_t psize)
{
    mfn_t smfn;
    unsigned long nr_pfns;
    int res;

    /*
     * Pages of statically shared memory shall be included
     * in domain_tot_pages().
     */
    nr_pfns = PFN_DOWN(psize);
    if ( d->max_page + nr_pfns > UINT_MAX )
    {
        printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
               d, psize);
        return INVALID_MFN;
    }
    d->max_pages += nr_pfns;

    smfn = maddr_to_mfn(pbase);
    res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
    if ( res )
    {
        printk(XENLOG_ERR
               "%pd: failed to acquire static memory: %d.\n", d, res);
        return INVALID_MFN;
    }

    return smfn
}
"

> > +{
> > +    /*
> > +     * Pages of statically shared memory shall be included
> > +     * in domain_tot_pages().
> > +     */
> > +    d->max_pages += PFN_DOWN(*psize);
> 
> ... it sounds a bit strange to use psize here. If psize, can't be modified than it
> should probably not be a pointer.
> 
> Also, where do you check that d->max_pages will not overflow?
> 

I'll check the overflow as follows:
"
    nr_pfns = PFN_DOWN(psize);
    if ( d->max_page + nr_pfns > UINT_MAX )
    {
        printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
               d, psize);
        return INVALID_MFN;
    }
    d->max_pages += nr_pfns;
"

> > +
> > +    return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> > +                                      pbase, psize);
> > +
> > +}
> > +
> > +/*
> > + * Func allocate_shared_memory is supposed to be only called
> 
> I am a bit concerned with the word "supposed". Are you implying that it may
> be called by someone that is not the owner? If not, then it should be
> "should".
> 
> Also NIT: Spell out completely "func". I.e "The function".
> 
> > + * from the owner.
> 
> I read from as "current should be the owner". But I guess this is not what you
> mean here. Instead it looks like you mean "d" is the owner. So I would write
> "d should be the owner of the shared area".
> 
> It would be good to have a check/ASSERT confirm this (assuming this is easy
> to write).
> 

The check is already in the calling path, I guess...
Only under certain circumstances, we could call allocate_shared_memory

> > + */
> > +static int __init allocate_shared_memory(struct domain *d,
> > +                                         u32 addr_cells, u32 size_cells,
> > +                                         paddr_t pbase, paddr_t
> > +psize) {
> > +    mfn_t smfn;
> > +
> > +    dprintk(XENLOG_INFO,
> > +            "Allocate static shared memory BANK %#"PRIpaddr"-
> %#"PRIpaddr".\n",
> > +            pbase, pbase + psize);
> 
> NIT: I would suggest to also print the domain. This could help to easily figure
> out that 'd' wasn't the owner.
>

Sure
 
> > +
> > +    smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
> > +                                      &psize);
> > +    if ( mfn_eq(smfn, INVALID_MFN) )
> > +        return -EINVAL;
> > +
> > +    /*
> > +     * 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
> > +     */
> > +    ASSERT(d == dom_io);
> > +    return 0;
> > +}
> > +
> > +static int __init process_shm(struct domain *d,
> > +                              const struct dt_device_node *node) {
> > +    struct dt_device_node *shm_node;
> > +    int ret = 0;
> > +    const struct dt_property *prop;
> > +    const __be32 *cells;
> > +    u32 shm_id;
> > +    u32 addr_cells, size_cells;
> > +    paddr_t gbase, pbase, psize;
> > +
> > +    dt_for_each_child_node(node, shm_node)
> > +    {
> > +        if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-
> memory-v1") )
> > +            continue;
> > +
> > +        if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
> > +        {
> > +            printk("Shared memory node does not provide \"xen,shm-id\"
> property.\n");
> > +            return -ENOENT;
> > +        }
> > +
> > +        addr_cells = dt_n_addr_cells(shm_node);
> > +        size_cells = dt_n_size_cells(shm_node);
> > +        prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> > +        if ( !prop )
> > +        {
> > +            printk("Shared memory node does not provide \"xen,shared-
> mem\" property.\n");
> > +            return -ENOENT;
> > +        }
> > +        cells = (const __be32 *)prop->value;
> > +        /* xen,shared-mem = <pbase, psize, gbase>; */
> > +        device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> > +        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> > + PAGE_SIZE));
> 
> See above about what ASSERT()s are for.
> 

Do you think address was suitably checked here, is it enough?
If it is enough, I'll modify above ASSERT() to mfn_valid()

> > +        gbase = dt_read_number(cells, addr_cells);
> > +
> > +        /* TODO: Consider owner domain is not the default dom_io. */
> > +        /*
> > +         * Per static shared memory region could be shared between multiple
> > +         * domains.
> > +         * In case re-allocating the same shared memory region, we check
> > +         * if it is already allocated to the default owner dom_io before
> > +         * the actual allocation.
> > +         */
> > +        if ( !is_shm_allocated_to_domio(pbase) )
> > +        {
> > +            /* Allocate statically shared pages to the default owner dom_io. */
> > +            ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> > +                                         pbase, psize);
> > +            if ( ret )
> > +                return ret;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +#endif /* CONFIG_STATIC_SHM */
> >   #else
> >   static void __init allocate_static_memory(struct domain *d,
> >                                             struct kernel_info *kinfo,
> > @@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain
> *d,
> >       else
> >           assign_static_memory_11(d, &kinfo, node);
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +    rc = process_shm(d, node);
> > +    if ( rc < 0 )
> > +        return rc;
> > +#endif
> > +
> >       /*
> >        * Base address and irq number are needed when creating vpl011
> device
> >        * tree node in prepare_dtb_domU, so initialization on related
> > variables diff --git a/xen/common/domain.c b/xen/common/domain.c
> index
> > 7570eae91a..7070f5a9b9 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -780,6 +780,9 @@ void __init setup_system_domains(void)
> >        * This domain owns I/O pages that are within the range of the
> page_info
> >        * array. Mappings occur at the priv of the caller.
> >        * Quarantined PCI devices will be associated with this domain.
> > +     *
> > +     * DOMID_IO is also the default owner of memory pre-shared among
> multiple
> > +     * domains at boot time.
> >        */
> >       dom_io = domain_create(DOMID_IO, NULL, 0);
> >       if ( IS_ERR(dom_io) )
> 
> Cheers,
> 
> --
> Julien Grall