[Qemu-arm] [PATCH for-2.8 0/2] loader fixes

[Qemu-arm] [PATCH for-2.8 0/2] loader fixes

[Laszlo Ersek]

While rebasing Michael's patch at
<http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02735.html>
to current master, I noticed something was wrong with rom_add_blob() in
current master. I'm proposing these fixes.

Thanks
Laszlo

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-devel@nongnu.org

Laszlo Ersek (2):
  loader: fix handling of custom address spaces when adding ROM blobs
  loader: fix undefined behavior in rom_order_compare()

 hw/lm32/lm32_hwsetup.h   | 2 +-
 include/hw/loader.h      | 6 +++---
 hw/arm/virt-acpi-build.c | 2 +-
 hw/core/loader.c         | 6 ++++--
 hw/i386/acpi-build.c     | 2 +-
 5 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.9.2

[Qemu-arm] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs

[Laszlo Ersek]

* Commit 3e76099aacb4 ("loader: Allow a custom AddressSpace when loading
  ROMs") introduced the "Rom.as" field:

  (1) It modified the utility callers of rom_insert() to take "as" as a
      new parameter from *their* callers, and set "rom->as" from that
      parameter. The functions covered were rom_add_file() and
      rom_add_elf_program().

  (2) It also modified rom_insert() itself, to auto-assign
      "&address_space_memory", in case the external caller passed -- and
      the utility caller forwarded -- as=NULL.

  Except, commit 3e76099aacb4 forgot to update the third utility caller of
  rom_insert(), under point (1), namely rom_add_blob().

* Later, commit 5e774eb3bd264 ("loader: Add AddressSpace loading support
  to uImages") added the load_uimage_as() function, and the
  rom_add_blob_fixed_as() function-like macro, with the necessary changes
  elsewhere to propagate the new "as" parameter to rom_add_blob():

    load_uimage_as()
      load_uboot_image()
        rom_add_blob_fixed_as()
          rom_add_blob()

  At this point, the signature (and workings) of rom_add_blob() had been
  broken already, and the rom_add_blob_fixed_as() macro passed its "_as"
  parameter to rom_add_blob() as "callback_opaque". Given that the
  "fw_callback" parameter itself was set to NULL (correctly), this did no
  additional damage (the opaque arg would never be used), but ultimately
  it broke the new functionality of load_uimage_as().

* The load_uimage_as() function would be put to use in one of the later
  patches, commit e481a1f63c93 ("generic-loader: Add a generic loader").

* We can fix this only in a unified patch now. Append "AddressSpace *as"
  to the signature of rom_add_blob(), and handle the new parameter. Pass
  NULL from all current callers, except from rom_add_blob_fixed_as(),
  where "_as" has to be bumped to the proper position.

* Note that rom_add_file() rejects the case when both "mr" and "as" are
  passed in as non-NULL. The action that this is apparently supposed to
  prevent is the

    rom->mr = mr;

  assignment (that's the only place where the "mr" parameter is used in
  rom_add_file()). In rom_add_blob() though, we have no "mr" parameter,
  and the actions done on the fw_cfg branch:

    if (fw_file_name && fw_cfg) {
        if (mc->rom_file_has_mr) {
            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
            mr = rom->mr;
        } else {
            data = rom->data;
        }

  reflect those that are performed by rom_add_file() too (with mr==NULL):

    if (rom->fw_file && fw_cfg) {
        if ((!option_rom || mc->option_rom_has_mr) &&
            mc->rom_file_has_mr) {
            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
        } else {
            data = rom->data;
        }

  Hence we need no additional restrictions in rom_add_blob().

* Stable is not affected as both problematic commits appeared first in
  v2.8.0-rc0.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-devel@nongnu.org
Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6
Fixes: 5e774eb3bd264c76484906f4bd0fb38e00b8090e
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/lm32/lm32_hwsetup.h   | 2 +-
 include/hw/loader.h      | 6 +++---
 hw/arm/virt-acpi-build.c | 2 +-
 hw/core/loader.c         | 4 +++-
 hw/i386/acpi-build.c     | 2 +-
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
index b71e6eafba48..23e18784dffd 100644
--- a/hw/lm32/lm32_hwsetup.h
+++ b/hw/lm32/lm32_hwsetup.h
@@ -75,7 +75,7 @@ static inline void hwsetup_create_rom(HWSetup *hw,
         hwaddr base)
 {
     rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
-                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
+                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL);
 }
 
 static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 038170624d8f..0c864cfd6046 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -180,7 +180,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
                            FWCfgReadCallback fw_callback,
-                           void *callback_opaque);
+                           void *callback_opaque, AddressSpace *as);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
                         size_t romsize, hwaddr addr, AddressSpace *as);
 int rom_check_and_register_reset(void);
@@ -194,7 +194,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
 #define rom_add_file_fixed(_f, _a, _i)          \
     rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
-    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
+    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
 #define rom_add_file_mr(_f, _mr, _i)            \
     rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
 #define rom_add_file_as(_f, _as, _i)            \
@@ -202,7 +202,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
 #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
     rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
 #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
-    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as)
+    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f953610018c4..d4160dfa7d34 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -809,7 +809,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
                                        uint64_t max_size)
 {
     return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, virt_acpi_build_update, build_state);
+                        name, virt_acpi_build_update, build_state, NULL);
 }
 
 static const VMStateDescription vmstate_virt_acpi_build = {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6e022b5ad583..c0d645a87134 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -978,7 +978,8 @@ err:
 
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                    size_t max_len, hwaddr addr, const char *fw_file_name,
-                   FWCfgReadCallback fw_callback, void *callback_opaque)
+                   FWCfgReadCallback fw_callback, void *callback_opaque,
+                   AddressSpace *as)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
@@ -986,6 +987,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
 
     rom           = g_malloc0(sizeof(*rom));
     rom->name     = g_strdup(name);
+    rom->as       = as;
     rom->addr     = addr;
     rom->romsize  = max_len ? max_len : len;
     rom->datasize = len;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 45a2ccfc4c60..9708cdc463df 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2936,7 +2936,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
                                        uint64_t max_size)
 {
     return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, acpi_build_update, build_state);
+                        name, acpi_build_update, build_state, NULL);
 }
 
 static const VMStateDescription vmstate_acpi_build = {
-- 
2.9.2

Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs

[Alistair Francis]

On Mon, Nov 28, 2016 at 11:57 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> * Commit 3e76099aacb4 ("loader: Allow a custom AddressSpace when loading
>   ROMs") introduced the "Rom.as" field:
>
>   (1) It modified the utility callers of rom_insert() to take "as" as a
>       new parameter from *their* callers, and set "rom->as" from that
>       parameter. The functions covered were rom_add_file() and
>       rom_add_elf_program().
>
>   (2) It also modified rom_insert() itself, to auto-assign
>       "&address_space_memory", in case the external caller passed -- and
>       the utility caller forwarded -- as=NULL.
>
>   Except, commit 3e76099aacb4 forgot to update the third utility caller of
>   rom_insert(), under point (1), namely rom_add_blob().

I was a little confused by this part. I though you were saying that
it's wrong that I didn't allow rom_add_blob() to add an address space
in that commit.

The idea was that an address space is not required to be set as
rom_insert() will default to address_space_memory if nothing is set.
Although as you mention later it actually did need to be added for the
uimage loading to work as expected.

>
> * Later, commit 5e774eb3bd264 ("loader: Add AddressSpace loading support
>   to uImages") added the load_uimage_as() function, and the
>   rom_add_blob_fixed_as() function-like macro, with the necessary changes
>   elsewhere to propagate the new "as" parameter to rom_add_blob():
>
>     load_uimage_as()
>       load_uboot_image()
>         rom_add_blob_fixed_as()
>           rom_add_blob()
>
>   At this point, the signature (and workings) of rom_add_blob() had been
>   broken already, and the rom_add_blob_fixed_as() macro passed its "_as"
>   parameter to rom_add_blob() as "callback_opaque". Given that the
>   "fw_callback" parameter itself was set to NULL (correctly), this did no
>   additional damage (the opaque arg would never be used), but ultimately
>   it broke the new functionality of load_uimage_as().
>
> * The load_uimage_as() function would be put to use in one of the later
>   patches, commit e481a1f63c93 ("generic-loader: Add a generic loader").
>
> * We can fix this only in a unified patch now. Append "AddressSpace *as"
>   to the signature of rom_add_blob(), and handle the new parameter. Pass
>   NULL from all current callers, except from rom_add_blob_fixed_as(),
>   where "_as" has to be bumped to the proper position.
>
> * Note that rom_add_file() rejects the case when both "mr" and "as" are
>   passed in as non-NULL. The action that this is apparently supposed to
>   prevent is the
>
>     rom->mr = mr;
>
>   assignment (that's the only place where the "mr" parameter is used in
>   rom_add_file()). In rom_add_blob() though, we have no "mr" parameter,
>   and the actions done on the fw_cfg branch:
>
>     if (fw_file_name && fw_cfg) {
>         if (mc->rom_file_has_mr) {
>             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>             mr = rom->mr;
>         } else {
>             data = rom->data;
>         }
>
>   reflect those that are performed by rom_add_file() too (with mr==NULL):
>
>     if (rom->fw_file && fw_cfg) {
>         if ((!option_rom || mc->option_rom_has_mr) &&
>             mc->rom_file_has_mr) {
>             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>         } else {
>             data = rom->data;
>         }
>
>   Hence we need no additional restrictions in rom_add_blob().
>
> * Stable is not affected as both problematic commits appeared first in
>   v2.8.0-rc0.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org
> Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6
> Fixes: 5e774eb3bd264c76484906f4bd0fb38e00b8090e
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Great catch. Sorry about the mistake in there.

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

Thanks,

Alistair

> ---
>  hw/lm32/lm32_hwsetup.h   | 2 +-
>  include/hw/loader.h      | 6 +++---
>  hw/arm/virt-acpi-build.c | 2 +-
>  hw/core/loader.c         | 4 +++-
>  hw/i386/acpi-build.c     | 2 +-
>  5 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
> index b71e6eafba48..23e18784dffd 100644
> --- a/hw/lm32/lm32_hwsetup.h
> +++ b/hw/lm32/lm32_hwsetup.h
> @@ -75,7 +75,7 @@ static inline void hwsetup_create_rom(HWSetup *hw,
>          hwaddr base)
>  {
>      rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
> -                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
> +                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL);
>  }
>
>  static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 038170624d8f..0c864cfd6046 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -180,7 +180,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                             size_t max_len, hwaddr addr,
>                             const char *fw_file_name,
>                             FWCfgReadCallback fw_callback,
> -                           void *callback_opaque);
> +                           void *callback_opaque, AddressSpace *as);
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
>                          size_t romsize, hwaddr addr, AddressSpace *as);
>  int rom_check_and_register_reset(void);
> @@ -194,7 +194,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed(_f, _a, _i)          \
>      rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
> -    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
> +    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
>  #define rom_add_file_mr(_f, _mr, _i)            \
>      rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
>  #define rom_add_file_as(_f, _as, _i)            \
> @@ -202,7 +202,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
>      rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
>  #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
> -    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as)
> +    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
>
>  #define PC_ROM_MIN_VGA     0xc0000
>  #define PC_ROM_MIN_OPTION  0xc8000
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f953610018c4..d4160dfa7d34 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -809,7 +809,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>                                         uint64_t max_size)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
> -                        name, virt_acpi_build_update, build_state);
> +                        name, virt_acpi_build_update, build_state, NULL);
>  }
>
>  static const VMStateDescription vmstate_virt_acpi_build = {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6e022b5ad583..c0d645a87134 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -978,7 +978,8 @@ err:
>
>  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                     size_t max_len, hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque)
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   AddressSpace *as)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>      Rom *rom;
> @@ -986,6 +987,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>
>      rom           = g_malloc0(sizeof(*rom));
>      rom->name     = g_strdup(name);
> +    rom->as       = as;
>      rom->addr     = addr;
>      rom->romsize  = max_len ? max_len : len;
>      rom->datasize = len;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 45a2ccfc4c60..9708cdc463df 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2936,7 +2936,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>                                         uint64_t max_size)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
> -                        name, acpi_build_update, build_state);
> +                        name, acpi_build_update, build_state, NULL);
>  }
>
>  static const VMStateDescription vmstate_acpi_build = {
> --
> 2.9.2
>
>
>

Re: [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs

[Michael S. Tsirkin]

On Mon, Nov 28, 2016 at 08:57:00PM +0100, Laszlo Ersek wrote:
> * Commit 3e76099aacb4 ("loader: Allow a custom AddressSpace when loading
>   ROMs") introduced the "Rom.as" field:
> 
>   (1) It modified the utility callers of rom_insert() to take "as" as a
>       new parameter from *their* callers, and set "rom->as" from that
>       parameter. The functions covered were rom_add_file() and
>       rom_add_elf_program().
> 
>   (2) It also modified rom_insert() itself, to auto-assign
>       "&address_space_memory", in case the external caller passed -- and
>       the utility caller forwarded -- as=NULL.
> 
>   Except, commit 3e76099aacb4 forgot to update the third utility caller of
>   rom_insert(), under point (1), namely rom_add_blob().
> 
> * Later, commit 5e774eb3bd264 ("loader: Add AddressSpace loading support
>   to uImages") added the load_uimage_as() function, and the
>   rom_add_blob_fixed_as() function-like macro, with the necessary changes
>   elsewhere to propagate the new "as" parameter to rom_add_blob():
> 
>     load_uimage_as()
>       load_uboot_image()
>         rom_add_blob_fixed_as()
>           rom_add_blob()
> 
>   At this point, the signature (and workings) of rom_add_blob() had been
>   broken already, and the rom_add_blob_fixed_as() macro passed its "_as"
>   parameter to rom_add_blob() as "callback_opaque". Given that the
>   "fw_callback" parameter itself was set to NULL (correctly), this did no
>   additional damage (the opaque arg would never be used), but ultimately
>   it broke the new functionality of load_uimage_as().
> 
> * The load_uimage_as() function would be put to use in one of the later
>   patches, commit e481a1f63c93 ("generic-loader: Add a generic loader").
> 
> * We can fix this only in a unified patch now. Append "AddressSpace *as"
>   to the signature of rom_add_blob(), and handle the new parameter. Pass
>   NULL from all current callers, except from rom_add_blob_fixed_as(),
>   where "_as" has to be bumped to the proper position.
> 
> * Note that rom_add_file() rejects the case when both "mr" and "as" are
>   passed in as non-NULL. The action that this is apparently supposed to
>   prevent is the
> 
>     rom->mr = mr;
> 
>   assignment (that's the only place where the "mr" parameter is used in
>   rom_add_file()). In rom_add_blob() though, we have no "mr" parameter,
>   and the actions done on the fw_cfg branch:
> 
>     if (fw_file_name && fw_cfg) {
>         if (mc->rom_file_has_mr) {
>             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>             mr = rom->mr;
>         } else {
>             data = rom->data;
>         }
> 
>   reflect those that are performed by rom_add_file() too (with mr==NULL):
> 
>     if (rom->fw_file && fw_cfg) {
>         if ((!option_rom || mc->option_rom_has_mr) &&
>             mc->rom_file_has_mr) {
>             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>         } else {
>             data = rom->data;
>         }
> 
>   Hence we need no additional restrictions in rom_add_blob().
> 
> * Stable is not affected as both problematic commits appeared first in
>   v2.8.0-rc0.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org
> Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6
> Fixes: 5e774eb3bd264c76484906f4bd0fb38e00b8090e
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/lm32/lm32_hwsetup.h   | 2 +-
>  include/hw/loader.h      | 6 +++---
>  hw/arm/virt-acpi-build.c | 2 +-
>  hw/core/loader.c         | 4 +++-
>  hw/i386/acpi-build.c     | 2 +-
>  5 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
> index b71e6eafba48..23e18784dffd 100644
> --- a/hw/lm32/lm32_hwsetup.h
> +++ b/hw/lm32/lm32_hwsetup.h
> @@ -75,7 +75,7 @@ static inline void hwsetup_create_rom(HWSetup *hw,
>          hwaddr base)
>  {
>      rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
> -                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
> +                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL);
>  }
>  
>  static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 038170624d8f..0c864cfd6046 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -180,7 +180,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                             size_t max_len, hwaddr addr,
>                             const char *fw_file_name,
>                             FWCfgReadCallback fw_callback,
> -                           void *callback_opaque);
> +                           void *callback_opaque, AddressSpace *as);

And so we are up to 10 parameters :(

No better ideas so

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
>                          size_t romsize, hwaddr addr, AddressSpace *as);
>  int rom_check_and_register_reset(void);
> @@ -194,7 +194,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed(_f, _a, _i)          \
>      rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
> -    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
> +    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
>  #define rom_add_file_mr(_f, _mr, _i)            \
>      rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
>  #define rom_add_file_as(_f, _as, _i)            \
> @@ -202,7 +202,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
>      rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
>  #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
> -    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as)
> +    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
>  
>  #define PC_ROM_MIN_VGA     0xc0000
>  #define PC_ROM_MIN_OPTION  0xc8000
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f953610018c4..d4160dfa7d34 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -809,7 +809,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>                                         uint64_t max_size)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
> -                        name, virt_acpi_build_update, build_state);
> +                        name, virt_acpi_build_update, build_state, NULL);
>  }
>  
>  static const VMStateDescription vmstate_virt_acpi_build = {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6e022b5ad583..c0d645a87134 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -978,7 +978,8 @@ err:
>  
>  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                     size_t max_len, hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque)
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   AddressSpace *as)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>      Rom *rom;
> @@ -986,6 +987,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>  
>      rom           = g_malloc0(sizeof(*rom));
>      rom->name     = g_strdup(name);
> +    rom->as       = as;
>      rom->addr     = addr;
>      rom->romsize  = max_len ? max_len : len;
>      rom->datasize = len;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 45a2ccfc4c60..9708cdc463df 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2936,7 +2936,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>                                         uint64_t max_size)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
> -                        name, acpi_build_update, build_state);
> +                        name, acpi_build_update, build_state, NULL);
>  }
>  
>  static const VMStateDescription vmstate_acpi_build = {
> -- 
> 2.9.2
> 

Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.8 0/2] loader fixes

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes
Type: series
Message-id: 20161128195701.24912-1-lersek@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
a700709 loader: fix undefined behavior in rom_order_compare()
54ae01b loader: fix handling of custom address spaces when adding ROM blobs

=== OUTPUT BEGIN ===
Checking PATCH 1/2: loader: fix handling of custom address spaces when adding ROM blobs...
Checking PATCH 2/2: loader: fix undefined behavior in rom_order_compare()...
ERROR: "(foo*)" should be "(foo *)"
#69: FILE: hw/core/loader.c:821:
+    return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) ||

total: 1 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.8 0/2] loader fixes

[Michael S. Tsirkin]

On Tue, Nov 29, 2016 at 05:37:02AM -0800, no-reply@patchew.org wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes
> Type: series
> Message-id: 20161128195701.24912-1-lersek@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> a700709 loader: fix undefined behavior in rom_order_compare()
> 54ae01b loader: fix handling of custom address spaces when adding ROM blobs
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/2: loader: fix handling of custom address spaces when adding ROM blobs...
> Checking PATCH 2/2: loader: fix undefined behavior in rom_order_compare()...
> ERROR: "(foo*)" should be "(foo *)"
> #69: FILE: hw/core/loader.c:821:
> +    return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) ||
> 
> total: 1 errors, 0 warnings, 8 lines checked

True. Can you fix this up pls?


> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.8 0/2] loader fixes

[Laszlo Ersek]

On 11/29/16 17:27, Michael S. Tsirkin wrote:
> On Tue, Nov 29, 2016 at 05:37:02AM -0800, no-reply@patchew.org wrote:
>> Hi,
>>
>> Your series seems to have some coding style problems. See output below for
>> more information:
>>
>> Subject: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes
>> Type: series
>> Message-id: 20161128195701.24912-1-lersek@redhat.com
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>>
>> BASE=base
>> n=1
>> total=$(git log --oneline $BASE.. | wc -l)
>> failed=0
>>
>> # Useful git options
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>>
>> commits="$(git log --format=%H --reverse $BASE..)"
>> for c in $commits; do
>>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>>         failed=1
>>         echo
>>     fi
>>     n=$((n+1))
>> done
>>
>> exit $failed
>> === TEST SCRIPT END ===
>>
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> a700709 loader: fix undefined behavior in rom_order_compare()
>> 54ae01b loader: fix handling of custom address spaces when adding ROM blobs
>>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/2: loader: fix handling of custom address spaces when adding ROM blobs...
>> Checking PATCH 2/2: loader: fix undefined behavior in rom_order_compare()...
>> ERROR: "(foo*)" should be "(foo *)"
>> #69: FILE: hw/core/loader.c:821:
>> +    return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) ||
>>
>> total: 1 errors, 0 warnings, 8 lines checked
> 
> True. Can you fix this up pls?

Yes, I'll post a new version tomorrow.

Thanks!
Laszlo

>> Your patch has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>>
>> === OUTPUT END ===
>>
>> Test command exited with code: 1
>>
>>
>> ---
>> Email generated automatically by Patchew [http://patchew.org/].
>> Please send your feedback to patchew-devel@freelists.org