From 5449c230fac9cd6a645fe8c85693444dad512b59 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Wed, 23 Nov 2016 22:20:56 -0500 Subject: [PATCH 1/5] spec/vhost-user: fix the VHOST_USER prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Wei Wang Reviewed-by: Marc-André Lureau Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- docs/specs/vhost-user.txt | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt index 7890d71698e3..d70bd83b13cc 100644 --- a/docs/specs/vhost-user.txt +++ b/docs/specs/vhost-user.txt @@ -123,22 +123,22 @@ The communication consists of master sending message requests and slave sending message replies. Most of the requests don't require replies. Here is a list of the ones that do: - * VHOST_GET_FEATURES - * VHOST_GET_PROTOCOL_FEATURES - * VHOST_GET_VRING_BASE - * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) + * VHOST_USER_GET_FEATURES + * VHOST_USER_GET_PROTOCOL_FEATURES + * VHOST_USER_GET_VRING_BASE + * VHOST_USER_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) [ Also see the section on REPLY_ACK protocol extension. ] There are several messages that the master sends with file descriptors passed in the ancillary data: - * VHOST_SET_MEM_TABLE - * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) - * VHOST_SET_LOG_FD - * VHOST_SET_VRING_KICK - * VHOST_SET_VRING_CALL - * VHOST_SET_VRING_ERR + * VHOST_USER_SET_MEM_TABLE + * VHOST_USER_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD) + * VHOST_USER_SET_LOG_FD + * VHOST_USER_SET_VRING_KICK + * VHOST_USER_SET_VRING_CALL + * VHOST_USER_SET_VRING_ERR If Master is unable to send the full message or receives a wrong reply it will close the connection. An optional reconnection mechanism can be implemented. From 6cb99acc2808cc41e2d772a23e9cc564515535cc Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 29 Nov 2016 13:43:40 +0800 Subject: [PATCH 2/5] intel_iommu: fix incorrect device invalidate "mask" needs to be inverted before use. Signed-off-by: Peter Xu Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/intel_iommu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1b706adf9ded..5f3e35123d46 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -988,6 +988,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, mask = 7; /* Mask bit 2:0 in the SID field */ break; } + mask = ~mask; VTD_DPRINTF(INV, "device-selective invalidation source 0x%"PRIx16 " mask %"PRIu16, source_id, mask); vtd_bus = vtd_find_as_from_bus_num(s, VTD_SID_TO_BUS(source_id)); From aa6c6ae843cbdc251224bc6170d2663ac929b04f Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 29 Nov 2016 20:55:32 +0100 Subject: [PATCH 3/5] loader: fix handling of custom address spaces when adding ROM blobs * 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" Cc: Alistair Francis Cc: Igor Mammedov Cc: Michael Walle Cc: Paolo Bonzini Cc: Peter Maydell Cc: Shannon Zhao Cc: qemu-arm@nongnu.org Cc: qemu-devel@nongnu.org Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 Fixes: 5e774eb3bd264c76484906f4bd0fb38e00b8090e Signed-off-by: Laszlo Ersek Reviewed-by: Alistair Francis Reviewed-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/arm/virt-acpi-build.c | 2 +- hw/core/loader.c | 4 +++- hw/i386/acpi-build.c | 2 +- hw/lm32/lm32_hwsetup.h | 2 +- include/hw/loader.h | 6 +++--- 5 files changed, 9 insertions(+), 7 deletions(-) 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 @@ int rom_add_file(const char *file, const char *fw_dir, 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 = { 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 From 1b57bd4f2f4993104a5cb48912435396faa10d58 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 29 Nov 2016 20:55:33 +0100 Subject: [PATCH 4/5] loader: fix undefined behavior in rom_order_compare() 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" Cc: Alistair Francis Cc: Paolo Bonzini Cc: Peter Maydell Cc: qemu-devel@nongnu.org Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 Signed-off-by: Laszlo Ersek Reviewed-by: Alistair Francis Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- 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..45742494e6fd 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); } From 9730280d54634caa5d63f0d8fcd85da8311d2ebf Mon Sep 17 00:00:00 2001 From: Gonglei Date: Sat, 26 Nov 2016 11:07:55 +0800 Subject: [PATCH 5/5] virtio-crypto: fix uninitialized variables Though crypto_cfg.reserve is an unused field, let me initialize the structure in order to make coverity happy. *** CID 1365923: Uninitialized variables (UNINIT) /hw/virtio/virtio-crypto.c: 851 in virtio_crypto_get_config() 845 stl_le_p(&crypto_cfg.mac_algo_h, c->conf.mac_algo_h); 846 stl_le_p(&crypto_cfg.aead_algo, c->conf.aead_algo); 847 stl_le_p(&crypto_cfg.max_cipher_key_len, c->conf.max_cipher_key_len); 848 stl_le_p(&crypto_cfg.max_auth_key_len, c->conf.max_auth_key_len); 849 stq_le_p(&crypto_cfg.max_size, c->conf.max_size); 850 >>> CID 1365923: Uninitialized variables (UNINIT) >>> Using uninitialized value "crypto_cfg". Field "crypto_cfg.reserve" is uninitialized when calling "memcpy". [Note: The source code implementation of the function has been overridden by a builtin model.] 851 memcpy(config, &crypto_cfg, c->config_size); 852 } 853 Rported-by: Markus Armbruster Signed-off-by: Gonglei Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c index 847dc9dafdf4..2f2467e859f0 100644 --- a/hw/virtio/virtio-crypto.c +++ b/hw/virtio/virtio-crypto.c @@ -838,7 +838,7 @@ static Property virtio_crypto_properties[] = { static void virtio_crypto_get_config(VirtIODevice *vdev, uint8_t *config) { VirtIOCrypto *c = VIRTIO_CRYPTO(vdev); - struct virtio_crypto_config crypto_cfg; + struct virtio_crypto_config crypto_cfg = {}; /* * Virtio-crypto device conforms to VIRTIO 1.0 which is always LE,