* [Qemu-devel] [PATCH] [STABLE] Fix virtio-blk hot add after remove
@ 2009-10-14 16:59 Anthony Liguori
2009-10-14 18:22 ` [Qemu-devel] " Dustin Kirkland
2009-10-15 8:00 ` Gerd Hoffmann
0 siblings, 2 replies; 3+ messages in thread
From: Anthony Liguori @ 2009-10-14 16:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Anthony Liguori, Gerd Hoffman, Dustin Kirkland
qdev_init_bdrv() expects that each drive added is the next logical unit for
the given interface type. However, when dealing with hotplug, there may
be holes in the units. drive_init reclaims holes in units but qdev_init_bdrv()
is not smart enough to do this.
Fortunately, in master, this has all been rewritten so for stable, let's hack
around this a bit. To fix this, we need to tell qdev that a device has been
removed so that it can keep track of which units are allocated. The only way
we can get a hook for this though is to attach to the pci uninit callback. In
order for virtio-blk to get this, another uninit callback needs to be added to
the virtio layer.
Suggestions for a less ugly solution are appreciated.
This fixes https://bugs.launchpad.net/bugs/419590
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/qdev.c | 33 ++++++++++++++++++++++++++++++---
hw/virtio-blk.c | 8 ++++++++
hw/virtio-pci.c | 19 ++++++++++++++++++-
hw/virtio.h | 1 +
sysemu.h | 2 ++
5 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index faecc76..af60e3e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -181,21 +181,48 @@ void qdev_get_macaddr(DeviceState *dev, uint8_t *macaddr)
memcpy(macaddr, dev->nd->macaddr, 6);
}
-static int next_block_unit[IF_COUNT];
+typedef struct BlockUnitState
+{
+ BlockDriverState *bs[32];
+} BlockUnitState;
+static BlockUnitState next_block_unit[IF_COUNT];
/* Get a block device. This should only be used for single-drive devices
(e.g. SD/Floppy/MTD). Multi-disk devices (scsi/ide) should use the
appropriate bus. */
BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type)
{
- int unit = next_block_unit[type]++;
+ BlockDriverState *bs;
+ int unit;
int index;
+ unit = 0;
+ while (next_block_unit[type].bs[unit]) {
+ unit++;
+ }
+
index = drive_get_index(type, 0, unit);
if (index == -1) {
return NULL;
}
- return drives_table[index].bdrv;
+
+ bs = drives_table[index].bdrv;
+ next_block_unit[type].bs[unit] = bs;
+
+ return bs;
+}
+
+void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type)
+{
+ int unit;
+
+ for (unit = 0; unit < 32; unit++) {
+ if (next_block_unit[type].bs[unit] == bs) {
+ break;
+ }
+ }
+
+ next_block_unit[type].bs[unit] = NULL;
}
BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 5036b5b..4bc11e6 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -414,6 +414,13 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
return 0;
}
+static void virtio_blk_uninit(VirtIODevice *vdev)
+{
+ VirtIOBlock *s = to_virtio_blk(vdev);
+
+ qdev_uninit_bdrv(s->bs, IF_VIRTIO);
+}
+
VirtIODevice *virtio_blk_init(DeviceState *dev)
{
VirtIOBlock *s;
@@ -430,6 +437,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev)
s->vdev.get_config = virtio_blk_update_config;
s->vdev.get_features = virtio_blk_get_features;
s->vdev.reset = virtio_blk_reset;
+ s->vdev.uninit = virtio_blk_uninit;
s->bs = bs;
s->rq = NULL;
if (strlen(ps = (char *)drive_get_serial(bs)))
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 703f4fe..00b3998 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -375,6 +375,22 @@ static const VirtIOBindings virtio_pci_bindings = {
.load_queue = virtio_pci_load_queue,
};
+static int virtio_pci_uninit(PCIDevice *pci_dev)
+{
+ VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev);
+ VirtIODevice *vdev = proxy->vdev;
+
+ if (vdev->uninit) {
+ vdev->uninit(vdev);
+ }
+
+ if (vdev->nvectors) {
+ return msix_uninit(pci_dev);
+ }
+
+ return 0;
+}
+
static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
uint16_t vendor, uint16_t device,
uint16_t class_code, uint8_t pif)
@@ -407,10 +423,11 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
PCI_ADDRESS_SPACE_MEM,
msix_mmio_map);
proxy->pci_dev.config_write = virtio_write_config;
- proxy->pci_dev.unregister = msix_uninit;
} else
vdev->nvectors = 0;
+ proxy->pci_dev.unregister = virtio_pci_uninit;
+
size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
if (size & (size-1))
size = 1 << qemu_fls(size);
diff --git a/hw/virtio.h b/hw/virtio.h
index aa55677..330b317 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -103,6 +103,7 @@ struct VirtIODevice
void (*get_config)(VirtIODevice *vdev, uint8_t *config);
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
+ void (*uninit)(VirtIODevice *vdev);
VirtQueue *vq;
const VirtIOBindings *binding;
void *binding_opaque;
diff --git a/sysemu.h b/sysemu.h
index ce25109..18f610b 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -193,6 +193,8 @@ extern const char *drive_get_serial(BlockDriverState *bdrv);
extern BlockInterfaceErrorAction drive_get_onerror(BlockDriverState *bdrv);
BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type);
+void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type);
+
struct drive_opt {
const char *file;
--
1.6.2.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] [STABLE] Fix virtio-blk hot add after remove
2009-10-14 16:59 [Qemu-devel] [PATCH] [STABLE] Fix virtio-blk hot add after remove Anthony Liguori
@ 2009-10-14 18:22 ` Dustin Kirkland
2009-10-15 8:00 ` Gerd Hoffmann
1 sibling, 0 replies; 3+ messages in thread
From: Dustin Kirkland @ 2009-10-14 18:22 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, Gerd Hoffman
[-- Attachment #1: Type: text/plain, Size: 6315 bytes --]
On Wed, 2009-10-14 at 11:59 -0500, Anthony Liguori wrote:
> qdev_init_bdrv() expects that each drive added is the next logical unit for
> the given interface type. However, when dealing with hotplug, there may
> be holes in the units. drive_init reclaims holes in units but qdev_init_bdrv()
> is not smart enough to do this.
>
> Fortunately, in master, this has all been rewritten so for stable, let's hack
> around this a bit. To fix this, we need to tell qdev that a device has been
> removed so that it can keep track of which units are allocated. The only way
> we can get a hook for this though is to attach to the pci uninit callback. In
> order for virtio-blk to get this, another uninit callback needs to be added to
> the virtio layer.
>
> Suggestions for a less ugly solution are appreciated.
>
> This fixes https://bugs.launchpad.net/bugs/419590
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> hw/qdev.c | 33 ++++++++++++++++++++++++++++++---
> hw/virtio-blk.c | 8 ++++++++
> hw/virtio-pci.c | 19 ++++++++++++++++++-
> hw/virtio.h | 1 +
> sysemu.h | 2 ++
> 5 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index faecc76..af60e3e 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -181,21 +181,48 @@ void qdev_get_macaddr(DeviceState *dev, uint8_t *macaddr)
> memcpy(macaddr, dev->nd->macaddr, 6);
> }
>
> -static int next_block_unit[IF_COUNT];
> +typedef struct BlockUnitState
> +{
> + BlockDriverState *bs[32];
> +} BlockUnitState;
> +static BlockUnitState next_block_unit[IF_COUNT];
>
> /* Get a block device. This should only be used for single-drive devices
> (e.g. SD/Floppy/MTD). Multi-disk devices (scsi/ide) should use the
> appropriate bus. */
> BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type)
> {
> - int unit = next_block_unit[type]++;
> + BlockDriverState *bs;
> + int unit;
> int index;
>
> + unit = 0;
> + while (next_block_unit[type].bs[unit]) {
> + unit++;
> + }
> +
> index = drive_get_index(type, 0, unit);
> if (index == -1) {
> return NULL;
> }
> - return drives_table[index].bdrv;
> +
> + bs = drives_table[index].bdrv;
> + next_block_unit[type].bs[unit] = bs;
> +
> + return bs;
> +}
> +
> +void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type)
> +{
> + int unit;
> +
> + for (unit = 0; unit < 32; unit++) {
> + if (next_block_unit[type].bs[unit] == bs) {
> + break;
> + }
> + }
> +
> + next_block_unit[type].bs[unit] = NULL;
> }
>
> BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 5036b5b..4bc11e6 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -414,6 +414,13 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
> return 0;
> }
>
> +static void virtio_blk_uninit(VirtIODevice *vdev)
> +{
> + VirtIOBlock *s = to_virtio_blk(vdev);
> +
> + qdev_uninit_bdrv(s->bs, IF_VIRTIO);
> +}
> +
> VirtIODevice *virtio_blk_init(DeviceState *dev)
> {
> VirtIOBlock *s;
> @@ -430,6 +437,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev)
> s->vdev.get_config = virtio_blk_update_config;
> s->vdev.get_features = virtio_blk_get_features;
> s->vdev.reset = virtio_blk_reset;
> + s->vdev.uninit = virtio_blk_uninit;
> s->bs = bs;
> s->rq = NULL;
> if (strlen(ps = (char *)drive_get_serial(bs)))
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 703f4fe..00b3998 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -375,6 +375,22 @@ static const VirtIOBindings virtio_pci_bindings = {
> .load_queue = virtio_pci_load_queue,
> };
>
> +static int virtio_pci_uninit(PCIDevice *pci_dev)
> +{
> + VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev);
> + VirtIODevice *vdev = proxy->vdev;
> +
> + if (vdev->uninit) {
> + vdev->uninit(vdev);
> + }
> +
> + if (vdev->nvectors) {
> + return msix_uninit(pci_dev);
> + }
> +
> + return 0;
> +}
> +
> static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> uint16_t vendor, uint16_t device,
> uint16_t class_code, uint8_t pif)
> @@ -407,10 +423,11 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> PCI_ADDRESS_SPACE_MEM,
> msix_mmio_map);
> proxy->pci_dev.config_write = virtio_write_config;
> - proxy->pci_dev.unregister = msix_uninit;
> } else
> vdev->nvectors = 0;
>
> + proxy->pci_dev.unregister = virtio_pci_uninit;
> +
> size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
> if (size & (size-1))
> size = 1 << qemu_fls(size);
> diff --git a/hw/virtio.h b/hw/virtio.h
> index aa55677..330b317 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -103,6 +103,7 @@ struct VirtIODevice
> void (*get_config)(VirtIODevice *vdev, uint8_t *config);
> void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
> void (*reset)(VirtIODevice *vdev);
> + void (*uninit)(VirtIODevice *vdev);
> VirtQueue *vq;
> const VirtIOBindings *binding;
> void *binding_opaque;
> diff --git a/sysemu.h b/sysemu.h
> index ce25109..18f610b 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -193,6 +193,8 @@ extern const char *drive_get_serial(BlockDriverState *bdrv);
> extern BlockInterfaceErrorAction drive_get_onerror(BlockDriverState *bdrv);
>
> BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type);
> +void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type);
> +
>
> struct drive_opt {
> const char *file;
Hi Anthony-
Thanks for the patch. I've tested this against qemu-kvm-0.11.0 and it's
working well. I can add/remove/add/remove virtio disks without
segfaulting qemu. We're going to carry this patch in Ubuntu.
Tested-by: Dustin Kirkland <kirkland@canonical.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] [STABLE] Fix virtio-blk hot add after remove
2009-10-14 16:59 [Qemu-devel] [PATCH] [STABLE] Fix virtio-blk hot add after remove Anthony Liguori
2009-10-14 18:22 ` [Qemu-devel] " Dustin Kirkland
@ 2009-10-15 8:00 ` Gerd Hoffmann
1 sibling, 0 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2009-10-15 8:00 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, Dustin Kirkland
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
On 10/14/09 18:59, Anthony Liguori wrote:
> qdev_init_bdrv() expects that each drive added is the next logical unit for
> the given interface type. However, when dealing with hotplug, there may
> be holes in the units. drive_init reclaims holes in units but qdev_init_bdrv()
> is not smart enough to do this.
Oh, right.
> Suggestions for a less ugly solution are appreciated.
See attached patch. Isn't exactly pretty. But at least it is less
invasive and IMHO the logic is easier to understand as well.
Warning: *untested*.
cheers,
Gerd
[-- Attachment #2: 0001-fix-virtio-blk-hotplugging.patch --]
[-- Type: text/plain, Size: 1891 bytes --]
>From 572ff4912ed021c7b4bb2076d8d72e497f0c434b Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 15 Oct 2009 09:55:11 +0200
Subject: [PATCH] fix virtio blk hotplugging.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/pci-hotplug.c | 2 ++
hw/qdev.c | 8 ++++++++
sysemu.h | 1 +
3 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 8bedea2..658564e 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -147,7 +147,9 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
drives_table[drive_idx].unit);
break;
case IF_VIRTIO:
+ drives_table[drive_idx].qdev_pick_this_one_please = 1;
dev = pci_create("virtio-blk-pci", devaddr);
+ drives_table[drive_idx].qdev_pick_this_one_please = 0;
break;
default:
dev = NULL;
diff --git a/hw/qdev.c b/hw/qdev.c
index faecc76..9dd896b 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -191,6 +191,14 @@ BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type)
int unit = next_block_unit[type]++;
int index;
+ for (index = 0; index < MAX_DRIVES; index++) {
+ if (!drives_table[index].used)
+ continue;
+ if (!drives_table[index].qdev_pick_this_one_please)
+ continue;
+ return drives_table[index].bdrv;
+ }
+
index = drive_get_index(type, 0, unit);
if (index == -1) {
return NULL;
diff --git a/sysemu.h b/sysemu.h
index ce25109..6dc2b4f 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -173,6 +173,7 @@ typedef struct DriveInfo {
int bus;
int unit;
int used;
+ int qdev_pick_this_one_please; /* band-aid for virtio-blk hotplug */
int drive_opt_idx;
BlockInterfaceErrorAction onerror;
char serial[BLOCK_SERIAL_STRLEN + 1];
--
1.6.2.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-10-15 8:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-14 16:59 [Qemu-devel] [PATCH] [STABLE] Fix virtio-blk hot add after remove Anthony Liguori
2009-10-14 18:22 ` [Qemu-devel] " Dustin Kirkland
2009-10-15 8:00 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).