10:33 a.m.
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
10:33 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "domcaps: Treat host models as case-insensitive strings"
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(a)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
10:33 a.m.
New subject: [libvirt] [PATCH 2/3] util: Introduce virStringListSearch
The function performs a case insensitive search in a string list.
Signed-off-by: Jiri Denemark <jdenemar(a)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
10:33 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Adapt to changed ppc64 CPU model names
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(a)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
4:02 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Adapt to changed ppc64 CPU model names
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
8:46 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Adapt to changed ppc64 CPU model names
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
9:51 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Adapt to changed ppc64 CPU model names
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
7:12 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Adapt to changed ppc64 CPU model names
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
2029
days inactive
2538
days old
7 comments
2 participants
participants (2)
-
Andrea Bolognani -
Jiri Denemark