[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.