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

[Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>]

On 11/29/2011 10:46 PM, Stefan Berger wrote:
> On 11/29/2011 10:03 AM, Prerna Saxena wrote:
> > From: Michael Ellerman<michael@xxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Prerna Saxena<prerna@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list