Re: [Xen-devel] [PATCH v8 7/8] xen/arm: export shared memory regions as reserved-memory on device tree

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 10/30/18 3:58 PM, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v8 7/8] xen/arm: export shared memory regions as 
> reserved-memory on device tree"):
> > Shared memory regions need to be advertised to the guest. Fortunately, a
> > device tree binding for special memory regions already exist:
> > reserved-memory.
> Oh!  Here is the guest ABI.
>
> But it's not documented.
Device-Tree bindings are documented in 
linux/Document/device-tree/bindings. Stefano sent it via the DT mailing 
list (see [1]).
Stefano, would you mind to add a pointer in the commit message?

> > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > index 054ad58..c1624c0 100644
> > --- a/tools/libxl/libxl_arm.c
> > +++ b/tools/libxl/libxl_arm.c
> > @@ -436,6 +436,58 @@ static int make_memory_nodes(libxl__gc *gc, void *fdt,
> >       return 0;
> >   }
> >   
> > +static int make_reserved_nodes(libxl__gc *gc, void *fdt,
> > +                               libxl_domain_config *d_config)
> > +{
> > +    int res, i;
> > +    const char *name;
> > +
> > +    if (d_config->num_sshms == 0)
> > +        return 0;
> > +
> > +    res = fdt_begin_node(fdt, "reserved-memory");
> > +    if (res) return res;
> > +
> > +    res = fdt_property_cell(fdt, "#address-cells", GUEST_ROOT_ADDRESS_CELLS);
> > +    if (res) return res;
> > +
> > +    res = fdt_property_cell(fdt, "#size-cells", GUEST_ROOT_SIZE_CELLS);
> > +    if (res) return res;
> > +
> > +    res = fdt_property(fdt, "ranges", NULL, 0);
> > +    if (res) return res;
> The line lengths here are very long. 
They should still be under 75-80 characters a line has mandated per the 
coding style. Is that an issue?
> Also it's quite formulaic.  I
> see make_psci_node is quite like that too.  IDK whether a local macro
> would help.
Possibly, but that not really related to this patch itself. Can we look 
that as a clean-up?
> > +    for (i = 0; i < d_config->num_sshms; i++) {
> > +        uint64_t start = d_config->sshms[i].begin;
> > +
> > +        if (d_config->sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
> > +            start += d_config->sshms[i].offset;
> Why is d_config->sshms[i].offset not 0 for the owner ?
> You could do this unconditionally.
From the documentation:

"Can only appear when B<role> = slave. If set, the address mapping will 
not start from the beginning the backing memory region, but from the middle
(B<offset> bytes away from the beginning) of it. See the graph below:

With B<offset> = 0, the mapping will look like:

  backing memory region:  #########################################
                          |                       |
                          |                       |
                          |                       |
                          V                       V
  slave's shared region:  #########################

With B<offset> > 0:

  backing memory region:   #########################################
                           |<-- offset -->||                       |
                                           |                       |
                                           |                       |
                                           V                       V
  slave's memory region:                   #########################
"

This allows the configuration in the owning domain to be simpler as you 
can share a big region and split between multiple domain.
> > -static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
> > +static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config,
> Can we have this NFC change and its consequences as a pre-patch ?
Cheers,


[1] https://lists.xen.org/archives/html/xen-devel/2018-10/msg01812.html


--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel