From decbc880289526d94495bcbe6e1ba2a11b92e7a8 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 21 Nov 2013 13:59:15 +0200 Subject: [PATCH 1/6] s390x: fix flat file load on 32 bit systems pc-bios/s390-zipl.rom is a flat image so it's expected that loading it as elf will fail. It should fall back on loading a flat file, but doesn't on 32 bit systems, instead it fails printing: qemu: hardware error: could not load bootloader 's390-zipl.rom' The result is boot failure. The reason is that a 64 bit unsigned interger which is set to -1 on error is compared to -1UL which on a 32 bit system with gcc is a 32 bit unsigned interger. Since both are unsigned, no sign extension takes place and comparison evaluates to non-equal. There's no reason to do clever tricks: all functions we call actually return int so just use int. And then we can use == -1 everywhere, consistently. Reviewed-by: Alexander Graf Reviewed-by: Cornelia Huck Signed-off-by: Michael S. Tsirkin --- hw/s390x/ipl.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index d69adb2f5b..65d39da314 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -62,10 +62,10 @@ typedef struct S390IPLState { static int s390_ipl_init(SysBusDevice *dev) { S390IPLState *ipl = S390_IPL(dev); - ram_addr_t kernel_size = 0; + int kernel_size; if (!ipl->kernel) { - ram_addr_t bios_size = 0; + int bios_size; char *bios_filename; /* Load zipl bootloader */ @@ -80,7 +80,7 @@ static int s390_ipl_init(SysBusDevice *dev) bios_size = load_elf(bios_filename, NULL, NULL, &ipl->start_addr, NULL, NULL, 1, ELF_MACHINE, 0); - if (bios_size == -1UL) { + if (bios_size == -1) { bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START, 4096); ipl->start_addr = ZIPL_IMAGE_START; @@ -90,17 +90,17 @@ static int s390_ipl_init(SysBusDevice *dev) } g_free(bios_filename); - if ((long)bios_size < 0) { + if (bios_size == -1) { hw_error("could not load bootloader '%s'\n", bios_name); } return 0; } else { kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL, NULL, 1, ELF_MACHINE, 0); - if (kernel_size == -1UL) { + if (kernel_size == -1) { kernel_size = load_image_targphys(ipl->kernel, 0, ram_size); } - if (kernel_size == -1UL) { + if (kernel_size == -1) { fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel); return -1; } @@ -115,7 +115,8 @@ static int s390_ipl_init(SysBusDevice *dev) ipl->start_addr = KERN_IMAGE_START; } if (ipl->initrd) { - ram_addr_t initrd_offset, initrd_size; + ram_addr_t initrd_offset; + int initrd_size; initrd_offset = INITRD_START; while (kernel_size + 0x100000 > initrd_offset) { @@ -123,7 +124,7 @@ static int s390_ipl_init(SysBusDevice *dev) } initrd_size = load_image_targphys(ipl->initrd, initrd_offset, ram_size - initrd_offset); - if (initrd_size == -1UL) { + if (initrd_size == -1) { fprintf(stderr, "qemu: could not load initrd '%s'\n", ipl->initrd); exit(1); } From 5c397242d5d53c1adecce31817bb439383cf8228 Mon Sep 17 00:00:00 2001 From: Bandan Das Date: Wed, 6 Nov 2013 17:52:17 -0500 Subject: [PATCH 2/6] pci: unregister vmstate_pcibus on unplug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PCIBus registers a vmstate during init. Unregister it upon removal/unplug. Signed-off-by: Bandan Das Cc: qemu-stable@nongnu.org Reviewed-by: Andreas Färber Signed-off-by: Michael S. Tsirkin --- hw/pci/pci.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index ed32059bf8..49eca955aa 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -47,6 +47,7 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); static char *pcibus_get_dev_path(DeviceState *dev); static char *pcibus_get_fw_dev_path(DeviceState *dev); static int pcibus_reset(BusState *qbus); +static void pci_bus_finalize(Object *obj); static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), @@ -73,6 +74,7 @@ static const TypeInfo pci_bus_info = { .name = TYPE_PCI_BUS, .parent = TYPE_BUS, .instance_size = sizeof(PCIBus), + .instance_finalize = pci_bus_finalize, .class_init = pci_bus_class_init, }; @@ -375,6 +377,12 @@ int pci_bus_num(PCIBus *s) return s->parent_dev->config[PCI_SECONDARY_BUS]; } +static void pci_bus_finalize(Object *obj) +{ + PCIBus *bus = PCI_BUS(obj); + vmstate_unregister(NULL, &vmstate_pcibus, bus); +} + static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) { PCIDevice *s = container_of(pv, PCIDevice, config); From 8b9c3b897c682cd9739c6aef73b3220c7204c243 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 21 Nov 2013 11:19:58 +0200 Subject: [PATCH 3/6] acpi-build: fix build on glib < 2.22 g_string_vprintf was only introduced in 2.24 so switch to vsnprintf instead. A bit uglier but name size is fixed at 4 bytes here so it's easy. Reviewed-by: Paolo Bonzini Reported-by: Richard Henderson Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 486e7055a6..59a17dfbd3 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -287,16 +287,17 @@ static inline void build_append_array(GArray *array, GArray *val) static void build_append_nameseg(GArray *array, const char *format, ...) { - GString *s = g_string_new(""); + /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */ + char s[] = "XXXX"; + int len; va_list args; va_start(args, format); - g_string_vprintf(s, format, args); + len = vsnprintf(s, sizeof s, format, args); va_end(args); - assert(s->len == 4); - g_array_append_vals(array, s->str, s->len); - g_string_free(s, true); + assert(len == 4); + g_array_append_vals(array, s, len); } /* 5.4 Definition Block Encoding */ From fd8f5e37557596e14a859d8edf3dc24523bd4400 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 21 Nov 2013 11:22:51 +0200 Subject: [PATCH 4/6] acpi-build: fix build on glib < 2.14 g_array_get_element_size was only added in glib 2.14, there's no way to find element size in with an older glib. Fortunately we only use a single table (linker) where element size > 1. Switch element size to 1 everywhere, then we can just look at len field to get table size in bytes. Add an assert to make sure we catch any violations of this rule. Reviewed-by: Paolo Bonzini Reported-by: Richard Henderson Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 5 ++++- hw/i386/bios-linker-loader.c | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 59a17dfbd3..5f36e7ec02 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -425,7 +425,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size) static unsigned acpi_data_len(GArray *table) { - return table->len * g_array_get_element_size(table); +#if GLIB_CHECK_VERSION(2, 14, 0) + assert(g_array_get_element_size(table) == 1); +#endif + return table->len; } static void acpi_align_size(GArray *blob, unsigned align) diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c index 083385332e..fd23611008 100644 --- a/hw/i386/bios-linker-loader.c +++ b/hw/i386/bios-linker-loader.c @@ -90,7 +90,7 @@ enum { GArray *bios_linker_loader_init(void) { - return g_array_new(false, true /* clear */, sizeof(BiosLinkerLoaderEntry)); + return g_array_new(false, true /* clear */, 1); } /* Free linker wrapper and return the linker array. */ @@ -115,7 +115,7 @@ void bios_linker_loader_alloc(GArray *linker, BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH); /* Alloc entries must come first, so prepend them */ - g_array_prepend_val(linker, entry); + g_array_prepend_vals(linker, &entry, sizeof entry); } void bios_linker_loader_add_checksum(GArray *linker, const char *file, @@ -132,7 +132,7 @@ void bios_linker_loader_add_checksum(GArray *linker, const char *file, entry.cksum.start = cpu_to_le32((uint8_t *)start - (uint8_t *)table); entry.cksum.length = cpu_to_le32(size); - g_array_append_val(linker, entry); + g_array_append_vals(linker, &entry, sizeof entry); } void bios_linker_loader_add_pointer(GArray *linker, @@ -154,5 +154,5 @@ void bios_linker_loader_add_pointer(GArray *linker, assert(pointer_size == 1 || pointer_size == 2 || pointer_size == 4 || pointer_size == 8); - g_array_append_val(linker, entry); + g_array_append_vals(linker, &entry, sizeof entry); } From 90d131fb6504ed12a37dc8433375cc683c30e9da Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 18 Nov 2013 21:41:44 +0200 Subject: [PATCH 5/6] Revert "e1000/rtl8139: update HMP NIC when every bit is written" This reverts commit cd5be5829c1ce87aa6b3a7806524fac07ac9a757. Digging into hardware specs shows this does not actually make QEMU behave more like hardware: There are valid arguments backed by the spec to indicate why the version of e1000 prior to cd5be582 was more correct: the high byte actually includes a valid bit, this is why all guests write it last. For rtl8139 there's actually a separate undocumented valid bit, but we don't implement it yet. To summarize all the drivers we know about behave in one way that allows us to make an assumption about write order and avoid spurious, incorrect mac address updates to the monitor. Let's stick to the tried heuristic for 1.7 and possibly revisit for 1.8. Reported-by: Vlad Yasevich Reviewed-by: Vlad Yasevich Cc: Amos Kong Signed-off-by: Michael S. Tsirkin --- hw/net/e1000.c | 2 +- hw/net/rtl8139.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/hw/net/e1000.c b/hw/net/e1000.c index ae6359117d..8387443ee3 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -1106,7 +1106,7 @@ mac_writereg(E1000State *s, int index, uint32_t val) s->mac_reg[index] = val; - if (index == RA || index == RA + 1) { + if (index == RA + 1) { macaddr[0] = cpu_to_le32(s->mac_reg[RA]); macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]); qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr); diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 7f2b4db449..5329f44a9d 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -2741,7 +2741,10 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val) switch (addr) { - case MAC0 ... MAC0+5: + case MAC0 ... MAC0+4: + s->phys[addr - MAC0] = val; + break; + case MAC0+5: s->phys[addr - MAC0] = val; qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys); break; From e007dbece5fc4e55e10116c6cb42753e35a945bf Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Sun, 24 Nov 2013 11:38:05 +0200 Subject: [PATCH 6/6] configure: make --iasl option actually work --iasl option was added to CC option parsing section by mistake, it's not effective there and attempts to use cause an 'unknown option' error. Fix this up. Tested-by: Marcel Apfelbaum Signed-off-by: Michael S. Tsirkin --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 508f6a574c..9b9b9fa0b5 100755 --- a/configure +++ b/configure @@ -272,8 +272,6 @@ for opt do ;; --cxx=*) CXX="$optarg" ;; - --iasl=*) iasl="$optarg" - ;; --source-path=*) source_path="$optarg" ;; --cpu=*) cpu="$optarg" @@ -649,6 +647,8 @@ for opt do ;; --cxx=*) ;; + --iasl=*) iasl="$optarg" + ;; --objcc=*) objcc="$optarg" ;; --make=*) make="$optarg"