Re: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
> ---
>  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@xxxxxxx>

~Michal