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

[Julien Grall <julien@xxxxxxx>]

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.
If the latter is desirable, then the documentation should be a bit 
clearer and you need to validate the input provided by the user.
Cheers,

--
Julien Grall