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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 5 Jul 2022, Julien Grall wrote:
> On 04/07/2022 08:45, Penny Zheng wrote:
> > Hi Stefano and Julien
> 
> Hi Penny,
> 
> > > -----Original Message-----
> > > From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > +        res = fdt_property_cell(fdt, "xen,id", shm_id);
> > > > 
> > > > Looking at the Linux binding, "xen,id" is meant to be a string. But
> > > > here you are writing it as an integer.
> > > 
> > > Good catch!
> > > 
> > > 
> > > > Given that the Linux binding is already merged, I think the Xen
> > > > binding should be changed.
> > > 
> > > We would be compliant with both bindings (xen and linux) just by writing
> > > shm_id as string here, but if it is not too difficult we might as well
> > > harmonize
> > > the two bindings and also define xen,shm-id as a string.
> > > 
> > > On the Xen side, I would suggest to put a clear size limit so that the
> > > string is
> > > easier to handle.
> > 
> > I've already made the xen,shm-id parsed as a string too, seeing the below
> > code:
> > "
> >      prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> >      if ( !prop_id )
> >          return -ENOENT;
> >      shm_id = simple_strtoul(prop_id->data, NULL, 10);
> 
> Why do you want to convert the string to a number?
> 
> 
> >      if ( shm_id >= NR_MEM_BANKS )
> 
> IIRC, you are not using "shm_id" to index any bank. So why do you want to
> check against NR_MEM_BANKS?
> 
> >      {
> >          printk("fdt: invalid `xen,shm-id` %lu for static shared memory
> > node.\n",
> >                 shm_id);
> >          return -EINVAL;
> >      }
> > "
> > The size limit is smaller than 256, just as stated in doc:
> > "
> > - xen,shm-id
> > 
> >      A string that represents the unique identifier of the shared memory
> >      region. The maximum identifier shall be "xen,shm-id = 255".
> 
> The first sentence reads as the xen,shm-id can a free form string. But then
> the second sentence suggests a number (not a string).
> 
> In any case, it is still unclear why you want to convert the string to an ID.
> From my understanding, Stefano was suggested a limit on the characters rather
> than a limit on the number.

Just confirming that yes, I was suggesting a strict limit on the number
of characters, assuming we accept a freeform string.

I think a freeform string is more convenient and flexible for the user.
But it is not required: our only requirement is that the Linux device
tree Xen generates has "xen,id" in the form of a string, but that could
be a string representing a number, e.g. "255".


> If the latter is desirable, then the documentation should be a bit clearer and
> you need to validate the input provided by the user.