[Qemu-devel] [PATCH 0/6] ARM: -bios/-kernel + DTB boot roundup

[Qemu-devel] [PATCH 0/6] ARM: -bios/-kernel + DTB boot roundup

[Ard Biesheuvel]

This series contains the remaining bits and pieces I keep in my local tree
to run the Tianocore/EDK2 UEFI bootloader under QEMU.

Patch #1 has been posted by Peter before, and has been included for
completeness.

Patch #2 adds an output parameter to load_dtb() that is used by the DTB loading
functionality for ELF images added in patch #6.

Patch #3 has been posted by me before, and has been updated to take into
account Peter's feedback.

Patch #4 register reset handlers for all CPUs when not using -kernel. Without
this, CPU reset is completely non-functional, i.e., qemu_system_reset_request()
has no apparent effect at all.

Patch #5 modifies load_dtb() so that it uses rom_add_blob_fixed() to load the
DTB, in order to make sure the DTB is available again at the expected offset
after a system reset.

Ard Biesheuvel (5):
  hw/arm/boot: return size of loaded DTB from load_dtb()
  hw/arm/boot: load device tree to base of DRAM if no -kernel option was
    passed
  hw/arm/boot: register cpu reset handlers if using -bios
  hw/arm/boot: load DTB as a ROM image
  hw/arm/boot: enable DTB support when booting ELF images

Peter Maydell (1):
  hw/arm/virt: Provide flash devices for boot ROMs

 hw/arm/boot.c | 45 +++++++++++++++++++++++++++++++++-----
 hw/arm/virt.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 5 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/6] hw/arm/virt: Provide flash devices for boot ROMs

[Ard Biesheuvel]

From: Peter Maydell <peter.maydell@linaro.org>

Add two flash devices to the virt board, so that it can be used for
running guests which want a bootrom image such as UEFI. We provide
two flash devices to make it more convenient to provide both a
read-only UEFI image and a read-write place to store guest-set
UEFI config variables. The '-bios' command line option is set up
to provide an image for the first of the two flash devices.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/arm/virt.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5853bb3ee4d5..c7f3680d6460 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -37,6 +37,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "hw/boards.h"
+#include "hw/loader.h"
 #include "exec/address-spaces.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
@@ -435,6 +436,73 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
     }
 }
 
+static void create_one_flash(const char *name, hwaddr flashbase,
+                             hwaddr flashsize)
+{
+    /* Create and map a single flash device. We use the same
+     * parameters as the flash devices on the Versatile Express board.
+     */
+    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+    const uint64_t sectorlength = 256 * 1024;
+
+    if (dinfo && qdev_prop_set_drive(dev, "drive", dinfo->bdrv)) {
+        abort();
+    }
+
+    qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
+    qdev_prop_set_uint64(dev, "sector-length", sectorlength);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_uint8(dev, "device-width", 2);
+    qdev_prop_set_uint8(dev, "big-endian", 0);
+    qdev_prop_set_uint16(dev, "id0", 0x89);
+    qdev_prop_set_uint16(dev, "id1", 0x18);
+    qdev_prop_set_uint16(dev, "id2", 0x00);
+    qdev_prop_set_uint16(dev, "id3", 0x00);
+    qdev_prop_set_string(dev, "name", name);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, flashbase);
+}
+
+static void create_flash(const VirtBoardInfo *vbi)
+{
+    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
+     * Any file passed via -bios goes in the first of these.
+     */
+    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
+    char *nodename;
+
+    if (bios_name) {
+        const char *fn;
+
+        if (drive_get(IF_PFLASH, 0, 0)) {
+            error_report("The contents of the first flash device may be "
+                         "specified with -bios or with -drive if=pflash... "
+                         "but you cannot use both options at once");
+            exit(1);
+        }
+        fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+        if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) {
+            error_report("Could not load ROM image '%s'", bios_name);
+            exit(1);
+        }
+    }
+
+    create_one_flash("virt.flash0", flashbase, flashsize);
+    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);
+
+    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+    qemu_fdt_add_subnode(vbi->fdt, nodename);
+    qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
+    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                 2, flashbase, 2, flashsize,
+                                 2, flashbase + flashsize, 2, flashsize);
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
+    g_free(nodename);
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
@@ -514,6 +582,8 @@ static void machvirt_init(MachineState *machine)
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
 
+    create_flash(vbi);
+
     create_gic(vbi, pic);
 
     create_uart(vbi, pic);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
>
> Add two flash devices to the virt board, so that it can be used for
> running guests which want a bootrom image such as UEFI. We provide
> two flash devices to make it more convenient to provide both a
> read-only UEFI image and a read-write place to store guest-set
> UEFI config variables. The '-bios' command line option is set up
> to provide an image for the first of the two flash devices.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/arm/virt.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)

This one's been around long enough that I'm going to add
it to target-arm.next now.

There were previously questions about whether we should
have flash or RAM at the bottom, but I think it makes
sense just to have a "like vexpress" config with two
flash devices. This does make telling QEMU about backing
storage for the 2nd flash a little complicated, but I
think anybody seriously running a config like that will
be using the management tools layer anyhow.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Provide flash devices for boot ROMs

[Ard Biesheuvel]

On 9 September 2014 20:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> From: Peter Maydell <peter.maydell@linaro.org>
>>
>> Add two flash devices to the virt board, so that it can be used for
>> running guests which want a bootrom image such as UEFI. We provide
>> two flash devices to make it more convenient to provide both a
>> read-only UEFI image and a read-write place to store guest-set
>> UEFI config variables. The '-bios' command line option is set up
>> to provide an image for the first of the two flash devices.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/arm/virt.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>
> This one's been around long enough that I'm going to add
> it to target-arm.next now.
>
> There were previously questions about whether we should
> have flash or RAM at the bottom, but I think it makes
> sense just to have a "like vexpress" config with two
> flash devices. This does make telling QEMU about backing
> storage for the 2nd flash a little complicated, but I
> think anybody seriously running a config like that will
> be using the management tools layer anyhow.
>

You mean having to use -pflash and pad the images out to 64 MB? I
wouldn't worry about that.

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

On 10 September 2014 10:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 20:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>> There were previously questions about whether we should
>> have flash or RAM at the bottom, but I think it makes
>> sense just to have a "like vexpress" config with two
>> flash devices. This does make telling QEMU about backing
>> storage for the 2nd flash a little complicated, but I
>> think anybody seriously running a config like that will
>> be using the management tools layer anyhow.

> You mean having to use -pflash and pad the images out to 64 MB? I
> wouldn't worry about that.

More particularly that if you don't want to provide backing
storage for the first flash but only the second, you can't just
use pflash but have to use the longer -drive options.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Provide flash devices for boot ROMs

[Ard Biesheuvel]

On 10 September 2014 11:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 September 2014 10:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 9 September 2014 20:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> There were previously questions about whether we should
>>> have flash or RAM at the bottom, but I think it makes
>>> sense just to have a "like vexpress" config with two
>>> flash devices. This does make telling QEMU about backing
>>> storage for the 2nd flash a little complicated, but I
>>> think anybody seriously running a config like that will
>>> be using the management tools layer anyhow.
>
>> You mean having to use -pflash and pad the images out to 64 MB? I
>> wouldn't worry about that.
>
> More particularly that if you don't want to provide backing
> storage for the first flash but only the second, you can't just
> use pflash but have to use the longer -drive options.
>

OK, I will drop this one from my series then.

[Qemu-devel] [PATCH 2/6] hw/arm/boot: return size of loaded DTB from load_dtb()

[Ard Biesheuvel]

Add a dtb_size output parameter to load_dtb() so that we can find out
what its memory footprint is.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e32f2f415885..c103a8fdc941 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -312,7 +312,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
     }
 }
 
-static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
+static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                    uint32_t *dtb_size)
 {
     void *fdt = NULL;
     int size, rc;
@@ -340,6 +341,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
             goto fail;
         }
     }
+    if (dtb_size != NULL) {
+        *dtb_size = size;
+    }
 
     acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
     scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
@@ -569,7 +573,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
              */
             hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
                                              4096);
-            if (load_dtb(dtb_start, info)) {
+            if (load_dtb(dtb_start, info, NULL)) {
                 exit(1);
             }
             fixupcontext[FIXUP_ARGPTR] = dtb_start;
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 2/6] hw/arm/boot: return size of loaded DTB from load_dtb()

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Add a dtb_size output parameter to load_dtb() so that we can find out
> what its memory footprint is.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index e32f2f415885..c103a8fdc941 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -312,7 +312,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>      }
>  }
>
> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
> +static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                    uint32_t *dtb_size)

Given that the return value is just a success/fail
indicator at the moment we could just change it
so we return the size of the dtb, with 0 meaning
"failure".

Either way, the size parameter/return should be
hwaddr, not uint32_t.

thanks
-- PMM

[Qemu-devel] [PATCH 3/6] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

If we are running the 'virt' machine, we may have a device tree blob but no
kernel to supply it to if no -kernel option was passed. In that case, copy it
to the base of RAM where it can be picked up by a bootloader.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c103a8fdc941..8f5649a250fd 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -463,6 +463,16 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
+
+        if (have_dtb(info)) {
+            /* If we have a device tree blob, but no kernel to supply it to,
+             * copy it to the base of RAM for a bootloader to pick up.
+             */
+            if (load_dtb(info->loader_start, info, NULL)) {
+                exit(1);
+            }
+        }
+
         /* If no kernel specified, do nothing; we will start from address 0
          * (typically a boot ROM image) in the same way as hardware.
          */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 3/6] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> If we are running the 'virt' machine, we may have a device tree blob but no
> kernel to supply it to if no -kernel option was passed. In that case, copy it
> to the base of RAM where it can be picked up by a bootloader.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c103a8fdc941..8f5649a250fd 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -463,6 +463,16 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
> +
> +        if (have_dtb(info)) {
> +            /* If we have a device tree blob, but no kernel to supply it to,
> +             * copy it to the base of RAM for a bootloader to pick up.
> +             */
> +            if (load_dtb(info->loader_start, info, NULL)) {
> +                exit(1);
> +            }
> +        }
> +
>          /* If no kernel specified, do nothing; we will start from address 0
>           * (typically a boot ROM image) in the same way as hardware.
>           */
> --
> 1.8.3.2

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(though obviously if you change the load_dtb() arg/return
this will need tweaking slightly).

-- PMM

[Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Ard Biesheuvel]

When booting with -bios or -pflash rather than -kernel, we need to make sure
reset handlers are registered.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 8f5649a250fd..0cfc11d42962 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             }
         }
 
+        for (; cs; cs = CPU_NEXT(cs)) {
+            qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
+        }
+
         /* If no kernel specified, do nothing; we will start from address 0
          * (typically a boot ROM image) in the same way as hardware.
          */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When booting with -bios or -pflash rather than -kernel, we need to make sure
> reset handlers are registered.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 8f5649a250fd..0cfc11d42962 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>              }
>          }
>
> +        for (; cs; cs = CPU_NEXT(cs)) {
> +            qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> +        }
> +
>          /* If no kernel specified, do nothing; we will start from address 0
>           * (typically a boot ROM image) in the same way as hardware.
>           */

Andreas, with QOM CPUs who is supposed to be responsible
for causing them to be reset when qemu_system_reset()
happens? At the moment for ARM it doesn't happen at all
unless you happened to pass -kernel. This patch makes
boot.c register the reset handlers, but that seems
a bit odd to me (among other things, it won't work
for v7M).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Ard Biesheuvel]

On 9 September 2014 11:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> When booting with -bios or -pflash rather than -kernel, we need to make sure
>> reset handlers are registered.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  hw/arm/boot.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 8f5649a250fd..0cfc11d42962 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>              }
>>          }
>>
>> +        for (; cs; cs = CPU_NEXT(cs)) {
>> +            qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>> +        }
>> +
>>          /* If no kernel specified, do nothing; we will start from address 0
>>           * (typically a boot ROM image) in the same way as hardware.
>>           */
>
> Andreas, with QOM CPUs who is supposed to be responsible
> for causing them to be reset when qemu_system_reset()
> happens? At the moment for ARM it doesn't happen at all
> unless you happened to pass -kernel. This patch makes
> boot.c register the reset handlers, but that seems
> a bit odd to me (among other things, it won't work
> for v7M).
>

Has there been any discussion on this topic in the mean time?

Cheers,
Ard.

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Andreas Färber]

Am 17.09.2014 um 17:50 schrieb Ard Biesheuvel:
> On 9 September 2014 11:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> When booting with -bios or -pflash rather than -kernel, we need to make sure
>>> reset handlers are registered.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  hw/arm/boot.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index 8f5649a250fd..0cfc11d42962 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -473,6 +473,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>              }
>>>          }
>>>
>>> +        for (; cs; cs = CPU_NEXT(cs)) {
>>> +            qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>>> +        }
>>> +
>>>          /* If no kernel specified, do nothing; we will start from address 0
>>>           * (typically a boot ROM image) in the same way as hardware.
>>>           */
>>
>> Andreas, with QOM CPUs who is supposed to be responsible
>> for causing them to be reset when qemu_system_reset()
>> happens? At the moment for ARM it doesn't happen at all
>> unless you happened to pass -kernel. This patch makes
>> boot.c register the reset handlers, but that seems
>> a bit odd to me (among other things, it won't work
>> for v7M).
>>
> 
> Has there been any discussion on this topic in the mean time?

No, thanks for the ping.

IIRC each machine is responsible for registering a reset hook that calls
- in most cases - cpu_reset().

The thing to look out for here is, does any machine already register a
reset hook and would reset twice with this patch? What's the issue with
v7-M?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Peter Maydell]

On 17 September 2014 08:55, Andreas Färber <afaerber@suse.de> wrote:
> IIRC each machine is responsible for registering a reset hook that calls
> - in most cases - cpu_reset().
>
> The thing to look out for here is, does any machine already register a
> reset hook and would reset twice with this patch?

Probably not -- any such double-reset would already be happening
if the user passed -kernel.

So in that sense this patch won't break things, but I wasn't
sure if it's the right direction to go -- should we be fixing
all the board and/or SoC models to do the CPU reset instead?

QOM devices get reset when their bus gets reset, right?
(so everything on a bus gets reset eventually as part of
the process that starts when the top level sysbus gets reset).

> What's the issue with v7-M?

Probably also broken, but IIRC we don't actually wire up the
register that's supposed to trigger a system reset...

-- PMM

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Andreas Färber]

Am 17.09.2014 um 18:17 schrieb Peter Maydell:
> On 17 September 2014 08:55, Andreas Färber <afaerber@suse.de> wrote:
>> IIRC each machine is responsible for registering a reset hook that calls
>> - in most cases - cpu_reset().
>>
>> The thing to look out for here is, does any machine already register a
>> reset hook and would reset twice with this patch?
> 
> Probably not -- any such double-reset would already be happening
> if the user passed -kernel.
> 
> So in that sense this patch won't break things, but I wasn't
> sure if it's the right direction to go -- should we be fixing
> all the board and/or SoC models to do the CPU reset instead?
> 
> QOM devices get reset when their bus gets reset, right?
> (so everything on a bus gets reset eventually as part of
> the process that starts when the top level sysbus gets reset).

We avoided that by not using DeviceClass::reset but CPUClass::reset.
It's a question of assuring appropriate reset ordering between CPU and
devices. PowerPC needed a special reset order via hook in (what is now)
MachineClass.

So while I agree that CPU reset registration is not ideal and needs
changing, I am not convinced that we can generally make the change and
hope for the best. I wouldn't mind an incremental transition though,
with arm taking the first step - still leaves the question of exact
direction. If you look at x86, you will find that despite my protest
against this inconsistency, the reset hook registration was moved into
CPU code but none of the other targets changed alongside.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Peter Maydell]

On 17 September 2014 09:40, Andreas Färber <afaerber@suse.de> wrote:
> We avoided that by not using DeviceClass::reset but CPUClass::reset.
> It's a question of assuring appropriate reset ordering between CPU and
> devices. PowerPC needed a special reset order via hook in (what is now)
> MachineClass.

> So while I agree that CPU reset registration is not ideal and needs
> changing, I am not convinced that we can generally make the change and
> hope for the best. I wouldn't mind an incremental transition though,
> with arm taking the first step - still leaves the question of exact
> direction. If you look at x86, you will find that despite my protest
> against this inconsistency, the reset hook registration was moved into
> CPU code but none of the other targets changed alongside.

I don't object to taking a pragmatic approach in the ARM code
(eg this patch). I just wanted to know if you had a preferred
direction we should be taking instead (which as you say we
kind of have to do in an incremental way). It sounds like you
don't have anything concrete in mind so maybe we should just
apply this patch.

In general I suspect there are a lot of unresolved issues in
our handling of reset -- it's a complicated area which we
attempt to address in an over-simplistic way at the moment :-(
But there's a pile of other problems on the ARM QEMU todo list
so as long as a change like this isn't actively working against
a transition you're working towards then it will solve the
immediate bug.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Andreas Färber]

Am 17.09.2014 um 18:47 schrieb Peter Maydell:
> On 17 September 2014 09:40, Andreas Färber <afaerber@suse.de> wrote:
>> We avoided that by not using DeviceClass::reset but CPUClass::reset.
>> It's a question of assuring appropriate reset ordering between CPU and
>> devices. PowerPC needed a special reset order via hook in (what is now)
>> MachineClass.
> 
>> So while I agree that CPU reset registration is not ideal and needs
>> changing, I am not convinced that we can generally make the change and
>> hope for the best. I wouldn't mind an incremental transition though,
>> with arm taking the first step - still leaves the question of exact
>> direction. If you look at x86, you will find that despite my protest
>> against this inconsistency, the reset hook registration was moved into
>> CPU code but none of the other targets changed alongside.
> 
> I don't object to taking a pragmatic approach in the ARM code
> (eg this patch). I just wanted to know if you had a preferred
> direction we should be taking instead (which as you say we
> kind of have to do in an incremental way). It sounds like you
> don't have anything concrete in mind so maybe we should just
> apply this patch.

Ack.

One other concern I have with this patch is the loop assuming that all
following CPUs will be of type ARMCPU, but I suspect there will be other
code making the same assumption - in that case Reviewed-by.

> In general I suspect there are a lot of unresolved issues in
> our handling of reset -- it's a complicated area which we
> attempt to address in an over-simplistic way at the moment :-(

Yes, having test cases for all machines would help refactor these things...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios

[Ard Biesheuvel]

On 17 September 2014 10:14, Andreas Färber <afaerber@suse.de> wrote:
> Am 17.09.2014 um 18:47 schrieb Peter Maydell:
>> On 17 September 2014 09:40, Andreas Färber <afaerber@suse.de> wrote:
>>> We avoided that by not using DeviceClass::reset but CPUClass::reset.
>>> It's a question of assuring appropriate reset ordering between CPU and
>>> devices. PowerPC needed a special reset order via hook in (what is now)
>>> MachineClass.
>>
>>> So while I agree that CPU reset registration is not ideal and needs
>>> changing, I am not convinced that we can generally make the change and
>>> hope for the best. I wouldn't mind an incremental transition though,
>>> with arm taking the first step - still leaves the question of exact
>>> direction. If you look at x86, you will find that despite my protest
>>> against this inconsistency, the reset hook registration was moved into
>>> CPU code but none of the other targets changed alongside.
>>
>> I don't object to taking a pragmatic approach in the ARM code
>> (eg this patch). I just wanted to know if you had a preferred
>> direction we should be taking instead (which as you say we
>> kind of have to do in an incremental way). It sounds like you
>> don't have anything concrete in mind so maybe we should just
>> apply this patch.
>
> Ack.
>
> One other concern I have with this patch is the loop assuming that all
> following CPUs will be of type ARMCPU, but I suspect there will be other
> code making the same assumption - in that case Reviewed-by.
>

Thanks.

So if this is the correct approach, it probably makes sense to modify
the patch and just move the existing code to the top of the function,
rather than having the duplicated for loop.
I.e.,

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c8dc34f0865b..fc6a3ebf853d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -488,6 +488,12 @@ void arm_load_kernel(ARMCPU *cpu, struct
arm_boot_info *info)
     int big_endian;
     static const ARMInsnFixup *primary_loader;

+    for (; cs; cs = CPU_NEXT(cs)) {
+        cpu = ARM_CPU(cs);
+        cpu->env.boot_info = info;
+        qemu_register_reset(do_cpu_reset, cpu);
+    }
+
     /* Load the kernel.  */
     if (!info->kernel_filename) {

@@ -651,10 +657,4 @@ void arm_load_kernel(ARMCPU *cpu, struct
arm_boot_info *info)
         }
     }
     info->is_linux = is_linux;
-
-    for (; cs; cs = CPU_NEXT(cs)) {
-        cpu = ARM_CPU(cs);
-        cpu->env.boot_info = info;
-        qemu_register_reset(do_cpu_reset, cpu);
-    }
 }

@Peter: would you like a new patch or are you happy to take it as is?

Cheers,
Ard.



>> In general I suspect there are a lot of unresolved issues in
>> our handling of reset -- it's a complicated area which we
>> attempt to address in an over-simplistic way at the moment :-(
>
> Yes, having test cases for all machines would help refactor these things...
>

[Qemu-devel] [PATCH 5/6] hw/arm/boot: load DTB as a ROM image

[Ard Biesheuvel]

In order to make the device tree blob (DTB) available in memory not only at
first boot, but also after system reset, use rom_blob_add_fixed() to install
it into memory.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 0cfc11d42962..8d8653978dfe 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -400,7 +400,10 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
 
     qemu_fdt_dumpdtb(fdt, size);
 
-    cpu_physical_memory_write(addr, fdt, size);
+    /* Put the DTB into the memory map as a ROM image: this will ensure
+     * the DTB is copied again upon reset, even if addr points into RAM.
+     */
+    rom_add_blob_fixed("dtb", fdt, size, addr);
 
     g_free(fdt);
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 5/6] hw/arm/boot: load DTB as a ROM image

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> In order to make the device tree blob (DTB) available in memory not only at
> first boot, but also after system reset, use rom_blob_add_fixed() to install
> it into memory.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 0cfc11d42962..8d8653978dfe 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -400,7 +400,10 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>
>      qemu_fdt_dumpdtb(fdt, size);
>
> -    cpu_physical_memory_write(addr, fdt, size);
> +    /* Put the DTB into the memory map as a ROM image: this will ensure
> +     * the DTB is copied again upon reset, even if addr points into RAM.
> +     */
> +    rom_add_blob_fixed("dtb", fdt, size, addr);
>
>      g_free(fdt);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

Add support for loading DTB images when booting ELF images via -kernel.
The DTB image is located at the next 4 KB boundary above the highest address
covered by the loaded ELF image.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 8d8653978dfe..60c4f6af7884 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -458,7 +458,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     int kernel_size;
     int initrd_size;
     int is_linux = 0;
-    uint64_t elf_entry;
+    uint64_t elf_entry, elf_low_addr;
     int elf_machine;
     hwaddr entry, kernel_load_offset;
     int big_endian;
@@ -529,7 +529,21 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Assume that raw images are linux kernels, and ELF images are not.  */
     kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
-                           NULL, NULL, big_endian, elf_machine, 1);
+                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
+    if (kernel_size > 0 && have_dtb(info)) {
+        /* If we have a DTB in combination with an ELF executable image,
+         * put the DTB at the base of RAM like we do for bootloaders.
+         */
+        uint32_t dtb_size;
+
+        if (load_dtb(info->loader_start, info, &dtb_size)) {
+            exit(1);
+        }
+        if (info->loader_start + dtb_size > elf_low_addr) {
+            fprintf(stderr, "Image %s overlaps DTB\n", info->kernel_filename);
+            exit(1);
+        }
+    }
     entry = elf_entry;
     if (kernel_size < 0) {
         kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

> On 5 sep. 2014, at 17:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> Add support for loading DTB images when booting ELF images via -kernel.
> The DTB image is located at the next 4 KB boundary above the highest address
> covered by the loaded ELF image.
> 

Apologies, this commit message is out of date: the DTB is put at base of RAM, bur only if it does not conflict with any of the ELF segments, otherwise we abort

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> hw/arm/boot.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 8d8653978dfe..60c4f6af7884 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -458,7 +458,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>     int kernel_size;
>     int initrd_size;
>     int is_linux = 0;
> -    uint64_t elf_entry;
> +    uint64_t elf_entry, elf_low_addr;
>     int elf_machine;
>     hwaddr entry, kernel_load_offset;
>     int big_endian;
> @@ -529,7 +529,21 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> 
>     /* Assume that raw images are linux kernels, and ELF images are not.  */
>     kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> -                           NULL, NULL, big_endian, elf_machine, 1);
> +                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
> +    if (kernel_size > 0 && have_dtb(info)) {
> +        /* If we have a DTB in combination with an ELF executable image,
> +         * put the DTB at the base of RAM like we do for bootloaders.
> +         */
> +        uint32_t dtb_size;
> +
> +        if (load_dtb(info->loader_start, info, &dtb_size)) {
> +            exit(1);
> +        }
> +        if (info->loader_start + dtb_size > elf_low_addr) {
> +            fprintf(stderr, "Image %s overlaps DTB\n", info->kernel_filename);
> +            exit(1);
> +        }
> +    }
>     entry = elf_entry;
>     if (kernel_size < 0) {
>         kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Peter Maydell]

On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Add support for loading DTB images when booting ELF images via -kernel.
> The DTB image is located at the next 4 KB boundary above the highest address
> covered by the loaded ELF image.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 8d8653978dfe..60c4f6af7884 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -458,7 +458,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> -    uint64_t elf_entry;
> +    uint64_t elf_entry, elf_low_addr;
>      int elf_machine;
>      hwaddr entry, kernel_load_offset;
>      int big_endian;
> @@ -529,7 +529,21 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>      kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> -                           NULL, NULL, big_endian, elf_machine, 1);
> +                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
> +    if (kernel_size > 0 && have_dtb(info)) {
> +        /* If we have a DTB in combination with an ELF executable image,
> +         * put the DTB at the base of RAM like we do for bootloaders.
> +         */
> +        uint32_t dtb_size;
> +
> +        if (load_dtb(info->loader_start, info, &dtb_size)) {
> +            exit(1);
> +        }
> +        if (info->loader_start + dtb_size > elf_low_addr) {
> +            fprintf(stderr, "Image %s overlaps DTB\n", info->kernel_filename);
> +            exit(1);
> +        }
> +    }

This shouldn't abort. The reason we need to manually check
for overlap is because the default rom-blob behaviour of
"overlap means don't start QEMU" isn't what we want.
In particular, boards like virt which autogenerate DTBs
will always have_dtb(), but we don't want to prevent
non-DTB-aware ELF blobs from loading anywhere they like.
The behaviour we need is "if blobs don't overlap then
load both, otherwise load the ELF file and ignore the
DTB".

(Thinking about it, that implies we either need a
rom_del_blob() or we need to tell load_dtb() about
areas of address space it needs to check for overlap
with before it registers the rom blob for the dtb.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

On 9 September 2014 20:08, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 16:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Add support for loading DTB images when booting ELF images via -kernel.
>> The DTB image is located at the next 4 KB boundary above the highest address
>> covered by the loaded ELF image.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  hw/arm/boot.c | 18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 8d8653978dfe..60c4f6af7884 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -458,7 +458,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>      int kernel_size;
>>      int initrd_size;
>>      int is_linux = 0;
>> -    uint64_t elf_entry;
>> +    uint64_t elf_entry, elf_low_addr;
>>      int elf_machine;
>>      hwaddr entry, kernel_load_offset;
>>      int big_endian;
>> @@ -529,7 +529,21 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>
>>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>>      kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
>> -                           NULL, NULL, big_endian, elf_machine, 1);
>> +                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
>> +    if (kernel_size > 0 && have_dtb(info)) {
>> +        /* If we have a DTB in combination with an ELF executable image,
>> +         * put the DTB at the base of RAM like we do for bootloaders.
>> +         */
>> +        uint32_t dtb_size;
>> +
>> +        if (load_dtb(info->loader_start, info, &dtb_size)) {
>> +            exit(1);
>> +        }
>> +        if (info->loader_start + dtb_size > elf_low_addr) {
>> +            fprintf(stderr, "Image %s overlaps DTB\n", info->kernel_filename);
>> +            exit(1);
>> +        }
>> +    }
>
> This shouldn't abort. The reason we need to manually check
> for overlap is because the default rom-blob behaviour of
> "overlap means don't start QEMU" isn't what we want.
> In particular, boards like virt which autogenerate DTBs
> will always have_dtb(), but we don't want to prevent
> non-DTB-aware ELF blobs from loading anywhere they like.
> The behaviour we need is "if blobs don't overlap then
> load both, otherwise load the ELF file and ignore the
> DTB".
>

OK

> (Thinking about it, that implies we either need a
> rom_del_blob() or we need to tell load_dtb() about
> areas of address space it needs to check for overlap
> with before it registers the rom blob for the dtb.)
>

... or we just call load_elf() again

Re: [Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Peter Maydell]

On 9 September 2014 19:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 20:08, Peter Maydell <peter.maydell@linaro.org> wrote:
>> (Thinking about it, that implies we either need a
>> rom_del_blob() or we need to tell load_dtb() about
>> areas of address space it needs to check for overlap
>> with before it registers the rom blob for the dtb.)
>>
>
> ... or we just call load_elf() again

That won't work, because we'll still trip the overlap
check in rom_load_all(), won't we?

-- PMM

Re: [Qemu-devel] [PATCH 6/6] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

On 9 September 2014 20:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 September 2014 19:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 9 September 2014 20:08, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> (Thinking about it, that implies we either need a
>>> rom_del_blob() or we need to tell load_dtb() about
>>> areas of address space it needs to check for overlap
>>> with before it registers the rom blob for the dtb.)
>>>
>>
>> ... or we just call load_elf() again
>
> That won't work, because we'll still trip the overlap
> check in rom_load_all(), won't we?
>

Yeah, you're right. My fingers are moving faster than my brain again

I will go ahead and rework load_dtb() to take a max_size parameter,
and load the dtb only if its size doesn't exceed max_size.

This should be sufficient to (a) implement the ELF case, and (b) not
complicate the other call sites too much