* [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells
@ 2022-09-08 9:31 Henry Wang
2022-09-08 10:38 ` Michal Orzel
0 siblings, 1 reply; 5+ messages in thread
From: Henry Wang @ 2022-09-08 9:31 UTC (permalink / raw)
To: xen-devel
Cc: Henry Wang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
In order to keep consistency in the device tree binding, there is
no need for static memory allocation feature to define a specific
set of address and size cells for "xen,static-mem" property.
Therefore, this commit reuses the regular #{address,size}-cells
for parsing the device tree "xen,static-mem" property. Update
the documentation accordingly.
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
docs/misc/arm/device-tree/booting.txt | 13 ++++++-------
docs/misc/arm/passthrough-noiommu.txt | 7 +++----
xen/arch/arm/bootfdt.c | 5 -----
xen/arch/arm/domain_build.c | 16 ++--------------
4 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..12d77e3b26 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -339,15 +339,15 @@ kernel will be able to discover the device.
Static Allocation
-=============
+=================
Static Allocation refers to system or sub-system(domains) for which memory
areas are pre-defined by configuration using physical address ranges.
Memory can be statically allocated to a domain using the property "xen,static-
mem" defined in the domain configuration. The number of cells for the address
-and the size must be defined using respectively the properties
-"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".
+and the size must be defined respectively by the parent node properties
+"#address-cells" and "#size-cells".
The property 'memory' is still needed and should match the amount of memory
given to the guest. Currently, it either comes from static memory or lets Xen
@@ -362,14 +362,13 @@ device-tree:
/ {
chosen {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ...
domU1 {
compatible = "xen,domain";
- #address-cells = <0x2>;
- #size-cells = <0x2>;
cpus = <2>;
memory = <0x0 0x80000>;
- #xen,static-mem-address-cells = <0x1>;
- #xen,static-mem-size-cells = <0x1>;
xen,static-mem = <0x30000000 0x20000000>;
...
};
diff --git a/docs/misc/arm/passthrough-noiommu.txt b/docs/misc/arm/passthrough-noiommu.txt
index 3e2ef21ad7..69b8de1975 100644
--- a/docs/misc/arm/passthrough-noiommu.txt
+++ b/docs/misc/arm/passthrough-noiommu.txt
@@ -33,14 +33,13 @@ on static allocation in the device-tree:
/ {
chosen {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ...
domU1 {
compatible = "xen,domain";
- #address-cells = <0x2>;
- #size-cells = <0x2>;
cpus = <2>;
memory = <0x0 0x80000>;
- #xen,static-mem-address-cells = <0x1>;
- #xen,static-mem-size-cells = <0x1>;
xen,static-mem = <0x30000000 0x20000000>;
direct-map;
...
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..cd264793d5 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -352,11 +352,6 @@ static int __init process_domain_node(const void *fdt, int node,
/* No "xen,static-mem" present. */
return 0;
- address_cells = device_tree_get_u32(fdt, node,
- "#xen,static-mem-address-cells", 0);
- size_cells = device_tree_get_u32(fdt, node,
- "#xen,static-mem-size-cells", 0);
-
return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
size_cells, &bootinfo.reserved_mem, true);
}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b76a84e8f5..258d74699d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const struct dt_device_node *node,
const struct dt_property *prop;
prop = dt_find_property(node, "xen,static-mem", NULL);
- if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
- addr_cells) )
- {
- printk(XENLOG_ERR
- "failed to read \"#xen,static-mem-address-cells\".\n");
- return -EINVAL;
- }
- if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
- size_cells) )
- {
- printk(XENLOG_ERR
- "failed to read \"#xen,static-mem-size-cells\".\n");
- return -EINVAL;
- }
+ *addr_cells = dt_n_addr_cells(node);
+ *size_cells = dt_n_size_cells(node);
*cell = (const __be32 *)prop->value;
*length = prop->length;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells
2022-09-08 9:31 [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells Henry Wang
@ 2022-09-08 10:38 ` Michal Orzel
2022-09-08 10:54 ` Henry Wang
0 siblings, 1 reply; 5+ messages in thread
From: Michal Orzel @ 2022-09-08 10:38 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Henry,
On 08/09/2022 11:31, Henry Wang wrote:
>
> In order to keep consistency in the device tree binding, there is
> no need for static memory allocation feature to define a specific
> set of address and size cells for "xen,static-mem" property.
>
> Therefore, this commit reuses the regular #{address,size}-cells
> for parsing the device tree "xen,static-mem" property. Update
> the documentation accordingly.
>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> docs/misc/arm/device-tree/booting.txt | 13 ++++++-------
> docs/misc/arm/passthrough-noiommu.txt | 7 +++----
> xen/arch/arm/bootfdt.c | 5 -----
> xen/arch/arm/domain_build.c | 16 ++--------------
> 4 files changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..12d77e3b26 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -339,15 +339,15 @@ kernel will be able to discover the device.
>
>
> Static Allocation
> -=============
> +=================
>
> Static Allocation refers to system or sub-system(domains) for which memory
> areas are pre-defined by configuration using physical address ranges.
>
> Memory can be statically allocated to a domain using the property "xen,static-
> mem" defined in the domain configuration. The number of cells for the address
> -and the size must be defined using respectively the properties
> -"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".
> +and the size must be defined respectively by the parent node properties
> +"#address-cells" and "#size-cells".
>
> The property 'memory' is still needed and should match the amount of memory
> given to the guest. Currently, it either comes from static memory or lets Xen
> @@ -362,14 +362,13 @@ device-tree:
>
> / {
> chosen {
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + ...
> domU1 {
> compatible = "xen,domain";
> - #address-cells = <0x2>;
> - #size-cells = <0x2>;
Why did you remove this set if it relates to the childs of domU1 (e.g. kernel, ramdisk) and not to domU1 itself?
> cpus = <2>;
> memory = <0x0 0x80000>;
> - #xen,static-mem-address-cells = <0x1>;
> - #xen,static-mem-size-cells = <0x1>;
> xen,static-mem = <0x30000000 0x20000000>;
> ...
> };
> diff --git a/docs/misc/arm/passthrough-noiommu.txt b/docs/misc/arm/passthrough-noiommu.txt
> index 3e2ef21ad7..69b8de1975 100644
> --- a/docs/misc/arm/passthrough-noiommu.txt
> +++ b/docs/misc/arm/passthrough-noiommu.txt
> @@ -33,14 +33,13 @@ on static allocation in the device-tree:
>
> / {
> chosen {
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + ...
> domU1 {
> compatible = "xen,domain";
> - #address-cells = <0x2>;
> - #size-cells = <0x2>;
The same here.
> cpus = <2>;
> memory = <0x0 0x80000>;
> - #xen,static-mem-address-cells = <0x1>;
> - #xen,static-mem-size-cells = <0x1>;
> xen,static-mem = <0x30000000 0x20000000>;
> direct-map;
> ...
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..cd264793d5 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -352,11 +352,6 @@ static int __init process_domain_node(const void *fdt, int node,
> /* No "xen,static-mem" present. */
> return 0;
>
> - address_cells = device_tree_get_u32(fdt, node,
> - "#xen,static-mem-address-cells", 0);
> - size_cells = device_tree_get_u32(fdt, node,
> - "#xen,static-mem-size-cells", 0);
> -
> return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
> size_cells, &bootinfo.reserved_mem, true);
> }
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b76a84e8f5..258d74699d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const struct dt_device_node *node,
> const struct dt_property *prop;
>
> prop = dt_find_property(node, "xen,static-mem", NULL);
> - if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> - addr_cells) )
> - {
> - printk(XENLOG_ERR
> - "failed to read \"#xen,static-mem-address-cells\".\n");
> - return -EINVAL;
> - }
>
> - if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> - size_cells) )
> - {
> - printk(XENLOG_ERR
> - "failed to read \"#xen,static-mem-size-cells\".\n");
> - return -EINVAL;
> - }
> + *addr_cells = dt_n_addr_cells(node);
> + *size_cells = dt_n_size_cells(node);
There is a type mismatch here as e.g. addr_cells is u32 and dt_n_addr_cells return type is int.
But I don't think this can be harmful and also it's strange for me that dt_n_addr_cells
is defined to return int given that it either returns 2 or be32_to_cpup, which means it should return u32.
>
> *cell = (const __be32 *)prop->value;
> *length = prop->length;
> --
> 2.17.1
>
>
Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells
2022-09-08 10:38 ` Michal Orzel
@ 2022-09-08 10:54 ` Henry Wang
2022-09-08 11:58 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Henry Wang @ 2022-09-08 10:54 UTC (permalink / raw)
To: Michal Orzel, xen-devel, Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
Thank you very much for your review, as always :))
> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> > @@ -362,14 +362,13 @@ device-tree:
> >
> > / {
> > chosen {
> > + #address-cells = <0x1>;
> > + #size-cells = <0x1>;
> > + ...
> > domU1 {
> > compatible = "xen,domain";
> > - #address-cells = <0x2>;
> > - #size-cells = <0x2>;
> Why did you remove this set if it relates to the childs of domU1 (e.g. kernel,
> ramdisk) and not to domU1 itself?
Well, I think here the example is just how we setup the static memory, so we just
want to emphasize the related part. I agree users can add another #address-cells
and #size-cells for domU1 node for the parts that you mentioned, but that is
not reflected by the current example (I can't find anything related to kernel,
ramdisk, etc. in current example). I might get it wrong but having two #address-cells
and #size-cells in that case would be quite misleading from my understanding.
So I decided to remove it.
But I am open to other opinions. So shall we let the discussion go on for a bit longer?
I will revert this change if most people think this removing is unnecessary.
>
> > cpus = <2>;
> > memory = <0x0 0x80000>;
> > - #xen,static-mem-address-cells = <0x1>;
> > - #xen,static-mem-size-cells = <0x1>;
> > xen,static-mem = <0x30000000 0x20000000>;
> > ...
> > };
> > diff --git a/docs/misc/arm/passthrough-noiommu.txt
> b/docs/misc/arm/passthrough-noiommu.txt
> > index 3e2ef21ad7..69b8de1975 100644
> > --- a/docs/misc/arm/passthrough-noiommu.txt
> > +++ b/docs/misc/arm/passthrough-noiommu.txt
> > @@ -33,14 +33,13 @@ on static allocation in the device-tree:
> >
> > / {
> > chosen {
> > + #address-cells = <0x1>;
> > + #size-cells = <0x1>;
> > + ...
> > domU1 {
> > compatible = "xen,domain";
> > - #address-cells = <0x2>;
> > - #size-cells = <0x2>;
> The same here.
Same as above.
>
> > cpus = <2>;
> > memory = <0x0 0x80000>;
> > - #xen,static-mem-address-cells = <0x1>;
> > - #xen,static-mem-size-cells = <0x1>;
> > xen,static-mem = <0x30000000 0x20000000>;
> > direct-map;
> > ...
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index ec81a45de9..cd264793d5 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -352,11 +352,6 @@ static int __init process_domain_node(const void
> *fdt, int node,
> > /* No "xen,static-mem" present. */
> > return 0;
> >
> > - address_cells = device_tree_get_u32(fdt, node,
> > - "#xen,static-mem-address-cells", 0);
> > - size_cells = device_tree_get_u32(fdt, node,
> > - "#xen,static-mem-size-cells", 0);
> > -
> > return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> > size_cells, &bootinfo.reserved_mem, true);
> > }
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b76a84e8f5..258d74699d 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const
> struct dt_device_node *node,
> > const struct dt_property *prop;
> >
> > prop = dt_find_property(node, "xen,static-mem", NULL);
> > - if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> > - addr_cells) )
> > - {
> > - printk(XENLOG_ERR
> > - "failed to read \"#xen,static-mem-address-cells\".\n");
> > - return -EINVAL;
> > - }
> >
> > - if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> > - size_cells) )
> > - {
> > - printk(XENLOG_ERR
> > - "failed to read \"#xen,static-mem-size-cells\".\n");
> > - return -EINVAL;
> > - }
> > + *addr_cells = dt_n_addr_cells(node);
> > + *size_cells = dt_n_size_cells(node);
> There is a type mismatch here as e.g. addr_cells is u32 and dt_n_addr_cells
> return type is int.
> But I don't think this can be harmful and also it's strange for me that
> dt_n_addr_cells
> is defined to return int given that it either returns 2 or be32_to_cpup, which
> means it should return u32.
Yeah. I agree. I did a git blame here and found this function is introduced 9
years ago in "dbd1243 xen/arm: Add helpers to use the device tree". So I think
probably it would be easier to ask the author for the following action directly :))
@Julien, what do you think? Shall we modify the return type of these two
functions?
>
> >
> > *cell = (const __be32 *)prop->value;
> > *length = prop->length;
> > --
> > 2.17.1
> >
> >
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Thanks!
Kind regards,
Henry
>
> ~Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells
2022-09-08 10:54 ` Henry Wang
@ 2022-09-08 11:58 ` Julien Grall
2022-09-08 12:20 ` Henry Wang
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2022-09-08 11:58 UTC (permalink / raw)
To: Henry Wang, Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 08/09/2022 11:54, Henry Wang wrote:
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>>> @@ -362,14 +362,13 @@ device-tree:
>>>
>>> / {
>>> chosen {
>>> + #address-cells = <0x1>;
>>> + #size-cells = <0x1>;
>>> + ...
>>> domU1 {
>>> compatible = "xen,domain";
>>> - #address-cells = <0x2>;
>>> - #size-cells = <0x2>;
>> Why did you remove this set if it relates to the childs of domU1 (e.g. kernel,
>> ramdisk) and not to domU1 itself?
>
> Well, I think here the example is just how we setup the static memory, so we just
> want to emphasize the related part. I agree users can add another #address-cells
> and #size-cells for domU1 node for the parts that you mentioned, but that is
> not reflected by the current example (I can't find anything related to kernel,
> ramdisk, etc. in current example). I might get it wrong but having two #address-cells
> and #size-cells in that case would be quite misleading from my understanding.
I agree with that. As this is only a small part of the DT we want to
focus on what is necessary for the current section.
> So I decided to remove it.
I would mention it in the commit message because the change seems
unrelated otherwise.
The same apply for replacing adding extra "====". But TBH, this change
feels completely unrelated to this patch. So I think it is better to
have a separate patch.
[...]
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index ec81a45de9..cd264793d5 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -352,11 +352,6 @@ static int __init process_domain_node(const void
>> *fdt, int node,
>>> /* No "xen,static-mem" present. */
>>> return 0;
>>>
>>> - address_cells = device_tree_get_u32(fdt, node,
>>> - "#xen,static-mem-address-cells", 0);
>>> - size_cells = device_tree_get_u32(fdt, node,
>>> - "#xen,static-mem-size-cells", 0);
>>> -
>>> return device_tree_get_meminfo(fdt, node, "xen,static-mem",
>> address_cells,
>>> size_cells, &bootinfo.reserved_mem, true);
>>> }
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index b76a84e8f5..258d74699d 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const
>> struct dt_device_node *node,
>>> const struct dt_property *prop;
>>>
>>> prop = dt_find_property(node, "xen,static-mem", NULL);
>>> - if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
>>> - addr_cells) )
>>> - {
>>> - printk(XENLOG_ERR
>>> - "failed to read \"#xen,static-mem-address-cells\".\n");
>>> - return -EINVAL;
>>> - }
>>>
>>> - if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
>>> - size_cells) )
>>> - {
>>> - printk(XENLOG_ERR
>>> - "failed to read \"#xen,static-mem-size-cells\".\n");
>>> - return -EINVAL;
>>> - }
>>> + *addr_cells = dt_n_addr_cells(node);
>>> + *size_cells = dt_n_size_cells(node);
>> There is a type mismatch here as e.g. addr_cells is u32 and dt_n_addr_cells
>> return type is int.
>> But I don't think this can be harmful and also it's strange for me that
>> dt_n_addr_cells
>> is defined to return int given that it either returns 2 or be32_to_cpup, which
>> means it should return u32.
>
> Yeah. I agree. I did a git blame here and found this function is introduced 9
> years ago in "dbd1243 xen/arm: Add helpers to use the device tree". So I think
> probably it would be easier to ask the author for the following action directly :))
The code was imported from Linux where it seems to be more common to use
"int" rather than "unsigned".
>
> @Julien, what do you think? Shall we modify the return type of these two
> functions?
I think it would be good to be consistent. However, there are other
users of d_n_addr_cells() (some are expecting 'int'). So if you switch
to a different type then this use will be consistent but not the others.
I would only suggest to look at it if you have if you have copious time
and fancy going down the rabbit hole :).
As to which type to chose, we are phasing out use of uXX in new code. So
this should be 'uint32_t'. I would also be fine to use 'unsigned int'
for the outside interface.
I don't have a strong opinion either way.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells
2022-09-08 11:58 ` Julien Grall
@ 2022-09-08 12:20 ` Henry Wang
0 siblings, 0 replies; 5+ messages in thread
From: Henry Wang @ 2022-09-08 12:20 UTC (permalink / raw)
To: Julien Grall, Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien and Michal,
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> >>> / {
> >>> chosen {
> >>> + #address-cells = <0x1>;
> >>> + #size-cells = <0x1>;
> >>> + ...
> >>> domU1 {
> >>> compatible = "xen,domain";
> >>> - #address-cells = <0x2>;
> >>> - #size-cells = <0x2>;
> >> Why did you remove this set if it relates to the childs of domU1 (e.g.
> kernel,
> >> ramdisk) and not to domU1 itself?
> >
> > Well, I think here the example is just how we setup the static memory, so
> we just
> > want to emphasize the related part. I agree users can add another
> #address-cells
> > and #size-cells for domU1 node for the parts that you mentioned, but that
> is
> > not reflected by the current example (I can't find anything related to kernel,
> > ramdisk, etc. in current example). I might get it wrong but having two
> #address-cells
> > and #size-cells in that case would be quite misleading from my
> understanding.
>
> I agree with that. As this is only a small part of the DT we want to
> focus on what is necessary for the current section.
>
> > So I decided to remove it.
>
> I would mention it in the commit message because the change seems
> unrelated otherwise.
Sure.
>
> The same apply for replacing adding extra "====". But TBH, this change
> feels completely unrelated to this patch. So I think it is better to
> have a separate patch.
I would revert this change for "====", as sending a patch adding
"====" seems to not make too much sense.....
>
> [...]
>
> >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> >>> index ec81a45de9..cd264793d5 100644
> >>> --- a/xen/arch/arm/bootfdt.c
> >>> +++ b/xen/arch/arm/bootfdt.c
> >>> @@ -352,11 +352,6 @@ static int __init process_domain_node(const
> void
> >> *fdt, int node,
> >>> /* No "xen,static-mem" present. */
> >>> return 0;
> >>>
> >>> - address_cells = device_tree_get_u32(fdt, node,
> >>> - "#xen,static-mem-address-cells", 0);
> >>> - size_cells = device_tree_get_u32(fdt, node,
> >>> - "#xen,static-mem-size-cells", 0);
> >>> -
> >>> return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> >> address_cells,
> >>> size_cells, &bootinfo.reserved_mem, true);
> >>> }
> >>> diff --git a/xen/arch/arm/domain_build.c
> b/xen/arch/arm/domain_build.c
> >>> index b76a84e8f5..258d74699d 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const
> >> struct dt_device_node *node,
> >>> const struct dt_property *prop;
> >>>
> >>> prop = dt_find_property(node, "xen,static-mem", NULL);
> >>> - if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> >>> - addr_cells) )
> >>> - {
> >>> - printk(XENLOG_ERR
> >>> - "failed to read \"#xen,static-mem-address-cells\".\n");
> >>> - return -EINVAL;
> >>> - }
> >>>
> >>> - if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> >>> - size_cells) )
> >>> - {
> >>> - printk(XENLOG_ERR
> >>> - "failed to read \"#xen,static-mem-size-cells\".\n");
> >>> - return -EINVAL;
> >>> - }
> >>> + *addr_cells = dt_n_addr_cells(node);
> >>> + *size_cells = dt_n_size_cells(node);
> >> There is a type mismatch here as e.g. addr_cells is u32 and
> dt_n_addr_cells
> >> return type is int.
> >> But I don't think this can be harmful and also it's strange for me that
> >> dt_n_addr_cells
> >> is defined to return int given that it either returns 2 or be32_to_cpup,
> which
> >> means it should return u32.
> >
> > Yeah. I agree. I did a git blame here and found this function is introduced 9
> > years ago in "dbd1243 xen/arm: Add helpers to use the device tree". So I
> think
> > probably it would be easier to ask the author for the following action
> directly :))
>
> The code was imported from Linux where it seems to be more common to
> use
> "int" rather than "unsigned".
>
> >
> > @Julien, what do you think? Shall we modify the return type of these two
> > functions?
>
> I think it would be good to be consistent. However, there are other
> users of d_n_addr_cells() (some are expecting 'int'). So if you switch
> to a different type then this use will be consistent but not the others.
>
> I would only suggest to look at it if you have if you have copious time
> and fancy going down the rabbit hole :).
>
> As to which type to chose, we are phasing out use of uXX in new code. So
> this should be 'uint32_t'. I would also be fine to use 'unsigned int'
> for the outside interface.
>
> I don't have a strong opinion either way.
I personally would like to keep the current way, changing the type to me
sounds like out of scope with this patch and will bring us the risk of breaking
other places.
So I will address the commit message and "====" changes and send a v2.
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-08 12:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-08 9:31 [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells Henry Wang
2022-09-08 10:38 ` Michal Orzel
2022-09-08 10:54 ` Henry Wang
2022-09-08 11:58 ` Julien Grall
2022-09-08 12:20 ` Henry Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.