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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Julien Grall writes ("Re: [PATCH v8 7/8] xen/arm: export shared memory regions 
as reserved-memory on device tree"):
> On 10/30/18 3:58 PM, Ian Jackson wrote:
> > 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]).

Oh great.

> Stefano, would you mind to add a pointer in the commit message?

Can we have a pointer in the xen.git documentation to the relevant
linux.git documentation ?  That would be better than just in the
commit message.

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

Hrm.  We recently tightened this up to specify precisely 75.  I'm not
going to argue the point here, so fine, leave this as it is.

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

This patch is adding more of this formulaic code.  I was asing whether
you thought it worth adding a macro for this specific new hunk.

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

Yes.

But I think that sshms[i].offset ought always to be 0 if .role ==
LIBXL_SSHM_ROLE_SLAVE.  So the test for LIBXL_SSHM_ROLE_SLAVE is
redundant.  It would be simpler and more obvious to always add
.offset.

Thanks,
Ian.

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