[PATCH v1 0/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[PATCH v1 0/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

When users try direct Linux runs on the ZynqMP models without enabling
EL3 (and using appropriate FW) they run into trouble because the
upstream kernel device-tree has EL3 based non-PSCI firmware nodes by default.
PSCI firmware nodes work because we emulate the firmware in QEMU.

This series avoids that problem by disabling zynqmp-firmware nodes that the
machine cannot support due to lack of EL3 or EL2 support.

This means we can now (without manually editing DTBs) run the following
in a current Linux tree:

qemu-system-aarch64 -M xlnx-zcu102 -m 2G -dtb arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dtb -serial mon:stdio -kernel arch/arm64/boot/Image -initrd zu-rootfs.cpio.gz -append rdinit=/bin/sh

Cheers,
Edgar

Edgar E. Iglesias (3):
  device_tree: Allow name wildcards in qemu_fdt_node_path()
  hw/arm: xlnx-zcu102: Move arm_boot_info into XlnxZCU102
  hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

 device_tree.c                |  2 +-
 hw/arm/xlnx-zcu102.c         | 40 ++++++++++++++++++++++++++++++++----
 include/sysemu/device_tree.h |  3 +++
 3 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH v1 1/3] device_tree: Allow name wildcards in qemu_fdt_node_path()

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Allow name wildcards in qemu_fdt_node_path(). This is useful
to find all nodes with a given compatibility string.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 device_tree.c                | 2 +-
 include/sysemu/device_tree.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/device_tree.c b/device_tree.c
index bba6cc2164..f5b4699aed 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -308,7 +308,7 @@ char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
             offset = len;
             break;
         }
-        if (!strcmp(iter_name, name)) {
+        if (!name || !strcmp(iter_name, name)) {
             char *path;
 
             path = g_malloc(path_len);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index c16fd69bc0..7c53ef7634 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -39,6 +39,9 @@ void *load_device_tree_from_sysfs(void);
  * NULL. If there is no error but no matching node was found, the
  * returned array contains a single element equal to NULL. If an error
  * was encountered when parsing the blob, the function returns NULL
+ *
+ * @name may be NULL to wildcard names and only match compatibility
+ * strings.
  */
 char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
                           Error **errp);
-- 
2.20.1

[PATCH v1 2/3] hw/arm: xlnx-zcu102: Move arm_boot_info into XlnxZCU102

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Move arm_boot_info into XlnxZCU102.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zcu102.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index bd645ad818..4eb117c755 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -31,13 +31,14 @@ typedef struct XlnxZCU102 {
 
     bool secure;
     bool virt;
+
+    struct arm_boot_info binfo;
 } XlnxZCU102;
 
 #define TYPE_ZCU102_MACHINE   MACHINE_TYPE_NAME("xlnx-zcu102")
 #define ZCU102_MACHINE(obj) \
     OBJECT_CHECK(XlnxZCU102, (obj), TYPE_ZCU102_MACHINE)
 
-static struct arm_boot_info xlnx_zcu102_binfo;
 
 static bool zcu102_get_secure(Object *obj, Error **errp)
 {
@@ -166,9 +167,9 @@ static void xlnx_zcu102_init(MachineState *machine)
 
     /* TODO create and connect IDE devices for ide_drive_get() */
 
-    xlnx_zcu102_binfo.ram_size = ram_size;
-    xlnx_zcu102_binfo.loader_start = 0;
-    arm_load_kernel(s->soc.boot_cpu_ptr, machine, &xlnx_zcu102_binfo);
+    s->binfo.ram_size = ram_size;
+    s->binfo.loader_start = 0;
+    arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
 }
 
 static void xlnx_zcu102_machine_instance_init(Object *obj)
-- 
2.20.1

[PATCH v1 3/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Disable unsupported FDT firmware nodes if a user passes us
a DTB with nodes enabled that the machine cannot support
due to lack of EL3 or EL2 support.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zcu102.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 4eb117c755..3332630380 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -23,6 +23,7 @@
 #include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "sysemu/qtest.h"
+#include "sysemu/device_tree.h"
 
 typedef struct XlnxZCU102 {
     MachineState parent_obj;
@@ -68,6 +69,35 @@ static void zcu102_set_virt(Object *obj, bool value, Error **errp)
     s->virt = value;
 }
 
+static void zcu102_modify_dtb(const struct arm_boot_info *binfo, void *fdt)
+{
+    XlnxZCU102 *s = container_of(binfo, XlnxZCU102, binfo);
+    bool method_is_hvc;
+    char **node_path;
+    const char *r;
+    int prop_len;
+    int i;
+
+    /* If EL3 is enabled, we keep all firmware nodes active.  */
+    if (!s->secure) {
+        node_path = qemu_fdt_node_path(fdt, NULL,
+                                       (char *)"xlnx,zynqmp-firmware",
+                                       &error_fatal);
+
+        for (i = 0; node_path && node_path[i]; i++) {
+            r = qemu_fdt_getprop(fdt, node_path[i], "method", &prop_len, NULL);
+            method_is_hvc = r && !strcmp("hvc", r);
+
+            /* Allow HVC based firmware if EL2 is enabled.  */
+            if (method_is_hvc && s->virt) {
+                continue;
+            }
+            qemu_fdt_setprop_string(fdt, node_path[i], "status", "disabled");
+        }
+        g_strfreev(node_path);
+    }
+}
+
 static void xlnx_zcu102_init(MachineState *machine)
 {
     XlnxZCU102 *s = ZCU102_MACHINE(machine);
@@ -169,6 +199,7 @@ static void xlnx_zcu102_init(MachineState *machine)
 
     s->binfo.ram_size = ram_size;
     s->binfo.loader_start = 0;
+    s->binfo.modify_dtb = zcu102_modify_dtb;
     arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
 }
 
-- 
2.20.1

Re: [PATCH v1 2/3] hw/arm: xlnx-zcu102: Move arm_boot_info into XlnxZCU102

[Philippe Mathieu-Daudé]

On 4/19/20 6:27 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Move arm_boot_info into XlnxZCU102.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zcu102.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index bd645ad818..4eb117c755 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -31,13 +31,14 @@ typedef struct XlnxZCU102 {
>  
>      bool secure;
>      bool virt;
> +
> +    struct arm_boot_info binfo;
>  } XlnxZCU102;
>  
>  #define TYPE_ZCU102_MACHINE   MACHINE_TYPE_NAME("xlnx-zcu102")
>  #define ZCU102_MACHINE(obj) \
>      OBJECT_CHECK(XlnxZCU102, (obj), TYPE_ZCU102_MACHINE)
>  
> -static struct arm_boot_info xlnx_zcu102_binfo;
>  
>  static bool zcu102_get_secure(Object *obj, Error **errp)
>  {
> @@ -166,9 +167,9 @@ static void xlnx_zcu102_init(MachineState *machine)
>  
>      /* TODO create and connect IDE devices for ide_drive_get() */
>  
> -    xlnx_zcu102_binfo.ram_size = ram_size;
> -    xlnx_zcu102_binfo.loader_start = 0;
> -    arm_load_kernel(s->soc.boot_cpu_ptr, machine, &xlnx_zcu102_binfo);
> +    s->binfo.ram_size = ram_size;
> +    s->binfo.loader_start = 0;
> +    arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
>  }
>  
>  static void xlnx_zcu102_machine_instance_init(Object *obj)
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v1 1/3] device_tree: Allow name wildcards in qemu_fdt_node_path()

[Alistair Francis]

On Sun, Apr 19, 2020 at 9:27 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Allow name wildcards in qemu_fdt_node_path(). This is useful
> to find all nodes with a given compatibility string.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  device_tree.c                | 2 +-
>  include/sysemu/device_tree.h | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index bba6cc2164..f5b4699aed 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -308,7 +308,7 @@ char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>              offset = len;
>              break;
>          }
> -        if (!strcmp(iter_name, name)) {
> +        if (!name || !strcmp(iter_name, name)) {
>              char *path;
>
>              path = g_malloc(path_len);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index c16fd69bc0..7c53ef7634 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -39,6 +39,9 @@ void *load_device_tree_from_sysfs(void);
>   * NULL. If there is no error but no matching node was found, the
>   * returned array contains a single element equal to NULL. If an error
>   * was encountered when parsing the blob, the function returns NULL
> + *
> + * @name may be NULL to wildcard names and only match compatibility
> + * strings.
>   */
>  char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>                            Error **errp);
> --
> 2.20.1
>
>

Re: [PATCH v1 3/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Alistair Francis]

On Sun, Apr 19, 2020 at 9:27 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Disable unsupported FDT firmware nodes if a user passes us
> a DTB with nodes enabled that the machine cannot support
> due to lack of EL3 or EL2 support.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/xlnx-zcu102.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 4eb117c755..3332630380 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -23,6 +23,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "sysemu/qtest.h"
> +#include "sysemu/device_tree.h"
>
>  typedef struct XlnxZCU102 {
>      MachineState parent_obj;
> @@ -68,6 +69,35 @@ static void zcu102_set_virt(Object *obj, bool value, Error **errp)
>      s->virt = value;
>  }
>
> +static void zcu102_modify_dtb(const struct arm_boot_info *binfo, void *fdt)
> +{
> +    XlnxZCU102 *s = container_of(binfo, XlnxZCU102, binfo);
> +    bool method_is_hvc;
> +    char **node_path;
> +    const char *r;
> +    int prop_len;
> +    int i;
> +
> +    /* If EL3 is enabled, we keep all firmware nodes active.  */
> +    if (!s->secure) {
> +        node_path = qemu_fdt_node_path(fdt, NULL,
> +                                       (char *)"xlnx,zynqmp-firmware",
> +                                       &error_fatal);
> +
> +        for (i = 0; node_path && node_path[i]; i++) {
> +            r = qemu_fdt_getprop(fdt, node_path[i], "method", &prop_len, NULL);
> +            method_is_hvc = r && !strcmp("hvc", r);
> +
> +            /* Allow HVC based firmware if EL2 is enabled.  */
> +            if (method_is_hvc && s->virt) {
> +                continue;
> +            }
> +            qemu_fdt_setprop_string(fdt, node_path[i], "status", "disabled");
> +        }
> +        g_strfreev(node_path);
> +    }
> +}
> +
>  static void xlnx_zcu102_init(MachineState *machine)
>  {
>      XlnxZCU102 *s = ZCU102_MACHINE(machine);
> @@ -169,6 +199,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>
>      s->binfo.ram_size = ram_size;
>      s->binfo.loader_start = 0;
> +    s->binfo.modify_dtb = zcu102_modify_dtb;
>      arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
>  }
>
> --
> 2.20.1
>
>

Re: [PATCH v1 3/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Peter Maydell]

On Sun, 19 Apr 2020 at 17:27, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Disable unsupported FDT firmware nodes if a user passes us
> a DTB with nodes enabled that the machine cannot support
> due to lack of EL3 or EL2 support.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zcu102.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> +static void zcu102_modify_dtb(const struct arm_boot_info *binfo, void *fdt)
> +{
> +    XlnxZCU102 *s = container_of(binfo, XlnxZCU102, binfo);
> +    bool method_is_hvc;
> +    char **node_path;
> +    const char *r;
> +    int prop_len;
> +    int i;
> +
> +    /* If EL3 is enabled, we keep all firmware nodes active.  */
> +    if (!s->secure) {
> +        node_path = qemu_fdt_node_path(fdt, NULL,
> +                                       (char *)"xlnx,zynqmp-firmware",
> +                                       &error_fatal);

Why do we need the 'char *' cast ?

thanks
-- PMM

Re: [PATCH v1 3/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Edgar E. Iglesias]

On Thu, Apr 23, 2020 at 12:21:11PM +0100, Peter Maydell wrote:
> On Sun, 19 Apr 2020 at 17:27, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Disable unsupported FDT firmware nodes if a user passes us
> > a DTB with nodes enabled that the machine cannot support
> > due to lack of EL3 or EL2 support.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  hw/arm/xlnx-zcu102.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > +static void zcu102_modify_dtb(const struct arm_boot_info *binfo, void *fdt)
> > +{
> > +    XlnxZCU102 *s = container_of(binfo, XlnxZCU102, binfo);
> > +    bool method_is_hvc;
> > +    char **node_path;
> > +    const char *r;
> > +    int prop_len;
> > +    int i;
> > +
> > +    /* If EL3 is enabled, we keep all firmware nodes active.  */
> > +    if (!s->secure) {
> > +        node_path = qemu_fdt_node_path(fdt, NULL,
> > +                                       (char *)"xlnx,zynqmp-firmware",
> > +                                       &error_fatal);
> 
> Why do we need the 'char *' cast ?


Without it, I see the following warning but compat in
qemu_fdt_node_path should probably be changed to const char *.
I can make that change in a v2 if you prefer.


  CC      aarch64-softmmu/hw/arm/xlnx-zcu102.o
/home/edgar/src/c/qemu/qemu/hw/arm/xlnx-zcu102.c: In function ‘zcu102_modify_dtb’:
/home/edgar/src/c/qemu/qemu/hw/arm/xlnx-zcu102.c:84:40: error: passing argument 3 of ‘qemu_fdt_node_path’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
   84 |                                        "xlnx,zynqmp-firmware",
      |                                        ^~~~~~~~~~~~~~~~~~~~~~
In file included from /home/edgar/src/c/qemu/qemu/hw/arm/xlnx-zcu102.c:26:
/home/edgar/src/c/qemu/qemu/include/sysemu/device_tree.h:46:8: note: expected ‘char *’ but argument is of type ‘const char *’
   46 | char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
      |        ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [/home/edgar/src/c/qemu/qemu/rules.mak:69: hw/arm/xlnx-zcu102.o] Error 1
make: *** [Makefile:527: aarch64-softmmu/all] Error 2

Cheers,
Edgar

Re: [PATCH v1 3/3] hw/arm: xlnx-zcu102: Disable unsupported FDT firmware nodes

[Peter Maydell]

On Thu, 23 Apr 2020 at 12:43, Edgar E. Iglesias
<edgar.iglesias@xilinx.com> wrote:
> Without it, I see the following warning but compat in
> qemu_fdt_node_path should probably be changed to const char *.
> I can make that change in a v2 if you prefer.

Yes, I think that would be better. I can't see any reason
why the compat argument needs to be non-const.

thanks
-- PMM