[virt-tools-list] [PATCH] virtinst:Add sPAPR-vSCSI controller configuration correctly for pseries machine

[Cole Robinson]

On 03/29/2012 03:44 AM, Li Zhang wrote:
> From: Qing Lin <qinglbj at linux.vnet.ibm.com>
> 
> When installing OS from cdrom, we cannot change its address type
> to spapr-vio (Power support bus type) through GUI. So, it needs 
> to change default cdrom disk type into vSCSI on pSeries machine.
> All the vSCSI disks share one sPAPR-vSCSI controller. So,
> when the first vSCSI disk is added, sPAPR-vSCSI controller
> should be added, if sPAPR-vSCSI controller is already existed,
> it's not necessary to add it.
> 
> Signed-off-by: Qing Lin <qinglbj at linux.vnet.ibm.com>
> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
> ---
>  tests/xmlconfig-xml/boot-many-devices.xml |   12 ++++++++--
>  tests/xmlconfig.py                        |    4 +++
>  virtinst/Guest.py                         |   28 ++++++++++++++++----------
>  3 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/xmlconfig-xml/boot-many-devices.xml b/tests/xmlconfig-xml/boot-many-devices.xml
> index 043e291..58107be 100644
> --- a/tests/xmlconfig-xml/boot-many-devices.xml
> +++ b/tests/xmlconfig-xml/boot-many-devices.xml
> @@ -39,9 +39,12 @@
>        <source file='/default-pool/testvol1.img'/>
>        <target dev='sdb' bus='scsi'/>
>      </disk>
> -    <controller type='scsi' index='0'>
> -      <address type='spapr-vio'/>
> -    </controller>
> +    <disk type='file' device='cdrom'>
> +      <driver name='qemu' type='qcow2'/>
> +      <source file='/default-pool/testvol2.img'/>
> +      <target dev='sdc' bus='scsi'/>
> +      <readonly/>
> +    </disk>
>      <controller type='ide' index='3'/>
>      <controller type='virtio-serial' index='0' ports='32' vectors='17'/>
>      <interface type='network'>
> @@ -93,6 +96,9 @@
>        </source>
>      </hostdev>
>      <watchdog model='ib700' action='none'/>
> +    <controller type='scsi' index='0'>
> +      <address type='spapr-vio'/>
> +    </controller>
>    </devices>
>    <seclabel type='static' model='selinux'>
>      <label>foolabel</label>
> diff --git a/tests/xmlconfig.py b/tests/xmlconfig.py
> index 1dd447d..f237fb2 100644
> --- a/tests/xmlconfig.py
> +++ b/tests/xmlconfig.py
> @@ -716,6 +716,10 @@ class TestXMLConfig(unittest.TestCase):
>                           bus="scsi", driverName="qemu")
>          d3.address.type = "spapr-vio"
>          g.disks.append(d3)
> +        d4 = VirtualDisk(conn=g.conn, path="/default-pool/testvol2.img", device="cdrom",
> +                         bus="scsi", driverName="qemu")
> +        d4.address.type = "spapr-vio"
> +        g.disks.append(d4)
>  
>          # Controller devices
>          c1 = VirtualController.get_class_for_type(VirtualController.CONTROLLER_TYPE_IDE)(g.conn)
> diff --git a/virtinst/Guest.py b/virtinst/Guest.py
> index cd529aa..3d69b96 100644
> --- a/virtinst/Guest.py
> +++ b/virtinst/Guest.py
> @@ -870,21 +870,23 @@ class Guest(XMLBuilderDomain.XMLBuilderDomain):
>              finally:
>                  if origpath:
>                      dev.path = origpath
> -        def get_vscsi_ctrl_xml():
> -            vscsi_class = virtinst.VirtualController.get_class_for_type(
> -                          virtinst.VirtualController.CONTROLLER_TYPE_SCSI)
> -            ctrl = vscsi_class(self.conn)
> -            ctrl.set_address("spapr-vio")
> -            return ctrl.get_xml_config()
> -
>          xml = self._get_emulator_xml()
>          # Build XML
> +        vscsi_need_add = False
> +        vscsi_is_exist = False
>          for dev in devs:
>              xml = _util.xml_append(xml, get_dev_xml(dev))
> -            if (dev.address.type == "spapr-vio" and
> -                  dev.virtual_device_type == virtinst.VirtualDevice.VIRTUAL_DEV_DISK):
> -                xml = _util.xml_append(xml, get_vscsi_ctrl_xml())
> -
> +            if (dev.virtual_device_type == virtinst.VirtualDevice.VIRTUAL_DEV_DISK and
> +                dev.address.type == "spapr-vio"):
> +                vscsi_need_add = True
> +            if (dev.get_virtual_device_type() == VirtualDevice.VIRTUAL_DEV_CONTROLLER and
> +                dev.address.type == "spapr-vio"):
> +                vscsi_is_exist = True
> +        if (vscsi_need_add and not vscsi_is_exist):
> +            ctrl = virtinst.VirtualController.get_class_for_type(virtinst.VirtualController.CONTROLLER_TYPE_SCSI)(self.conn)
> +            ctrl.set_address("spapr-vio")
> +            self.add_device(ctrl)
> +            xml = _util.xml_append(xml, ctrl.get_xml_config())
>          return xml
>  

Hmm, this is getting a little out of hand. We wouldn't need any of this if
libvirt was adding an implicit controller for us. Which I think is the correct
way forward here, since libvirt does it for just about everything else.

In fact I should have just rejected the original patch that changed this function.

Ideally what we would do here is just specify a cdrom device like

<disk type='file' device='cdrom'>
  <source file='foobar'/>
</disk>

and libvirt could generate a device target, and fill in the default bus value.
Then users/apps wouldn't need to know the supported buses for each
hv+arch+machine combination. But that's for another day. Until then, the rest
of the patch is fine.

- Cole

>      def _get_emulator_xml(self):
> @@ -1503,6 +1505,10 @@ class Guest(XMLBuilderDomain.XMLBuilderDomain):
>                  else:
>                      if self.installer.is_hvm():
>                          disk.bus = "ide"
> +                        if (self.installer.type == "kvm" and
> +                            self.installer.machine == "pseries"):
> +                            disk.bus = "scsi"
> +                            disk.set_address("spapr-vio")
>                      elif self.installer.is_xenpv():
>                          disk.bus = "xen"
>              used_targets.append(disk.generate_target(used_targets))