Re: [Minios-devel] [UNIKRAFT PATCHv3 3/6] plat/common: Introduce fdt_get_address helper

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 3/21/19 8:05 AM, Jia He wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
> This helper will be used very frequently for device libraries
> to parse their addresses. Introduce this helper to avoid using
> fdt_address_cells and fdt_size_cells everywhere.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/common/arm/irq_fdt.c         | 65 +++++++++++++++++++++++++++++++
>   plat/common/include/arm/irq_fdt.h | 19 +++++++++
>   2 files changed, 84 insertions(+)
>
> diff --git a/plat/common/arm/irq_fdt.c b/plat/common/arm/irq_fdt.c
> index 9b4bd48..90260c4 100644
> --- a/plat/common/arm/irq_fdt.c
> +++ b/plat/common/arm/irq_fdt.c
> @@ -38,6 +38,8 @@
>   #include <libfdt.h>
>   
>   #include "libfdt_internal.h"
> +#include "uk/print.h"
> +
>   int fdt_getprop_u32_by_offset(const void *fdt, int offset,
>                 const char *name, uint32_t *out)
>   {
> @@ -97,3 +99,66 @@ int fdt_interrupt_cells(const void *fdt, int offset)
>   
>   	return val;
>   }
> +
> +static uint64_t fdt_reg_read_number(const fdt32_t *regs, uint32_t size)
> +{
> +       uint64_t number = 0;
> +
> +       if (size >= 3 || size <= 0)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       for (uint32_t i = 0; i < size; i++) {
> +               number <<= 32;
> +               number |= fdt32_to_cpu(*regs);
> +               regs++;
> +       }
> +
> +       return number;
> +}
> +
> +int fdt_get_address(const void *fdt, int nodeoffset, int index,
NIT: Please use "unsigned int" if you number cannot be signed. For 
instance, I doubt you support signed index.
> +                       uint64_t *addr, uint64_t *size)
> +{
> +       int len, prop_addr, prop_size;
> +       int naddr, nsize, term_size;
> +       const void *regs;
> +       const uint32_t *ranges;
> +
> +       naddr = fdt_address_cells(fdt, nodeoffset);
AFAICT, fdt_address_cells will return the #address-cells property for 
the given node. However, here, you want the #address-cells of the parent.
> +       if (naddr < 0 || naddr >= FDT_MAX_NCELLS)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       nsize = fdt_size_cells(fdt, nodeoffset);
Same here. Furthermore, the default value returned by fdt_size_cells 
will not match the DT spec. The spec requires to return 1 and not 2.
> +       if (nsize < 0 || nsize >= FDT_MAX_NCELLS)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       /* Handle ranges property */
> +       ranges = fdt_getprop(fdt, nodeoffset, "ranges", &len);
> +       if (ranges == NULL) {
> +               uk_pr_warn("ranges property is not found\n");
Similar as above, this should be lookup in the parent. Also, the "root" 
node does not contain a property "ranges". So you will not be able to 
translate node that are directly under the "root" node.
A lot of the comments here could have been caught by reading DTS example 
along with the spec and testing the code. Can you please make sure you 
do the both before sending a new version.

> +               return -FDT_ERR_NOTFOUND;
> +       }
> +       /* Only 1:1 translation is supported */
This check is a bit fragile. It only promises you the address space of 
this bus and the address space of this bus node's parent are the same. 
It does not tell you if the bus above you is using a different address 
space.
> +       if (len != 0) {
> +               uk_pr_warn("Only 1:1 translation is supported\n");
> +               return -FDT_ERR_BADVALUE;
> +       }
> +
> +       /* Get reg content */
> +       regs = fdt_getprop(fdt, nodeoffset, "reg", &len);
> +       if (regs == NULL)
> +               return -FDT_ERR_NOTFOUND;
> +
> +       term_size = (int)sizeof(fdt32_t) * (nsize + naddr);
Why do you need to cast to int?

> +       prop_addr = term_size * index;
> +       prop_size = prop_addr + (int)sizeof(fdt32_t) * naddr;
Same here.

> +
> +       /* The reg content must cover the reg term[index] at least */
> +       if (len < (prop_addr + term_size))
> +               return -FDT_ERR_NOSPACE;
> +
> +       *addr = fdt_reg_read_number(regs + prop_addr, naddr);
> +       *size = fdt_reg_read_number(regs + prop_size, nsize);
> +
> +       return 0;
> +}
> diff --git a/plat/common/include/arm/irq_fdt.h 
> b/plat/common/include/arm/irq_fdt.h
> index 4b76c98..2637a58 100644
> --- a/plat/common/include/arm/irq_fdt.h
> +++ b/plat/common/include/arm/irq_fdt.h
> @@ -73,3 +73,22 @@ int fdt_getprop_u32_by_offset(const void *fdt, int 
> nodeoffset,
>    *     -FDT_ERR_TRUNCATED, standard meanings
>    */
>   int fdt_interrupt_cells(const void *fdt, int nodeoffset);
> +
> +/**
> + * fdt_get_address - retrieve device address of a given index
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node to find the address for.
> + * @index: index of region
> + * @addr: return the region address
> + * @size: return the region size
> + *
> + * returns:
> + *     0, on success
> + *      -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> + *             address property
> + *      -FDT_ERR_NOTFOUND, if the node doesn't have address property
> + *      -FDT_ERR_NOSPACE, if the node doesn't have address for index
> + */
> +int fdt_get_address(const void *fdt, int nodeoffset, int index,
> +                       uint64_t *addr, uint64_t *size);
> +
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel