* [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
* [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 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 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 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 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 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
* 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-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.