Re: [PATCH] design: design doc for shared memory on a dom0less system

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 26 Jan 2022, Julien Grall wrote:
> > +
> > +- role(Optional)
> > +
> > +    A string property specifying the ownership of a shared memory region,
> > +    the value must be one of the following: "owner", or "borrower"
> > +    A shared memory region could be explicitly backed by one domain, which
> > is
> > +    called "owner domain", and all the other domains who are also sharing
> > +    this region are called "borrower domain".
> > +    If not specified, the default value is "borrower" and owner is
> > +    "dom_shared", a system domain.
> 
> I don't particularly like adding another system domain. Instead, it would be
> better to always specify the owner.
 
I wonder if we could reuse one of the existing system domains. DOM_IO
looks very close to what we need here, except that the current
description doesn't quite fit:

"DOMID_IO is used to restrict page-table updates to mapping I/O memory."

Also DOMID_XEN doesn't fit:

"DOMID_XEN is used to allow privileged domains to map restricted parts
of Xen's heap space (e.g., the machine_to_phys table)."


On the other hand the description of DOMID_COW seems to be exactly what
we want:

"DOMID_COW is used as the owner of sharable pages"

This is technically the description of what we need :-D  However, we
know that DOMID_COW was introduced with a different goal in mind.


> > +
> > +## Example
> > +
> > +chosen {
> > +    #address-cells = <0x1>;
> > +    #size-cells = <0x1>;
> > +    xen,xen-bootargs = "console=dtuart dtuart=serial0 bootscrub=0";
> > +
> > +    ......
> > +
> > +    /* this is for Dom0 */
> > +    dom0-shared-mem@10000000 {
> > +        compatible = "xen,domain-shared-memory-v1";
> > +        xen,shm-id = <0x0>;
> > +        role = "owner";
> > +        xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
> > +    }
> > +
> > +    domU1 {
> > +        compatible = "xen,domain";
> > +        #address-cells = <0x1>;
> > +        #size-cells = <0x1>;
> > +        memory = <0 131072>;
> > +        cpus = <2>;
> > +        vpl011;
> > +
> > +        /*
> > +         * shared memory region identified as 0x0(xen,shm-id = <0x0>)
> > +         * shared between dom0.
> > +         */
> > +        domU1-shared-mem@10000000 {
> > +            compatible = "xen,domain-shared-memory-v1";
> > +            xen,shm-id = <0x0>;
> > +            role = "borrower";
> > +            xen,shared-mem = <0x10000000 0x10000000 0x50000000>;
> 
> Technically, you already know the physical address from the owner. In fact, it
> will only increase the risk to get the wrong binding. So I would like to
> suggest a different binding.
> 
> 1) Reserve the region in the host memory using reserved-memory binding
> 2) Create a binding per domain that contains a phandle to the host memory and
> the role.
> 
> The advantage with this is we could easily support region that are not backed
> by a reserved-memory.

I see what you mean but given that we have to specify the guest physical
address anyway I don't think that replacing the physical address with a
phandle would make things easier. We would still need to specify guest
physical address and maybe size. We already have xen,reg that also
works similarly. So I think it would be best to follow the same format
as xen,reg.

That would also handle the case where the shared memory region is an
sram region: we just need to use an address range that corresponds to
mmio-sram instead of memory. Note that with a phandle we would have the
problem that we could only share the entire sram region and not a part
of it (because we could only link to the full mmio-sram node and not to
a subset of it).

We just need to clearly specify the number of address cells and size
cells to use. We could introduce #shared-mem-address-cells or reuse
#address-cells. Either way parsing it should be easy.


If we wanted to use a phandle, I think it should point to a
reserved-memory region outside of /reserved-memory. Long explanation
follows.

Reserved-memory is a bit tricky to get right because reserved-memory is a
configuration node and we don't have a way to tell who the
reserved-memory configuration is for. Is it for Xen? Or for dom0? Or for
domUs? When discussing system device tree topics (system device tree is
very similar to device tree with Xen domain nodes) reserved-memory was
one of the firsts concerns that Rob raised. The solution for system
device tree was that each domain node should have its own
reserved-memory sub-node instead of using /reserved-memory (e.g.
/chosen/domU/reserved-memory).

Going back to Xen, if we take domUs out of the picture, who is
/reserved-memory for? In practice, as most drivers are in dom0, we have
been assuming /reserved-memory is for dom0. Xen copies reserved-memory
nodes to dom0.

This is why I would like to suggest that if we want to use the phandle
model then I think we should use a new and different reserved-memory
node to specify the shared memory. E.g. /chosen/reserved-memory. We
could call it differently and/or have an unambiguous compatible string:

chosen {
    reserved-memory {
        compatible = "xen,domains-reserved-memory-v1";

        ...
    };

This way we clearly distinguish it from /reserved-memory and we avoid
ownership conflicts with dom0.

 
> > +        }
> > +
> > +        domU1-shared-mem@50000000 {
> > +            compatible = "xen,domain-shared-memory-v1";
> > +            xen,shm-id = <0x1>;
> > +            xen,shared-mem = <0x50000000 0x20000000 0x60000000>;
> > +        }
> > +
> > +        ......
> > +
> > +    };
> > +
> > +    domU2 {
> > +        compatible = "xen,domain";
> > +        #address-cells = <0x1>;
> > +        #size-cells = <0x1>;
> > +        memory = <0 65536>;
> > +        cpus = <1>;
> > +
> > +        /*
> > +         * shared memory region identified as 0x1(xen,shm-id = <0x1>)
> > +         * shared between domU1.
> > +         */
> > +        domU2-shared-mem@50000000 {
> > +            compatible = "xen,domain-shared-memory-v1";
> > +            xen,shm-id = <0x1>;
> > +            xen,shared-mem = <0x50000000 0x20000000 0x70000000>;
> > +        }
> > +
> > +        ......
> > +    };
> > +};