On 07/22/2015 03:54 PM, John Ferlan wrote:
On 07/17/2015 02:43 PM, Laine Stump wrote:
This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):
1) pci-bridge - sets <model> type attribute default as "pci-bridge"
2) dmi-to-pci-bridge - sets <model> type attribute default as "i82801b11-bridge".
These both match current hardcoded practice.
The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added. This is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.
qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. --- new in V2 (previously was a part of the patch to add pcie-root-port)
src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 64 insertions(+), 6 deletions(-)
Without trying to follow all the discussion from v1 of the series, I am assuming this methodology has been "agreed" upon... Probably should have noted that in the previous patch ;-)
Yes, the end of the discussion is that <model type='blah'/> <target blah='blurg' .../> is the least ugly of the available options.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f02f5..8868e18 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options; + const char *deviceName = NULL;
if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;
addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (!options->type) + deviceName = "pci-bridge"; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (!options->type) + deviceName = "i82801b11-bridge"; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + if (deviceName && + VIR_STRDUP(options->type, deviceName) < 0) + goto cleanup; + As has been told to me before - virStrdup/VIR_STRDUP is "tolerant" of deviceName == NULL returning 0 if it's NULL, so just:
if (VIR_STRDUP(options->type, deviceName) < 0) goto cleanup;
is necessary
But that would set options->type to NULL in the case that deviceName hadn't been set, and we don't want that. Consider for a moment that options->type had already been explicitly set to "pci-bridge". Since options->type != NULL, deviceName would remain NULL, we would get to the VIR_STRDUP() and unconditionally set options->type to NULL, losing the value that we actually wanted (and leaking it to boot). Hmm. I guess my logic *is* a bit backwards though :-) Instead I should say "if (!options->type && VIR_STRDUP(blah)...)"
/* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4614,17 +4640,49 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "pci-bridge")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown pci-bridge device '%s'"),
unknown pci-bridge option model '%s' ?
Yes, that sounds better.
+ def->opts.pciopts.type); + goto error; + } Alternatively
if (STREQ_NULLABLE() { } else { error message using NULLSTR(def->opts.pciopts.type)
I don't care either way - it's obvious
I kind of like having the "autogenerated ..." error message because it makes it clearer that libvirt was supposed to add something and didn't.
+ virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + def->opts.pciopts.type, def->idx, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The dmi-to-pci-bridge (i82801b11-bridge) " - "controller is not supported in this QEMU binary")); + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "i82801b11-bridge")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the dmi-to-pci-bridge (i82801b11-bridge) " + "controller is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown dmi-to-pci-bridge device '%s'"), unknown dmi-to-pci-bridge option model '%s' ??
+ def->opts.pciopts.type); goto error; } Same here - again - doesn't really matter
ACK with the VIR_STRDUP() cleanup - whether you adjust the error message or the if/else conditions is your call.
John
- virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias); + virBufferAsprintf(&buf, "%s,id=%s", + def->opts.pciopts.type, def->info.alias); break; } break;