[libvirt] [PATCH v5 00/12] qemu: Validate PCI controller options

[libvirt] [PATCH] util: mdev:...

[libvirt] [PATCH] qemu: Fix memory...

Andrea Bolognani

Friday, 2 March 2018 Fri, 2 Mar '18

9:13 a.m.

Applies cleanly on top of 328b8dbe8bee9939c7108fdec4fda05fd02511f6. Changes from [v4]: * patch 1/12 is new; * use virReportEnumRangeError(), as suggested by laine. Changes from [v3]: * don't introduce new test cases that won't be able to provide full test coverage anyway, as suggested by laine. Changes from [v2]: * replace the old implementation bit by bit using a clever trick suggested by pkrempa; * don't move QEMU capability validation; * add a default: label to all switch statements as recommended by danpb. Changes from [v1]: * error out instead of silently accept invalid options; * shave quite a lot of yaks. [v4] https://www.redhat.com/archives/libvir-list/2018-February/msg01232.html [v3] https://www.redhat.com/archives/libvir-list/2018-February/msg00996.html [v2] https://www.redhat.com/archives/libvir-list/2018-February/msg00813.html [v1] https://www.redhat.com/archives/libvir-list/2018-February/msg00244.html Andrea Bolognani (12): conf: Assign explicit value to VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE qemu: Create new qemuDomainDeviceDefValidateControllerPCI() qemu: Validate PCI controller options (modelName) qemu: Validate PCI controller options (index) qemu: Validate PCI controller options (targetIndex) qemu: Validate PCI controller options (pcihole64) qemu: Validate PCI controller options (busNr) qemu: Validate PCI controller options (numaNode) qemu: Validate PCI controller options (chassisNr) qemu: Validate PCI controller options (chassis and port) qemu: Validate PCI controllers (QEMU capabilities) qemu: Remove old qemuDomainDeviceDefValidateControllerPCI() src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 571 ++++++++++++++++++------- tests/qemuxml2argvdata/pcie-expander-bus.xml | 3 - tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 4 +- 4 files changed, 418 insertions(+), 162 deletions(-) -- 2.14.3

Show replies by date

Andrea Bolognani

Friday, 2 March Fri, 2 Mar

9:13 a.m.

New subject: [libvirt] [PATCH v5 01/12] conf: Assign explicit value to VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE

Pretty much any reasonable compiler would do this automatically, but there's no harm in being explicit about it. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/conf/domain_conf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 368f16f3fb..a04f96169c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -700,7 +700,7 @@ typedef enum { } virDomainControllerModelPCI; typedef enum { - VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE, + VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE = 0, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE, VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420, -- 2.14.3

Laine Stump

12:31 p.m.

New subject: [libvirt] [PATCH v5 01/12] conf: Assign explicit value to VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

I'm not sure why you want it, but don't see any harm (if it's for consistency, there are many other enums that don't explicitly list the first item as 0). Reviewed-by: Laine Stump <laine(a)laine.org>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

The esisting function is renamed and called from the new one, so that even while we're in the process of implementing new checks all the existing ones will be performed. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b4efc82de..a45a676520 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4268,9 +4268,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; @@ -4547,6 +4547,29 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle } +static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) + +{ + const virDomainPCIControllerOpts *pciopts = &cont->opts.pciopts; + const char *model = virDomainControllerModelPCITypeToString(cont->model); + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + + if (!model) { + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + if (!modelName) { + virReportEnumRangeError(virDomainControllerPCIModelName, pciopts->modelName); + return -1; + } + + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); +} + + static int qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef *controller, const virDomainDef *def, -- 2.14.3

Laine Stump

12:32 p.m.

New subject: [libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

Sure. Seems as reasonable a way as any to add the new and remove the old without ending up with confusing diffs. Reviewed-by: Laine Stump <laine(a)laine.org>

Laine Stump

4:09 p.m.

New subject: [libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

The esisting function is renamed and called from the new one, so

Forgot to mention this before: s/esisting/existing/

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 229 +++++++++++++++++++++++++++++++------------------ 1 file changed, 145 insertions(+), 84 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a45a676520..13c2b557fb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; /* skip pcie-root */ if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) @@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro } pciopts = &controller->opts.pciopts; - if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) { - if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("autogenerated %s options not set"), - virDomainControllerModelPCITypeToString(controller->model)); - return -1; - } - - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown %s modelName value %d"), - virDomainControllerModelPCITypeToString(controller->model), - pciopts->modelName); - return -1; - } - } /* Second pass - now the model specific checks */ switch (model) { @@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro return -1; } - if (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); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pci-bridge controller is not supported " @@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro return -1; } - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pci-expander-bus"), - modelName); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb controller is not supported in this " @@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (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); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the dmi-to-pci-bridge (i82801b11-bridge) " @@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro return -1; } - if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-root-port"), - modelName); - return -1; - } - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4434,14 +4382,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-switch-upstream-port"), - modelName); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pcie-switch-upstream-port (x3130-upstream) " @@ -4459,14 +4399,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro return -1; } - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-switch-downstream-port"), - modelName); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("The pcie-switch-downstream-port " @@ -4484,14 +4416,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro return -1; } - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-expander-bus"), - modelName); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb-pcie controller is not supported " @@ -4512,14 +4436,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro if (pciopts->targetIndex == 0) return 0; - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pci-root"), - modelName); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the spapr-pci-host-bridge controller is not " @@ -4566,6 +4482,151 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* modelName */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + /* modelName should have been set automatically */ + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "modelName", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* modelName must be set for pSeries guests, but it's an error + * for it to be set for any other guest */ + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "modelName", model); + return -1; + } + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "modelName", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "modelName", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + + /* modelName (cont'd) */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-root"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-bridge"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "dmi-to-pci-bridge"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-switch-upstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-switch-downstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

3:16 p.m.

New subject: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

I'm undecided if it's useful to have a special function for this (since it's going to be used so many times), or if it's just as efficient to spell it out every time (since the compiler will combine all the uses of the same literal string into a single instance of the string in memory / in the translations). maybe virReportControllerMissingOption(model, option) - less characters to type, and simpler to change the wording for all occurrences if we decide to, but maybe less informative when reading the code.). As an example of how we might decide to change the exact message - I think it might be better if it was "Required option '%s' not set for '%s' controller". Hmm. Although, I see from at least one example that there will need to be variations of the message - in the case of targetIndex, for example, it is only required if the pci-root controller is an spapr-pci-host-bridge - the error in that case really should have that info (or maybe the index of the controller would be even better).

...

+ return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* modelName must be set for pSeries guests, but it's an error + * for it to be set for any other guest */ + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "modelName", model); + return -1; + } + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + !qemuDomainIsPSeries(def)) {

micro-optimization - you could structure this as: if (qemuDomainIsPSeries(def) { if (pciopts->modelName == .....NONE) { BigFatGreekError(); return -1; } } else { if (pciopts->modelName != ....NONE) { BigFatGreekError(); return -1; } } so that qemuDomainIsPSeries() was only called once.

...

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "modelName", model);

Maybe a helper function for this error too. (virReportInvalidControllerModelName(model, modelName)?) Again, I'm undecided if having a separate function (actually a #defined macro so that function/line number info would be correct in the log) would be useful for maintenance, or just extra work for nothing. > + return -1; > + } > + break; > + > + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: > + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {

...

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "modelName", model);

> + return -1; > + } > + break; > + > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: > + default: > + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); > + return -1; > + } > + > + /* modelName (cont'd) */ > + switch ((virDomainControllerModelPCI) cont->model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: > + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && > + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Model name '%s' is not valid for '%s' controller"), > + modelName, "pci-root"); Why hardcode the model instead of just using the string that has already been set with that value?

...

+ return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-bridge");

same. etc etc.

...

+ return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "dmi-to-pci-bridge"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-switch-upstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-switch-downstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); }

I'm not sure I see the advantage of grouping the validation for a single attribute on several different controller models together, rather than the current code's grouping validation for all attributes on a single controller together. Potato-potahto. It does mean that if you're hand tracing the code you need to go through the entire function rather than just one section. On the other hand, I guess it does mean that many instances of "attribute X isn't valid for Y controller" can be combined.

Andrea Bolognani

Monday, 5 March Mon, 5 Mar

9:40 a.m.

New subject: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

On Fri, 2018-03-02 at 16:16 -0500, Laine Stump wrote: [...]

...

> + /* modelName */ > + switch ((virDomainControllerModelPCI) cont->model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: > + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: > + /* modelName should have been set automatically */ > + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Option '%s' not set for '%s' controller"), > + "modelName", model); I'm undecided if it's useful to have a special function for this (since it's going to be used so many times), or if it's just as efficient to spell it out every time (since the compiler will combine all the uses of the same literal string into a single instance of the string in memory / in the translations). maybe virReportControllerMissingOption(model, option) - less characters to type, and simpler to change the wording for all occurrences if we decide to, but maybe less informative when reading the code.). As an example of how we might decide to change the exact message - I think it might be better if it was "Required option '%s' not set for '%s' controller". Hmm. Although, I see from at least one example that there will need to be variations of the message - in the case of targetIndex, for example, it is only required if the pci-root controller is an spapr-pci-host-bridge - the error in that case really should have that info (or maybe the index of the controller would be even better).

...

> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Option '%s' is not valid for '%s' controller"), > + "modelName", model); Maybe a helper function for this error too. (virReportInvalidControllerModelName(model, modelName)?) Again, I'm undecided if having a separate function (actually a #defined macro so that function/line number info would be correct in the log) would be useful for maintenance, or just extra work for nothing.

I've given it a try and it makes things quite a bit nicer. Showing the controller index is also very useful, as some controllers can show up multiple times in a guest. A couple error messages might have gotten slightly less informative as a result, but I'd say it's acceptable as long as there's enough context to help the user look it up in the documentation. [...]

...

> + /* modelName (cont'd) */ > + switch ((virDomainControllerModelPCI) cont->model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: > + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && > + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Model name '%s' is not valid for '%s' controller"), > + modelName, "pci-root"); Why hardcode the model instead of just using the string that has already been set with that value?

Just a temporary brain fart: I've gotten it right in subsequent patches, and I've now fixed it here as well.

...

Yeah, structuring the code this way allows to provide better error messages (eg. spelling out which specific option is missing) without resulting in an endless amount of repetition; it also makes adding checks for all options when introducing a new controller model more difficult to forget, though unfortunately it doesn't help when adding a new option instead. -- Andrea Bolognani / Red Hat / Virtualization

Andrea Bolognani

Friday, 2 March Fri, 2 Mar

9:13 a.m.

New subject: [libvirt] [PATCH v5 04/12] qemu: Validate PCI controller options (index)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 68 +++++++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 13c2b557fb..3ef5d74e7a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4285,31 +4285,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) return 0; - /* First pass - just check the controller index for the model's - * that we care to check... */ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (controller->idx == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("index for pci controllers of model '%s' must be > 0"), - virDomainControllerModelPCITypeToString(model)); - return -1; - } - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - break; - } - pciopts = &controller->opts.pciopts; /* Second pass - now the model specific checks */ @@ -4627,6 +4602,49 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* index */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (cont->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controllers must be > 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pSeries guests can have multiple PHBs, so it's expected that + * the index will not be zero for some of them */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + break; + } + + /* For all other pci-root and pcie-root controllers, though, + * the index must be zero*/ + if (cont->idx != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controllers must be 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

3:25 p.m.

New subject: [libvirt] [PATCH v5 04/12] qemu: Validate PCI controller options (index)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

Reviewed-by: Laine Stump <laine(a)laine.org>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 05/12] qemu: Validate PCI controller options (targetIndex)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 52 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3ef5d74e7a..54e47acd99 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4401,12 +4401,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - if (pciopts->targetIndex == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-root options not set")); - return -1; - } - /* Skip the implicit one */ if (pciopts->targetIndex == 0) return 0; @@ -4645,6 +4639,52 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* targetIndex */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* PHBs for pSeries guests must have been assigned a targetIndex */ + if (pciopts->targetIndex == -1 && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "targetIndex", model); + return -1; + } + + /* targetIndex only applies to PHBs, so for any other pci-root + * controller it being present is an error */ + if (pciopts->targetIndex != -1 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->targetIndex != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

4:08 p.m.

New subject: [libvirt] [PATCH v5 05/12] qemu: Validate PCI controller options (targetIndex)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

This is the option I was talking about in the review for 03/12. As it is, the message is going to say: Option 'targetIndex' not set for 'pci-root' controller It would be better if it were something like Required option 'targetIndex' not set for 'spapr-pci-host-bridge' controller (Maybe there's a way to work in 'pci-root' to the message too. It's too late on Friday afternoon for me to try and figure it out). Otherwise it fits with the pattern of everything else, so Reviewed-by: Laine Stump <laine(a)laine.org>

...

+ return -1; + } + + /* targetIndex only applies to PHBs, so for any other pci-root + * controller it being present is an error */ + if (pciopts->targetIndex != -1 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->targetIndex != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); }

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 06/12] qemu: Validate PCI controller options (pcihole64)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 54e47acd99..9b8d2e864d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4685,6 +4685,46 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* pcihole64 */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* The pcihole64 option only applies to x86 guests */ + if ((pciopts->pcihole64 || + pciopts->pcihole64size != 0) && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "on '%s' architecture or '%s' machine type"), + "pcihole64", model, + virArchToString(def->os.arch), def->os.machine); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->pcihole64 || + pciopts->pcihole64size != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "pcihole64", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

4:22 p.m.

New subject: [libvirt] [PATCH v5 06/12] qemu: Validate PCI controller options (pcihole64)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

...

--- src/qemu/qemu_domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 54e47acd99..9b8d2e864d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4685,6 +4685,46 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* pcihole64 */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* The pcihole64 option only applies to x86 guests */ + if ((pciopts->pcihole64 || + pciopts->pcihole64size != 0) && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "on '%s' architecture or '%s' machine type"), + "pcihole64", model, + virArchToString(def->os.arch), def->os.machine); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->pcihole64 || + pciopts->pcihole64size != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "pcihole64", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); }

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 07/12] qemu: Validate PCI controller options (busNr)

This change catches an invalid use of the option in our test suite. https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 46 +++++++++++++++++++------- tests/qemuxml2argvdata/pcie-expander-bus.xml | 2 +- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 2 +- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b8d2e864d..1354d9850a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4306,12 +4306,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-expander-bus options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb controller is not supported in this " @@ -4385,12 +4379,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-expander-bus options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb-pcie controller is not supported " @@ -4725,6 +4713,40 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* busNr */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->busNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->busNr != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index ac01c26ccf..237430a1e5 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,7 +35,7 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target busNr='220'> + <target> <node>1</node> </target> <address type='pci' bus='0x01'/> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index aaac423cac..d5e741b80d 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,7 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0' busNr='220'> + <target chassis='2' port='0x0'> <node>1</node> </target> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> -- 2.14.3

Laine Stump

4:24 p.m.

New subject: [libvirt] [PATCH v5 07/12] qemu: Validate PCI controller options (busNr)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

This change catches an invalid use of the option in our test suite. https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

...

--- src/qemu/qemu_domain.c | 46 +++++++++++++++++++------- tests/qemuxml2argvdata/pcie-expander-bus.xml | 2 +- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 2 +- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b8d2e864d..1354d9850a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4306,12 +4306,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-expander-bus options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb controller is not supported in this " @@ -4385,12 +4379,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-expander-bus options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb-pcie controller is not supported " @@ -4725,6 +4713,40 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* busNr */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->busNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->busNr != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index ac01c26ccf..237430a1e5 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,7 +35,7 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target busNr='220'> + <target> <node>1</node> </target> <address type='pci' bus='0x01'/> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index aaac423cac..d5e741b80d 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,7 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0' busNr='220'> + <target chassis='2' port='0x0'> <node>1</node> </target> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 08/12] qemu: Validate PCI controller options (numaNode)

This change catches an invalid use of the option in our test suite. https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 52 ++++++++++++++++++++++++++ tests/qemuxml2argvdata/pcie-expander-bus.xml | 3 -- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 4 +- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1354d9850a..07ed006f70 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4747,6 +4747,58 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* numaNode */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + /* numaNode can be used for these controllers, but it's not set + * automatically so it can be missing */ + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* Only PHBs support numaNode */ + if (pciopts->numaNode != -1 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "numaNode", model); + return -1; + } + + /* However, the default PHB doesn't support numaNode */ + if (pciopts->numaNode != -1 && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "with '%s' equal to 0"), + "numaNode", model, + "targetIndex"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->numaNode != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "numaNode", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index 237430a1e5..5c5d34d1e0 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,9 +35,6 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target> - <node>1</node> - </target> <address type='pci' bus='0x01'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index d5e741b80d..b6498fd2eb 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,9 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0'> - <node>1</node> - </target> + <target chassis='2' port='0x0'/> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'> -- 2.14.3

Laine Stump

4:32 p.m.

New subject: [libvirt] [PATCH v5 08/12] qemu: Validate PCI controller options (numaNode)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

This change catches an invalid use of the option in our test suite. https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

...

--- src/qemu/qemu_domain.c | 52 ++++++++++++++++++++++++++ tests/qemuxml2argvdata/pcie-expander-bus.xml | 3 -- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 4 +- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1354d9850a..07ed006f70 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4747,6 +4747,58 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* numaNode */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + /* numaNode can be used for these controllers, but it's not set + * automatically so it can be missing */ + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* Only PHBs support numaNode */ + if (pciopts->numaNode != -1 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "numaNode", model); + return -1; + } + + /* However, the default PHB doesn't support numaNode */ + if (pciopts->numaNode != -1 && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "with '%s' equal to 0"), + "numaNode", model, + "targetIndex"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->numaNode != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "numaNode", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index 237430a1e5..5c5d34d1e0 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,9 +35,6 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target> - <node>1</node> - </target> <address type='pci' bus='0x01'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index d5e741b80d..b6498fd2eb 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,9 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0'> - <node>1</node> - </target> + <target chassis='2' port='0x0'/> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 09/12] qemu: Validate PCI controller options (chassisNr)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 07ed006f70..d4ac4e2038 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4290,12 +4290,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro /* Second pass - now the model specific checks */ switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (pciopts->chassisNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-bridge options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pci-bridge controller is not supported " @@ -4799,6 +4793,40 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* chassisNr */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->chassisNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "chassisNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->chassisNr != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "chassisNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + return -1; + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

5:34 p.m.

New subject: [libvirt] [PATCH v5 09/12] qemu: Validate PCI controller options (chassisNr)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 10/12] qemu: Validate PCI controller options (chassis and port)

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 58 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d4ac4e2038..f54e7b87ae 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4320,12 +4320,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if (pciopts->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-root-port options not set")); - return -1; - } - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4355,13 +4349,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (pciopts->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-switch-downstream-port " - "options not set")); - return -1; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("The pcie-switch-downstream-port " @@ -4827,6 +4814,51 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, return -1; } + /* chassis and port */ + switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->chassis == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "chassis", model); + return -1; + } + if (pciopts->port == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "port", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->chassis != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "chassis", model); + return -1; + } + if (pciopts->port != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "port", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + default: + virReportEnumRangeError(virDomainControllerModelPCI, cont->model); + } + return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

5:35 p.m.

New subject: [libvirt] [PATCH v5 10/12] qemu: Validate PCI controller options (chassis and port)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1483816 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

Andrea Bolognani

9:13 a.m.

New subject: [libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 190 ++++++++++++++++++++----------------------------- 1 file changed, 77 insertions(+), 113 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f54e7b87ae..e0ab43e139 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4270,11 +4270,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *controller, const virDomainDef *def, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) { - virDomainControllerModelPCI model = controller->model; - const virDomainPCIControllerOpts *pciopts; - /* skip pcie-root */ if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) return 0; @@ -4285,119 +4282,50 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) return 0; - pciopts = &controller->opts.pciopts; - - /* Second pass - now the model specific checks */ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_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")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb controller is not supported in this " - "QEMU binary")); - return -1; - } - - 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")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (ioh3420) controller " - "is not supported in this QEMU binary")); - return -1; - } - - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (pcie-root-port) controller " - "is not supported in this QEMU binary")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-switch-upstream-port (x3130-upstream) " - "controller is not supported in this QEMU binary")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The pcie-switch-downstream-port " - "(xio3130-downstream) controller is not " - "supported in this QEMU binary")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb-pcie controller is not supported " - "in this QEMU binary")); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - /* Skip the implicit one */ - if (pciopts->targetIndex == 0) - return 0; - - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller is not " - "supported in this QEMU binary")); - return -1; - } - - if (pciopts->numaNode != -1 && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller doesn't " - "support numa_node in this QEMU binary")); - return -1; - } + return 0; +} - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: - break; +/** + * virDomainControllerPCIModelNameToQEMUCaps: + * @modelName: model name + * + * Maps model names for PCI controllers (virDomainControllerPCIModelName) + * to the QEMU capabilities required to use them (virQEMUCapsFlags). + * + * Returns: the QEMU capability itself (>0) on success; 0 if no QEMU + * capability is needed; <0 on error. + */ +static int +virDomainControllerPCIModelNameToQEMUCaps(int modelName) +{ + switch ((virDomainControllerPCIModelName) modelName) { + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE: + return QEMU_CAPS_DEVICE_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE: + return QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420: + return QEMU_CAPS_DEVICE_IOH3420; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM: + return QEMU_CAPS_DEVICE_X3130_UPSTREAM; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM: + return QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB: + return QEMU_CAPS_DEVICE_PXB; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE: + return QEMU_CAPS_DEVICE_PXB_PCIE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT: + return QEMU_CAPS_DEVICE_PCIE_ROOT_PORT; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE: + return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE: + return 0; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST: + default: + return 1; } - return 0; + return -1; } @@ -4410,6 +4338,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, const virDomainPCIControllerOpts *pciopts = &cont->opts.pciopts; const char *model = virDomainControllerModelPCITypeToString(cont->model); const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + int cap = virDomainControllerPCIModelNameToQEMUCaps(pciopts->modelName); if (!model) { virReportEnumRangeError(virDomainControllerModelPCI, cont->model); @@ -4859,6 +4788,41 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, virReportEnumRangeError(virDomainControllerModelPCI, cont->model); } + /* The default PHB for pSeries guests doesn't need any QEMU capability + * to be used, so we should skip the capabilities check altogether */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0) { + goto done; + } + + /* QEMU device availability */ + if (cap < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown QEMU device for '%s' controller"), + modelName); + return -1; + } + if (cap > 0 && !virQEMUCapsGet(qemuCaps, cap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' device is not supported by this QEMU binary"), + modelName); + return -1; + } + + /* PHBs didn't support numaNode from the very beginning, so an extra + * capability check is required */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->numaNode != -1 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not supported by '%s' device with this QEMU binary"), + "numaNode", modelName); + return -1; + } + + done: return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); } -- 2.14.3

Laine Stump

5:44 p.m.

New subject: [libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

What's the deal with "1"?

...

} - return 0; + return -1; } @@ -4410,6 +4338,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, const virDomainPCIControllerOpts *pciopts = &cont->opts.pciopts; const char *model = virDomainControllerModelPCITypeToString(cont->model); const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + int cap = virDomainControllerPCIModelNameToQEMUCaps(pciopts->modelName); if (!model) { virReportEnumRangeError(virDomainControllerModelPCI, cont->model); @@ -4859,6 +4788,41 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, virReportEnumRangeError(virDomainControllerModelPCI, cont->model); } + /* The default PHB for pSeries guests doesn't need any QEMU capability + * to be used, so we should skip the capabilities check altogether */

... and the check would fail if we didn't skip? If so, then okay. Depending on the answers to the two questions above: Conditional-Reviewed-by: Laine Stump <laine(a)laine.org> :-)

...

+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0) { + goto done; + } + + /* QEMU device availability */ + if (cap < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown QEMU device for '%s' controller"), + modelName); + return -1; + } + if (cap > 0 && !virQEMUCapsGet(qemuCaps, cap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' device is not supported by this QEMU binary"), + modelName); + return -1; + } + + /* PHBs didn't support numaNode from the very beginning, so an extra + * capability check is required */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->numaNode != -1 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not supported by '%s' device with this QEMU binary"), + "numaNode", modelName); + return -1; + } + + done: return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); }

Andrea Bolognani

Monday, 5 March Mon, 5 Mar

9:58 a.m.

New subject: [libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

On Fri, 2018-03-02 at 18:44 -0500, Laine Stump wrote: [...]

...

> +static int > +virDomainControllerPCIModelNameToQEMUCaps(int modelName) > +{ > + switch ((virDomainControllerPCIModelName) modelName) { > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE: > + return QEMU_CAPS_DEVICE_PCI_BRIDGE; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE: > + return QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420: > + return QEMU_CAPS_DEVICE_IOH3420; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM: > + return QEMU_CAPS_DEVICE_X3130_UPSTREAM; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM: > + return QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB: > + return QEMU_CAPS_DEVICE_PXB; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE: > + return QEMU_CAPS_DEVICE_PXB_PCIE; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT: > + return QEMU_CAPS_DEVICE_PCIE_ROOT_PORT; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE: > + return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE: > + return 0; > + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST: > + default: > + return 1; What's the deal with "1"?

Should have been -1, of course :) [...]

...

> + /* The default PHB for pSeries guests doesn't need any QEMU capability > + * to be used, so we should skip the capabilities check altogether */ ... and the check would fail if we didn't skip?

Correct: virDomainControllerPCIModelNameToQEMUCaps() would return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, and the check below would fail. I'm actually fairly confident we could drop that hunk with no ill effect, because the spapr-pci-host-bridge must have existed even before we started checking for it, even if it might possibly have been, at some point, not user-instantiable; however, the capabilities data for ppc64 in our test suite doesn't go far back enough to prove this is the case, so special-casing it seems like the safest option. I'll try to see if I can build QEMU 1.2.0 (that's the earliest we claim support for, right?) on ppc64 with a reasonable amount of work, and if my hunch is correct I'll post a follow-up cleanup patch that gets rid of it. -- Andrea Bolognani / Red Hat / Virtualization

Andrea Bolognani

Wednesday, 7 March Wed, 7 Mar

10:12 a.m.

New subject: [libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

On Mon, 2018-03-05 at 16:58 +0100, Andrea Bolognani wrote:

...

I'm actually fairly confident we could drop that hunk with no ill effect, because the spapr-pci-host-bridge must have existed even before we started checking for it, even if it might possibly have been, at some point, not user-instantiable; however, the capabilities data for ppc64 in our test suite doesn't go far back enough to prove this is the case, so special-casing it seems like the safest option. I'll try to see if I can build QEMU 1.2.0 (that's the earliest we claim support for, right?) on ppc64 with a reasonable amount of work, and if my hunch is correct I'll post a follow-up cleanup patch that gets rid of it.

It's actually 0.12, not 1.2.0. Whoops. According to my research, the pSeries machine type was introduced in 0.15 but didn't have PCI support until 1.0, by which point the spapr-pci-host-bridge device had been introduced too. tl;dr the hunk in question can safely be dropped. I'll respin. -- Andrea Bolognani / Red Hat / Virtualization

Andrea Bolognani

Friday, 2 March Fri, 2 Mar

9:13 a.m.

New subject: [libvirt] [PATCH v5 12/12] qemu: Remove old qemuDomainDeviceDefValidateControllerPCI()

We've implemented all existing checks, and more, in the new function, so we can finally drop the old one. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/qemu/qemu_domain.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e0ab43e139..735341ae16 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4267,25 +4267,6 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll } -static int -qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *controller, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) -{ - /* skip pcie-root */ - if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) - return 0; - - /* Skip pci-root, except for pSeries guests (which actually - * support more than one PCI Host Bridge per guest) */ - if (!qemuDomainIsPSeries(def) && - controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) - return 0; - - return 0; -} - - /** * virDomainControllerPCIModelNameToQEMUCaps: * @modelName: model name @@ -4823,7 +4804,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont, } done: - return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps); + return 0; } -- 2.14.3

Laine Stump

5:45 p.m.

New subject: [libvirt] [PATCH v5 12/12] qemu: Remove old qemuDomainDeviceDefValidateControllerPCI()

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:

...

We've implemented all existing checks, and more, in the new function, so we can finally drop the old one. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>

Reviewed-by: Laine Stump <laine(a)laine.org>

2488

days inactive

2493

days old

devel@lists.libvirt.org

Manage subscription

28 comments

2 participants

tags (0)

participants (2)

Andrea Bolognani
Laine Stump

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v5 00/12] qemu: Validate PCI controller options