[libvirt] [PATCH] qemu: forbid user aliases for implicit pci-root controllers
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias. However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC) Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there. Forbid user aliases for the implicit pci-root controllers so that we retain this information. This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either. Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers. https://bugzilla.redhat.com/show_bug.cgi?id=1518148 --- While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line. For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions. src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66536653b..d3cd598c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6623,7 +6623,6 @@ virDomainDeviceAddressParseXML(xmlNodePtr address, } -#define USER_ALIAS_PREFIX "ua-" #define USER_ALIAS_CHARS \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-" @@ -6680,7 +6679,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) || (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_USER_ALIAS && - STRPREFIX(aliasStr, USER_ALIAS_PREFIX) && + STRPREFIX(aliasStr, VIR_DOMAIN_USER_ALIAS_PREFIX) && strspn(aliasStr, USER_ALIAS_CHARS) == strlen(aliasStr))) VIR_STEAL_PTR(info->alias, aliasStr); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index dabbff1c2..de34fb163 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,8 @@ # include "virtypedparam.h" # include "virsavecookie.h" +# define VIR_DOMAIN_USER_ALIAS_PREFIX "ua-" + /* forward declarations of all device types, required by * virDomainDeviceDef */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0eb591253..89e8b5350 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -321,16 +321,12 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, contIsPHB = virDomainControllerIsPSeriesPHB(cont); contTargetIndex = cont->opts.pciopts.targetIndex; - /* When domain has builtin pci-root controller we don't put it - * onto cmd line. Therefore we can't set its alias. In that - * case, use the default one. */ - if (!qemuDomainIsPSeries(domainDef) && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { - if (virQEMUCapsHasPCIMultiBus(qemuCaps, domainDef)) - contAlias = "pci.0"; - else - contAlias = "pci"; - } else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { + /* The builtin pcie-root controller is not put on the command line. + * We cannot set a user alias, but we can reliably predict the one + * qemu will use. + * For builtin pci-root controllers we cannot do that once a domain + * is started, but user aliases are forbidden for those. */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { contAlias = "pcie.0"; } else { contAlias = cont->info.alias; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9dab6b24b..ec33f3bad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3718,6 +3718,27 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int +qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + /* Forbid user aliases for implicit controllers where + * the qemu's device alias depends on the version and therefore + * we cannot figure it out for an already running domain. */ + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + !qemuDomainIsPSeries(def) && + controller->info.alias && + STRPREFIX(controller->info.alias, VIR_DOMAIN_USER_ALIAS_PREFIX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("user aliases are not available for " + "implicit pci-root controller")); + return -1; + } + + return 0; +} + + +static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, void *opaque ATTRIBUTE_UNUSED) @@ -3761,11 +3782,14 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, ret = qemuDomainDeviceDefValidateDisk(dev->data.disk); break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_MEMBALLOON: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml b/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml index c760098fe..d1cb8fea6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-user-aliases.xml @@ -74,9 +74,7 @@ <alias name='ua-SomeWeirdController'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='pci' index='0' model='pci-root'> - <alias name='ua-MyPCIRootController'/> - </controller> + <controller type='pci' index='0' model='pci-root'/> <controller type='ide' index='0'> <alias name='ua-DoesAnybodyStillUseIDE'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> -- 2.13.6
On 11/28/2017 02:51 PM, Ján Tomko wrote:
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias.
However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC)
Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there.
Forbid user aliases for the implicit pci-root controllers so that we retain this information.
This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either.
Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers.
https://bugzilla.redhat.com/show_bug.cgi?id=1518148 ---
While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line.
For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions.
src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-)
I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different results before and after libvirtd is restarted than it will bite us again in the future. While you're fixing one instance of the bug, the bug itself is still present. I suggest we extend our status XML to contain all the info needed so that we can make right decision regardless of how many times libvirtd is restarted. Michal
On Wed, Nov 29, 2017 at 11:57:30 +0100, Michal Privoznik wrote:
On 11/28/2017 02:51 PM, Ján Tomko wrote:
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias.
However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC)
Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there.
Forbid user aliases for the implicit pci-root controllers so that we retain this information.
This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either.
Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers.
https://bugzilla.redhat.com/show_bug.cgi?id=1518148 ---
While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line.
For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions.
src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-)
I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different results before and after libvirtd is restarted than it will bite us again in the future. While you're fixing one instance of the bug, the bug itself is still present. I suggest we extend our status XML to contain all the info needed so that we can make right decision regardless of how many times libvirtd is restarted.
Which would break any running domains, because they won't have the right info there... Jirka
On 11/29/2017 03:15 PM, Jiri Denemark wrote:
On Wed, Nov 29, 2017 at 11:57:30 +0100, Michal Privoznik wrote:
On 11/28/2017 02:51 PM, Ján Tomko wrote:
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias.
However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC)
Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there.
Forbid user aliases for the implicit pci-root controllers so that we retain this information.
This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either.
Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers.
https://bugzilla.redhat.com/show_bug.cgi?id=1518148 ---
While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line.
For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions.
src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-)
I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different results before and after libvirtd is restarted than it will bite us again in the future. While you're fixing one instance of the bug, the bug itself is still present. I suggest we extend our status XML to contain all the info needed so that we can make right decision regardless of how many times libvirtd is restarted.
Which would break any running domains, because they won't have the right info there...
Well, there's no good solution here that'd work 100%, is there. What we might do though is to forbid setting user aliases on those ancient qemu which lack the multibus capability. Michal
On Wed, Nov 29, 2017 at 04:27:16PM +0100, Michal Privoznik wrote:
On 11/29/2017 03:15 PM, Jiri Denemark wrote:
On Wed, Nov 29, 2017 at 11:57:30 +0100, Michal Privoznik wrote:
On 11/28/2017 02:51 PM, Ján Tomko wrote:
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias.
However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC)
Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there.
Forbid user aliases for the implicit pci-root controllers so that we retain this information.
This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either.
Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers.
https://bugzilla.redhat.com/show_bug.cgi?id=1518148 ---
While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line.
For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions.
src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-)
I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different results before and after libvirtd is restarted than it will bite us again in the future. While you're fixing one instance of the bug, the bug itself is still present. I suggest we extend our status XML to contain all the info needed so that we can make right decision regardless of how many times libvirtd is restarted.
Which would break any running domains, because they won't have the right info there...
Well, there's no good solution here that'd work 100%, is there. What we might do though is to forbid setting user aliases on those ancient qemu which lack the multibus capability.
Either way, commit 937f3195 needs to be completely or partially reverted before the release, since it breaks hotplug for domains that don't even use aliases. I do not particularly care whether the partial revert will be accompanied by breaking user aliases for implicit pci-root (as proposed here) or breaking user aliases for old QEMUs where HasPCIMultiBus is unfixable (proposed in: [PATCH 0/4] Fix hotplug after daemon restart) https://www.redhat.com/archives/libvir-list/2017-November/msg01151.html Jan
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/30/2017 02:46 PM, Ján Tomko wrote:
On Wed, Nov 29, 2017 at 04:27:16PM +0100, Michal Privoznik wrote:
On 11/29/2017 03:15 PM, Jiri Denemark wrote:
On Wed, Nov 29, 2017 at 11:57:30 +0100, Michal Privoznik wrote:
On 11/28/2017 02:51 PM, Ján Tomko wrote:
For implicit controllers, we do not format user aliases on the command line so we provide a translation between the user alias and the qemu alias.
However for pci-root controllers, the logic determining whether to use 'pci' or 'pci.0' depends on: * domain architecture * machine type * QEMU version (for PPC)
Since we do not store the QEMU version in status XML and as of commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS capability formatted there either, we can only rely on the alias of the controller formatted there.
Forbid user aliases for the implicit pci-root controllers so that we retain this information.
This allows us to drop the call to virQEMUCapsHasPCIMultiBus, and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus relies on qemuCaps->arch which is not filled out by reading the status XML either.
Partially reverts commit 937f3195 which added the user alias -> qemu alias mapping for implicit PCI controllers.
https://bugzilla.redhat.com/show_bug.cgi?id=1518148 ---
While we cannot reliably map user aliases to qemu aliases in every corner case, I am not sure where to draw the line.
For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead of qemuCaps->arch. So only PPC is unfixable, but this looked like the option with the least amount of exceptions.
src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 16 +++++-------- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml | 4 +--- 5 files changed, 35 insertions(+), 16 deletions(-)
I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different results before and after libvirtd is restarted than it will bite us again in the future. While you're fixing one instance of the bug, the bug itself is still present. I suggest we extend our status XML to contain all the info needed so that we can make right decision regardless of how many times libvirtd is restarted.
Which would break any running domains, because they won't have the right info there...
Well, there's no good solution here that'd work 100%, is there. What we might do though is to forbid setting user aliases on those ancient qemu which lack the multibus capability.
Either way, commit 937f3195 needs to be completely or partially reverted before the release, since it breaks hotplug for domains that don't even use aliases.
Okay. If you propose the patch I can review it. Michal
participants (3)
-
Jiri Denemark -
Ján Tomko -
Michal Privoznik