On 04/07/2022 08:45, Penny Zheng wrote:
Hi Stefano and Julien
Hi Penny,
-----Original Message-----
From: Stefano Stabellini <[email protected]>
+ 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
