| Message ID | 20120523135206.GA14654@chinaltcdragon.cn.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 23, 2012 at 05:12:16PM +0300, Michael S. Tsirkin wrote: > On Wed, May 23, 2012 at 09:52:06PM +0800, Kelvin Wang wrote: > > Support the virtio-scsi-pci adapter hot-plug. However, this patch can only make > > adapter hot-plugable. More effort is needed for LUN hot-plug. Actually, that is > > the most valuable feature to virtio-scsi. > > > > Following the steps as follows can give an intuitive understanding of this > > feature after applying the patch to QEMU v1.1.0-rc3. > > > > 1, Generate a image file: > > qemu-img create -f qcow2 test.img 2G > > > > 2, Run qemu with the option -monitor. > > > > 3, In the guest, insert necessary modules: > > for m in acpiphp pci_hotplug; do sudo modprobe ${m}; done > > > > 4, In the qemu monitor,hot add a virtio-scsi-pci adapter: > > (qemu)pci_add auto storage if=virtio-scsi-pci > > > > 5, Check whether the controller was added: > > Guest: lspci > > Qemu: (qemu)info qtree > > > > Signed-off-by: Kelvin Wang <senwang@linux.vnet.ibm.com> > > Signed-off-by: Sheng Liu <liusheng@linux.vnet.ibm.com> > > NAK > > Do not use pci_add. It is a compatibility command. > Use the new style device_add. > Same for if=. > > I think you won't need any changes then? OK, Thanks! > > > --- > > blockdev.c | 19 +++++++++++++++---- > > blockdev.h | 3 ++- > > hw/pci-hotplug.c | 15 +++++++++++++++ > > 3 files changed, 32 insertions(+), 5 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 67895b2..ecb82bf 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -31,6 +31,7 @@ static const char *const if_name[IF_COUNT] = { > > [IF_MTD] = "mtd", > > [IF_SD] = "sd", > > [IF_VIRTIO] = "virtio", > > + [IF_VIRTIO_SCSI] = "virtio-scsi", > > weird indentation > > > [IF_XEN] = "xen", > > }; > > > > I think blockdev is a legacy thing. You are > supposed to use the new style device-add. No? > > > @@ -436,7 +437,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > > > on_write_error = BLOCK_ERR_STOP_ENOSPC; > > if ((buf = qemu_opt_get(opts, "werror")) != NULL) { > > - if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) { > > + if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && > > + type != IF_VIRTIO_SCSI && type != IF_NONE) { > > code dupicated here and below. add a function? > > > error_report("werror is not supported by this bus type"); > > return NULL; > > } > > @@ -449,7 +451,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > > > on_read_error = BLOCK_ERR_REPORT; > > if ((buf = qemu_opt_get(opts, "rerror")) != NULL) { > > - if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) { > > + if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && > > + IF_VIRTIO_SCSI && type != IF_NONE) { > > typo? IF_VIRTIO_SCSI is always != 0 ... Ouch, ashamed for my carelessness. > > > error_report("rerror is not supported by this bus type"); > > return NULL; > > } > > @@ -461,7 +464,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > } > > > > if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) { > > - if (type != IF_VIRTIO) { > > + if (type != IF_VIRTIO || type != IF_VIRTIO_SCSI) { > > error_report("addr is not supported by this bus type"); > > return NULL; > > } > > @@ -579,6 +582,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > if (devaddr) > > qemu_opt_set(opts, "addr", devaddr); > > break; > > + case IF_VIRTIO_SCSI: > > + opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0); > > + qemu_opt_set(opts, "driver", "virtio-scsi-pci"); > > + qemu_opt_set(opts, "drive", dinfo->id); > > + if (devaddr) { > > + qemu_opt_set(opts, "addr", devaddr); > > + } > > + break; > > default: > > abort(); > > } > > @@ -604,7 +615,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > ro = 1; > > } else if (ro == 1) { > > if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && > > - type != IF_NONE && type != IF_PFLASH) { > > + type != IF_NONE && type != IF_PFLASH && IF_VIRTIO_SCSI) { > > error_report("readonly not supported by this bus type"); > > goto err; > > } > > diff --git a/blockdev.h b/blockdev.h > > index 260e16b..96f40a5 100644 > > --- a/blockdev.h > > +++ b/blockdev.h > > @@ -22,7 +22,8 @@ void blockdev_auto_del(BlockDriverState *bs); > > typedef enum { > > IF_DEFAULT = -1, /* for use with drive_add() only */ > > IF_NONE, > > - IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, > > + IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, > > + IF_VIRTIO_SCSI, > > weird indentation > > > IF_COUNT > > } BlockInterfaceType; > > > > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > > index c55d8b9..4f5c022 100644 > > --- a/hw/pci-hotplug.c > > +++ b/hw/pci-hotplug.c > > @@ -154,6 +154,8 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, > > type = IF_SCSI; > > else if (!strcmp(buf, "virtio")) { > > type = IF_VIRTIO; > > + } else if (!strcmp(buf, "virtio-scsi")) { > > + type = IF_VIRTIO_SCSI; > > } else { > > monitor_printf(mon, "type %s not a hotpluggable PCI device.\n", buf); > > return NULL; > > @@ -211,6 +213,19 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, > > if (qdev_init(&dev->qdev) < 0) > > dev = NULL; > > break; > > + case IF_VIRTIO_SCSI: > > + dev = pci_create(bus, devfn, "virtio-scsi-pci"); > > + if (qdev_init(&dev->qdev) < 0) { > > + dev = NULL; > > + } > > + > > + if (dev && dinfo) { > > + if (scsi_hot_add(mon, &dev->qdev, dinfo, 0) != 0) { > > + qdev_unplug(&dev->qdev, NULL); > > + dev = NULL; > > + } > > + } > > + break; > > default: > > dev = NULL; > > } > > Here I am sure. > Do not keep adding stuff to pci-hotplug. > New devices should use device-add. > > > -- > > 1.7.1 >
On Wed, May 23, 2012 at 05:45:33PM +0300, Michael S. Tsirkin wrote: > On Wed, May 23, 2012 at 04:30:06PM +0200, Paolo Bonzini wrote: > > Il 23/05/2012 16:12, Michael S. Tsirkin ha scritto: > > >> > 2, Run qemu with the option -monitor. > > >> > > > >> > 3, In the guest, insert necessary modules: > > >> > for m in acpiphp pci_hotplug; do sudo modprobe ${m}; done > > >> > > > >> > 4, In the qemu monitor,hot add a virtio-scsi-pci adapter: > > >> > (qemu)pci_add auto storage if=virtio-scsi-pci > > >> > > > >> > 5, Check whether the controller was added: > > >> > Guest: lspci > > >> > Qemu: (qemu)info qtree > > >> > > > >> > Signed-off-by: Kelvin Wang <senwang@linux.vnet.ibm.com> > > >> > Signed-off-by: Sheng Liu <liusheng@linux.vnet.ibm.com> > > > NAK > > > > > > Do not use pci_add. It is a compatibility command. > > > Use the new style device_add. > > > Same for if=. > > > > > > I think you won't need any changes then? > > > > > > > You don't. You need to rescan the bus manually in the guest, that's all. > > > > Paolo > > If the point is to avoid need for manual bus rescans that's > good. But please do not touch the legacy commands. So, may I sent another patch to "avoid need for manual bus rescans"? device_add should be used by users, but another way supplied to users is not necessarily, but always harmless, right? > If anyone wants to use new devices, new commands > drive_add and device_add should be used. > Same for command line flags. > > -- > MST >
On Thu, May 24, 2012 at 02:31:00PM +0800, Kelvin Wang wrote: > On Wed, May 23, 2012 at 05:45:33PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 23, 2012 at 04:30:06PM +0200, Paolo Bonzini wrote: > > > Il 23/05/2012 16:12, Michael S. Tsirkin ha scritto: > > > >> > 2, Run qemu with the option -monitor. > > > >> > > > > >> > 3, In the guest, insert necessary modules: > > > >> > for m in acpiphp pci_hotplug; do sudo modprobe ${m}; done > > > >> > > > > >> > 4, In the qemu monitor,hot add a virtio-scsi-pci adapter: > > > >> > (qemu)pci_add auto storage if=virtio-scsi-pci > > > >> > > > > >> > 5, Check whether the controller was added: > > > >> > Guest: lspci > > > >> > Qemu: (qemu)info qtree > > > >> > > > > >> > Signed-off-by: Kelvin Wang <senwang@linux.vnet.ibm.com> > > > >> > Signed-off-by: Sheng Liu <liusheng@linux.vnet.ibm.com> > > > > NAK > > > > > > > > Do not use pci_add. It is a compatibility command. > > > > Use the new style device_add. > > > > Same for if=. > > > > > > > > I think you won't need any changes then? > > > > > > > > > > You don't. You need to rescan the bus manually in the guest, that's all. > > > > > > Paolo > > > > If the point is to avoid need for manual bus rescans that's > > good. But please do not touch the legacy commands. > So, may I sent another patch to "avoid need for manual bus rescans"? Let's separate bugfixes from adding new commands. > device_add should be used by users, but another way supplied to users is not > necessarily, but always harmless, right? No, it has support costs. > > If anyone wants to use new devices, new commands > > drive_add and device_add should be used. > > Same for command line flags. > > > > -- > > MST > >
Il 24/05/2012 08:31, Kelvin Wang ha scritto: >> > >> > If the point is to avoid need for manual bus rescans that's >> > good. But please do not touch the legacy commands. > So, may I sent another patch to "avoid need for manual bus rescans"? The virtio spec supports sending events for added and removed devices. I'm pretty sure it will be more than one patch though (at least one in QEMU and one in the kernel :)). Paolo
On Thu, May 24, 2012 at 12:00:29PM +0200, Paolo Bonzini wrote: > Il 24/05/2012 08:31, Kelvin Wang ha scritto: > >> > > >> > If the point is to avoid need for manual bus rescans that's > >> > good. But please do not touch the legacy commands. > > So, may I sent another patch to "avoid need for manual bus rescans"? > > The virtio spec supports sending events for added and removed devices. > I'm pretty sure it will be more than one patch though (at least one in > QEMU and one in the kernel :)). OK, thanks. I'll take more efforts to investigate it. > > Paolo >
On Thu, May 24, 2012 at 7:31 AM, Kelvin Wang <senwang@linux.vnet.ibm.com> wrote: > On Wed, May 23, 2012 at 05:45:33PM +0300, Michael S. Tsirkin wrote: >> On Wed, May 23, 2012 at 04:30:06PM +0200, Paolo Bonzini wrote: >> > Il 23/05/2012 16:12, Michael S. Tsirkin ha scritto: >> > >> > 2, Run qemu with the option -monitor. >> > >> > >> > >> > 3, In the guest, insert necessary modules: >> > >> > for m in acpiphp pci_hotplug; do sudo modprobe ${m}; done >> > >> > >> > >> > 4, In the qemu monitor,hot add a virtio-scsi-pci adapter: >> > >> > (qemu)pci_add auto storage if=virtio-scsi-pci >> > >> > >> > >> > 5, Check whether the controller was added: >> > >> > Guest: lspci >> > >> > Qemu: (qemu)info qtree >> > >> > >> > >> > Signed-off-by: Kelvin Wang <senwang@linux.vnet.ibm.com> >> > >> > Signed-off-by: Sheng Liu <liusheng@linux.vnet.ibm.com> >> > > NAK >> > > >> > > Do not use pci_add. It is a compatibility command. >> > > Use the new style device_add. >> > > Same for if=. >> > > >> > > I think you won't need any changes then? >> > > >> > >> > You don't. You need to rescan the bus manually in the guest, that's all. >> > >> > Paolo >> >> If the point is to avoid need for manual bus rescans that's >> good. But please do not touch the legacy commands. > So, may I sent another patch to "avoid need for manual bus rescans"? This is LUN hotplug support. Stefan
diff --git a/blockdev.c b/blockdev.c index 67895b2..ecb82bf 100644 --- a/blockdev.c +++ b/blockdev.c @@ -31,6 +31,7 @@ static const char *const if_name[IF_COUNT] = { [IF_MTD] = "mtd", [IF_SD] = "sd", [IF_VIRTIO] = "virtio", + [IF_VIRTIO_SCSI] = "virtio-scsi", [IF_XEN] = "xen", }; @@ -436,7 +437,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) on_write_error = BLOCK_ERR_STOP_ENOSPC; if ((buf = qemu_opt_get(opts, "werror")) != NULL) { - if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) { + if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && + type != IF_VIRTIO_SCSI && type != IF_NONE) { error_report("werror is not supported by this bus type"); return NULL; } @@ -449,7 +451,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) on_read_error = BLOCK_ERR_REPORT; if ((buf = qemu_opt_get(opts, "rerror")) != NULL) { - if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) { + if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && + IF_VIRTIO_SCSI && type != IF_NONE) { error_report("rerror is not supported by this bus type"); return NULL; } @@ -461,7 +464,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) } if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) { - if (type != IF_VIRTIO) { + if (type != IF_VIRTIO || type != IF_VIRTIO_SCSI) { error_report("addr is not supported by this bus type"); return NULL; } @@ -579,6 +582,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) if (devaddr) qemu_opt_set(opts, "addr", devaddr); break; + case IF_VIRTIO_SCSI: + opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0); + qemu_opt_set(opts, "driver", "virtio-scsi-pci"); + qemu_opt_set(opts, "drive", dinfo->id); + if (devaddr) { + qemu_opt_set(opts, "addr", devaddr); + } + break; default: abort(); } @@ -604,7 +615,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) ro = 1; } else if (ro == 1) { if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && - type != IF_NONE && type != IF_PFLASH) { + type != IF_NONE && type != IF_PFLASH && IF_VIRTIO_SCSI) { error_report("readonly not supported by this bus type"); goto err; } diff --git a/blockdev.h b/blockdev.h index 260e16b..96f40a5 100644 --- a/blockdev.h +++ b/blockdev.h @@ -22,7 +22,8 @@ void blockdev_auto_del(BlockDriverState *bs); typedef enum { IF_DEFAULT = -1, /* for use with drive_add() only */ IF_NONE, - IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, + IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, + IF_VIRTIO_SCSI, IF_COUNT } BlockInterfaceType; diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index c55d8b9..4f5c022 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -154,6 +154,8 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, type = IF_SCSI; else if (!strcmp(buf, "virtio")) { type = IF_VIRTIO; + } else if (!strcmp(buf, "virtio-scsi")) { + type = IF_VIRTIO_SCSI; } else { monitor_printf(mon, "type %s not a hotpluggable PCI device.\n", buf); return NULL; @@ -211,6 +213,19 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, if (qdev_init(&dev->qdev) < 0) dev = NULL; break; + case IF_VIRTIO_SCSI: + dev = pci_create(bus, devfn, "virtio-scsi-pci"); + if (qdev_init(&dev->qdev) < 0) { + dev = NULL; + } + + if (dev && dinfo) { + if (scsi_hot_add(mon, &dev->qdev, dinfo, 0) != 0) { + qdev_unplug(&dev->qdev, NULL); + dev = NULL; + } + } + break; default: dev = NULL; }
Support the virtio-scsi-pci adapter hot-plug. However, this patch can only make adapter hot-plugable. More effort is needed for LUN hot-plug. Actually, that is the most valuable feature to virtio-scsi. Following the steps as follows can give an intuitive understanding of this feature after applying the patch to QEMU v1.1.0-rc3. 1, Generate a image file: qemu-img create -f qcow2 test.img 2G 2, Run qemu with the option -monitor. 3, In the guest, insert necessary modules: for m in acpiphp pci_hotplug; do sudo modprobe ${m}; done 4, In the qemu monitor,hot add a virtio-scsi-pci adapter: (qemu)pci_add auto storage if=virtio-scsi-pci 5, Check whether the controller was added: Guest: lspci Qemu: (qemu)info qtree Signed-off-by: Kelvin Wang <senwang@linux.vnet.ibm.com> Signed-off-by: Sheng Liu <liusheng@linux.vnet.ibm.com> --- blockdev.c | 19 +++++++++++++++---- blockdev.h | 3 ++- hw/pci-hotplug.c | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-)