Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Saturday, June 25, 2022 3:07 AM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>
> Subject: Re: [PATCH v5 3/8] xen/arm: allocate static shared memory to a
> specific owner domain
>
> Hi Penny,
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > If owner property is defined, then owner domain of a static shared
> > memory region is not the default dom_io anymore, but a specific domain.
> >
> > This commit implements allocating static shared memory to a specific
> > domain when owner property is defined.
> >
> > Coding flow for dealing borrower domain will be introduced later in
> > the following commits.
> >
> > Signed-off-by: Penny Zheng <penny.zh...@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 change:
> > - no changes
> > ---
> > v3 change:
> > - simplify the code since o_gbase is not used if the domain is dom_io
> > ---
> > v2 change:
> > - P2M mapping is restricted to normal domain
> > - in-code comment fix
> > ---
> > xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++--------
> --
> > 1 file changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 91a5ace851..d4fd64e2bd 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -805,9 +805,11 @@ static mfn_t __init
> acquire_shared_memory_bank(struct domain *d,
> > */
> > static int __init allocate_shared_memory(struct domain *d,
> > u32 addr_cells, u32 size_cells,
> > - paddr_t pbase, paddr_t psize)
> > + paddr_t pbase, paddr_t psize,
> > + paddr_t gbase)
> > {
> > mfn_t smfn;
> > + int ret = 0;
> >
> > dprintk(XENLOG_INFO,
> > "Allocate static shared memory BANK
> > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -822,8 +824,18 @@ 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
> > */
> > - ASSERT(d == dom_io);
> > - return 0;
> > + if ( d != dom_io )
> > + {
> > + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
> > + PFN_DOWN(psize));
>
> Coding style: this line is over 80 characters. And...
>
> > + if ( ret )
> > + {
> > + printk(XENLOG_ERR
> > + "Failed to map shared memory to %pd.\n", d);
>
> ... this line could be merged with the previous one.
>
> > + return ret;
> > + }
> > + }
> > +
> > + return ret;
> > }
> >
> > static int __init process_shm(struct domain *d, @@ -836,6 +848,8 @@
> > static int __init process_shm(struct domain *d,
> > u32 shm_id;
> > u32 addr_cells, size_cells;
> > paddr_t gbase, pbase, psize;
> > + const char *role_str;
> > + bool owner_dom_io = true;
>
> I think it would be best if role_str and owner_dom_io are defined within the
> loop. Same goes for all the other declarations.
>
> >
> > dt_for_each_child_node(node, shm_node)
> > {
> > @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
> > ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> PAGE_SIZE));
> > gbase = dt_read_number(cells, addr_cells);
> >
> > - /* TODO: Consider owner domain is not the default dom_io. */
> > + /*
> > + * "role" property is optional and if it is defined explicitly,
> > + * then the owner domain is not the default "dom_io" domain.
> > + */
> > + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
> > + owner_dom_io = false
> IIUC, the role is per-region. However, owner_dom_io is first initialized to
> false outside the loop. Therefore, the variable may not be correct on the next
> region.
>
> So I think you want to write:
>
> owner_dom_io = !dt_property_read_string(...);
>
> This can also be avoided if you reduce the scope of the variable (it is meant
> to only be used in the loop).
>
Yes, it is a bug, thx!!! I'll reduce the scope.
> > +
> > /*
> > * 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.
> > + * So when owner domain is the default dom_io, 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) )
> > + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> > + (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> > {
> > - /* Allocate statically shared pages to the default owner
> > dom_io. */
> > - ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> > - pbase, psize);
> > + /* Allocate statically shared pages to the owner domain. */
> > + ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
> > + addr_cells, size_cells,
> > + pbase, psize, gbase);
> > if ( ret )
> > return ret;
> > }
>
> Cheers,
>
> --
> Julien Grall
