Re: [Qemu-devel] [PATCH] Support virtio-scsi-pci adapter hot-plug

["Michael S. Tsirkin" <mst@redhat.com>]

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?

> ---
>  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 ...

>              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