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

[Julien Grall <julien@xxxxxxx>]

Hi,


Replying to Penny and Stefano answer in the same e-mail.

On 08/09/2022 04:21, Penny Zheng wrote:
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Sent: Thursday, September 8, 2022 8:16 AM
> > To: Julien Grall <julien@xxxxxxx>
> > Cc: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> > <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Subject: Re: [PATCH v7 7/9] xen/arm: create shared memory nodes in guest
> > device tree
> >
> > On Wed, 7 Sep 2022, Julien Grall wrote:
> > > On 06/09/2022 09:59, 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.
> > > So technically, there is a property "xen,offset" that should be
> > > specified for the borrowers.
> > >
> > > TBH, I don't quite understand what this property is used for. So it is
> > > not quite clear why this is skipped.
> > >
> > > The Stefano is the author of the binding. So maybe he can explain the
> > > purpose of the property and help to document it in the commit message
> > > why this is ignored.
> > It looks like it is something we introduced to handle the case where memory
> > from the owner is shared with multiple borrowers. Each borrower would
> > have its own offset within the region shared by the owner:
> >
> > https://marc.info/?l=xen-devel&m=154110446604365&w=2
Thanks for the pointer :). Now, I remember what this was meant for.

> >
> IMHO, "xen,offset" more fits in the xen dts? We configure it in borrower memory 
> node,
> then later we shall only set up foreign memory map starting at the offset?
Yes and Yes.

> '''
>          domU1-shared-mem@10000000 {
>              compatible = "xen,domain-shared-memory-v1";
>              role = "borrower";
>              xen,shm-id = "my-shared-mem-0";
>              xen,shared-mem = <0x10000000 0x50000000 0x10000000>;
>              xen,offset = <0x0 0x01000000>;
>          }
> '''
> For borrower domain, only [0x11000000, 0x20000000) need to get mapped...
> Of course, we could limit the memory map in related Linux driver, but for 
> safety,
> it should be at Xen?
Correct.

>   
> > The use-case is a bit of a corner case but it looks valid. If I had to do it 
> > now, I
> > would at least mark "xen,offset" as "optional".
> >
> > I think we have two options here and I am happy with either one:
> >
> > 1) we add xen,offset = <0x0>;
> I will let v8 stay with this configuration, and a TODO for actual 
> implementation.
Agree. It sounds the best approach for now.

Cheers,

--
Julien Grall