[libvirt] [PATCH 0/3] Fix ppc64 CPU configuration for QEMU 2.11+
The original fix was both incomplete and too general. It only fixed domain startup, but libvirt would still report empty list of supported CPU models with recent QEMU for ppc64. On the other hand, while ppc64 QEMU ignores case when looking up CPU model names, x86_64 QEMU does case sensitive lookup. Jiri Denemark (3): Revert "domcaps: Treat host models as case-insensitive strings" util: Introduce virStringListSearch qemu: Adapt to changed ppc64 CPU model names src/conf/domain_capabilities.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 28 +++++++++++++++++-- src/qemu/qemu_capabilities.h | 3 +- src/qemu/qemu_process.c | 2 +- src/util/virstring.c | 28 +++++++++++++++++++ src/util/virstring.h | 3 ++ .../qemu_2.12.0.ppc64.xml | 6 +++- .../caps_2.12.0.ppc64.xml | 12 ++++---- 9 files changed, 73 insertions(+), 12 deletions(-) -- 2.17.0
This reverts commit 2d8721e2606806164782028ecf1ee33a9bbaa8fe. This fix was both incomplete and too general. It only fixed domain startup, but libvirt would still report empty list of supported CPU models with recent QEMU for ppc64. On the other hand, while ppc64 QEMU ignores case when looking up CPU model names, x86_64 QEMU does case sensitive lookup. Without reverting this patch, libvirt could happily accept CPU model names which are not supported by QEMU. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 6e2ab0a287..5f82ad2c10 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -264,7 +264,7 @@ virDomainCapsCPUModelsGet(virDomainCapsCPUModelsPtr cpuModels, return NULL; for (i = 0; i < cpuModels->nmodels; i++) { - if (STRCASEEQ(cpuModels->models[i].name, name)) + if (STREQ(cpuModels->models[i].name, name)) return cpuModels->models + i; } -- 2.17.0
The function performs a case insensitive search in a string list. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 28 ++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ 3 files changed, 32 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3dece252df..9448bca596 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2868,6 +2868,7 @@ virStringListJoin; virStringListLength; virStringListMerge; virStringListRemove; +virStringListSearch; virStringMatch; virStringParsePort; virStringReplace; diff --git a/src/util/virstring.c b/src/util/virstring.c index 15f367af7c..17c3282043 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1499,3 +1499,31 @@ virStringParsePort(const char *str, return 0; } + + +/** + * virStringListSearch: + * @strings: string list + * @needle: string to be searched for + * + * Searches for @needle in @strings ignoring case. See virStringListHasString + * for case sensitive search. + * + * Returns the matching string in @strings or NULL if not found. + */ +const char * +virStringListSearch(const char **strings, + const char *needle) +{ + size_t i; + + if (!strings) + return NULL; + + for (i = 0; strings[i]; i++) { + if (STRCASEEQ(strings[i], needle)) + return strings[i]; + } + + return NULL; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index fa2ec1df4d..90afd244f6 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -309,4 +309,7 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +const char *virStringListSearch(const char **strings, + const char *needle); + #endif /* __VIR_STRING_H__ */ -- 2.17.0
QEMU 2.11 for ppc64 changed all CPU model names to lower case. Since libvirt can't change the model names for compatibility reasons, we need to translate the matching lower case models to the names known by libvirt. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 28 +++++++++++++++++-- src/qemu/qemu_capabilities.h | 3 +- src/qemu/qemu_process.c | 2 +- .../qemu_2.12.0.ppc64.xml | 6 +++- .../caps_2.12.0.ppc64.xml | 12 ++++---- 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a5cb24fec6..9fee5dcd8a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2217,16 +2217,39 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, virDomainCapsCPUModelsPtr -virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon) +virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon, + virArch arch) { virDomainCapsCPUModelsPtr models = NULL; qemuMonitorCPUDefInfoPtr *cpus = NULL; + char **libvirtModels = NULL; int ncpus = 0; size_t i; if ((ncpus = qemuMonitorGetCPUDefinitions(mon, &cpus)) < 0) return NULL; + if (ARCH_IS_PPC(arch)) { + /* QEMU 2.11 for Power renamed all CPU models to lower case, we need + * to translate them back to libvirt's upper case model names. */ + if (virCPUGetModels(arch, &libvirtModels) < 0) + goto error; + + if (virStringListLength((const char **)libvirtModels) > 0) { + for (i = 0; i < ncpus; i++) { + const char *name; + + if (!(name = virStringListSearch((const char **)libvirtModels, + cpus[i]->name))) + continue; + + VIR_FREE(cpus[i]->name); + if (VIR_STRDUP(cpus[i]->name, name) < 0) + goto error; + } + } + } + if (!(models = virDomainCapsCPUModelsNew(ncpus))) goto error; @@ -2247,6 +2270,7 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon) for (i = 0; i < ncpus; i++) qemuMonitorCPUDefInfoFree(cpus[i]); VIR_FREE(cpus); + virStringListFree(libvirtModels); return models; error: @@ -2266,7 +2290,7 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps, if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS)) return 0; - if (!(models = virQEMUCapsFetchCPUDefinitions(mon))) + if (!(models = virQEMUCapsFetchCPUDefinitions(mon, qemuCaps->arch))) return -1; if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d23c34c24d..3ab108a667 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -513,7 +513,8 @@ int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainCapsCPUUsable usable); virDomainCapsCPUModelsPtr virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainVirtType type); -virDomainCapsCPUModelsPtr virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon); +virDomainCapsCPUModelsPtr virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon, + virArch arch); typedef enum { /* Host CPU definition reported in domain capabilities. */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5b73a61962..d09ff7fb87 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4127,7 +4127,7 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto error; - models = virQEMUCapsFetchCPUDefinitions(priv->mon); + models = virQEMUCapsFetchCPUDefinitions(priv->mon, vm->def->os.arch); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error; diff --git a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml index 5fac2ed772..1762417ee2 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -25,7 +25,11 @@ <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode> - <mode name='custom' supported='no'/> + <mode name='custom' supported='yes'> + <model usable='unknown'>POWER9</model> + <model usable='unknown'>POWER8</model> + <model usable='unknown'>POWER7</model> + </mode> </cpu> <devices> <disk supported='yes'> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index bffe3b3b97..144b8883a2 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -171,12 +171,12 @@ <cpu type='kvm' name='970mp'/> <cpu type='kvm' name='970fx'/> <cpu type='kvm' name='970'/> - <cpu type='kvm' name='power9'/> + <cpu type='kvm' name='POWER9'/> <cpu type='kvm' name='power8nvl'/> - <cpu type='kvm' name='power8'/> + <cpu type='kvm' name='POWER8'/> <cpu type='kvm' name='power8e'/> <cpu type='kvm' name='power7+'/> - <cpu type='kvm' name='power7'/> + <cpu type='kvm' name='POWER7'/> <cpu type='kvm' name='power5gs'/> <cpu type='kvm' name='power5+'/> <cpu type='kvm' name='apollo7pm'/> @@ -609,12 +609,12 @@ <cpu type='tcg' name='970mp'/> <cpu type='tcg' name='970fx'/> <cpu type='tcg' name='970'/> - <cpu type='tcg' name='power9'/> + <cpu type='tcg' name='POWER9'/> <cpu type='tcg' name='power8nvl'/> - <cpu type='tcg' name='power8'/> + <cpu type='tcg' name='POWER8'/> <cpu type='tcg' name='power8e'/> <cpu type='tcg' name='power7+'/> - <cpu type='tcg' name='power7'/> + <cpu type='tcg' name='POWER7'/> <cpu type='tcg' name='power5gs'/> <cpu type='tcg' name='power5+'/> <cpu type='tcg' name='apollo7pm'/> -- 2.17.0
On Thu, 2018-05-17 at 17:33 +0200, Jiri Denemark wrote: [...]
--- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -25,7 +25,11 @@ <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode>
This is quite suspicious - it looks like a proper CPU model, but it's really a compatibility mode, so it should be lowercase rather than uppercase. You certainly won't be able to use <cpu mode='host-model> <model>POWER8</model> </cpu> so why are we advertising the uppercase variant here? Am I missing something?
- <mode name='custom' supported='no'/> + <mode name='custom' supported='yes'> + <model usable='unknown'>POWER9</model> + <model usable='unknown'>POWER8</model> + <model usable='unknown'>POWER7</model> + </mode>
This is of course an improvement, but I'm not sure we want to keep exposing uppercase model names to users. I understand we need to keep accepting them for compatibility reasons, but since QEMU has moved to lowercase CPU model names wouldn't it make sense for libvirt to follow suit? Doing so would have the interesting side effect of making the whole mess with compat modes somewhat sane, at least when it comes to not having two entirely separate set of names differing only in case. Then again, I might just be missing some very obvious issues preventing us from using lowercase names :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, May 22, 2018 at 11:02:17 +0200, Andrea Bolognani wrote:
On Thu, 2018-05-17 at 17:33 +0200, Jiri Denemark wrote: [...]
--- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -25,7 +25,11 @@ <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode>
This is quite suspicious - it looks like a proper CPU model, but it's really a compatibility mode, so it should be lowercase rather than uppercase. You certainly won't be able to use
<cpu mode='host-model> <model>POWER8</model> </cpu>
so why are we advertising the uppercase variant here? Am I missing something?
Hmm, you're right. In general, this is advertising the host CPU (ideally as seen by QEMU), which doesn't really work for ppc since host-model was misused for compatibility modes. I think we'll have to add a special hack to produce <mode name='host-model' supported='yes'/> without showing any CPU model. Ideally, we would somehow list all supported compatibility modes, but this can be left for the future.
- <mode name='custom' supported='no'/> + <mode name='custom' supported='yes'> + <model usable='unknown'>POWER9</model> + <model usable='unknown'>POWER8</model> + <model usable='unknown'>POWER7</model> + </mode>
This is of course an improvement, but I'm not sure we want to keep exposing uppercase model names to users.
I understand we need to keep accepting them for compatibility reasons, but since QEMU has moved to lowercase CPU model names wouldn't it make sense for libvirt to follow suit?
I don't think so. Introducing new aliases (i.e., lower case variants) for the existing models would IMHO cause more troubles than having a mixture of upper case and lower case names (once something like power10 is introduced). Users or apps would have to use a crystal ball to check which CPU model name would be compatible with older libvirt. Jirka
On Tue, 2018-05-22 at 15:46 +0200, Jiri Denemark wrote:
On Tue, May 22, 2018 at 11:02:17 +0200, Andrea Bolognani wrote:
On Thu, 2018-05-17 at 17:33 +0200, Jiri Denemark wrote: [...]
--- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -25,7 +25,11 @@ <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode>
This is quite suspicious - it looks like a proper CPU model, but it's really a compatibility mode, so it should be lowercase rather than uppercase. You certainly won't be able to use
<cpu mode='host-model> <model>POWER8</model> </cpu>
so why are we advertising the uppercase variant here? Am I missing something?
Hmm, you're right. In general, this is advertising the host CPU (ideally as seen by QEMU), which doesn't really work for ppc since host-model was misused for compatibility modes. I think we'll have to add a special hack to produce <mode name='host-model' supported='yes'/> without showing any CPU model. Ideally, we would somehow list all supported compatibility modes, but this can be left for the future.
Sounds good.
- <mode name='custom' supported='no'/> + <mode name='custom' supported='yes'> + <model usable='unknown'>POWER9</model> + <model usable='unknown'>POWER8</model> + <model usable='unknown'>POWER7</model> + </mode>
This is of course an improvement, but I'm not sure we want to keep exposing uppercase model names to users.
I understand we need to keep accepting them for compatibility reasons, but since QEMU has moved to lowercase CPU model names wouldn't it make sense for libvirt to follow suit?
I don't think so. Introducing new aliases (i.e., lower case variants) for the existing models would IMHO cause more troubles than having a mixture of upper case and lower case names (once something like power10 is introduced). Users or apps would have to use a crystal ball to check which CPU model name would be compatible with older libvirt.
You have a point. The current situation is a bit confusing, again because of the misuse of host-model, but it's probably better to stick with the confusing situation we've grown used to rather than change things around for cosmetic reasons. Plus, it's already strongly recommended to use <cpu mode='host-model'> <model>power8</model> </cpu> rather than <cpu mode='custom'> <model>POWER8</model> </cpu> because the resulting QEMU command line is more idiomatic, so applications and users sticking with the best practices wouldn't benefit from the change either way. I disagree on having a mixture of uppercase and lowercase model, though: that's just bad UI, and a clear violation of the principle of least surprise; if and when a 'power10' CPU model will be added to QEMU, we should introduce a suitable 'POWER10' alias along with the existing ones. -- Andrea Bolognani / Red Hat / Virtualization
Resurrecting an old forgotten series... It should fix PPC64 issues with my recent "qemu: Store default CPU in domain XML" patches. On Tue, May 22, 2018 at 16:51:44 +0200, Andrea Bolognani wrote:
On Tue, 2018-05-22 at 15:46 +0200, Jiri Denemark wrote:
On Tue, May 22, 2018 at 11:02:17 +0200, Andrea Bolognani wrote:
On Thu, 2018-05-17 at 17:33 +0200, Jiri Denemark wrote: [...]
--- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml @@ -25,7 +25,11 @@ <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode>
This is quite suspicious - it looks like a proper CPU model, but it's really a compatibility mode, so it should be lowercase rather than uppercase. You certainly won't be able to use
<cpu mode='host-model> <model>POWER8</model> </cpu>
so why are we advertising the uppercase variant here? Am I missing something?
Actually the current way of reporting host-model in domain capabilities for ppc64 is not incorrect given the way domain capabilities are documented. <mode name='host-model' supported='yes'> <model fallback='allow'>POWER8</model> </mode> means <cpu mode='host-model'> CPU definition can be used in domain XML and the CPU model corresponding to the host CPU is POWER8. This is not supposed to be translated to <cpu mode='host-model'> <model>POWER8</model> </cpu> The interpretation of the domain capabilities snippet is either <cpu mode='host-model'/> or <cpu mode='custom' match='exact'> <model>POWER8</model> </cpu> And both will work, although the first one will not do what a user would expect due to the way host-model is misused for ppc. There's just no way of reporting this misuse in domain capabilities now. Perhaps we will come up with a way to solve this in the future. But we can stick with the current state now. ...
You have a point. The current situation is a bit confusing, again because of the misuse of host-model, but it's probably better to stick with the confusing situation we've grown used to rather than change things around for cosmetic reasons.
Plus, it's already strongly recommended to use
<cpu mode='host-model'> <model>power8</model> </cpu>
rather than
<cpu mode='custom'> <model>POWER8</model> </cpu>
because the resulting QEMU command line is more idiomatic, so applications and users sticking with the best practices wouldn't benefit from the change either way.
I disagree on having a mixture of uppercase and lowercase model, though: that's just bad UI, and a clear violation of the principle of least surprise; if and when a 'power10' CPU model will be added to QEMU, we should introduce a suitable 'POWER10' alias along with the existing ones.
OK, we can revisit this discussion later when a new power CPU model is introduced. I'll rebase this series on current master and resend it. Jirka
participants (2)
-
Andrea Bolognani -
Jiri Denemark