Re: [PATCH v2 8/9] xen/arm: create shared memory nodes in guest device tree

[Julien Grall <julien@xxxxxxx>]

Hi,

On 07/05/2022 02:09, Stefano Stabellini wrote:
> On Fri, 6 May 2022, Penny Zheng wrote:
> > We expose the shared memory to the domU using the "xen,shared-memory-v1"
> > reserved-memory binding. See
> > Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt
> > in Linux for the corresponding device tree binding.
> >
> > To save the cost of re-parsing shared memory device tree configuration when
> > creating shared memory nodes in guest device tree, this commit adds new field
> > "shm_mem" to store shm-info per domain.
> >
> > For each shared memory region, a range is exposed under
> > the /reserved-memory node as a child node. Each range sub-node is
> > named xen-shmem@<address> and has the following properties:
> > - compatible:
> >          compatible = "xen,shared-memory-v1"
> > - reg:
> >          the base guest physical address and size of the shared memory region
> > - xen,id:
> >          a string that identifies the shared memory region.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > v2 change:
> > - using xzalloc
> > - shm_id should be uint8_t
> > - make reg a local variable
> > - add #address-cells and #size-cells properties
> > - fix alignment
> > ---
> >   xen/arch/arm/domain_build.c       | 144 ++++++++++++++++++++++++++++++
> >   xen/arch/arm/include/asm/domain.h |   1 +
> >   xen/arch/arm/include/asm/setup.h  |   1 +
> >   3 files changed, 146 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 8d299a3616..f08606d2c0 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -894,6 +894,26 @@ static int __init allocate_shared_memory(struct domain *d,
> >       return ret;
> >   }
> >   
> > +static int __init append_shm_bank_to_domain(struct domain *d,
> > +                                            paddr_t start, paddr_t size,
> > +                                            u32 shm_id)
> > +{
> > +    /* Allocate memory at first insertion. */
> > +    if ( d->arch.shm_mem == NULL )
> > +    {
> > +        d->arch.shm_mem = xzalloc(struct meminfo);
> > +        if ( d->arch.shm_mem == NULL )
> > +            return -ENOMEM;
> > +    }
> > +
> > +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].start = start;
> > +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].size = size;
> > +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].shm_id = shm_id;
> > +    d->arch.shm_mem->nr_banks++;
bank[] is a fixed size array. So we should check we don't overflow.

> > +
> > +    return 0;
> > +}
> Can we call xfree(d->arch.shm_mem) at domain destruction?
>
> Even better, we might be able to call it earlier, soon after the
> make_resv_memory_node() call because we don't need it any longer after
> that?
If this is only used during domain build, then the field should move to 
kernel_info. With that, there will also be no concern on whether we need 
to allocate the structure or not.
Cheers,

--
Julien Grall