From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48966) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmPxE-0004RM-SQ for qemu-devel@nongnu.org; Mon, 19 May 2014 11:58:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WmPx8-0006Pc-MR for qemu-devel@nongnu.org; Mon, 19 May 2014 11:58:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmPx8-0006PV-Ey for qemu-devel@nongnu.org; Mon, 19 May 2014 11:58:14 -0400 Date: Mon, 19 May 2014 18:57:03 +0300 From: "Michael S. Tsirkin" Message-ID: <20140519155703.GC31595@redhat.com> References: <1400511808-16929-1-git-send-email-junmuzi@gmail.com> <537A23DF.30807@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <537A23DF.30807@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] Add remove_boot_device_path() function for hot-unplug device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: kwolf@redhat.com, famz@redhat.com, aliguori@amazon.com, marcel.a@redhat.com, juli@redhat.com, qemu-devel@nongnu.org, Gerd Hoffmann , stefanha@redhat.com, pbonzini@redhat.com, Jun Li On Mon, May 19, 2014 at 05:31:43PM +0200, Andreas F=E4rber wrote: > Hi, >=20 > Am 19.05.2014 17:03, schrieb Jun Li: > > Add remove_boot_device_path() function to remove bootindex when hot-u= nplug > > a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-gener= ic device. > >=20 > > Signed-off-by: Jun Li > > --- > > This patch also fixed bug1086603, ref: > > https://bugzilla.redhat.com/show_bug.cgi?id=3D1086603 > >=20 > > This version of patch delete dev and suffix parameter from function r= emove_boot_device_path(). > > --- > > hw/block/virtio-blk.c | 1 + > > hw/net/virtio-net.c | 1 + > > hw/scsi/scsi-disk.c | 1 + > > hw/scsi/scsi-generic.c | 1 + >=20 > On v1 I believe I reminded you of spapr_llan. Your patch is adding a ne= w > remove_*() function, but is using it only for roughly half of the > devices that currently call add_boot_device_path(). Why? I can > understand that ISA devices will not be hot-unpluggable, but all PCI an= d > USB devices are. Can we remove the device from boot path automatically when it's going away? > hw/block/fdc.c: add_boot_device_path(isa->bootindexA, dev, "/floppy@= 0"); > hw/block/fdc.c: add_boot_device_path(isa->bootindexB, dev, "/floppy@= 1"); > hw/block/virtio-blk.c: add_boot_device_path(s->conf->bootindex, dev, > "/disk@0,0"); > hw/core/loader.c: add_boot_device_path(bootindex, NULL, devpath); > hw/i386/kvm/pci-assign.c: add_boot_device_path(dev->bootindex, > &pci_dev->qdev, NULL); > hw/ide/qdev.c: add_boot_device_path(dev->conf.bootindex, &dev->qdev, > hw/misc/vfio.c: add_boot_device_path(vdev->bootindex, &pdev->qdev, N= ULL); > hw/net/e1000.c: add_boot_device_path(d->conf.bootindex, dev, > "/ethernet-phy@0"); > hw/net/eepro100.c: add_boot_device_path(s->conf.bootindex, > &pci_dev->qdev, "/ethernet-phy@0"); > hw/net/ne2000.c: add_boot_device_path(s->c.bootindex, &pci_dev->qdev= , > "/ethernet-phy@0"); > hw/net/pcnet.c: add_boot_device_path(s->conf.bootindex, dev, > "/ethernet-phy@0"); > hw/net/rtl8139.c: add_boot_device_path(s->conf.bootindex, d, > "/ethernet-phy@0"); > hw/net/spapr_llan.c: add_boot_device_path(dev->nicconf.bootindex, > DEVICE(dev), ""); > hw/net/virtio-net.c: add_boot_device_path(n->nic_conf.bootindex, dev= , > "/ethernet-phy@0"); > hw/net/vmxnet3.c: add_boot_device_path(s->conf.bootindex, dev, > "/ethernet-phy@0"); > hw/scsi/scsi-disk.c: add_boot_device_path(s->qdev.conf.bootindex, > &dev->qdev, NULL); > hw/scsi/scsi-generic.c: add_boot_device_path(s->conf.bootindex, > &s->qdev, NULL); > hw/usb/dev-network.c: add_boot_device_path(s->conf.bootindex, > &dev->qdev, "/ethernet@0"); > hw/usb/host-libusb.c: add_boot_device_path(s->bootindex, &udev->qdev= , > NULL); > hw/usb/redirect.c: add_boot_device_path(dev->bootindex, &udev->qdev, > NULL); >=20 > > include/sysemu/sysemu.h | 1 + > > vl.c | 15 +++++++++++++++ > > 6 files changed, 20 insertions(+) > >=20 > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 8a568e5..497eb40 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceSta= te *dev, Error **errp) > > unregister_savevm(dev, "virtio-blk", s); > > blockdev_mark_auto_del(s->bs); > > virtio_cleanup(vdev); > > + remove_boot_device_path(s->conf->bootindex); > > } > > =20 > > static Property virtio_blk_properties[] =3D { > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 940a7cf..6afb1ca 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -1645,6 +1645,7 @@ static void virtio_net_device_unrealize(DeviceS= tate *dev, Error **errp) > > g_free(n->vqs); > > qemu_del_nic(n->nic); > > virtio_cleanup(vdev); > > + remove_boot_device_path(n->nic_conf.bootindex); > > } > > =20 > > static void virtio_net_instance_init(Object *obj) > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > > index 4bcef55..d33df2e 100644 > > --- a/hw/scsi/scsi-disk.c > > +++ b/hw/scsi/scsi-disk.c > > @@ -2156,6 +2156,7 @@ static void scsi_destroy(SCSIDevice *dev) > > =20 > > scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); > > blockdev_mark_auto_del(s->qdev.conf.bs); > > + remove_boot_device_path(s->qdev.conf.bootindex); >=20 > Didn't I previously ask that you use dev->config.bootindex here? > Pointers to parent's struct shouldn't be accessed anywhere except > VMStateDescription. >=20 > > } > > =20 > > static void scsi_disk_resize_cb(void *opaque) > > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > > index 3733d2c..d387f3e 100644 > > --- a/hw/scsi/scsi-generic.c > > +++ b/hw/scsi/scsi-generic.c > > @@ -390,6 +390,7 @@ static void scsi_destroy(SCSIDevice *s) > > { > > scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); > > blockdev_mark_auto_del(s->conf.bs); > > + remove_boot_device_path(s->conf.bootindex); > > } > > =20 > > static int scsi_generic_initfn(SCSIDevice *s) > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > > index ba5c7f8..2510e3b 100644 > > --- a/include/sysemu/sysemu.h > > +++ b/include/sysemu/sysemu.h > > @@ -193,6 +193,7 @@ void rtc_change_mon_event(struct tm *tm); > > =20 > > void add_boot_device_path(int32_t bootindex, DeviceState *dev, > > const char *suffix); > > +void remove_boot_device_path(int32_t bootindex); > > char *get_boot_devices_list(size_t *size, bool ignore_suffixes); > > =20 > > DeviceState *get_boot_device(uint32_t position); > > diff --git a/vl.c b/vl.c > > index 709d8cd..f31d008 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1189,6 +1189,21 @@ void add_boot_device_path(int32_t bootindex, D= eviceState *dev, > > QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); > > } > > =20 > > +void remove_boot_device_path(int32_t bootindex) > > +{ > > + FWBootEntry *node, *next_node; > > + > > + if (bootindex =3D=3D -1) { > > + return; > > + } > > + > > + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) >=20 > Coding Style requires braces around the below single-statement if. >=20 > > + if (node->bootindex =3D=3D bootindex) { > > + QTAILQ_REMOVE(&fw_boot_order, node, link); > > + return; > > + } > > +} > > + > > DeviceState *get_boot_device(uint32_t position) > > { > > uint32_t counter =3D 0; >=20 > Regards, > Andreas >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg