| Message ID | 20220908093129.30124-1-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm, device-tree: Make static-mem use #{address,size}-cells | expand |
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
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
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,
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
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;
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(-)