From 532cee4184877053398a2bdae4edc965084fc79e Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Sat, 7 Mar 2015 10:14:30 +0100 Subject: [PATCH 01/11] iscsi: Fix check for username The variable user in struct iscsi_url is a character array, not a pointer. Therefore its address will never be NULL. clang reports this error: block/iscsi.c:1329:20: warning: comparison of array 'iscsi_url->user' not equal to a null pointer is always true [-Wtautological-pointer-compare] Reviewed-by: Peter Lieven Acked-by: Peter Lieven Signed-off-by: Stefan Weil Message-Id: <1425719670-5486-1-git-send-email-sw@weilnetz.de> Signed-off-by: Paolo Bonzini --- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/iscsi.c b/block/iscsi.c index 1fa855acdd..3e34b1f3a2 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1326,7 +1326,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, goto out; } - if (iscsi_url->user != NULL) { + if (iscsi_url->user[0] != '\0') { ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user, iscsi_url->passwd); if (ret != 0) { From 9a7dcb711bdaf4082bf333dbecfeb729bbff1f8e Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 2 Mar 2015 17:29:06 -0600 Subject: [PATCH 02/11] kvm_stat: add column headers to text UI The curses user interface shows both the accumulated total and the current event counts. Add column headers so it's clear what the numbers mean. Signed-off-by: Stefan Hajnoczi Reviewed-by: Ademar Reis Reviewed-by: Wei Huang Message-Id: <1425338947-10296-2-git-send-email-stefanha@redhat.com> Signed-off-by: Paolo Bonzini --- scripts/kvm/kvm_stat | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/kvm/kvm_stat b/scripts/kvm/kvm_stat index c65cabda5a..7e5d25612b 100755 --- a/scripts/kvm/kvm_stat +++ b/scripts/kvm/kvm_stat @@ -519,7 +519,10 @@ def tui(screen, stats): def refresh(sleeptime): screen.erase() screen.addstr(0, 0, 'kvm statistics') - row = 2 + screen.addstr(2, 1, 'Event') + screen.addstr(2, 1 + label_width + number_width - len('Total'), 'Total') + screen.addstr(2, 1 + label_width + number_width + 8 - len('Current'), 'Current') + row = 3 s = stats.get() def sortkey(x): if s[x][1]: From 811c5727765eba00824c29a696350d4780d86c19 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 2 Mar 2015 17:29:07 -0600 Subject: [PATCH 03/11] kvm_stat: add kvm_stat.1 man page Signed-off-by: Stefan Hajnoczi Reviewed-by: Ademar Reis Reviewed-by: Wei Huang Message-Id: <1425338947-10296-3-git-send-email-stefanha@redhat.com> Signed-off-by: Paolo Bonzini --- Makefile | 9 +++++++ scripts/kvm/kvm_stat.texi | 55 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 scripts/kvm/kvm_stat.texi diff --git a/Makefile b/Makefile index d92d4cd573..884b59dc06 100644 --- a/Makefile +++ b/Makefile @@ -84,6 +84,9 @@ HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF) ifdef BUILD_DOCS DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 qmp-commands.txt +ifdef CONFIG_LINUX +DOCS+=kvm_stat.1 +endif ifdef CONFIG_VIRTFS DOCS+=fsdev/virtfs-proxy-helper.1 endif @@ -490,6 +493,12 @@ qemu-nbd.8: qemu-nbd.texi $(POD2MAN) --section=8 --center=" " --release=" " qemu-nbd.pod > $@, \ " GEN $@") +kvm_stat.1: scripts/kvm/kvm_stat.texi + $(call quiet-command, \ + perl -Ww -- $(SRC_PATH)/scripts/texi2pod.pl $< kvm_stat.pod && \ + $(POD2MAN) --section=1 --center=" " --release=" " kvm_stat.pod > $@, \ + " GEN $@") + dvi: qemu-doc.dvi qemu-tech.dvi html: qemu-doc.html qemu-tech.html info: qemu-doc.info qemu-tech.info diff --git a/scripts/kvm/kvm_stat.texi b/scripts/kvm/kvm_stat.texi new file mode 100644 index 0000000000..6ce00d80e7 --- /dev/null +++ b/scripts/kvm/kvm_stat.texi @@ -0,0 +1,55 @@ +@example +@c man begin SYNOPSIS +usage: kvm_stat [OPTION]... +@c man end +@end example + +@c man begin DESCRIPTION + +kvm_stat prints counts of KVM kernel module trace events. These events signify +state transitions such as guest mode entry and exit. + +This tool is useful for observing guest behavior from the host perspective. +Often conclusions about performance or buggy behavior can be drawn from the +output. + +The set of KVM kernel module trace events may be specific to the kernel version +or architecture. It is best to check the KVM kernel module source code for the +meaning of events. + +Note that trace events are counted globally across all running guests. + +@c man end + +@c man begin OPTIONS +@table @option +@item -1, --once, --batch + run in batch mode for one second +@item -l, --log + run in logging mode (like vmstat) +@item -t, --tracepoints + retrieve statistics from tracepoints +@item -d, --debugfs + retrieve statistics from debugfs +@item -f, --fields=@var{fields} + fields to display (regex) +@item -h, --help + show help message +@end table + +@c man end + +@ignore + +@setfilename kvm_stat +@settitle Report KVM kernel module event counters. + +@c man begin AUTHOR +Stefan Hajnoczi +@c man end + +@c man begin SEEALSO +perf(1), trace-cmd(1) +@c man end + +@end ignore From 24fa90499f8b24bcba2960a3316d797f9b80b5e9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 5 Mar 2015 16:47:14 +0100 Subject: [PATCH 04/11] qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork. The way to safely do fork is to bring all threads to a quiescent state by acquiring locks (either in callers---as we do for the iothread mutex---or using pthread_atfork's prepare callbacks) and then release them in the child. The problem is that releasing error-checking locks in the child fails under glibc with EPERM, because the mutex stores a different owner tid than the duplicated thread in the child process. We could make it work for locks acquired via pthread_atfork, by recreating the mutex in the child instead of unlocking it (we know that there are no other threads that could have taken the mutex; but when the lock is acquired in fork's caller that would not be possible. The simplest solution is just to forgo error checking. Signed-off-by: Paolo Bonzini --- util/qemu-thread-posix.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index 50a29d8f7a..ba67cec62b 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -51,12 +51,8 @@ static void error_exit(int err, const char *msg) void qemu_mutex_init(QemuMutex *mutex) { int err; - pthread_mutexattr_t mutexattr; - pthread_mutexattr_init(&mutexattr); - pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK); - err = pthread_mutex_init(&mutex->lock, &mutexattr); - pthread_mutexattr_destroy(&mutexattr); + err = pthread_mutex_init(&mutex->lock, NULL); if (err) error_exit(err, __func__); } From 21b7cf9e07e5991c57b461181cfb5bbb6fe7a9d6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 5 Mar 2015 16:53:48 +0100 Subject: [PATCH 05/11] rcu: handle forks safely After forking, only the calling thread is duplicated in the child process. The call_rcu thread has to be recreated in the child. Exploit the fact that only one thread exists (same as when constructors run), and just redo the entire initialization to ensure the threads are in the proper state. The only additional things to do are emptying the list of threads registered with RCU, and unlocking the lock that was taken in the prepare callback (implementations are allowed to fail pthread_mutex_init() if the mutex is still locked). Signed-off-by: Paolo Bonzini --- util/rcu.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/util/rcu.c b/util/rcu.c index bd73b8eb47..27802a4bed 100644 --- a/util/rcu.c +++ b/util/rcu.c @@ -283,7 +283,7 @@ void rcu_unregister_thread(void) qemu_mutex_unlock(&rcu_gp_lock); } -static void __attribute__((__constructor__)) rcu_init(void) +static void rcu_init_complete(void) { QemuThread thread; @@ -291,8 +291,39 @@ static void __attribute__((__constructor__)) rcu_init(void) qemu_event_init(&rcu_gp_event, true); qemu_event_init(&rcu_call_ready_event, false); + + /* The caller is assumed to have iothread lock, so the call_rcu thread + * must have been quiescent even after forking, just recreate it. + */ qemu_thread_create(&thread, "call_rcu", call_rcu_thread, NULL, QEMU_THREAD_DETACHED); rcu_register_thread(); } + +#ifdef CONFIG_POSIX +static void rcu_init_lock(void) +{ + qemu_mutex_lock(&rcu_gp_lock); +} + +static void rcu_init_unlock(void) +{ + qemu_mutex_unlock(&rcu_gp_lock); +} + +static void rcu_init_child(void) +{ + qemu_mutex_unlock(&rcu_gp_lock); + memset(®istry, 0, sizeof(registry)); + rcu_init_complete(); +} +#endif + +static void __attribute__((__constructor__)) rcu_init(void) +{ +#ifdef CONFIG_POSIX + pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child); +#endif + rcu_init_complete(); +} From cba7054928b10a7fda57c64807451bbc9a31e42e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 9 Mar 2015 15:28:37 +0100 Subject: [PATCH 06/11] cpus: initialize cpu->memory_dispatch This fixes a NULL pointer dereference in s390x-softmmu. On pretty much all other architectures, creating an MMIO region calls cpu_reload_memory_map. On s390, however, there are no MMIO regions and everything is done via hypercalls. Fixes: 9d82b5a792236db31a75b9db5c93af69ac07c7c5 Reported-by: Christian Borntraeger Signed-off-by: Paolo Bonzini --- exec.c | 1 + 1 file changed, 1 insertion(+) diff --git a/exec.c b/exec.c index 6a5adab502..a6cb4a241c 100644 --- a/exec.c +++ b/exec.c @@ -548,6 +548,7 @@ void cpu_exec_init(CPUArchState *env) #ifndef CONFIG_USER_ONLY cpu->as = &address_space_memory; cpu->thread_id = qemu_get_thread_id(); + cpu_reload_memory_map(cpu); #endif QTAILQ_INSERT_TAIL(&cpus, cpu, node); #if defined(CONFIG_USER_ONLY) From fa617181839741727d0067ea68807133f498f29b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Mar 2015 19:17:25 +0100 Subject: [PATCH 07/11] scsi: Clean up duplicated error in legacy if=scsi code Commit a818a4b changed scsi_bus_legacy_handle_cmdline() to report errors from scsi_bus_legacy_add_drive() with error_report() in addition to returning them. That's inappropriate. Two kinds of callers: 1. realize methods (devices "esp", "virtio-scsi-device" and "spapr-vscsi") The error object gets passed up the call chain until it gets reported again and freed. Example: $ qemu-system-arm -M virt -S -display none \ > -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 \ > -device nec-usb-xhci -device usb-storage,drive=foo \ > -device virtio-scsi-pci qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed qemu-system-arm: -device virtio-scsi-pci: Device initialization failed qemu-system-arm: -device virtio-scsi-pci: Device 'virtio-scsi-pci' could not be initialized The second message in this error cascade comes from scsi_bus_legacy_handle_cmdline(). The error object then gets passed up to the qdev_init() called from virtio_scsi_pci_init_pci(), which reports it again. 2. init methods (devices "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas", "megasas-gen2") init methods need to report their errors with qerror_report(). These don't. The inappropriate error_report() papers over the bug. error_report() isn't the same as qerror_report() in QMP context, but this can't actually happen: QMP can still only hot-plug, and callers call scsi_bus_legacy_handle_cmdline() only on cold-plug. Except for sysbus_esp_realize(), but that can't be hot-plugged at all, as far as I can tell. Fix the init methods and drop the inappropriate error_report() in scsi_bus_legacy_handle_cmdline(). Signed-off-by: Markus Armbruster Reviewed-by: Peter Crosthwaite Message-Id: <1425925048-15482-2-git-send-email-armbru@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/esp-pci.c | 2 ++ hw/scsi/lsi53c895a.c | 3 ++- hw/scsi/megasas.c | 2 ++ hw/scsi/scsi-bus.c | 1 - 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index 00b7297354..a75fcfab41 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -28,6 +28,7 @@ #include "hw/scsi/esp.h" #include "trace.h" #include "qemu/log.h" +#include "qapi/qmp/qerror.h" #define TYPE_AM53C974_DEVICE "am53c974" @@ -369,6 +370,7 @@ static int esp_pci_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index db7d4b8c9c..4ffab038f0 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -19,7 +19,7 @@ #include "hw/pci/pci.h" #include "hw/scsi/scsi.h" #include "sysemu/dma.h" -#include "qemu/error-report.h" +#include "qapi/qmp/qerror.h" //#define DEBUG_LSI //#define DEBUG_LSI_REG @@ -2119,6 +2119,7 @@ static int lsi_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 4852237a79..69ffdbd724 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -28,6 +28,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" +#include "qapi/qmp/qerror.h" #include "mfi.h" @@ -2409,6 +2410,7 @@ static int megasas_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index dca9576828..f8de56996a 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -273,7 +273,6 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp) scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo), unit, false, -1, NULL, &err); if (err != NULL) { - error_report("%s", error_get_pretty(err)); error_propagate(errp, err); break; } From 9b3d111ad90886546614b2579eedcb4675b35d14 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Mar 2015 19:17:26 +0100 Subject: [PATCH 08/11] hw: Propagate errors through qdev_prop_set_drive() Three kinds of callers: 1. On failure, report the error and abort Passing &error_abort does the job. No functional change. 2. On failure, report the error and exit() This is qdev_prop_set_drive_nofail(). Error reporting moves from qdev_prop_set_drive() to its caller. Because hiding away the error in the monitor right before exit() isn't helpful, replace qerror_report_err() by error_report_err(). Shouldn't make a difference, because qdev_prop_set_drive_nofail() should never be used in QMP context. 3. On failure, report the error and recover This is usb_msd_init() and scsi_bus_legacy_add_drive(). Error reporting and freeing the error object moves from qdev_prop_set_drive() to its callers. Because usb_msd_init() can't run in QMP context, replace qerror_report_err() by error_report_err() there. No functional change. scsi_bus_legacy_add_drive() calling qerror_report_err() is of course inappropriate, but this commit merely makes it more obvious. The next one will clean it up. Signed-off-by: Markus Armbruster Reviewed-by: Peter Crosthwaite Message-Id: <1425925048-15482-3-git-send-email-armbru@redhat.com> Signed-off-by: Paolo Bonzini --- hw/arm/vexpress.c | 6 +++--- hw/arm/virt.c | 6 +++--- hw/block/pflash_cfi01.c | 4 ++-- hw/block/pflash_cfi02.c | 4 ++-- hw/core/qdev-properties-system.c | 22 ++++++++++------------ hw/scsi/scsi-bus.c | 6 +++++- hw/usb/dev-storage.c | 7 +++++-- include/hw/qdev-properties.h | 4 ++-- 8 files changed, 32 insertions(+), 27 deletions(-) diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index 5933454cfd..8496c1622a 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -515,9 +515,9 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name, { DeviceState *dev = qdev_create(NULL, "cfi.pflash01"); - if (di && qdev_prop_set_drive(dev, "drive", - blk_by_legacy_dinfo(di))) { - abort(); + if (di) { + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di), + &error_abort); } qdev_prop_set_uint32(dev, "num-blocks", diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 69f51ac0da..93b7605722 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -522,9 +522,9 @@ static void create_one_flash(const char *name, hwaddr flashbase, DeviceState *dev = qdev_create(NULL, "cfi.pflash01"); const uint64_t sectorlength = 256 * 1024; - if (dinfo && qdev_prop_set_drive(dev, "drive", - blk_by_legacy_dinfo(dinfo))) { - abort(); + if (dinfo) { + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo), + &error_abort); } qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength); diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 89d380e59d..d282695086 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -969,8 +969,8 @@ pflash_t *pflash_cfi01_register(hwaddr base, { DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01); - if (blk && qdev_prop_set_drive(dev, "drive", blk)) { - abort(); + if (blk) { + qdev_prop_set_drive(dev, "drive", blk, &error_abort); } qdev_prop_set_uint32(dev, "num-blocks", nb_blocs); qdev_prop_set_uint64(dev, "sector-length", sector_len); diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c index 389b4aa1f4..074a005f69 100644 --- a/hw/block/pflash_cfi02.c +++ b/hw/block/pflash_cfi02.c @@ -773,8 +773,8 @@ pflash_t *pflash_cfi02_register(hwaddr base, { DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH02); - if (blk && qdev_prop_set_drive(dev, "drive", blk)) { - abort(); + if (blk) { + qdev_prop_set_drive(dev, "drive", blk, &error_abort); } qdev_prop_set_uint32(dev, "num-blocks", nb_blocs); qdev_prop_set_uint32(dev, "sector-length", sector_len); diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index a2e44bd4e8..c413226a97 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -341,27 +341,25 @@ PropertyInfo qdev_prop_vlan = { .set = set_vlan, }; -int qdev_prop_set_drive(DeviceState *dev, const char *name, - BlockBackend *value) +void qdev_prop_set_drive(DeviceState *dev, const char *name, + BlockBackend *value, Error **errp) { - Error *err = NULL; - object_property_set_str(OBJECT(dev), - value ? blk_name(value) : "", name, &err); - if (err) { - qerror_report_err(err); - error_free(err); - return -1; - } - return 0; + object_property_set_str(OBJECT(dev), value ? blk_name(value) : "", + name, errp); } void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockBackend *value) { - if (qdev_prop_set_drive(dev, name, value) < 0) { + Error *err = NULL; + + qdev_prop_set_drive(dev, name, value, &err); + if (err) { + error_report_err(err); exit(1); } } + void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value) { diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index f8de56996a..61c595fbfa 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -7,6 +7,7 @@ #include "sysemu/blockdev.h" #include "trace.h" #include "sysemu/dma.h" +#include "qapi/qmp/qerror.h" static char *scsibus_get_dev_path(DeviceState *dev); static char *scsibus_get_fw_dev_path(DeviceState *dev); @@ -242,7 +243,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, if (serial && object_property_find(OBJECT(dev), "serial", NULL)) { qdev_prop_set_string(dev, "serial", serial); } - if (qdev_prop_set_drive(dev, "drive", blk) < 0) { + qdev_prop_set_drive(dev, "drive", blk, &err); + if (err) { + qerror_report_err(err); + error_free(err); error_setg(errp, "Setting drive property failed"); object_unparent(OBJECT(dev)); return NULL; diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index 65d9aa6147..3f5b32d666 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -663,6 +663,7 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp) static USBDevice *usb_msd_init(USBBus *bus, const char *filename) { static int nr=0; + Error *err = NULL; char id[8]; QemuOpts *opts; DriveInfo *dinfo; @@ -706,8 +707,10 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename) /* create guest device */ dev = usb_create(bus, "usb-storage"); - if (qdev_prop_set_drive(&dev->qdev, "drive", - blk_by_legacy_dinfo(dinfo)) < 0) { + qdev_prop_set_drive(&dev->qdev, "drive", blk_by_legacy_dinfo(dinfo), + &err); + if (err) { + error_report_err(err); object_unparent(OBJECT(dev)); return NULL; } diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 57ee363b89..2d47c0c640 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -168,8 +168,8 @@ void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value); void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value); void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value); void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value); -int qdev_prop_set_drive(DeviceState *dev, const char *name, - BlockBackend *value) QEMU_WARN_UNUSED_RESULT; +void qdev_prop_set_drive(DeviceState *dev, const char *name, + BlockBackend *value, Error **errp); void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockBackend *value); void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); From 390e90a90736f98ca47f2e767d7f2a15d68d6bc4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Mar 2015 19:17:27 +0100 Subject: [PATCH 09/11] scsi: Improve error reporting for invalid drive property When setting "realized" fails, scsi_bus_legacy_add_drive() passes the error to qerror_report_err(), then returns an unspecific "Setting drive property failed" error, which is reported further up the call chain. Example: $ qemu-system-x86_64 -nodefaults -S -display none \ > -drive if=scsi,id=foo,file=tmp.qcow2 -global isa-fdc.driveA=foo qemu-system-x86_64: -drive if=scsi,id=foo,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use qemu-system-x86_64: Setting drive property failed qemu-system-x86_64: Initialization of device lsi53c895a failed: Device initialization failed Clean up the obvious way: simply return the original error to the caller. Gets rid of the second message in the above error cascade. Signed-off-by: Markus Armbruster Reviewed-by: Peter Crosthwaite Message-Id: <1425925048-15482-4-git-send-email-armbru@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 61c595fbfa..bd2c0e4caa 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -7,7 +7,6 @@ #include "sysemu/blockdev.h" #include "trace.h" #include "sysemu/dma.h" -#include "qapi/qmp/qerror.h" static char *scsibus_get_dev_path(DeviceState *dev); static char *scsibus_get_fw_dev_path(DeviceState *dev); @@ -245,9 +244,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, } qdev_prop_set_drive(dev, "drive", blk, &err); if (err) { - qerror_report_err(err); - error_free(err); - error_setg(errp, "Setting drive property failed"); + error_propagate(errp, err); object_unparent(OBJECT(dev)); return NULL; } From ae071cc851d7150d3a9950c642570830bb85729e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Mar 2015 19:17:28 +0100 Subject: [PATCH 10/11] scsi: Convert remaining PCI HBAs to realize() These are "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas", "megasas-gen2". Signed-off-by: Markus Armbruster Reviewed-by: Peter Crosthwaite Message-Id: <1425925048-15482-5-git-send-email-armbru@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/esp-pci.c | 30 +++++++++++------------------- hw/scsi/lsi53c895a.c | 14 +++----------- hw/scsi/megasas.c | 14 +++----------- 3 files changed, 17 insertions(+), 41 deletions(-) diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index a75fcfab41..8d2242d0a4 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -28,7 +28,6 @@ #include "hw/scsi/esp.h" #include "trace.h" #include "qemu/log.h" -#include "qapi/qmp/qerror.h" #define TYPE_AM53C974_DEVICE "am53c974" @@ -343,13 +342,12 @@ static const struct SCSIBusInfo esp_pci_scsi_info = { .cancel = esp_request_cancelled, }; -static int esp_pci_scsi_init(PCIDevice *dev) +static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp) { PCIESPState *pci = PCI_ESP(dev); DeviceState *d = DEVICE(dev); ESPState *s = &pci->esp; uint8_t *pci_conf; - Error *err = NULL; pci_conf = dev->config; @@ -368,14 +366,8 @@ static int esp_pci_scsi_init(PCIDevice *dev) scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL); if (!d->hotplugged) { - scsi_bus_legacy_handle_cmdline(&s->bus, &err); - if (err != NULL) { - qerror_report_err(err); - error_free(err); - return -1; - } + scsi_bus_legacy_handle_cmdline(&s->bus, errp); } - return 0; } static void esp_pci_scsi_uninit(PCIDevice *d) @@ -390,7 +382,7 @@ static void esp_pci_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - k->init = esp_pci_scsi_init; + k->realize = esp_pci_scsi_realize; k->exit = esp_pci_scsi_uninit; k->vendor_id = PCI_VENDOR_ID_AMD; k->device_id = PCI_DEVICE_ID_AMD_SCSI; @@ -468,17 +460,19 @@ static void dc390_write_config(PCIDevice *dev, } } -static int dc390_scsi_init(PCIDevice *dev) +static void dc390_scsi_realize(PCIDevice *dev, Error **errp) { DC390State *pci = DC390(dev); + Error *err = NULL; uint8_t *contents; uint16_t chksum = 0; - int i, ret; + int i; /* init base class */ - ret = esp_pci_scsi_init(dev); - if (ret < 0) { - return ret; + esp_pci_scsi_realize(dev, &err); + if (err) { + error_propagate(errp, err); + return; } /* EEPROM */ @@ -505,8 +499,6 @@ static int dc390_scsi_init(PCIDevice *dev) chksum = 0x1234 - chksum; contents[EE_CHKSUM1] = chksum & 0xff; contents[EE_CHKSUM2] = chksum >> 8; - - return 0; } static void dc390_class_init(ObjectClass *klass, void *data) @@ -514,7 +506,7 @@ static void dc390_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - k->init = dc390_scsi_init; + k->realize = dc390_scsi_realize; k->config_read = dc390_read_config; k->config_write = dc390_write_config; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index 4ffab038f0..c5b0cc5caf 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -19,7 +19,6 @@ #include "hw/pci/pci.h" #include "hw/scsi/scsi.h" #include "sysemu/dma.h" -#include "qapi/qmp/qerror.h" //#define DEBUG_LSI //#define DEBUG_LSI_REG @@ -2089,12 +2088,11 @@ static const struct SCSIBusInfo lsi_scsi_info = { .cancel = lsi_request_cancelled }; -static int lsi_scsi_init(PCIDevice *dev) +static void lsi_scsi_realize(PCIDevice *dev, Error **errp) { LSIState *s = LSI53C895A(dev); DeviceState *d = DEVICE(dev); uint8_t *pci_conf; - Error *err = NULL; pci_conf = dev->config; @@ -2117,14 +2115,8 @@ static int lsi_scsi_init(PCIDevice *dev) scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL); if (!d->hotplugged) { - scsi_bus_legacy_handle_cmdline(&s->bus, &err); - if (err != NULL) { - qerror_report_err(err); - error_free(err); - return -1; - } + scsi_bus_legacy_handle_cmdline(&s->bus, errp); } - return 0; } static void lsi_class_init(ObjectClass *klass, void *data) @@ -2132,7 +2124,7 @@ static void lsi_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - k->init = lsi_scsi_init; + k->realize = lsi_scsi_realize; k->vendor_id = PCI_VENDOR_ID_LSI_LOGIC; k->device_id = PCI_DEVICE_ID_LSI_53C895A; k->class_id = PCI_CLASS_STORAGE_SCSI; diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 69ffdbd724..bf83b65383 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -28,7 +28,6 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" -#include "qapi/qmp/qerror.h" #include "mfi.h" @@ -2321,14 +2320,13 @@ static const struct SCSIBusInfo megasas_scsi_info = { .cancel = megasas_command_cancel, }; -static int megasas_scsi_init(PCIDevice *dev) +static void megasas_scsi_realize(PCIDevice *dev, Error **errp) { DeviceState *d = DEVICE(dev); MegasasState *s = MEGASAS(dev); MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s); uint8_t *pci_conf; int i, bar_type; - Error *err = NULL; pci_conf = dev->config; @@ -2408,14 +2406,8 @@ static int megasas_scsi_init(PCIDevice *dev) scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev), &megasas_scsi_info, NULL); if (!d->hotplugged) { - scsi_bus_legacy_handle_cmdline(&s->bus, &err); - if (err != NULL) { - qerror_report_err(err); - error_free(err); - return -1; - } + scsi_bus_legacy_handle_cmdline(&s->bus, errp); } - return 0; } static void @@ -2509,7 +2501,7 @@ static void megasas_class_init(ObjectClass *oc, void *data) MegasasBaseClass *e = MEGASAS_DEVICE_CLASS(oc); const MegasasInfo *info = data; - pc->init = megasas_scsi_init; + pc->realize = megasas_scsi_realize; pc->exit = megasas_scsi_uninit; pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC; pc->device_id = info->device_id; From ac57622985220de064059971f9ccb00905e9bd04 Mon Sep 17 00:00:00 2001 From: Bill Paul Date: Mon, 9 Mar 2015 15:48:01 -0700 Subject: [PATCH 11/11] x86: fix SS selector in SYSRET According to my reading of the Intel documentation, the SYSRET instruction is supposed to force the RPL bits of the %ss register to 3 when returning to user mode. The actual sequence is: SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) However, the code in helper_sysret() leaves them at 0 (in other words, the "OR 3" part of the above sequence is missing). It does set the privilege level bits of %cs correctly though. This has caused me trouble with some of my VxWorks development: code that runs okay on real hardware will crash on QEMU, unless I apply the patch below. Signed-off-by: Bill Paul Message-Id: <201503091548.01462.wpaul@windriver.com> Signed-off-by: Paolo Bonzini --- target-i386/seg_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index fa374d0c0b..2bc757af31 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env->eip = (uint32_t)env->regs[R_ECX]; } - cpu_x86_load_seg_cache(env, R_SS, selector + 8, + cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0xffffffff, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 << DESC_DPL_SHIFT) | @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_S_MASK | (3 << DESC_DPL_SHIFT) | DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env->eip = (uint32_t)env->regs[R_ECX]; - cpu_x86_load_seg_cache(env, R_SS, selector + 8, + cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0xffffffff, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 << DESC_DPL_SHIFT) |