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

Fri Mar 30 11:57:58 UTC 2012

On 03/29/2012 10:13 PM, Cole Robinson wrote:
> 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.
>
Libvirt will generate the xml for it.
But the address type of controller is pci, not spapr-vio.
In libvirt, we didn't change the default setting.

What we want to do is to change the bus address of controller to spapr-vio.

It's a little like my spapr-vlan patch set. But it seems that
the scsi patches is a little complicated. Because when we set
the scsi as vscsi, it needs to set the controller's address type as
spapr-vio,not the disk device's address type. The disk device's address
type is always drive. Although we want to set the disk device as
spapr-vscsi, from the source code, it's a little hard to get the
controller instance which the scsi device is connected on to set
address type.

There is a little tricky here, we pass the disk's address type as
spapr-vio from virt-manager GUI, before the disk's address type is
changed to drive, we use it to change the controller's address type.

In fact, after the Guest instance is created, the disk's address type
won't be spapr-vio.

If we add a controller attribute in scsi device, it will be much easier
to get controller instance to set address type when we set scsi device
as vscsi. If there is controller option in detail page, it will much
better to set the controller. I saw there are some USB controller
configuration in virtinst, is it planed to add the add controller option
in GUI?

On power platform, the following devices are based on spapr-vio bus.
spapr-vssi, spapr-vlan, spapr-vty.
For some devices, for example, usb, memory balloon, are based on pci
bus.

So we didn't change spapr-vio bus address of as default.

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

I considered this before, comparing with changing it in libvirt, I think
it is much better to set the configuration from virt-manager and virsh.

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