On Wed, 20 Oct 2021, Laine Stump wrote:
> On 10/18/21 12:31 AM, Ani Sinha wrote:
> > Error messages must conform to spec as specified here:
> > https://www.libvirt.org/coding-style.html#error-message-format
> >
> > This change encloses format specifiers in quotes and unbreaks error
> > messages.
> >
> > Fixes: 8eadf82fb5 ("conf: introduce option to enable/disable pci hotplug on
> > pci-root controller")
> > Fixes: 7300ccc9b3 ("conf: introduce support for acpi-bridge-hotplug
> > feature")
> >
> > Signed-off-by: Ani Sinha <a...@anisinha.ca>
>
> Hi Ani,
>
> Thanks for following up on your contributions even after they've been pushed
> :-)
>
> The parts of this patch that are relevent to the pci-root hotplug look fine
> and can be pushed.
>
I will send out a separate patch.
> But the QEMU people have discovered problems with the
> "acpi-pci-hotplug-with-bridge-support" switch used by libvirt's
> <acpi-bridge-hotplug> switch that is forcing them to rethink not just their
> implementation/design.
>
> Basically the way that the QEMU switch works (setting it off will enable
> native and disable ACPI hotplug, while setting it on will disable native and
> enable ACPI), while making logical sense from the outside, is "confusing" some
> guests - that's as much detail as I have, but it means that most likely that
> QEMU switch will be scrapped and one or more new (and possibly different)
> switches will be added in their place; it all seems to be up in the air right
> now.
Hmm. sadly I am not part of the discussion and I have no visibility
either. I guess I need to find a job with redhat :-)
Anyway, since we don't want to have code in libvirt for a QEMU switch
> that didn't work correctly in the beginning and was then replaced, and since
> it's highly likely that the new QEMU hotplug-selection method will work
> differently (and anyway isn't known now), our best course of action seems to
> be reverting all the acpi-bridge-hotplug patches for now - this is still
> possible since there hasn't been an official release since they were added.
>
> I've already made the revert patches (for the original 4 of the feature, plus
> 6 patches from pkrempa that fixed up test cases and such) and will be posting
> them in the morning with a (hopefully) slightly better explanation.
Sad. That was lot of effort seems to have gone waste ...
>
> I hope you don't find this too discouraging - just keep in mind that the
> reason for reverting isn't your code, but rather is because the QEMU feature
> your code is using turns out to not work as advertised, and if the libvirt
> code that is using it makes it into a release, then we will have to support it
> essentially forever.
>
> (NB: it's also completely possible (but looks to be very unlikely) that QEMU
> will figure out a way to solve their problems without modifying this switch,
> and we'll be able to simply re-push the reverted commits; we can't be sure of
> that right now though).
>
> More in the morning...
>
> > ---
> > src/conf/domain_conf.c | 6 ++----
> > src/qemu/qemu_validate.c | 6 +++---
> > .../pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err | 2 +-
> > .../pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err | 2 +-
> > 4 files changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 6fcf86ba58..d5de07d13d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -17557,8 +17557,7 @@ virDomainFeaturesPCIDefParse(virDomainDef *def,
> > feature = virDomainPCITypeFromString((const char *)node->name);
> > if (feature < 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("unsupported PCI feature: %s"),
> > - node->name);
> > + _("unsupported PCI feature: '%s'"), node->name);
> > return -1;
> > }
> > @@ -21833,8 +21832,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef
> > *src,
> > case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG:
> > if (src->pci_features[i] != dst->pci_features[i]) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("State of PCI feature '%s' differs: "
> > - "source: '%s', destination: '%s'"),
> > + _("State of PCI feature '%s' differs:
> > source: '%s', destination: '%s'"),
> > virDomainPCITypeToString(i),
> >
> > virTristateSwitchTypeToString(src->pci_features[i]),
> >
> > virTristateSwitchTypeToString(dst->pci_features[i]));
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index 3045e4b64b..f93b697265 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -3947,7 +3947,7 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > virDomainControllerDef *cont,
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > if (!virQEMUCapsGet(qemuCaps,
> > QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG)) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("setting the %s property on a '%s' device
> > is not supported by this QEMU binary"),
> > + _("setting the '%s' property on a '%s'
> > device is not supported by this QEMU binary"),
> > "hotplug", "pci-root");
> > return -1;
> > }
> > @@ -3956,8 +3956,8 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > virDomainControllerDef *cont,
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> > if (!virQEMUCapsGet(qemuCaps,
> > QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("setting the hotplug property on a '%s'
> > device is not supported by this QEMU binary"),
> > - modelName);
> > + _("setting the '%s' property on a '%s'
> > device is not supported by this QEMU binary"),
> > + "hotplug", modelName);
> > return -1;
> > }
> > break;
> > diff --git
> > a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
> > index b507f1f8bc..55ec41c476 100644
> > ---
> > a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
> > +++
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
> > @@ -1 +1 @@
> > -unsupported configuration: setting the hotplug property on a 'pci-root'
> > device is not supported by this QEMU binary
> > +unsupported configuration: setting the 'hotplug' property on a 'pci-root'
> > device is not supported by this QEMU binary
> > diff --git
> > a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
> > index b507f1f8bc..55ec41c476 100644
> > ---
> > a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
> > +++
> > b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
> > @@ -1 +1 @@
> > -unsupported configuration: setting the hotplug property on a 'pci-root'
> > device is not supported by this QEMU binary
> > +unsupported configuration: setting the 'hotplug' property on a 'pci-root'
> > device is not supported by this QEMU binary
> >
>
>
