4:49 a.m.
On 05/04/2012 05:04 PM, Daniel P. Berrange wrote:
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.
The problem is that when using two kinds of SCSI controller,
vscsi and scsi, which are based on different address type and
model at the same time, it needs to specify the model and address
type externally. And it's hard to find the right controller for
disk to use, only by the 'sdx'.
> ---
> 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.
If two kinds of SCSI controllers work together, it will be a problem.
For example, there are 2 disks, sda and sdb.
sda is on vscsi, and sdb is on scsi. sda and sdb can't be on the
same controller. With the original method, sda will be on
controller 0, unit 0. sdb will be controller 0, unit 1.
If controller 0 is based on spapr-vio address type, sdb can't work
at all.
Do you have any suggestion to fix this problem?
Thanks.
Li
> @@ -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
--
Best Regards
Li
IBM LTC, China System&Technology Lab, Beijing