Re: [Minios-devel] [PATCH v3 33/43] arm64: parse out the address/size for gicd/gicc

[Julien Grall <julien.grall@xxxxxxx>]

On 16/04/18 07:32, Huang Shijie wrote:
> This patch parses out the address/size for gicd/gicc.
The code was already parsing address/size for gicd/gicc. So what's the 
difference?
> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> ---
>   arch/arm/gic.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 71 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/gic.c b/arch/arm/gic.c
> index 1e37cdc..687f242 100644
> --- a/arch/arm/gic.c
> +++ b/arch/arm/gic.c
> @@ -177,6 +177,72 @@ static void gic_handler(void) {
>       gic_eoir(&gic, irq);
>   }
>   
> +/*
> + * Parse the "reg" property.
> + *
> + * Note: *regp will increase.
> + */
> +static uint64_t parse_reg(const uint32_t **regp, uint32_t cell)
> +{
> +    uint32_t i;
> +    uint32_t buf[2];
> +    uint64_t *buf64 = (uint64_t *)buf;
The cast is quite dangerous, you are going to unchartered territory with 
such a cast (this is undefined by the C spec). Also, if cell is 1, 
buf[0] will not get initialized to 0. So would get garbagge.
AFAICT, mini-OS does not compile with -fno-strict-aliasing. So the best 
solution here is to use shift to prevent undefined behavior.
Something like:

uint64_t val = 0;

BUG_ON(!cell || cell > 2);

/* Assumption that cell > 0 */
if ( cell == 2 )
{
  val = fdt32_to_cpu(reg[i + 1]);
  val <<= 32;
}

val |= fdt32_to_cpu(reg[i]);

return val;

> +    uint32_t *reg = (uint32_t *)(*regp);
It would be nicer if you use fdt*_t when you get value from the 
Device-Tree. This would help differentiate DT value (in big endian) vs 
Mini-OS value (in little endian).
But why do you need the cast here? Is it because you are removing the 
const? If so why removing the const?
> +
> +    if (cell > 2)
Coding style:

if ( ... )

> +        BUG();
Please use BUG_ON(cell > 2);

> +
> +    for (i = 0; i < cell; i++)
Coding style.

> +    {
> +        buf[i] = reg[i];
> +    }
NIT: {} are not necessary.

> +    *regp = reg + cell;
Newline here please.

> +    return fdt64_to_cpu(buf64[0]);
> +}
> +
> +/*
> + * Parse out the address/size for gicd/gicc.
> + *
> + * Return 0 on success; return 1 on error.
Returning 1 on error is slightly weird. It would be better to return a 
negative value. But as you only return 2 distinct values it would make 
sense to use bool here.
> + */
> +static int gic_parse(int node, uint64_t *gicd_addr, uint64_t *gicd_size,
> +                               uint64_t *gicc_addr, uint64_t *gicc_size)
The indentation looks wrong here.

Also, looking at the function you don't handle "ranges" property. This 
will be used to handle different address space. If you are going to 
handle DT parsing, then you should do it properly or at least describing 
your assumption.
> +{
> +    uint32_t addr_cells = 2, size_cells = 1; /* The default, refer to Spec. */
> +    const uint32_t *reg32;
> +    int pnode;
> +
> +    pnode = fdt_parent_offset(device_tree, node);
> +    if (pnode < 0)
Coding style.

> +         return 1;
> +
> +    reg32 = fdt_getprop(device_tree, pnode, "#address-cells", NULL);
> +    if (reg32)
Ditto.

> +         addr_cells = fdt32_to_cpu(reg32[0]);
> +
> +    reg32 = fdt_getprop(device_tree, pnode, "#size-cells", NULL);
> +    if (reg32)
Ditto.

> +         size_cells = fdt32_to_cpu(reg32[0]);
> +
> +    if (addr_cells > 2 || size_cells > 2)
Ditto

> +    {
> +         printk("Unsupported #address-cells: %d, #size-cells: %d\n",
> +                addr_cells, size_cells);
> +        return 1;
The indentation looks wrong here.

> +    }
> +
> +    reg32 = fdt_getprop(device_tree, node, "reg", NULL);
> +    if (reg32)
Coding style.

> +    {
> +         *gicd_addr = parse_reg(&reg32, addr_cells);
> +         *gicd_size = parse_reg(&reg32, size_cells);
> +         *gicc_addr = parse_reg(&reg32, addr_cells);
> +         *gicc_size = parse_reg(&reg32, size_cells);
> +    }
> +
> +    return 0;
> +}
> +
>   void gic_init(void) {
>       gic.gicd_base = NULL;
>       int node = 0;
> @@ -188,7 +254,7 @@ void gic_init(void) {
>               break;
>   
>           if (fdt_getprop(device_tree, node, "interrupt-controller", NULL)) {
> -            int len = 0;
> +            uint64_t gicd_addr, gicd_size, gicc_addr, gicc_size;
>   
>               if (fdt_node_check_compatible(device_tree, node, "arm,cortex-a15-gic") &&
>                   fdt_node_check_compatible(device_tree, node, 
> "arm,cortex-a7-gic")) {
> @@ -196,21 +262,11 @@ void gic_init(void) {
>                   continue;
>               }
>   
> -            const uint64_t *reg = fdt_getprop(device_tree, node, "reg", &len);
> -
> -            /* We have two registers (GICC and GICD), each of which contains
> -             * two parts (an address and a size), each of which is a 64-bit
> -             * value (8 bytes), so we expect a length of 2 * 2 * 8 = 32.
> -             * If any extra values are passed in future, we ignore them. */
> -            if (reg == NULL || len < 32) {
> -                printk("Bad 'reg' property: %p %d\n", reg, len);
> -                continue;
> -            }
> +            if (gic_parse(node, &gicd_addr, &gicd_size, &gicc_addr, 
> &gicc_size))
> +                   continue;
>   
> -            gic.gicd_base = ioremap((unsigned long) fdt64_to_cpu(reg[0]),
> -                                    (unsigned long) fdt64_to_cpu(reg[1]));
> -            gic.gicc_base = ioremap((unsigned long) fdt64_to_cpu(reg[2]),
> -                                    (unsigned long) fdt64_to_cpu(reg[3]));
> +            gic.gicd_base = ioremap(gicd_addr, gicd_size);
> +            gic.gicc_base = ioremap(gicc_addr, gicc_size);
>               printk("Found GIC: gicd_base = %p, gicc_base = %p\n", 
> gic.gicd_base, gic.gicc_base);
>               break;
>           }
Cheers,

--
Julien Grall

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