[libvirt] [RFC v3 PATCH 5/5] PowerPC : Add address-type "spapr-vio"

Wed Nov 30 12:23:29 UTC 2011

On 11/29/2011 10:46 PM, Stefan Berger wrote:
> On 11/29/2011 10:03 AM, Prerna Saxena wrote:
>> From: Michael Ellerman<michael at ellerman.id.au>
>> Date: Tue, 29 Nov 2011 13:48:09 +0530
>> Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on
>> the same bus.
>>
>> For QEMU PPC64 we have a machine type ("pseries") which
>> has a virtual bus called "spapr-vio". We need to be
>> able to create devices on this bus, and as such need a
>> way to specify the address for those devices.
>>
>> This patch adds a new address type "spapr-vio", which achieves this.
>>
>> The addressing is specified with a "reg" property in the address
>> definition. The reg is optional, if it is not specified QEMU will
>> auto-assign an address for the device.
>>
>> Additionally, this patch forces an "spapr-vscsi" controller to be
>> selected for a guest running on architecture 'ppc64' and platform
>> 'pseries'.
>>
>> Signed-off-by: Michael Ellerman<michael at ellerman.id.au>
>> Signed-off-by: Prerna Saxena<prerna at linux.vnet.ibm.com>
>> ---
>> src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>> src/conf/domain_conf.h | 9 +++++++++
>> src/qemu/qemu_command.c | 18 ++++++++++++++----
>> src/qemu/qemu_command.h | 3 ++-
>> src/qemu/qemu_hotplug.c | 2 +-
>> 5 files changed, 67 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3ea99f7..5c3809e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress,
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
>> "drive",
>> "virtio-serial",
>> "ccid",
>> - "usb")
>> + "usb",
>> + "spapr-vio")
>>
>> VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti,
>> VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST,
>> @@ -1844,6 +1845,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>> info->addr.usb.port);
>> break;
>>
>> + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
>> + if (info->addr.spaprvio.has_reg)
>> + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg);
>> + break;
>> +
>> default:
>> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> _("unknown address type '%d'"), info->type);
>> @@ -2103,6 +2109,34 @@ cleanup:
>> }
>>
>> static int
>> +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node,
>> + virDomainDeviceSpaprVioAddressPtr addr)
>> +{
>> + char *reg;
>> + int ret;
>> +
>> + memset(addr, 0, sizeof(*addr));
>> + addr->has_reg = false;
>> +
>> + reg = virXMLPropString(node, "reg");
>> + if (reg) {
>> + if (virStrToLong_ull(reg, NULL, 16,&addr->reg)< 0) {
>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Cannot parse<address> 'reg' attribute"));
>> + ret = -1;
>> + goto cleanup;
>> + }
>> +
>> + addr->has_reg = true;
>> + }
>> +
>> + ret = 0;
>> +cleanup:
>> + VIR_FREE(reg);
>> + return ret;
>> +}
>> +
>> +static int
>> virDomainDeviceUSBMasterParseXML(xmlNodePtr node,
>> virDomainDeviceUSBMasterPtr master)
>> {
>> @@ -2215,6 +2249,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
>> goto cleanup;
>> break;
>>
>> + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
>> + if
>> (virDomainDeviceSpaprVioAddressParseXML(address,&info->addr.spaprvio)< 0)
>> + goto cleanup;
>> + break;
>> +
>> default:
>> /* Should not happen */
>> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -3048,6 +3087,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>> }
>>
>> if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE&&
>> + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO&&
>> def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("Controllers must use the 'pci' address type"));
> Is this still the proper error message for this type of bus? You may
> want to look at def->os.arch / def->os.machine to see what is expected
> here in terms of bus and then have a more specific error message.

Will do.

>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 4439f55..b1f9260 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -69,6 +69,7 @@ enum virDomainDeviceAddressType {
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL,
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID,
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB,
>> + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO,
>>
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
>> };
>> @@ -121,6 +122,13 @@ struct _virDomainDeviceUSBAddress {
>> char *port;
>> };
>>
>> +typedef struct _virDomainDeviceSpaprVioAddress
>> virDomainDeviceSpaprVioAddress;
>> +typedef virDomainDeviceSpaprVioAddress
>> *virDomainDeviceSpaprVioAddressPtr;
>> +struct _virDomainDeviceSpaprVioAddress {
>> + unsigned long long reg;
>> + bool has_reg;
>> +};
>> +
>> enum virDomainControllerMaster {
>> VIR_DOMAIN_CONTROLLER_MASTER_NONE,
>> VIR_DOMAIN_CONTROLLER_MASTER_USB,
>> @@ -145,6 +153,7 @@ struct _virDomainDeviceInfo {
>> virDomainDeviceVirtioSerialAddress vioserial;
>> virDomainDeviceCcidAddress ccid;
>> virDomainDeviceUSBAddress usb;
>> + virDomainDeviceSpaprVioAddress spaprvio;
>> } addr;
>> int mastertype;
>> union {
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 57b25d6..0d03fbc 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1253,6 +1253,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>> qemuDomainPCIAddressSetPtr addrs)
>> def->controllers[i]->idx == 0)
>> continue;
>>
>> + if (def->controllers[i]->info.type ==
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO)
>> + continue;
>> if (def->controllers[i]->info.type !=
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>> continue;
>> if (qemuDomainPCIAddressSetNextAddr(addrs,&def->controllers[i]->info)< 0)
>> @@ -1403,6 +1405,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>> virBufferAsprintf(buf, ",bus=");
>> qemuUsbId(buf, info->addr.usb.bus);
>> virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port);
>> + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
>> + if (info->addr.spaprvio.has_reg)
>> + virBufferAsprintf(buf, ",reg=%#llx", info->addr.spaprvio.reg);
>> }
>>
>> return 0;
>> @@ -2087,7 +2092,8 @@
>> qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def,
>> }
>>
>> char *
>> -qemuBuildControllerDevStr(virDomainControllerDefPtr def,
>> +qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>> + virDomainControllerDefPtr def,
>> virBitmapPtr qemuCaps,
>> int *nusbcontroller)
>> {
>> @@ -2095,7 +2101,11 @@
>> qemuBuildControllerDevStr(virDomainControllerDefPtr def,
>>
>> switch (def->type) {
>> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>> - virBufferAddLit(&buf, "lsi");
>> + if (STREQ(domainDef->os.arch, "ppc64")&&
>> STREQ(domainDef->os.machine, "pseries")) {
>> + virBufferAddLit(&buf, "spapr-vscsi");
>> + } else {
>> + virBufferAddLit(&buf, "lsi");
>> + }
>> virBufferAsprintf(&buf, ",id=scsi%d", def->idx);
>> break;
>>
>> @@ -4009,7 +4019,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>> char *devstr;
>>
>> virCommandAddArg(cmd, "-device");
>> - if (!(devstr = qemuBuildControllerDevStr(cont, qemuCaps, NULL)))
>> + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, NULL)))
>> goto error;
>>
>> virCommandAddArg(cmd, devstr);
>> @@ -4028,7 +4038,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>> virCommandAddArg(cmd, "-device");
>>
>> char *devstr;
>> - if (!(devstr = qemuBuildControllerDevStr(def->controllers[i], qemuCaps,
>> + if (!(devstr = qemuBuildControllerDevStr(def, def->controllers[i],
>> qemuCaps,
>> &usbcontroller)))
>> goto error;
>>
>> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
>> index 1fe0394..3978b2b 100644
>> --- a/src/qemu/qemu_command.h
>> +++ b/src/qemu/qemu_command.h
>> @@ -95,7 +95,8 @@ char * qemuBuildDriveDevStr(virDomainDiskDefPtr disk,
>> char * qemuBuildFSDevStr(virDomainFSDefPtr fs,
>> virBitmapPtr qemuCaps);
>> /* Current, best practice */
>> -char * qemuBuildControllerDevStr(virDomainControllerDefPtr def,
>> +char * qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>> + virDomainControllerDefPtr def,
>> virBitmapPtr qemuCaps,
>> int *nusbcontroller);
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 96c0070..eabfeaa 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -329,7 +329,7 @@ int qemuDomainAttachPciControllerDevice(struct
>> qemud_driver *driver,
>> goto cleanup;
>> }
>>
>> - if (!(devstr = qemuBuildControllerDevStr(controller, priv->qemuCaps,
>> NULL))) {
>> + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller,
>> priv->qemuCaps, NULL))) {
>> goto cleanup;
>> }
>> }
> Otherwise looks good to me.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India