On Wed, Nov 02, 2016 at 10:22:30AM +0100, Jiri Denemark wrote:
> QEMU 2.8.0 adds support for unavailable-features in
> query-cpu-definitions reply. The unavailable-features array lists CPU
> features which prevent a corresponding CPU model from being usable on
> current host. It can only be used when all the unavailable features are
> disabled. Empty array means the CPU model can be used without
> modifications.
>
> We can use unavailable-features for providing CPU model usability info
> in domain capabilities XML:
>
> <domainCapabilities>
> ...
> <cpu>
> <mode name='host-passthrough' supported='yes'/>
> <mode name='host-model' supported='yes'>
> <model fallback='allow'>Skylake-Client</model>
> ...
> </mode>
> <mode name='custom' supported='yes'>
> <model usable='yes'>qemu64</model>
> <model usable='yes'>qemu32</model>
> <model usable='no'>phenom</model>
> <model usable='yes'>pentium3</model>
> <model usable='yes'>pentium2</model>
> <model usable='yes'>pentium</model>
> <model usable='yes'>n270</model>
> <model usable='yes'>kvm64</model>
> <model usable='yes'>kvm32</model>
> <model usable='yes'>coreduo</model>
> <model usable='yes'>core2duo</model>
> <model usable='no'>athlon</model>
> <model usable='yes'>Westmere</model>
> <model usable='yes'>Skylake-Client</model>
> ...
> </mode>
> </cpu>
> ...
> </domainCapabilities>
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 53 ++++++++++++++++++++++++++++++++++----------
> src/qemu/qemu_capabilities.h | 6 +++--
> src/qemu/qemu_monitor.h | 1 +
> src/qemu/qemu_monitor_json.c | 27 ++++++++++++++++------
> src/qemu/qemu_process.c | 2 +-
> tests/qemumonitorjsontest.c | 25 +++++++++++++++++----
> tests/qemuxml2argvtest.c | 12 ++++++----
> 7 files changed, 96 insertions(+), 30 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 7a8202a..9fce7a6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2306,7 +2306,8 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps)
> int
> virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
> const char **name,
> - size_t count)
> + size_t count,
> + virDomainCapsCPUUsable usable)
> {
> size_t i;
>
> @@ -2316,7 +2317,7 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>
> for (i = 0; i < count; i++) {
> if (virDomainCapsCPUModelsAdd(qemuCaps->cpuDefinitions, name[i], -1,
> - VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> + usable) < 0)
> return -1;
> }
>
> @@ -2327,10 +2328,12 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
> int
> virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> char ***names,
> - size_t *count)
> + size_t *count,
> + bool usable)
> {
> size_t i;
> char **models = NULL;
> + size_t n = 0;
>
> *count = 0;
> if (names)
> @@ -2344,17 +2347,21 @@ virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
>
> for (i = 0; i < qemuCaps->cpuDefinitions->nmodels; i++) {
> virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> - if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> - goto error;
> + if (!usable ||
> + cpu->usable == VIR_DOMCAPS_CPU_USABLE_YES) {
> + if (models && VIR_STRDUP(models[n], cpu->name) < 0)
> + goto error;
> + n++;
> + }
This function is called only on one place and the argument *usable* is always
false and it's not used in any of the following patches in this series so there
is no need to modify this function at all.
> }
>
> if (names)
> *names = models;
> - *count = qemuCaps->cpuDefinitions->nmodels;
> + *count = n;
> return 0;
>
> error:
> - virStringFreeListCount(models, i);
> + virStringFreeListCount(models, n);
> return -1;
> }
>
> @@ -2713,9 +2720,15 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps,
> goto cleanup;
>
> for (i = 0; i < ncpus; i++) {
> + virDomainCapsCPUUsable usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
> +
> + if (cpus[i]->usable == VIR_TRISTATE_BOOL_YES)
> + usable = VIR_DOMCAPS_CPU_USABLE_YES;
> + else if (cpus[i]->usable == VIR_TRISTATE_BOOL_NO)
> + usable = VIR_DOMCAPS_CPU_USABLE_NO;
> +
> if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
> - &cpus[i]->name,
> - VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> + &cpus[i]->name, usable) < 0)
> goto cleanup;
> }
>
> @@ -3128,6 +3141,17 @@ virQEMUCapsLoadCache(virCapsPtr caps,
> goto cleanup;
>
> for (i = 0; i < n; i++) {
> + int usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
> +
> + if ((str = virXMLPropString(nodes[i], "usable")) &&
> + (usable = virDomainCapsCPUUsableTypeFromString(str)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown value '%s' in attribute 'usable'"),
> + str);
> + goto cleanup;
> + }
> + VIR_FREE(str);
> +
> if (!(str = virXMLPropString(nodes[i], "name"))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("missing cpu name in QEMU capabilities cache"));
> @@ -3135,8 +3159,7 @@ virQEMUCapsLoadCache(virCapsPtr caps,
> }
>
> if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
> - &str,
> - VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> + &str, usable) < 0)
> goto cleanup;
> }
> }
> @@ -3301,7 +3324,13 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps,
> if (qemuCaps->cpuDefinitions) {
> for (i = 0; i < qemuCaps->cpuDefinitions->nmodels; i++) {
> virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> - virBufferEscapeString(&buf, "<cpu name='%s'/>\n", cpu->name);
> + virBufferEscapeString(&buf, "<cpu name='%s'", cpu->name);
> + if (cpu->usable) {
> + const char *usable;
> + usable = virDomainCapsCPUUsableTypeToString(cpu->usable);
> + virBufferAsprintf(&buf, " usable='%s'", usable);
> + }
> + virBufferAddLit(&buf, "/>\n");
> }
> }
>
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 6e7a855..c77ba13 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -428,10 +428,12 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps);
> unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps);
> int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
> const char **name,
> - size_t count);
> + size_t count,
> + virDomainCapsCPUUsable usable);
> int virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> char ***names,
> - size_t *count);
> + size_t *count,
> + bool usable);
> virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps);
> bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps,
> virCapsPtr caps,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index c3133c4..f81de68 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -913,6 +913,7 @@ typedef struct _qemuMonitorCPUDefInfo qemuMonitorCPUDefInfo;
> typedef qemuMonitorCPUDefInfo *qemuMonitorCPUDefInfoPtr;
>
> struct _qemuMonitorCPUDefInfo {
> + virTristateBool usable;
> char *name;
> };
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 6c13832..e2dfb34 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4925,13 +4925,8 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> goto cleanup;
>
> - if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("query-cpu-definitions reply was missing return data"));
> - goto cleanup;
> - }
> -
> - if ((n = virJSONValueArraySize(data)) < 0) {
> + if (!(data = virJSONValueObjectGetArray(reply, "return")) ||
> + (n = virJSONValueArraySize(data)) < 0) {
This change is unrelated, it would be probably better to put in into separate
patch.
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("query-cpu-definitions reply data was not an array"));
> goto cleanup;
> @@ -4958,6 +4953,24 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>
> if (VIR_STRDUP(cpu->name, tmp) < 0)
> goto cleanup;
> +
> + if (virJSONValueObjectHasKey(child, "unavailable-features")) {
> + virJSONValuePtr blockers;
> +
> + blockers = virJSONValueObjectGetArray(child,
> + "unavailable-features");
> + if (!blockers) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("unavailable-features in query-cpu-definitions "
> + "reply data was not an array"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueArraySize(blockers) > 0)
> + cpu->usable = VIR_TRISTATE_BOOL_NO;
> + else
> + cpu->usable = VIR_TRISTATE_BOOL_YES;
> + }
> }
>
> ret = n;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1b67aee..50fb3de 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5057,7 +5057,7 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,
> virQEMUCapsGetHostModel(qemuCaps)) < 0)
> goto cleanup;
>
> - if (virQEMUCapsGetCPUDefinitions(qemuCaps, &models, &nmodels) < 0 ||
> + if (virQEMUCapsGetCPUDefinitions(qemuCaps, &models, &nmodels, false) < 0 ||
> virCPUTranslate(def->os.arch, def->cpu, models, nmodels) < 0)
> goto cleanup;
>
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index d8fd958..7111bc8 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -432,10 +432,12 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data)
> " \"name\": \"qemu64\" "
> " }, "
> " { "
> - " \"name\": \"Opteron_G4\" "
> + " \"name\": \"Opteron_G4\", "
> + " \"unavailable-features\": [\"vme\"]"
> " }, "
> " { "
> - " \"name\": \"Westmere\" "
> + " \"name\": \"Westmere\", "
> + " \"unavailable-features\": []"
> " } "
> " ]"
> "}") < 0)
> @@ -452,6 +454,13 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data)
> }
>
> #define CHECK(i, wantname) \
> + CHECK_FULL(i, wantname, VIR_TRISTATE_BOOL_ABSENT)
> +
> +#define CHECK_USABLE(i, wantname, usable) \
> + CHECK_FULL(i, wantname, \
> + usable ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO)
> +
The ordering of macros is strange. The above macros uses CHECK_FULL so I would
rather move them after the CHECK_FULL macro.
ACK with the issues fixed.
Pavel
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list