From: Michael Ellerman <michael(a)ellerman.id.au&gt; On the PPC64 pseries machine type we need to use the spapr-vscsi device rather than an lsi. Signed-off-by: Michael Ellerman <michael(a)ellerman.id.au&gt; --- src/qemu/qemu_command.c | 13 +++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 032ead1..ca812de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2118,7 +2118,8 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, } char * -qemuBuildControllerDevStr(virDomainControllerDefPtr def, +qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller) { @@ -2126,7 +2127,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; @@ -4040,7 +4045,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); @@ -4059,7 +4064,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; } } -- 1.7.7.3

From: Michael Ellerman <michaele(a)au1.ibm.com&gt; 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. Signed-off-by: Michael Ellerman <michael(a)ellerman.id.au&gt; --- src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 5 +++++ 3 files changed, 56 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5de33b9..665a0cd 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, @@ -1909,6 +1910,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); @@ -2168,6 +2174,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) { @@ -2280,6 +2314,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, @@ -3174,6 +3213,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")); @@ -3563,6 +3603,7 @@ virDomainNetDefParseXML(virCapsPtr caps, /* XXX what about ISA/USB based NIC models - once we support * them we should make sure address type is correct */ 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", _("Network interfaces must use 'pci' address type")); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d6ed898..0ed6aba 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 eaef0e1..17cc972 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; -- 1.7.7.3

On Fri, 2011-12-09 at 14:49 -0700, Eric Blake wrote: Sorry that's a bit sloppy of me. They are all my addresses (obviously), I use michael(a)ellerman.id.au as my canonical address. I'm not sure why the mails are coming from michael(a)ozlabs.org, they didn't use to and I haven't had time to debug it yet. Ah sorry. We actually have some patches internally to do that but I didn't write them so I didn't send them. They look similar to your changes below so I'll sort that out for v2. OK I'll have a look at it and see if I can come up with something. No it can't be used as a controller. There's just a single instance of the bus with a flat 64-bit address space. OK, I haven't looked at the help doco yet, I'll have a look, and let me know what you want done there. It is usually specified in hex, but that's just a stylistic convention. I guess it's better to switch to "0x%llx" though just for 100% consistency. No they can take any value. 4K is just "neater". No 0 is a valid address. Which is a pity because has_reg is a bit ugly. But I think it's better than picking 0 or some other value as a magic "unassigned" value - that might come back to bite us. Thanks, I'll incorporate that into v2. cheers

On 12/07/2011 11:41 PM, Michael Ellerman wrote: Depends on patch 4, but I'll review anyway... The cast isn't strictly necessary in C. As I asked in 4, can a valid reg ever be 0, or must it be non-zero? If the latter, then you don't need has_reg. Looks reasonable at either honoring the user's choice, or at iterating by 0x1000 until a free slot is found. Does that match the qemu algorithm? Again, I assume these three defaults match qemu. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/07/2011 11:41 PM, Michael Ellerman wrote: Thanks for contributing! I ran out of time to fully review this today, but at least this one looks promising. I'm not sure if we need to tweak our RNG grammar to also allow this in the XML validation. I'll check that out tomorrow, when I get around to applying this one and reviewing the rest of the series. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/08/2011 05:38 PM, Eric Blake wrote: Hmm - an O(n) function call on an O(n) loop, for a quadratic action (of course, it's in the noise, since the user's model name is likely short). But we can do better with a more efficient search for bogus bytes (strspn is O(n), if implemented well in libc). It turns out the XML didn't do any validation at all. Here's what I came up with - tightening the RNG and relaxing the domain_conf code, so that they now match. Since the concept is the same as yours, I went ahead and pushed it, but I claimed authorship on this one, since I practically rewrote it. diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index b8fbcf9..27ec1da 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -1276,7 +1276,11 @@ </optional> <optional> <element name="model"> - <attribute name="type"/> + <attribute name="type"> + <data type="string"> + <param name='pattern'>[a-zA-Z0-9\-_]+</param> + </data> + </attribute> <empty/> </element> </optional> diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 5de33b9..f3ec777 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -3385,6 +3385,9 @@ error: return ret; } +#define NET_MODEL_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" + /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure @@ -3699,15 +3702,13 @@ virDomainNetDefParseXML(virCapsPtr caps, * reasonable, not that it is a supported NIC type. FWIW kvm * supports these types as of April 2008: * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio + * QEMU PPC64 supports spapr-vlan */ if (model != NULL) { - int i; - for (i = 0 ; i < strlen(model) ; i++) { - if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') { - virDomainReportError(VIR_ERR_INVALID_ARG, "%s", - _("Model name contains invalid characters")); - goto error; - } + if (strspn(model, NET_MODEL_CHARS) < strlen(model)) { + virDomainReportError(VIR_ERR_INVALID_ARG, "%s", + _("Model name contains invalid characters")); + goto error; } def->model = model; model = NULL; -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org