diff mbox series

[v2,2/4] xen/arm: bootfdt: Make process_chosen_node() return int

Message ID 20220905072635.16294-3-Henry.Wang@arm.com (mailing list archive)
State New, archived
Headers show
Series Introduce reserved heap | expand

Commit Message

Henry Wang Sept. 5, 2022, 7:26 a.m. UTC 
At the boot time, it is saner to stop booting early if an error occurs
when parsing the device tree chosen node, rather than seeing random
behavior afterwards. Therefore, this commit changes the return type of
the process_chosen_node() from void to int, and return correct errno
based on the error type.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
Changes from v1 to v2:
- New commit.
---
 xen/arch/arm/bootfdt.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Michal Orzel Sept. 5, 2022, 9:16 a.m. UTC | #1 
Hi Henry,

On 05/09/2022 09:26, Henry Wang wrote:
> 
> At the boot time, it is saner to stop booting early if an error occurs
> when parsing the device tree chosen node, rather than seeing random
> behavior afterwards. Therefore, this commit changes the return type of
> the process_chosen_node() from void to int, and return correct errno
> based on the error type.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> Changes from v1 to v2:
> - New commit.
> ---

The patch looks good but I think it should be put as the first one in the series
as this is a natural prerequisite for the introduction of the reserved heap. This
will also help not to modify the same reserved heap code you introduced in the first patch.

~Michal
Julien Grall Sept. 5, 2022, 9:17 a.m. UTC | #2 
On 05/09/2022 10:16, Michal Orzel wrote:
> Hi Henry,
> 
> On 05/09/2022 09:26, Henry Wang wrote:
>>
>> At the boot time, it is saner to stop booting early if an error occurs
>> when parsing the device tree chosen node, rather than seeing random
>> behavior afterwards. Therefore, this commit changes the return type of
>> the process_chosen_node() from void to int, and return correct errno
>> based on the error type.
>>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>> ---
>> Changes from v1 to v2:
>> - New commit.
>> ---
> 
> The patch looks good but I think it should be put as the first one in the series
> as this is a natural prerequisite for the introduction of the reserved heap. This
> will also help not to modify the same reserved heap code you introduced in the first patch.

+1. The same could be said with patch #4. The renaming should be before 
patch #3.

Cheers,
Henry Wang Sept. 5, 2022, 9:21 a.m. UTC | #3 
Hi Julien and Michal,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> >> Changes from v1 to v2:
> >> - New commit.
> >> ---
> >
> > The patch looks good but I think it should be put as the first one in the
> series
> > as this is a natural prerequisite for the introduction of the reserved heap.
> This
> > will also help not to modify the same reserved heap code you introduced in
> the first patch.
> 
> +1. The same could be said with patch #4. The renaming should be before
> patch #3.

Cool, I will send a v3 with these adjusted. Will send after fixing more possible
comments for v2.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 5af71dc8ba..3796a4bd75 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -293,9 +293,9 @@  static void __init process_multiboot_node(const void *fdt, int node,
                      kind, start, domU);
 }
 
-static void __init process_chosen_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
+static int __init process_chosen_node(const void *fdt, int node,
+                                      const char *name,
+                                      u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
     paddr_t start, end;
@@ -303,6 +303,7 @@  static void __init process_chosen_node(const void *fdt, int node,
 
     if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
     {
+        int rc;
         u32 address_cells = device_tree_get_u32(fdt, node,
                                 "#xen,static-heap-address-cells", 0);
         u32 size_cells = device_tree_get_u32(fdt, node,
@@ -313,14 +314,14 @@  static void __init process_chosen_node(const void *fdt, int node,
         {
             printk("fdt: node `%s': invalid #xen,static-heap-address-cells or #xen,static-heap-size-cells\n",
                    name);
-            return;
+            return -EINVAL;
         }
 
-        if ( device_tree_get_meminfo(fdt, node, "xen,static-heap",
+        rc = device_tree_get_meminfo(fdt, node, "xen,static-heap",
                                      address_cells, size_cells,
-                                     &bootinfo.reserved_mem,
-                                     MEMBANK_RSVD_HEAP) )
-            return;
+                                     &bootinfo.reserved_mem, MEMBANK_RSVD_HEAP);
+        if ( rc )
+            return rc;
     }
 
     printk("Checking for initrd in /chosen\n");
@@ -328,11 +329,11 @@  static void __init process_chosen_node(const void *fdt, int node,
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
     if ( !prop )
         /* No initrd present. */
-        return;
+        return 0;
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-start property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -340,12 +341,12 @@  static void __init process_chosen_node(const void *fdt, int node,
     if ( !prop )
     {
         printk("linux,initrd-end not present but -start was\n");
-        return;
+        return -EINVAL;
     }
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-end property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -353,12 +354,14 @@  static void __init process_chosen_node(const void *fdt, int node,
     {
         printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
                   start, end);
-        return;
+        return -EINVAL;
     }
 
     printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
 
     add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
+
+    return 0;
 }
 
 static int __init process_domain_node(const void *fdt, int node,
@@ -406,7 +409,7 @@  static int __init early_scan_node(const void *fdt,
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
-        process_chosen_node(fdt, node, name, address_cells, size_cells);
+        rc = process_chosen_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 2 && device_tree_node_compatible(fdt, node, "xen,domain") )
         rc = process_domain_node(fdt, node, name, address_cells, size_cells);