[libvirt] [PATCH 1/2] Add VSCSI bus type and VSCSI controller type for pseries guest.

[Daniel P. Berrange]

On Fri, May 04, 2012 at 10:05:13AM +0800, Li Zhang wrote:
> Now, there is only SCSI bus and controller type in libvirt.
> And when configuring VSCSI controller, it needs to configure
> the spapr-vio bus address type externally. It's a little
> inconvenient to configure the controller type.
> 
> This patch is to add VSCSI bus type and VSCSI controller type.
> And handle with the problems when VSCSI and SCSI devices
> working together, by assign the even number to index of
> VSCSI controller and odd number to index of SCSI controller.
> 
> And when the VSCSI controller is always assigned as SPAPRVIO
> bus address type and SCSI controller will be always assigned
> as PCI bus address, which is implemented according to the 
> controllers' type.
> 
> So when one disk is based on VSCSI controller, then assign 
> the bus as 'vscsi', and one right VSCSI controller will be 
> selected. 

I'm not at all convinced by this description that we need
a new controller type. At most we need a separate <address/>
type for the VSCSI controller under an existing SCSI controller
element.


> ---
>  src/conf/domain_conf.c  |   62 ++++++++++++++++++++++++++++++++++++++++++-----
>  src/conf/domain_conf.h  |    2 ++
>  src/qemu/qemu_command.c |   11 +++++++--
>  3 files changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 184ff23..99005b7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -177,7 +177,8 @@ VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST,
>                "xen",
>                "usb",
>                "uml",
> -              "sata")
> +              "sata",
> +              "vscsi")
>  
>  VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST,
>                "default",
> @@ -236,7 +237,8 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST,
>                "sata",
>                "virtio-serial",
>                "ccid",
> -              "usb")
> +              "usb",
> +              "vscsi")
>  
>  VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
>                "auto",
> @@ -2973,6 +2975,7 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
>  
>      switch (def->bus) {
>      case VIR_DOMAIN_DISK_BUS_SCSI:
> +    case VIR_DOMAIN_DISK_BUS_VSCSI:
>          def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
>  
>          if (caps->hasWideScsiBus) {
> @@ -2981,6 +2984,16 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
>               * Unit 7 is the SCSI controller itself. Therefore unit 7
>               * cannot be assigned to disks and is skipped.
>               */
> +
> +            /* assign even number to index of vscsi controller,
> +             * and odd number to index of scsi controller, which can
> +             * make vscsi controller and scsi controller work together.
> +             */
> +            if (def->bus == VIR_DOMAIN_DISK_BUS_VSCSI)
> +                def->info.addr.drive.controller = (idx / 15) * 2;
> +            else
> +                def->info.addr.drive.controller = (idx / 15) * 2 + 1;

No, controller numbers *must* be contiguous starting from 0.

> @@ -2992,6 +3005,17 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
>          } else {
>              /* For a narrow SCSI bus we define the default mapping to be
>               * 7 units per bus, 1 bus per controller, many controllers */
> +
> +            /* assign even number to index of vscsi controller,
> +             * and odd number to index of scsi controller, which can
> +             * make vscsi controller and scsi controller work together.
> +             */
> +
> +            if (def->bus == VIR_DOMAIN_DISK_BUS_VSCSI)
> +                def->info.addr.drive.controller = (idx / 7) * 2;
> +            else
> +                def->info.addr.drive.controller = (idx / 7 ) * 2 + 1;
> +

Again NACK to this.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|