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

[Julien Grall <julien@xen.org>]

On 04/07/2022 08:45, Penny Zheng wrote:
> Hi Stefano and Julien

Hi Penny,

>> -----Original Message-----
>> From: Stefano Stabellini <sstabellini@kernel.org>
>>>> +        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