From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1My8US-00071R-5o for qemu-devel@nongnu.org; Wed, 14 Oct 2009 14:22:24 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1My8UQ-000706-U5 for qemu-devel@nongnu.org; Wed, 14 Oct 2009 14:22:23 -0400 Received: from [199.232.76.173] (port=33900 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1My8UQ-0006zl-Fb for qemu-devel@nongnu.org; Wed, 14 Oct 2009 14:22:22 -0400 Received: from adelie.canonical.com ([91.189.90.139]:45578) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1My8UP-0008JI-VM for qemu-devel@nongnu.org; Wed, 14 Oct 2009 14:22:22 -0400 From: Dustin Kirkland In-Reply-To: <1255539554-7956-1-git-send-email-aliguori@us.ibm.com> References: <1255539554-7956-1-git-send-email-aliguori@us.ibm.com> Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-YhrEkDOyJlnoz7Jn1fst" Date: Wed, 14 Oct 2009 13:22:16 -0500 Message-Id: <1255544536.12108.11.camel@x200> Mime-Version: 1.0 Subject: [Qemu-devel] Re: [PATCH] [STABLE] Fix virtio-blk hot add after remove Reply-To: kirkland@canonical.com List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, Gerd Hoffman --=-YhrEkDOyJlnoz7Jn1fst Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 f= or > 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. >=20 > 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 b= een > 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 add= ed to > the virtio layer. >=20 > Suggestions for a less ugly solution are appreciated. >=20 > This fixes https://bugs.launchpad.net/bugs/419590 >=20 > Signed-off-by: Anthony Liguori > --- > 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(-) >=20 > 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 *ma= caddr) > memcpy(macaddr, dev->nd->macaddr, 6); > } > =20 > -static int next_block_unit[IF_COUNT]; > +typedef struct BlockUnitState > +{ > + BlockDriverState *bs[32]; > +} BlockUnitState; > +static BlockUnitState next_block_unit[IF_COUNT]; > =20 > /* Get a block device. This should only be used for single-drive device= s > (e.g. SD/Floppy/MTD). Multi-disk devices (scsi/ide) should use the > appropriate bus. */ > BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType ty= pe) > { > - int unit =3D next_block_unit[type]++; > + BlockDriverState *bs; > + int unit; > int index; > =20 > + unit =3D 0; > + while (next_block_unit[type].bs[unit]) { > + unit++; > + } > + > index =3D drive_get_index(type, 0, unit); > if (index =3D=3D -1) { > return NULL; > } > - return drives_table[index].bdrv; > + > + bs =3D drives_table[index].bdrv; > + next_block_unit[type].bs[unit] =3D bs; > + > + return bs; > +} > + > +void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type) > +{ > + int unit; > + > + for (unit =3D 0; unit < 32; unit++) { > + if (next_block_unit[type].bs[unit] =3D=3D bs) { > + break; > + } > + } > + > + next_block_unit[type].bs[unit] =3D NULL; > } > =20 > 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; > } > =20 > +static void virtio_blk_uninit(VirtIODevice *vdev) > +{ > + VirtIOBlock *s =3D 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 =3D virtio_blk_update_config; > s->vdev.get_features =3D virtio_blk_get_features; > s->vdev.reset =3D virtio_blk_reset; > + s->vdev.uninit =3D virtio_blk_uninit; > s->bs =3D bs; > s->rq =3D NULL; > if (strlen(ps =3D (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 =3D = { > .load_queue =3D virtio_pci_load_queue, > }; > =20 > +static int virtio_pci_uninit(PCIDevice *pci_dev) > +{ > + VirtIOPCIProxy *proxy =3D container_of(pci_dev, VirtIOPCIProxy, pci_= dev); > + VirtIODevice *vdev =3D 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 =3D virtio_write_config; > - proxy->pci_dev.unregister =3D msix_uninit; > } else > vdev->nvectors =3D 0; > =20 > + proxy->pci_dev.unregister =3D virtio_pci_uninit; > + > size =3D VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len; > if (size & (size-1)) > size =3D 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 *bdr= v); > =20 > BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType ty= pe); > +void qdev_uninit_bdrv(BlockDriverState *bs, BlockInterfaceType type); > + > =20 > 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 --=-YhrEkDOyJlnoz7Jn1fst Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEABECAAYFAkrWFtcACgkQs7pNXIOmEZTvTACfa5DuTLmLU7iGqhWWezcQbvbx 5GIAoM5rjtU9OISSp/yw5Yf52daMg4MI =6XVG -----END PGP SIGNATURE----- --=-YhrEkDOyJlnoz7Jn1fst--