From a77690c41da67d85bab1e784a9f24f18bc63dbd9 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 17 Mar 2017 20:32:42 +0800 Subject: [PATCH 1/4] virtio: Fix error handling in virtio_bus_device_plugged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For one thing we shouldn't continue if an error happened, for the other two steps failing can cause an abort() in error_setg because we reuse the same errp blindly. Add error handling checks to fix both issues. Signed-off-by: Fam Zheng Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Reviewed-by: Andrew Jones Reviewed-by: Philippe Mathieu-Daudé --- hw/virtio/virtio-bus.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index a886011e75..3042232daf 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #include "hw/hw.h" #include "qemu/error-report.h" +#include "qapi/error.h" #include "hw/qdev.h" #include "hw/virtio/virtio-bus.h" #include "hw/virtio/virtio.h" @@ -48,20 +49,33 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); + Error *local_err = NULL; DPRINTF("%s: plug device.\n", qbus->name); if (klass->pre_plugged != NULL) { - klass->pre_plugged(qbus->parent, errp); + klass->pre_plugged(qbus->parent, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } } /* Get the features of the plugged device. */ assert(vdc->get_features != NULL); vdev->host_features = vdc->get_features(vdev, vdev->host_features, - errp); + &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } if (klass->device_plugged != NULL) { - klass->device_plugged(qbus->parent, errp); + klass->device_plugged(qbus->parent, &local_err); + } + if (local_err) { + error_propagate(errp, local_err); + return; } if (klass->get_dma_as != NULL && has_iommu) { From e49a6618400d11e51e30328dfe8d7cafce82d4bc Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 28 Feb 2017 14:21:32 +0100 Subject: [PATCH 2/4] virtio: always use handle_aio_output if registered Commit ad07cd6 ("virtio-scsi: always use dataplane path if ioeventfd is active", 2016-10-30) and 9ffe337 ("virtio-blk: always use dataplane path if ioeventfd is active", 2016-10-30) broke the virtio 1.0 indirect access registers. The indirect access registers bypass the ioeventfd, so that virtio-blk and virtio-scsi now repeatedly try to initialize dataplane instead of triggering the guest->host EventNotifier. Detect the situation by checking vq->handle_aio_output; if it is not NULL, trigger the EventNotifier, which is how the device expects to get notifications and in fact the only thread-safe manner to deliver them. Fixes: ad07cd6 Fixes: 9ffe337 Cc: qemu-stable@nongnu.org Signed-off-by: Paolo Bonzini Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Stefan Hajnoczi --- hw/virtio/virtio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 82b6060b2a..03592c542a 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1528,7 +1528,18 @@ static void virtio_queue_notify_vq(VirtQueue *vq) void virtio_queue_notify(VirtIODevice *vdev, int n) { - virtio_queue_notify_vq(&vdev->vq[n]); + VirtQueue *vq = &vdev->vq[n]; + + if (unlikely(!vq->vring.desc || vdev->broken)) { + return; + } + + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); + if (vq->handle_aio_output) { + event_notifier_set(&vq->host_notifier); + } else if (vq->handle_output) { + vq->handle_output(vdev, vq); + } } uint16_t virtio_queue_vector(VirtIODevice *vdev, int n) From f2a1ae45d8ec5ad494e66a9234499a2e0fbf4b40 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Mon, 20 Mar 2017 18:05:56 +0100 Subject: [PATCH 3/4] hw/acpi/vmgenid: prevent device realization on pre-2.5 machine types The WRITE_POINTER linker/loader command that underlies VMGENID depends on commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12), which in turn depends on fw_cfg DMA. DMA for fw_cfg is enabled in 2.5+ machine types only (see commit e6915b5f3a87, "fw_cfg: unbreak migration compatibility for 2.4 and earlier machines", 2016-02-18). Cc: "Michael S. Tsirkin" Cc: Ben Warren Cc: Igor Mammedov Cc: Paolo Bonzini Signed-off-by: Laszlo Ersek Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Ben Warren > Reviewed-by: Igor Mammedov --- hw/acpi/vmgenid.c | 14 ++++++++++++++ include/hw/acpi/vmgenid.h | 1 + include/hw/compat.h | 4 ++++ 3 files changed, 19 insertions(+) diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index 7a3ad17d66..c3ddcc8e7c 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque) memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)); } +static Property vmgenid_properties[] = { + DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, + write_pointer_available, true), + DEFINE_PROP_END_OF_LIST(), +}; + static void vmgenid_realize(DeviceState *dev, Error **errp) { VmGenIdState *vms = VMGENID(dev); + + if (!vms->write_pointer_available) { + error_setg(errp, "%s requires DMA write support in fw_cfg, " + "which this machine type does not provide", VMGENID_DEVICE); + return; + } + qemu_register_reset(vmgenid_handle_reset, vms); } @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_vmgenid; dc->realize = vmgenid_realize; dc->hotpluggable = false; + dc->props = vmgenid_properties; object_class_property_add_str(klass, VMGENID_GUID, NULL, vmgenid_set_guid, NULL); diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h index db7fa0e633..8578476bae 100644 --- a/include/hw/acpi/vmgenid.h +++ b/include/hw/acpi/vmgenid.h @@ -21,6 +21,7 @@ typedef struct VmGenIdState { DeviceClass parent_obj; QemuUUID guid; /* The 128-bit GUID seen by the guest */ uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ + bool write_pointer_available; } VmGenIdState; static inline Object *find_vmgenid_dev(void) diff --git a/include/hw/compat.h b/include/hw/compat.h index fc8c3e0600..5d5be91daf 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -131,6 +131,10 @@ .driver = "fw_cfg_io",\ .property = "dma_enabled",\ .value = "off",\ + },{\ + .driver = "vmgenid",\ + .property = "x-write-pointer-available",\ + .value = "off",\ }, #define HW_COMPAT_2_3 \ From f92063028a0ea9e15d8f962644bce76c0e8aa7d1 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Mon, 20 Mar 2017 18:05:57 +0100 Subject: [PATCH 4/4] hw/acpi/vmgenid: prevent more than one vmgenid device A system with multiple VMGENID devices is undefined in the VMGENID spec by omission. Cc: "Michael S. Tsirkin" Cc: Ben Warren Cc: Igor Mammedov Cc: Paolo Bonzini Signed-off-by: Laszlo Ersek Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Igor Mammedov --- hw/acpi/vmgenid.c | 8 ++++++++ include/hw/acpi/vmgenid.h | 1 + 2 files changed, 9 insertions(+) diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index c3ddcc8e7c..a32b847fe0 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -221,6 +221,14 @@ static void vmgenid_realize(DeviceState *dev, Error **errp) return; } + /* Given that this function is executing, there is at least one VMGENID + * device. Check if there are several. + */ + if (!find_vmgenid_dev()) { + error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE); + return; + } + qemu_register_reset(vmgenid_handle_reset, vms); } diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h index 8578476bae..7beb9592fb 100644 --- a/include/hw/acpi/vmgenid.h +++ b/include/hw/acpi/vmgenid.h @@ -24,6 +24,7 @@ typedef struct VmGenIdState { bool write_pointer_available; } VmGenIdState; +/* returns NULL unless there is exactly one device */ static inline Object *find_vmgenid_dev(void) { return object_resolve_path_type("", VMGENID_DEVICE, NULL);