Patchwork Support virtio-scsi-pci adapter hot-plug

login
register
mail settings
Submitter Kelvin Wang
Date May 23, 2012, 1:52 p.m.
Message ID <20120523135206.GA14654@chinaltcdragon.cn.ibm.com>
Download mbox | patch
Permalink /patch/160942/
State New
Headers show

Comments

Kelvin Wang - May 23, 2012, 1:52 p.m.
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(-)
Kelvin Wang - May 24, 2012, 5:43 a.m.
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
>
Kelvin Wang - May 24, 2012, 6:31 a.m.
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
>
Michael S. Tsirkin - May 24, 2012, 6:51 a.m.
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
> >
Paolo Bonzini - May 24, 2012, 10 a.m.
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
Kelvin Wang - May 24, 2012, 11:42 a.m.
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
>
Stefan Hajnoczi - May 25, 2012, 11:57 a.m.
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

Patch

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;
     }