[PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Leif Lindholm]

Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
systems when creating an empty DT at boot time. This resolves an issue
seen in the wild with kexec on certain 64-bit ARM systems.

First part is moving out the prop_entry_size macro from lib/fdt.c
and make it available in <grub/fdt.h> (with the required name
change).

Second part is adding the two properties to the empty tree.

Leif Lindholm (2):
  fdt: move prop_entry_size to fdt.h
  efi/fdt: set address/size cells to 2 for empty tree

 grub-core/lib/fdt.c        | 25 ++++++++++---------------
 grub-core/loader/efi/fdt.c | 18 ++++++++++++++++--
 include/grub/fdt.h         |  5 +++++
 3 files changed, 31 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH 1/2] fdt: move prop_entry_size to fdt.h

[Leif Lindholm]

To be able to resuse the prop_entry_size macro, move it to
<grub/fdt.h> and rename it grub_fdt_prop_entry_size.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/lib/fdt.c | 25 ++++++++++---------------
 include/grub/fdt.h  |  5 +++++
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/grub-core/lib/fdt.c b/grub-core/lib/fdt.c
index 2705f2629..0d371c563 100644
--- a/grub-core/lib/fdt.c
+++ b/grub-core/lib/fdt.c
@@ -41,11 +41,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
 	(2 * sizeof(grub_uint32_t)	\
 	+ ALIGN_UP (grub_strlen (name) + 1, sizeof(grub_uint32_t)))
 
-/* Size needed by a property entry: 1 token (FDT_PROPERTY), plus len and nameoff
-   fields, plus the property value, plus padding if needed. */
-#define prop_entry_size(prop_len)	\
-	(3 * sizeof(grub_uint32_t) + ALIGN_UP(prop_len, sizeof(grub_uint32_t)))
-
 #define SKIP_NODE_NAME(name, token, end)	\
   name = (char *) ((token) + 1);	\
   while (name < (char *) end)	\
@@ -86,7 +81,7 @@ static grub_uint32_t *get_next_node (const void *fdt, char *node_name)
       case FDT_PROP:
         /* Skip property token and following data (len, nameoff and property
            value). */
-        token += prop_entry_size(grub_be_to_cpu32(*(token + 1)))
+        token += grub_fdt_prop_entry_size(grub_be_to_cpu32(*(token + 1)))
                  / sizeof(*token);
         break;
       case FDT_NOP:
@@ -150,7 +145,7 @@ static int add_subnode (void *fdt, int parentoffset, const char *name)
     {
       case FDT_PROP:
         /* Skip len, nameoff and property value. */
-        token += prop_entry_size(grub_be_to_cpu32(*(token + 1)))
+        token += grub_fdt_prop_entry_size(grub_be_to_cpu32(*(token + 1)))
                  / sizeof(*token);
         break;
       case FDT_BEGIN_NODE:
@@ -249,12 +244,12 @@ static grub_uint32_t *find_prop (const void *fdt, unsigned int nodeoffset,
             && !grub_strcmp (name, (char *) fdt +
                              grub_fdt_get_off_dt_strings (fdt) + nameoff))
         {
-          if (prop + prop_entry_size(grub_be_to_cpu32(*(prop + 1)))
+          if (prop + grub_fdt_prop_entry_size(grub_be_to_cpu32(*(prop + 1)))
               / sizeof (*prop) >= end)
             return NULL;
           return prop;
         }
-        prop += prop_entry_size(grub_be_to_cpu32(*(prop + 1))) / sizeof (*prop);
+        prop += grub_fdt_prop_entry_size(grub_be_to_cpu32(*(prop + 1))) / sizeof (*prop);
       }
     else if (grub_be_to_cpu32(*prop) == FDT_NOP)
       prop++;
@@ -319,7 +314,7 @@ advance_token (const void *fdt, const grub_uint32_t *token, const grub_uint32_t
            value). */
         if (token >= end - 1)
           return 0;
-        token += prop_entry_size(grub_be_to_cpu32(*(token + 1)))
+        token += grub_fdt_prop_entry_size(grub_be_to_cpu32(*(token + 1)))
                  / sizeof(*token);
         break;
       case FDT_NOP:
@@ -467,12 +462,12 @@ int grub_fdt_set_prop (void *fdt, unsigned int nodeoffset, const char *name,
     unsigned int needed_space = 0;
 
     if (!prop)
-      needed_space = prop_entry_size(len);
+      needed_space = grub_fdt_prop_entry_size(len);
     if (!prop_name_present)
       needed_space += grub_strlen (name) + 1;
     if (needed_space > get_free_space (fdt))
       return -1;
-    if (rearrange_blocks (fdt, !prop ? prop_entry_size(len) : 0) < 0)
+    if (rearrange_blocks (fdt, !prop ? grub_fdt_prop_entry_size(len) : 0) < 0)
       return -1;
   }
   if (!prop_name_present) {
@@ -489,10 +484,10 @@ int grub_fdt_set_prop (void *fdt, unsigned int nodeoffset, const char *name,
                                 + sizeof(grub_uint32_t));
 
     prop = (void *) (node_name + ALIGN_UP(grub_strlen(node_name) + 1, 4));
-    grub_memmove (prop + prop_entry_size(len) / sizeof(*prop), prop,
+    grub_memmove (prop + grub_fdt_prop_entry_size(len) / sizeof(*prop), prop,
                   struct_end(fdt) - (grub_addr_t) prop);
     grub_fdt_set_size_dt_struct (fdt, grub_fdt_get_size_dt_struct (fdt)
-                                 + prop_entry_size(len));
+                                 + grub_fdt_prop_entry_size(len));
     *prop = grub_cpu_to_be32_compile_time (FDT_PROP);
     *(prop + 2) = grub_cpu_to_be32 (nameoff);
   }
@@ -500,7 +495,7 @@ int grub_fdt_set_prop (void *fdt, unsigned int nodeoffset, const char *name,
 
   /* Insert padding bytes at the end of the value; if they are not needed, they
      will be overwritten by the following memcpy. */
-  *(prop + prop_entry_size(len) / sizeof(grub_uint32_t) - 1) = 0;
+  *(prop + grub_fdt_prop_entry_size(len) / sizeof(grub_uint32_t) - 1) = 0;
 
   grub_memcpy (prop + 3, val, len);
   return 0;
diff --git a/include/grub/fdt.h b/include/grub/fdt.h
index 75525fa31..158b1bc4b 100644
--- a/include/grub/fdt.h
+++ b/include/grub/fdt.h
@@ -50,6 +50,11 @@ struct grub_fdt_empty_tree {
 
 #define GRUB_FDT_EMPTY_TREE_SZ  sizeof (struct grub_fdt_empty_tree)
 
+/* Size needed by a property entry: 1 token (FDT_PROPERTY), plus len and nameoff
+   fields, plus the property value, plus padding if needed. */
+#define grub_fdt_prop_entry_size(prop_len)						\
+  (3 * sizeof(grub_uint32_t) + ALIGN_UP(prop_len, sizeof(grub_uint32_t)))
+
 #define grub_fdt_get_header(fdt, field)	\
 	grub_be_to_cpu32(((const grub_fdt_header_t *)(fdt))->field)
 #define grub_fdt_set_header(fdt, field, value)	\
-- 
2.11.0

[PATCH 2/2] efi/fdt: set address/size cells to 2 for empty tree

[Leif Lindholm]

When booting an arm* system on UEFI with an empty device tree (currently
only when hardware description comes from ACPI), we don't currently set
default to 1 cell (32 bits).

Set both of these properties, to 2 cells (64 bits), to resolve issues
with kexec on some platforms.

This change corresponds with linux kernel commit ae8a442dfdc4
("efi/libstub/arm*: Set default address and size cells values for an empty dtb")
and ensures booting through grub does not behave differently from booting
the stub loader directly.

See also https://patchwork.kernel.org/patch/9561201/

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/loader/efi/fdt.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index c0c6800f7..477ef7485 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -29,6 +29,12 @@
 static void *loaded_fdt;
 static void *fdt;
 
+#define FDT_ADDR_CELLS_STRING "#address-cells"
+#define FDT_SIZE_CELLS_STRING "#size-cells"
+#define FDT_ADDR_SIZE_EXTRA ((2 * grub_fdt_prop_entry_size (sizeof(grub_uint32_t))) + \
+                             sizeof (FDT_ADDR_CELLS_STRING) + \
+                             sizeof (FDT_SIZE_CELLS_STRING))
+
 void *
 grub_fdt_load (grub_size_t additional_size)
 {
@@ -46,8 +52,14 @@ grub_fdt_load (grub_size_t additional_size)
   else
     raw_fdt = grub_efi_get_firmware_fdt();
 
-  size =
-    raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
+  if (raw_fdt)
+    {
+      size = grub_fdt_get_totalsize (raw_fdt);
+    }
+  else
+    {
+      size =  GRUB_FDT_EMPTY_TREE_SZ + FDT_ADDR_SIZE_EXTRA;
+    }
   size += additional_size;
 
   grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
@@ -63,6 +75,8 @@ grub_fdt_load (grub_size_t additional_size)
   else
     {
       grub_fdt_create_empty_tree (fdt, size);
+      grub_fdt_set_prop32 (fdt, 0, FDT_ADDR_CELLS_STRING, 2);
+      grub_fdt_set_prop32 (fdt, 0, FDT_SIZE_CELLS_STRING, 2);
     }
   return fdt;
 }
-- 
2.11.0

Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Daniel Kiper]

On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote:
> Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
> systems when creating an empty DT at boot time. This resolves an issue
> seen in the wild with kexec on certain 64-bit ARM systems.
>
> First part is moving out the prop_entry_size macro from lib/fdt.c
> and make it available in <grub/fdt.h> (with the required name
> change).
>
> Second part is adding the two properties to the empty tree.

In general LGTM. Though I would prefer "?" instead of "if" in patch #2.
Or at least drop curly braces. Anyway, if you are OK with proposed
changes I can incorporate them before push.

Daniel

Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Leif Lindholm]

On Thu, Jun 14, 2018 at 03:38:13PM +0200, Daniel Kiper wrote:
> On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote:
> > Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
> > systems when creating an empty DT at boot time. This resolves an issue
> > seen in the wild with kexec on certain 64-bit ARM systems.
> >
> > First part is moving out the prop_entry_size macro from lib/fdt.c
> > and make it available in <grub/fdt.h> (with the required name
> > change).
> >
> > Second part is adding the two properties to the empty tree.
> 
> In general LGTM. Though I would prefer "?" instead of "if" in patch #2.
> Or at least drop curly braces. Anyway, if you are OK with proposed
> changes I can incorporate them before push.

I thought the added additional bits made the ternary a bit painful,
but I'd be happy for you to get rid of the braces (TianoCore coding
style is making me use those more heavily :)

/
    Leif

Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Daniel Kiper]

On Thu, Jun 14, 2018 at 04:37:34PM +0100, Leif Lindholm wrote:
> On Thu, Jun 14, 2018 at 03:38:13PM +0200, Daniel Kiper wrote:
> > On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote:
> > > Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
> > > systems when creating an empty DT at boot time. This resolves an issue
> > > seen in the wild with kexec on certain 64-bit ARM systems.
> > >
> > > First part is moving out the prop_entry_size macro from lib/fdt.c
> > > and make it available in <grub/fdt.h> (with the required name
> > > change).
> > >
> > > Second part is adding the two properties to the empty tree.
> >
> > In general LGTM. Though I would prefer "?" instead of "if" in patch #2.
> > Or at least drop curly braces. Anyway, if you are OK with proposed
> > changes I can incorporate them before push.
>
> I thought the added additional bits made the ternary a bit painful,
> but I'd be happy for you to get rid of the braces (TianoCore coding

Granted! I will drop the braces.

> style is making me use those more heavily :)

:-)))

If there are no objections I apply the patches in a week or so.

Daniel