[I reduced the Cc list so I don't need to hear jtomko's whining again] On Sat, Jul 25, 2015 at 03:58:26PM -0400, 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> name attribute default as "pci-bridge"
2) dmi-to-pci-bridge - sets <model> name 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, and that hasn't been done at
the time the PostParse callbacks are being
run. qemuDomainAssignPCIAddresses() 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.
---
Changes from V2: use enum instead of string model name.
src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 09f30c4..6a19d15 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
virDomainControllerDefPtr cont = def->controllers[i];
int idx = cont->idx;
virDevicePCIAddressPtr addr;
+ virDomainPCIControllerOptsPtr options;
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->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
+ options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE;
+ break;
+ case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+ if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
+ options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_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;
+ }
+
/* check if every PCI bridge controller's ID is greater than
* the bus it is placed onto
*/
@@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
int model = def->model;
+ const char *modelName = NULL;
if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0)
@@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
}
switch (def->model) {
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
- virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
- def->idx, def->info.alias);
+ if (def->opts.pciopts.modelName
+ == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("autogenerated pci-bridge options not set"));
+ goto error;
+ }
+
+ modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
+ if (!modelName) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown pci-bridge model name value %d"),
+ def->opts.pciopts.modelName);
+ goto error;
+ }
+ if (def->opts.pciopts.modelName
+ != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("PCI controller model name '%s' "
+ "is not valid for a pci-bridge"),
+ modelName);
+ goto error;
+ }
+ 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;
+ }
+ virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
+ modelName, def->idx, def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+ if (def->opts.pciopts.modelName
+ == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("autogenerated dmi-to-pci-bridge options not set"));
+ goto error;
+ }
+
+ modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
+ if (!modelName) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown dmi-to-pci-bridge model name value %d"),
+ def->opts.pciopts.modelName);
+ goto error;
+ }
+ if (def->opts.pciopts.modelName
+ != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("PCI controller model name '%s' "
+ "is not valid for a dmi-to-pci-bridge"),
+ modelName);
+ goto error;
+ }
I see why you didn't care whether it will be implemented this way or the other. That's because you're checking for stuff we usually leave the parsing to handle. I haven't checked that there is a possibility of having domain parsed without assigning PCI addresses, but the def->opts.pciopts.modelName cannot be invalid in this case. Anyway, being _too_ careful cannot hurt, no need to change it unless you want to.
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("The dmi-to-pci-bridge (i82801b11-bridge) "
+ _("the dmi-to-pci-bridge (i82801b11-bridge) "
"controller is not supported in this QEMU binary"));
goto error;
}
You could also save a bunch of lines using what I suggested, that is
concentrating device->caps mapping in a separate function and using
one error message for all models, not one for every model, e.g.:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("the '%s' ('%s') controller is not supported "
"in this QEMU binary"),
blahTypeToString(def->model),
blehTypeToString(def->opts.pciopts.modelName));
but that can be done later on, the code works properly as it is now.
ACK
- virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias);
+ virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias);
break;
}
break;
--
2.1.0
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list