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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi stefano

> -----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
> 

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?
'''
        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?
 
> 
> 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.

> 2) we do *not* add xen,offset and instead send a patch to the LKML to fix
> Documentation/devicetree/bindings/reserved-memory/xen,shared-
> memory.txt
> so that it clearly states that xen,offset is optional. I don't know if Rob 
> would
> accept such a patch without changing the version in the compatible string.
> 
> Given the release deadline, I would go with 1). We can always remove it once
> it becomes clearly optional.