[Qemu-devel] [PATCH 0/5] hw/arm/boot: Support DTB autoload for firmware-only boots

[Qemu-devel] [PATCH 0/5] hw/arm/boot: Support DTB autoload for firmware-only boots

[Peter Maydell]

The arm_boot_info struct has a skip_dtb_autoload flag: if this is
set to true by the board code then arm_load_kernel() will not
load the DTB itself, but will leave this for the board code to
do itself later. However, the check for this is done in a
code path which is only executed for the case where we load
a kernel image file. If we're taking the "boot via firmware"
code path then the flag isn't honoured and the DTB is never
loaded.
    
We didn't notice this because the only real user of "boot
via firmware" that cares about the DTB is the virt board
(for UEFI boot), and that always wants skip_dtb_autoload
anyway. But the SBSA reference board model we're planning to
add will want the flag to behave correctly.

The first four patches in this set are just refactoring,
splitting the code in arm_load_kernel() out into a
couple of sub-functions for "direct kernel boot" and
"firmware boot". The last patch that fixes the issue is
then just a one-liner. (Without the refactoring we'd
need to use a goto, which is what I wanted to avoid.)

thanks
-- PMM

Peter Maydell (5):
  hw/arm/boot: Fix block comment style in arm_load_kernel()
  hw/arm/boot: Factor out "direct kernel boot" code into its own
    function
  hw/arm/boot: Factor out "set up firmware boot" code
  hw/arm/boot: Clarify why arm_setup_firmware_boot() doesn't set
    env->boot_info
  hw/arm/boot: Support DTB autoload for firmware-only boots

 hw/arm/boot.c | 166 +++++++++++++++++++++++++++++---------------------
 1 file changed, 96 insertions(+), 70 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/5] hw/arm/boot: Fix block comment style in arm_load_kernel()

[Peter Maydell]

Fix the block comment style in arm_load_kernel() to QEMU's
current style preferences. This will allow us to do some
refactoring of this function without checkpatch complaining
about the code-motion patches.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c7a67af7a97..6f9ceeb0e89 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -965,7 +965,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     static const ARMInsnFixup *primary_loader;
     AddressSpace *as = arm_boot_address_space(cpu, info);
 
-    /* CPU objects (unlike devices) are not automatically reset on system
+    /*
+     * CPU objects (unlike devices) are not automatically reset on system
      * reset, so we must always register a handler to do so. If we're
      * actually loading a kernel, the handler is also responsible for
      * arranging that we start it correctly.
@@ -974,7 +975,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
     }
 
-    /* The board code is not supposed to set secure_board_setup unless
+    /*
+     * The board code is not supposed to set secure_board_setup unless
      * running its code in secure mode is actually possible, and KVM
      * doesn't support secure.
      */
@@ -987,7 +989,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     if (!info->kernel_filename || info->firmware_loaded) {
 
         if (have_dtb(info)) {
-            /* If we have a device tree blob, but no kernel to supply it to (or
+            /*
+             * If we have a device tree blob, but no kernel to supply it to (or
              * the kernel is supposed to be loaded by the bootloader), copy the
              * DTB to the base of RAM for the bootloader to pick up.
              */
@@ -1002,7 +1005,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             try_decompressing_kernel = arm_feature(&cpu->env,
                                                    ARM_FEATURE_AARCH64);
 
-            /* Expose the kernel, the command line, and the initrd in fw_cfg.
+            /*
+             * Expose the kernel, the command line, and the initrd in fw_cfg.
              * We don't process them here at all, it's all left to the
              * firmware.
              */
@@ -1022,7 +1026,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             }
         }
 
-        /* We will start from address 0 (typically a boot ROM image) in the
+        /*
+         * We will start from address 0 (typically a boot ROM image) in the
          * same way as hardware.
          */
         return;
@@ -1049,7 +1054,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     if (info->nb_cpus == 0)
         info->nb_cpus = 1;
 
-    /* We want to put the initrd far enough into RAM that when the
+    /*
+     * We want to put the initrd far enough into RAM that when the
      * kernel is uncompressed it will not clobber the initrd. However
      * on boards without much RAM we must ensure that we still leave
      * enough room for a decent sized initrd, and on boards with large
@@ -1066,12 +1072,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
                                &elf_high_addr, elf_machine, as);
     if (kernel_size > 0 && have_dtb(info)) {
-        /* If there is still some room left at the base of RAM, try and put
+        /*
+         * If there is still some room left at the base of RAM, try and put
          * the DTB there like we do for images loaded with -bios or -pflash.
          */
         if (elf_low_addr > info->loader_start
             || elf_high_addr < info->loader_start) {
-            /* Set elf_low_addr as address limit for arm_load_dtb if it may be
+            /*
+             * Set elf_low_addr as address limit for arm_load_dtb if it may be
              * pointing into RAM, otherwise pass '0' (no limit)
              */
             if (elf_low_addr < info->loader_start) {
@@ -1132,7 +1140,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         fixupcontext[FIXUP_BOARDID] = info->board_id;
         fixupcontext[FIXUP_BOARD_SETUP] = info->board_setup_addr;
 
-        /* for device tree boot, we pass the DTB directly in r2. Otherwise
+        /*
+         * for device tree boot, we pass the DTB directly in r2. Otherwise
          * we point to the kernel args.
          */
         if (have_dtb(info)) {
@@ -1185,7 +1194,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             info->write_board_setup(cpu, info);
         }
 
-        /* Notify devices which need to fake up firmware initialization
+        /*
+         * Notify devices which need to fake up firmware initialization
          * that we're doing a direct kernel boot.
          */
         object_child_foreach_recursive(object_get_root(),
-- 
2.20.1

[Qemu-devel] [PATCH 2/5] hw/arm/boot: Factor out "direct kernel boot" code into its own function

[Peter Maydell]

Factor out the "direct kernel boot" code path from arm_load_kernel()
into its own function; this function is getting long enough that
the code flow is a bit confusing.

This commit only moves code around; no semantic changes.

We leave the "load the dtb" code in arm_load_kernel() -- this
is currently only used by the "direct kernel boot" path, but
this is a bug which we will fix shortly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 150 +++++++++++++++++++++++++++-----------------------
 1 file changed, 80 insertions(+), 70 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 6f9ceeb0e89..108e9b979f8 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -953,9 +953,12 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
     return size;
 }
 
-void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
+static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
+                                         struct arm_boot_info *info)
 {
+    /* Set up for a direct boot of a kernel image file. */
     CPUState *cs;
+    AddressSpace *as = arm_boot_address_space(cpu, info);
     int kernel_size;
     int initrd_size;
     int is_linux = 0;
@@ -963,75 +966,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     int elf_machine;
     hwaddr entry;
     static const ARMInsnFixup *primary_loader;
-    AddressSpace *as = arm_boot_address_space(cpu, info);
-
-    /*
-     * CPU objects (unlike devices) are not automatically reset on system
-     * reset, so we must always register a handler to do so. If we're
-     * actually loading a kernel, the handler is also responsible for
-     * arranging that we start it correctly.
-     */
-    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
-        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
-    }
-
-    /*
-     * The board code is not supposed to set secure_board_setup unless
-     * running its code in secure mode is actually possible, and KVM
-     * doesn't support secure.
-     */
-    assert(!(info->secure_board_setup && kvm_enabled()));
-
-    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
-    info->dtb_limit = 0;
-
-    /* Load the kernel.  */
-    if (!info->kernel_filename || info->firmware_loaded) {
-
-        if (have_dtb(info)) {
-            /*
-             * If we have a device tree blob, but no kernel to supply it to (or
-             * the kernel is supposed to be loaded by the bootloader), copy the
-             * DTB to the base of RAM for the bootloader to pick up.
-             */
-            info->dtb_start = info->loader_start;
-        }
-
-        if (info->kernel_filename) {
-            FWCfgState *fw_cfg;
-            bool try_decompressing_kernel;
-
-            fw_cfg = fw_cfg_find();
-            try_decompressing_kernel = arm_feature(&cpu->env,
-                                                   ARM_FEATURE_AARCH64);
-
-            /*
-             * Expose the kernel, the command line, and the initrd in fw_cfg.
-             * We don't process them here at all, it's all left to the
-             * firmware.
-             */
-            load_image_to_fw_cfg(fw_cfg,
-                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
-                                 info->kernel_filename,
-                                 try_decompressing_kernel);
-            load_image_to_fw_cfg(fw_cfg,
-                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
-                                 info->initrd_filename, false);
-
-            if (info->kernel_cmdline) {
-                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
-                               strlen(info->kernel_cmdline) + 1);
-                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
-                                  info->kernel_cmdline);
-            }
-        }
-
-        /*
-         * We will start from address 0 (typically a boot ROM image) in the
-         * same way as hardware.
-         */
-        return;
-    }
 
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         primary_loader = bootloader_aarch64;
@@ -1206,6 +1140,82 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
         ARM_CPU(cs)->env.boot_info = info;
     }
+}
+
+void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
+{
+    CPUState *cs;
+    AddressSpace *as = arm_boot_address_space(cpu, info);
+
+    /*
+     * CPU objects (unlike devices) are not automatically reset on system
+     * reset, so we must always register a handler to do so. If we're
+     * actually loading a kernel, the handler is also responsible for
+     * arranging that we start it correctly.
+     */
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
+    }
+
+    /*
+     * The board code is not supposed to set secure_board_setup unless
+     * running its code in secure mode is actually possible, and KVM
+     * doesn't support secure.
+     */
+    assert(!(info->secure_board_setup && kvm_enabled()));
+
+    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+    info->dtb_limit = 0;
+
+    /* Load the kernel.  */
+    if (!info->kernel_filename || info->firmware_loaded) {
+
+        if (have_dtb(info)) {
+            /*
+             * If we have a device tree blob, but no kernel to supply it to (or
+             * the kernel is supposed to be loaded by the bootloader), copy the
+             * DTB to the base of RAM for the bootloader to pick up.
+             */
+            info->dtb_start = info->loader_start;
+        }
+
+        if (info->kernel_filename) {
+            FWCfgState *fw_cfg;
+            bool try_decompressing_kernel;
+
+            fw_cfg = fw_cfg_find();
+            try_decompressing_kernel = arm_feature(&cpu->env,
+                                                   ARM_FEATURE_AARCH64);
+
+            /*
+             * Expose the kernel, the command line, and the initrd in fw_cfg.
+             * We don't process them here at all, it's all left to the
+             * firmware.
+             */
+            load_image_to_fw_cfg(fw_cfg,
+                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+                                 info->kernel_filename,
+                                 try_decompressing_kernel);
+            load_image_to_fw_cfg(fw_cfg,
+                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+                                 info->initrd_filename, false);
+
+            if (info->kernel_cmdline) {
+                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+                               strlen(info->kernel_cmdline) + 1);
+                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+                                  info->kernel_cmdline);
+            }
+        }
+
+        /*
+         * We will start from address 0 (typically a boot ROM image) in the
+         * same way as hardware.
+         */
+        return;
+    } else {
+        arm_setup_direct_kernel_boot(cpu, info);
+    }
 
     if (!info->skip_dtb_autoload && have_dtb(info)) {
         if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
-- 
2.20.1

[Qemu-devel] [PATCH 3/5] hw/arm/boot: Factor out "set up firmware boot" code

[Peter Maydell]

Factor out the "boot via firmware" code path from arm_load_kernel()
into its own function.

This commit only moves code around; no semantic changes.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 92 +++++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 108e9b979f8..a2e724ac68a 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1142,6 +1142,54 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     }
 }
 
+static void arm_setup_firmware_boot(ARMCPU *cpu, struct arm_boot_info *info)
+{
+    /* Set up for booting firmware (which might load a kernel via fw_cfg) */
+
+    if (have_dtb(info)) {
+        /*
+         * If we have a device tree blob, but no kernel to supply it to (or
+         * the kernel is supposed to be loaded by the bootloader), copy the
+         * DTB to the base of RAM for the bootloader to pick up.
+         */
+        info->dtb_start = info->loader_start;
+    }
+
+    if (info->kernel_filename) {
+        FWCfgState *fw_cfg;
+        bool try_decompressing_kernel;
+
+        fw_cfg = fw_cfg_find();
+        try_decompressing_kernel = arm_feature(&cpu->env,
+                                               ARM_FEATURE_AARCH64);
+
+        /*
+         * Expose the kernel, the command line, and the initrd in fw_cfg.
+         * We don't process them here at all, it's all left to the
+         * firmware.
+         */
+        load_image_to_fw_cfg(fw_cfg,
+                             FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+                             info->kernel_filename,
+                             try_decompressing_kernel);
+        load_image_to_fw_cfg(fw_cfg,
+                             FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+                             info->initrd_filename, false);
+
+        if (info->kernel_cmdline) {
+            fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+                           strlen(info->kernel_cmdline) + 1);
+            fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+                              info->kernel_cmdline);
+        }
+    }
+
+    /*
+     * We will start from address 0 (typically a boot ROM image) in the
+     * same way as hardware.
+     */
+}
+
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
     CPUState *cs;
@@ -1169,49 +1217,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
-
-        if (have_dtb(info)) {
-            /*
-             * If we have a device tree blob, but no kernel to supply it to (or
-             * the kernel is supposed to be loaded by the bootloader), copy the
-             * DTB to the base of RAM for the bootloader to pick up.
-             */
-            info->dtb_start = info->loader_start;
-        }
-
-        if (info->kernel_filename) {
-            FWCfgState *fw_cfg;
-            bool try_decompressing_kernel;
-
-            fw_cfg = fw_cfg_find();
-            try_decompressing_kernel = arm_feature(&cpu->env,
-                                                   ARM_FEATURE_AARCH64);
-
-            /*
-             * Expose the kernel, the command line, and the initrd in fw_cfg.
-             * We don't process them here at all, it's all left to the
-             * firmware.
-             */
-            load_image_to_fw_cfg(fw_cfg,
-                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
-                                 info->kernel_filename,
-                                 try_decompressing_kernel);
-            load_image_to_fw_cfg(fw_cfg,
-                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
-                                 info->initrd_filename, false);
-
-            if (info->kernel_cmdline) {
-                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
-                               strlen(info->kernel_cmdline) + 1);
-                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
-                                  info->kernel_cmdline);
-            }
-        }
-
-        /*
-         * We will start from address 0 (typically a boot ROM image) in the
-         * same way as hardware.
-         */
+        arm_setup_firmware_boot(cpu, info);
         return;
     } else {
         arm_setup_direct_kernel_boot(cpu, info);
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] hw/arm/boot: Clarify why arm_setup_firmware_boot() doesn't set env->boot_info

[Peter Maydell]

The code path for booting firmware doesn't set env->boot_info. At
first sight this looks odd, so add a comment saying why we don't.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index a2e724ac68a..cfcfdf421cf 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1186,7 +1186,8 @@ static void arm_setup_firmware_boot(ARMCPU *cpu, struct arm_boot_info *info)
 
     /*
      * We will start from address 0 (typically a boot ROM image) in the
-     * same way as hardware.
+     * same way as hardware. Leave env->boot_info NULL, so that
+     * do_cpu_reset() knows it does not need to alter the PC on reset.
      */
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH 5/5] hw/arm/boot: Support DTB autoload for firmware-only boots

[Peter Maydell]

The arm_boot_info struct has a skip_dtb_autoload flag: if this is
set to true by the board code then arm_load_kernel() will not
load the DTB itself, but will leave this for the board code to
do itself later. However, the check for this is done in a
code path which is only executed for the case where we load
a kernel image file. If we're taking the "boot via firmware"
code path then the flag isn't honoured and the DTB is never
loaded.

We didn't notice this because the only real user of "boot
via firmware" that cares about the DTB is the virt board
(for UEFI boot), and that always wants skip_dtb_autoload
anyway. But the SBSA reference board model we're planning to
add will want the flag to behave correctly.

Now we've refactored the arm_load_kernel() function, the
fix is simple: drop the early 'return' so we fall into
the same "load the DTB" code the boot-direct-kernel path uses.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index cfcfdf421cf..8e792a911f9 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1219,7 +1219,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
         arm_setup_firmware_boot(cpu, info);
-        return;
     } else {
         arm_setup_direct_kernel_boot(cpu, info);
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 0/5] hw/arm/boot: Support DTB autoload for firmware-only boots

[Richard Henderson]

On 1/31/19 3:22 AM, Peter Maydell wrote:
> Peter Maydell (5):
>   hw/arm/boot: Fix block comment style in arm_load_kernel()
>   hw/arm/boot: Factor out "direct kernel boot" code into its own
>     function
>   hw/arm/boot: Factor out "set up firmware boot" code
>   hw/arm/boot: Clarify why arm_setup_firmware_boot() doesn't set
>     env->boot_info
>   hw/arm/boot: Support DTB autoload for firmware-only boots
> 
>  hw/arm/boot.c | 166 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 96 insertions(+), 70 deletions(-)

Nice cleanup.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/5] hw/arm/boot: Factor out "direct kernel boot" code into its own function

[Igor Mammedov]

On Thu, 31 Jan 2019 11:22:37 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Factor out the "direct kernel boot" code path from arm_load_kernel()
> into its own function; this function is getting long enough that
> the code flow is a bit confusing.
> 
> This commit only moves code around; no semantic changes.
> 
> We leave the "load the dtb" code in arm_load_kernel() -- this
> is currently only used by the "direct kernel boot" path, but
> this is a bug which we will fix shortly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/boot.c | 150 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 80 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 6f9ceeb0e89..108e9b979f8 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -953,9 +953,12 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>      return size;
>  }
>  
> -void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> +static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> +                                         struct arm_boot_info *info)
>  {
> +    /* Set up for a direct boot of a kernel image file. */
>      CPUState *cs;
> +    AddressSpace *as = arm_boot_address_space(cpu, info);
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> @@ -963,75 +966,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> -    AddressSpace *as = arm_boot_address_space(cpu, info);
> -
> -    /*
> -     * CPU objects (unlike devices) are not automatically reset on system
> -     * reset, so we must always register a handler to do so. If we're
> -     * actually loading a kernel, the handler is also responsible for
> -     * arranging that we start it correctly.
> -     */
> -    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> -        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> -    }
> -
> -    /*
> -     * The board code is not supposed to set secure_board_setup unless
> -     * running its code in secure mode is actually possible, and KVM
> -     * doesn't support secure.
> -     */
> -    assert(!(info->secure_board_setup && kvm_enabled()));
> -
> -    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> -    info->dtb_limit = 0;
> -
> -    /* Load the kernel.  */
> -    if (!info->kernel_filename || info->firmware_loaded) {
> -
> -        if (have_dtb(info)) {
> -            /*
> -             * If we have a device tree blob, but no kernel to supply it to (or
> -             * the kernel is supposed to be loaded by the bootloader), copy the
> -             * DTB to the base of RAM for the bootloader to pick up.
> -             */
> -            info->dtb_start = info->loader_start;
> -        }
> -
> -        if (info->kernel_filename) {
> -            FWCfgState *fw_cfg;
> -            bool try_decompressing_kernel;
> -
> -            fw_cfg = fw_cfg_find();
> -            try_decompressing_kernel = arm_feature(&cpu->env,
> -                                                   ARM_FEATURE_AARCH64);
> -
> -            /*
> -             * Expose the kernel, the command line, and the initrd in fw_cfg.
> -             * We don't process them here at all, it's all left to the
> -             * firmware.
> -             */
> -            load_image_to_fw_cfg(fw_cfg,
> -                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
> -                                 info->kernel_filename,
> -                                 try_decompressing_kernel);
> -            load_image_to_fw_cfg(fw_cfg,
> -                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
> -                                 info->initrd_filename, false);
> -
> -            if (info->kernel_cmdline) {
> -                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
> -                               strlen(info->kernel_cmdline) + 1);
> -                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
> -                                  info->kernel_cmdline);
> -            }
> -        }
> -
> -        /*
> -         * We will start from address 0 (typically a boot ROM image) in the
> -         * same way as hardware.
> -         */
> -        return;
> -    }
>  
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          primary_loader = bootloader_aarch64;
> @@ -1206,6 +1140,82 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
>          ARM_CPU(cs)->env.boot_info = info;
>      }
> +}
> +
> +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> +{
> +    CPUState *cs;
> +    AddressSpace *as = arm_boot_address_space(cpu, info);
> +
> +    /*
> +     * CPU objects (unlike devices) are not automatically reset on system
> +     * reset, so we must always register a handler to do so. If we're
> +     * actually loading a kernel, the handler is also responsible for
> +     * arranging that we start it correctly.
> +     */
> +    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> +        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> +    }
Should we move this hunk to cpu code instead, like it's done for x86?

> +
> +    /*
> +     * The board code is not supposed to set secure_board_setup unless
> +     * running its code in secure mode is actually possible, and KVM
> +     * doesn't support secure.
> +     */
> +    assert(!(info->secure_board_setup && kvm_enabled()));
> +
> +    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> +    info->dtb_limit = 0;
> +
> +    /* Load the kernel.  */
> +    if (!info->kernel_filename || info->firmware_loaded) {
> +
> +        if (have_dtb(info)) {
> +            /*
> +             * If we have a device tree blob, but no kernel to supply it to (or
> +             * the kernel is supposed to be loaded by the bootloader), copy the
> +             * DTB to the base of RAM for the bootloader to pick up.
> +             */
> +            info->dtb_start = info->loader_start;
> +        }
> +
> +        if (info->kernel_filename) {
> +            FWCfgState *fw_cfg;
> +            bool try_decompressing_kernel;
> +
> +            fw_cfg = fw_cfg_find();
> +            try_decompressing_kernel = arm_feature(&cpu->env,
> +                                                   ARM_FEATURE_AARCH64);
> +
> +            /*
> +             * Expose the kernel, the command line, and the initrd in fw_cfg.
> +             * We don't process them here at all, it's all left to the
> +             * firmware.
> +             */
> +            load_image_to_fw_cfg(fw_cfg,
> +                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
> +                                 info->kernel_filename,
> +                                 try_decompressing_kernel);
> +            load_image_to_fw_cfg(fw_cfg,
> +                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
> +                                 info->initrd_filename, false);
> +
> +            if (info->kernel_cmdline) {
> +                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
> +                               strlen(info->kernel_cmdline) + 1);
> +                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
> +                                  info->kernel_cmdline);
> +            }
> +        }
> +
> +        /*
> +         * We will start from address 0 (typically a boot ROM image) in the
> +         * same way as hardware.
> +         */
> +        return;
> +    } else {
> +        arm_setup_direct_kernel_boot(cpu, info);
> +    }
>  
>      if (!info->skip_dtb_autoload && have_dtb(info)) {
>          if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {

Re: [Qemu-devel] [PATCH 0/5] hw/arm/boot: Support DTB autoload for firmware-only boots

[Igor Mammedov]

On Thu, 31 Jan 2019 11:22:35 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> The arm_boot_info struct has a skip_dtb_autoload flag: if this is
> set to true by the board code then arm_load_kernel() will not
> load the DTB itself, but will leave this for the board code to
> do itself later. However, the check for this is done in a
> code path which is only executed for the case where we load
> a kernel image file. If we're taking the "boot via firmware"
> code path then the flag isn't honoured and the DTB is never
> loaded.
>     
> We didn't notice this because the only real user of "boot
> via firmware" that cares about the DTB is the virt board
> (for UEFI boot), and that always wants skip_dtb_autoload
> anyway. But the SBSA reference board model we're planning to
> add will want the flag to behave correctly.
> 
> The first four patches in this set are just refactoring,
> splitting the code in arm_load_kernel() out into a
> couple of sub-functions for "direct kernel boot" and
> "firmware boot". The last patch that fixes the issue is
> then just a one-liner. (Without the refactoring we'd
> need to use a goto, which is what I wanted to avoid.)
> 
> thanks
> -- PMM
> 
> Peter Maydell (5):
>   hw/arm/boot: Fix block comment style in arm_load_kernel()
>   hw/arm/boot: Factor out "direct kernel boot" code into its own
>     function
>   hw/arm/boot: Factor out "set up firmware boot" code
>   hw/arm/boot: Clarify why arm_setup_firmware_boot() doesn't set
>     env->boot_info
>   hw/arm/boot: Support DTB autoload for firmware-only boots
> 
>  hw/arm/boot.c | 166 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 96 insertions(+), 70 deletions(-)
> 

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH 2/5] hw/arm/boot: Factor out "direct kernel boot" code into its own function

[Peter Maydell]

On Tue, 5 Feb 2019 at 15:16, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 31 Jan 2019 11:22:37 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> > +{
> > +    CPUState *cs;
> > +    AddressSpace *as = arm_boot_address_space(cpu, info);
> > +
> > +    /*
> > +     * CPU objects (unlike devices) are not automatically reset on system
> > +     * reset, so we must always register a handler to do so. If we're
> > +     * actually loading a kernel, the handler is also responsible for
> > +     * arranging that we start it correctly.
> > +     */
> > +    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> > +        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> > +    }
> Should we move this hunk to cpu code instead, like it's done for x86?

I don't know how x86 does it, but CPU reset is a mess, so if
we can clean it up that would be nice. Most of what do_cpu_reset()
is doing here is handling image load though, which doesn't seem
to me to fit very well in the CPU code: it's pretty much what
hw/arm/boot.c's job is.

I'd rather do that in a different patchset, though: this is just
a code movement refactoring.

thanks
-- PMM