* [Qemu-devel] [PATCH for-2.8 0/2] loader fixes @ 2016-11-28 19:56 Laszlo Ersek 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-28 19:56 UTC (permalink / raw) To: qemu devel list Cc: Michael S. Tsirkin, Alistair Francis, Igor Mammedov, Michael Walle, Paolo Bonzini, Peter Maydell, Shannon Zhao, qemu-arm 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs 2016-11-28 19:56 [Qemu-devel] [PATCH for-2.8 0/2] loader fixes Laszlo Ersek @ 2016-11-28 19:57 ` Laszlo Ersek 2016-11-28 23:07 ` Alistair Francis 2016-11-29 16:31 ` Michael S. Tsirkin 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() Laszlo Ersek 2016-11-29 13:37 ` [Qemu-devel] [PATCH for-2.8 0/2] loader fixes no-reply 2 siblings, 2 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-28 19:57 UTC (permalink / raw) To: qemu devel list Cc: Michael S. Tsirkin, Alistair Francis, Igor Mammedov, Michael Walle, Paolo Bonzini, Peter Maydell, Shannon Zhao, qemu-arm * 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek @ 2016-11-28 23:07 ` Alistair Francis 2016-11-29 16:31 ` Michael S. Tsirkin 1 sibling, 0 replies; 11+ messages in thread From: Alistair Francis @ 2016-11-28 23:07 UTC (permalink / raw) To: Laszlo Ersek Cc: qemu devel list, Peter Maydell, Michael S. Tsirkin, Shannon Zhao, Alistair Francis, Michael Walle, qemu-arm, Paolo Bonzini, Igor Mammedov 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 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek 2016-11-28 23:07 ` Alistair Francis @ 2016-11-29 16:31 ` Michael S. Tsirkin 1 sibling, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2016-11-29 16:31 UTC (permalink / raw) To: Laszlo Ersek Cc: qemu devel list, Alistair Francis, Igor Mammedov, Michael Walle, Paolo Bonzini, Peter Maydell, Shannon Zhao, qemu-arm 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() 2016-11-28 19:56 [Qemu-devel] [PATCH for-2.8 0/2] loader fixes Laszlo Ersek 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek @ 2016-11-28 19:57 ` Laszlo Ersek 2016-11-28 23:13 ` Alistair Francis 2016-11-29 16:29 ` Michael S. Tsirkin 2016-11-29 13:37 ` [Qemu-devel] [PATCH for-2.8 0/2] loader fixes no-reply 2 siblings, 2 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-28 19:57 UTC (permalink / raw) To: qemu devel list Cc: Michael S. Tsirkin, Alistair Francis, Paolo Bonzini, Peter Maydell According to ISO C99 / N1256 (referenced in HACKING): > 6.5.8 Relational operators > > 4 For the purposes of these operators, a pointer to an object that is > not an element of an array behaves the same as a pointer to the first > element of an array of length one with the type of the object as its > element type. > > 5 When two pointers are compared, the result depends on the relative > locations in the address space of the objects pointed to. If two > pointers to object or incomplete types both point to the same object, > or both point one past the last element of the same array object, they > compare equal. If the objects pointed to are members of the same > aggregate object, pointers to structure members declared later compare > greater than pointers to members declared earlier in the structure, > and pointers to array elements with larger subscript values compare > greater than pointers to elements of the same array with lower > subscript values. All pointers to members of the same union object > compare equal. If the expression /P/ points to an element of an array > object and the expression /Q/ points to the last element of the same > array object, the pointer expression /Q+1/ compares greater than /P/. > In all other cases, the behavior is undefined. Our AddressSpace objects are allocated generally individually, and kept in the "address_spaces" linked list, so we mustn't compare their addresses with relops. Convert the pointers subjected to the relop in rom_order_compare() to "uintptr_t": > 7.18.1.4 Integer types capable of holding object pointers > > 1 [...] > > The following type designates an unsigned integer type with the > property that any valid pointer to void can be converted to this type, > then converted back to pointer to void, and the result will compare > equal to the original pointer: > > /uintptr_t/ > > These types are optional. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Alistair Francis <alistair.francis@xilinx.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: qemu-devel@nongnu.org Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/core/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/core/loader.c b/hw/core/loader.c index c0d645a87134..766e48f2aec2 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -818,7 +818,7 @@ static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms); static inline bool rom_order_compare(Rom *rom, Rom *item) { - return (rom->as > item->as) || + return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) || (rom->as == item->as && rom->addr >= item->addr); } -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() Laszlo Ersek @ 2016-11-28 23:13 ` Alistair Francis 2016-11-29 16:29 ` Michael S. Tsirkin 1 sibling, 0 replies; 11+ messages in thread From: Alistair Francis @ 2016-11-28 23:13 UTC (permalink / raw) To: Laszlo Ersek Cc: qemu devel list, Peter Maydell, Paolo Bonzini, Alistair Francis, Michael S. Tsirkin On Mon, Nov 28, 2016 at 11:57 AM, Laszlo Ersek <lersek@redhat.com> wrote: > According to ISO C99 / N1256 (referenced in HACKING): > >> 6.5.8 Relational operators >> >> 4 For the purposes of these operators, a pointer to an object that is >> not an element of an array behaves the same as a pointer to the first >> element of an array of length one with the type of the object as its >> element type. >> >> 5 When two pointers are compared, the result depends on the relative >> locations in the address space of the objects pointed to. If two >> pointers to object or incomplete types both point to the same object, >> or both point one past the last element of the same array object, they >> compare equal. If the objects pointed to are members of the same >> aggregate object, pointers to structure members declared later compare >> greater than pointers to members declared earlier in the structure, >> and pointers to array elements with larger subscript values compare >> greater than pointers to elements of the same array with lower >> subscript values. All pointers to members of the same union object >> compare equal. If the expression /P/ points to an element of an array >> object and the expression /Q/ points to the last element of the same >> array object, the pointer expression /Q+1/ compares greater than /P/. >> In all other cases, the behavior is undefined. > > Our AddressSpace objects are allocated generally individually, and kept in > the "address_spaces" linked list, so we mustn't compare their addresses > with relops. > > Convert the pointers subjected to the relop in rom_order_compare() to > "uintptr_t": > >> 7.18.1.4 Integer types capable of holding object pointers >> >> 1 [...] >> >> The following type designates an unsigned integer type with the >> property that any valid pointer to void can be converted to this type, >> then converted back to pointer to void, and the result will compare >> equal to the original pointer: >> >> /uintptr_t/ >> >> These types are optional. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Alistair Francis <alistair.francis@xilinx.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-devel@nongnu.org > Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com> Thanks, Alistair > --- > hw/core/loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index c0d645a87134..766e48f2aec2 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -818,7 +818,7 @@ static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms); > > static inline bool rom_order_compare(Rom *rom, Rom *item) > { > - return (rom->as > item->as) || > + return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) || > (rom->as == item->as && rom->addr >= item->addr); > } > > -- > 2.9.2 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() Laszlo Ersek 2016-11-28 23:13 ` Alistair Francis @ 2016-11-29 16:29 ` Michael S. Tsirkin 2016-11-29 18:40 ` Laszlo Ersek 1 sibling, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2016-11-29 16:29 UTC (permalink / raw) To: Laszlo Ersek Cc: qemu devel list, Alistair Francis, Paolo Bonzini, Peter Maydell On Mon, Nov 28, 2016 at 08:57:01PM +0100, Laszlo Ersek wrote: > According to ISO C99 / N1256 (referenced in HACKING): > > > 6.5.8 Relational operators > > > > 4 For the purposes of these operators, a pointer to an object that is > > not an element of an array behaves the same as a pointer to the first > > element of an array of length one with the type of the object as its > > element type. > > > > 5 When two pointers are compared, the result depends on the relative > > locations in the address space of the objects pointed to. If two > > pointers to object or incomplete types both point to the same object, > > or both point one past the last element of the same array object, they > > compare equal. If the objects pointed to are members of the same > > aggregate object, pointers to structure members declared later compare > > greater than pointers to members declared earlier in the structure, > > and pointers to array elements with larger subscript values compare > > greater than pointers to elements of the same array with lower > > subscript values. All pointers to members of the same union object > > compare equal. If the expression /P/ points to an element of an array > > object and the expression /Q/ points to the last element of the same > > array object, the pointer expression /Q+1/ compares greater than /P/. > > In all other cases, the behavior is undefined. > > Our AddressSpace objects are allocated generally individually, and kept in > the "address_spaces" linked list, so we mustn't compare their addresses > with relops. > > Convert the pointers subjected to the relop in rom_order_compare() to > "uintptr_t": > > > 7.18.1.4 Integer types capable of holding object pointers > > > > 1 [...] > > > > The following type designates an unsigned integer type with the > > property that any valid pointer to void can be converted to this type, > > then converted back to pointer to void, and the result will compare > > equal to the original pointer: > > > > /uintptr_t/ > > > > These types are optional. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Alistair Francis <alistair.francis@xilinx.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-devel@nongnu.org > Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/core/loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index c0d645a87134..766e48f2aec2 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -818,7 +818,7 @@ static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms); > > static inline bool rom_order_compare(Rom *rom, Rom *item) > { > - return (rom->as > item->as) || > + return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) || > (rom->as == item->as && rom->addr >= item->addr); > } Can't hurt but why cast to void *? Should not be needed. > -- > 2.9.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() 2016-11-29 16:29 ` Michael S. Tsirkin @ 2016-11-29 18:40 ` Laszlo Ersek 0 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-29 18:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu devel list, Alistair Francis, Paolo Bonzini, Peter Maydell On 11/29/16 17:29, Michael S. Tsirkin wrote: > On Mon, Nov 28, 2016 at 08:57:01PM +0100, Laszlo Ersek wrote: >> According to ISO C99 / N1256 (referenced in HACKING): >> >>> 6.5.8 Relational operators >>> >>> 4 For the purposes of these operators, a pointer to an object that is >>> not an element of an array behaves the same as a pointer to the first >>> element of an array of length one with the type of the object as its >>> element type. >>> >>> 5 When two pointers are compared, the result depends on the relative >>> locations in the address space of the objects pointed to. If two >>> pointers to object or incomplete types both point to the same object, >>> or both point one past the last element of the same array object, they >>> compare equal. If the objects pointed to are members of the same >>> aggregate object, pointers to structure members declared later compare >>> greater than pointers to members declared earlier in the structure, >>> and pointers to array elements with larger subscript values compare >>> greater than pointers to elements of the same array with lower >>> subscript values. All pointers to members of the same union object >>> compare equal. If the expression /P/ points to an element of an array >>> object and the expression /Q/ points to the last element of the same >>> array object, the pointer expression /Q+1/ compares greater than /P/. >>> In all other cases, the behavior is undefined. >> >> Our AddressSpace objects are allocated generally individually, and kept in >> the "address_spaces" linked list, so we mustn't compare their addresses >> with relops. >> >> Convert the pointers subjected to the relop in rom_order_compare() to >> "uintptr_t": >> >>> 7.18.1.4 Integer types capable of holding object pointers >>> >>> 1 [...] >>> >>> The following type designates an unsigned integer type with the >>> property that any valid pointer to void can be converted to this type, >>> then converted back to pointer to void, and the result will compare >>> equal to the original pointer: >>> >>> /uintptr_t/ >>> >>> These types are optional. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Alistair Francis <alistair.francis@xilinx.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: qemu-devel@nongnu.org >> Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> hw/core/loader.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/core/loader.c b/hw/core/loader.c >> index c0d645a87134..766e48f2aec2 100644 >> --- a/hw/core/loader.c >> +++ b/hw/core/loader.c >> @@ -818,7 +818,7 @@ static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms); >> >> static inline bool rom_order_compare(Rom *rom, Rom *item) >> { >> - return (rom->as > item->as) || >> + return ((uintptr_t)(void*)rom->as > (uintptr_t)(void*)item->as) || >> (rom->as == item->as && rom->addr >= item->addr); >> } > > Can't hurt but why cast to void *? > Should not be needed. Just to comply with the word of the standard above; it says "any valid pointer to void". > >> -- >> 2.9.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes 2016-11-28 19:56 [Qemu-devel] [PATCH for-2.8 0/2] loader fixes Laszlo Ersek 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() Laszlo Ersek @ 2016-11-29 13:37 ` no-reply 2016-11-29 16:27 ` Michael S. Tsirkin 2 siblings, 1 reply; 11+ messages in thread From: no-reply @ 2016-11-29 13:37 UTC (permalink / raw) To: lersek Cc: famz, qemu-devel, peter.maydell, mst, zhaoshenglong, alistair.francis, michael, qemu-arm, pbonzini, imammedo 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes 2016-11-29 13:37 ` [Qemu-devel] [PATCH for-2.8 0/2] loader fixes no-reply @ 2016-11-29 16:27 ` Michael S. Tsirkin 2016-11-29 18:41 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2016-11-29 16:27 UTC (permalink / raw) To: qemu-devel Cc: lersek, famz, peter.maydell, zhaoshenglong, alistair.francis, michael, qemu-arm, pbonzini, imammedo 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 0/2] loader fixes 2016-11-29 16:27 ` Michael S. Tsirkin @ 2016-11-29 18:41 ` Laszlo Ersek 0 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-29 18:41 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel Cc: famz, peter.maydell, zhaoshenglong, alistair.francis, michael, qemu-arm, pbonzini, imammedo 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-29 18:41 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-28 19:56 [Qemu-devel] [PATCH for-2.8 0/2] loader fixes Laszlo Ersek 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 1/2] loader: fix handling of custom address spaces when adding ROM blobs Laszlo Ersek 2016-11-28 23:07 ` Alistair Francis 2016-11-29 16:31 ` Michael S. Tsirkin 2016-11-28 19:57 ` [Qemu-devel] [PATCH for-2.8 2/2] loader: fix undefined behavior in rom_order_compare() Laszlo Ersek 2016-11-28 23:13 ` Alistair Francis 2016-11-29 16:29 ` Michael S. Tsirkin 2016-11-29 18:40 ` Laszlo Ersek 2016-11-29 13:37 ` [Qemu-devel] [PATCH for-2.8 0/2] loader fixes no-reply 2016-11-29 16:27 ` Michael S. Tsirkin 2016-11-29 18:41 ` Laszlo Ersek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.