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 <[email protected]>
> ---
> 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 <[email protected]>
~Michal
