Re: [libvirt] [PATCH v3] Add flag to BaselineCPU API to return detailed CPU features

Currently the virConnectBaselineCPU API does not expose the CPU features that are part of the CPU's model. This patch adds a new flag, VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, that causes the API to explictly list all features that are part of that model. Signed-off-by: Don Dugger <donald.d.dugger(a)intel.com&gt; --- [V2 - per review comments change the flag name to be more descriptive and change errors from warnings to propogated errors.] [V3 - Incorporate review suggestions to better handle flags and errors. Also add documentation about the API change.] include/libvirt/libvirt.h.in | 9 ++++++ src/cpu/cpu.c | 12 ++++--- src/cpu/cpu.h | 12 ++++--- src/cpu/cpu_arm.c | 6 +++- src/cpu/cpu_generic.c | 5 ++- src/cpu/cpu_powerpc.c | 10 ++++-- src/cpu/cpu_s390.c | 6 +++- src/cpu/cpu_x86.c | 45 ++++++++++++++++++++++++--- src/libvirt.c | 7 ++++- src/qemu/qemu_driver.c | 4 +-- tests/cputest.c | 30 +++++++++--------- tests/cputestdata/x86-baseline-3-result.xml | 35 +++++++++++++++++++++ tests/cputestdata/x86-baseline-3.xml | 7 +++++ tools/virsh-domain.c | 10 +++++- tools/virsh.pod | 7 +++-- 15 files changed, 166 insertions(+), 39 deletions(-) create mode 100644 tests/cputestdata/x86-baseline-3-result.xml create mode 100644 tests/cputestdata/x86-baseline-3.xml diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7bd3559..b843355 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4008,6 +4008,15 @@ int virConnectCompareCPU(virConnectPtr conn, /** + * virConnectBaselineCPUFlags + * + * Flags when getting XML description of a computed CPU + */ +typedef enum { + VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES = (1 << 0), /* show all features */ +} virConnectBaselineCPUFlags; + +/** * virConnectBaselineCPU: * * @conn: virConnect connection diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 4124354..023ce26 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -167,7 +167,7 @@ cpuDecode(virCPUDefPtr cpu, return -1; } - return driver->decode(cpu, data, models, nmodels, preferred); + return driver->decode(cpu, data, models, nmodels, preferred, 0); } @@ -276,7 +276,8 @@ char * cpuBaselineXML(const char **xmlCPUs, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) { xmlDocPtr doc = NULL; xmlXPathContextPtr ctxt = NULL; @@ -323,7 +324,7 @@ cpuBaselineXML(const char **xmlCPUs, doc = NULL; } - if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels))) + if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels, flags))) goto error; cpustr = virCPUDefFormat(cpu, 0); @@ -350,7 +351,8 @@ virCPUDefPtr cpuBaseline(virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) { struct cpuArchDriver *driver; size_t i; @@ -392,7 +394,7 @@ cpuBaseline(virCPUDefPtr *cpus, return NULL; } - return driver->baseline(cpus, ncpus, models, nmodels); + return driver->baseline(cpus, ncpus, models, nmodels, flags); } diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 4003435..7f1d4bd 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -53,7 +53,8 @@ typedef int const virCPUDataPtr data, const char **models, unsigned int nmodels, - const char *preferred); + const char *preferred, + unsigned int flags); typedef int (*cpuArchEncode) (virArch arch, @@ -81,7 +82,8 @@ typedef virCPUDefPtr (*cpuArchBaseline) (virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels); + unsigned int nmodels, + unsigned int flags); typedef int (*cpuArchUpdate) (virCPUDefPtr guest, @@ -149,13 +151,15 @@ extern char * cpuBaselineXML(const char **xmlCPUs, unsigned int ncpus, const char **models, - unsigned int nmodels); + unsigned int nmodels, + unsigned int flags); extern virCPUDefPtr cpuBaseline (virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels); + unsigned int nmodels, + unsigned int flags); extern int cpuUpdate (virCPUDefPtr guest, diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c index 25e25ba..d1b2a99 100644 --- a/src/cpu/cpu_arm.c +++ b/src/cpu/cpu_arm.c @@ -44,8 +44,12 @@ ArmDecode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, const virCPUDataPtr data ATTRIBUTE_UNUSED, const char **models ATTRIBUTE_UNUSED, unsigned int nmodels ATTRIBUTE_UNUSED, - const char *preferred ATTRIBUTE_UNUSED) + const char *preferred ATTRIBUTE_UNUSED, + unsigned int flags) { + + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); + return 0; } diff --git a/src/cpu/cpu_generic.c b/src/cpu/cpu_generic.c index 2fe792f..1264da4 100644 --- a/src/cpu/cpu_generic.c +++ b/src/cpu/cpu_generic.c @@ -113,7 +113,8 @@ static virCPUDefPtr genericBaseline(virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) { virCPUDefPtr cpu = NULL; virCPUFeatureDefPtr features = NULL; @@ -121,6 +122,8 @@ genericBaseline(virCPUDefPtr *cpus, unsigned int count; size_t i, j; + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL); + if (!cpuModelIsAllowed(cpus[0]->model, models, nmodels)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU model %s is not supported by hypervisor"), diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 55a4153..647a8a1 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -304,12 +304,15 @@ ppcDecode(virCPUDefPtr cpu, const virCPUDataPtr data, const char **models, unsigned int nmodels, - const char *preferred ATTRIBUTE_UNUSED) + const char *preferred ATTRIBUTE_UNUSED, + unsigned int flags) { int ret = -1; struct ppc_map *map; const struct ppc_model *model; + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); + if (data == NULL || (map = ppcLoadMap()) == NULL) return -1; @@ -377,7 +380,8 @@ static virCPUDefPtr ppcBaseline(virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) { struct ppc_map *map = NULL; const struct ppc_model *model; @@ -385,6 +389,8 @@ ppcBaseline(virCPUDefPtr *cpus, virCPUDefPtr cpu = NULL; size_t i; + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL); + if (!(map = ppcLoadMap())) goto error; diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c index cbfae42..d997e06 100644 --- a/src/cpu/cpu_s390.c +++ b/src/cpu/cpu_s390.c @@ -48,8 +48,12 @@ s390Decode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, const virCPUDataPtr data ATTRIBUTE_UNUSED, const char **models ATTRIBUTE_UNUSED, unsigned int nmodels ATTRIBUTE_UNUSED, - const char *preferred ATTRIBUTE_UNUSED) + const char *preferred ATTRIBUTE_UNUSED, + unsigned int flags) { + + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); + return 0; } diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a388f0f..f16a3cb 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1319,13 +1319,41 @@ x86GuestData(virCPUDefPtr host, return x86Compute(host, guest, data, message); } +static int +x86AddFeatures(virCPUDefPtr cpu, + struct x86_map *map) +{ + const struct x86_model *candidate; + const struct x86_feature *feature = map->features; + + candidate = map->models; + while (candidate != NULL) { + if (STREQ(cpu->model, candidate->name)) + break; + candidate = candidate->next; + } + if (!candidate) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s not a known CPU model"), cpu->model); + return -1; + } + while (feature != NULL) { + if (x86DataIsSubset(candidate->data, feature->data) && + (virCPUDefAddFeature(cpu, feature->name, VIR_CPU_FEATURE_REQUIRE) < 0)) + return -1; + feature = feature->next; + } + return 0; +} + static int x86Decode(virCPUDefPtr cpu, const struct cpuX86Data *data, const char **models, unsigned int nmodels, - const char *preferred) + const char *preferred, + unsigned int flags) { int ret = -1; struct x86_map *map; @@ -1334,6 +1362,8 @@ x86Decode(virCPUDefPtr cpu, virCPUDefPtr cpuModel = NULL; size_t i; + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); + if (data == NULL || (map = x86LoadMap()) == NULL) return -1; @@ -1406,6 +1436,9 @@ x86Decode(virCPUDefPtr cpu, goto out; } + if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES && + (x86AddFeatures(cpuModel, map) < 0)) + goto out; cpu->model = cpuModel->model; cpu->vendor = cpuModel->vendor; cpu->nfeatures = cpuModel->nfeatures; @@ -1426,9 +1459,10 @@ x86DecodeCPUData(virCPUDefPtr cpu, const virCPUDataPtr data, const char **models, unsigned int nmodels, - const char *preferred) + const char *preferred, + unsigned int flags) { - return x86Decode(cpu, data->data.x86, models, nmodels, preferred); + return x86Decode(cpu, data->data.x86, models, nmodels, preferred, flags); } @@ -1674,7 +1708,8 @@ static virCPUDefPtr x86Baseline(virCPUDefPtr *cpus, unsigned int ncpus, const char **models, - unsigned int nmodels) + unsigned int nmodels, + unsigned int flags) { struct x86_map *map = NULL; struct x86_model *base_model = NULL; @@ -1755,7 +1790,7 @@ x86Baseline(virCPUDefPtr *cpus, if (vendor && x86DataAddCpuid(base_model->data, &vendor->cpuid) < 0) goto error; - if (x86Decode(cpu, base_model->data, models, nmodels, NULL) < 0) + if (x86Decode(cpu, base_model->data, models, nmodels, NULL, flags) < 0) goto error; if (!outputVendor) diff --git a/src/libvirt.c b/src/libvirt.c index 8157488..6ce6c32 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18522,11 +18522,16 @@ error: * @conn: virConnect connection * @xmlCPUs: array of XML descriptions of host CPUs * @ncpus: number of CPUs in xmlCPUs - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virConnectBaselineCPUFlags * * Computes the most feature-rich CPU which is compatible with all given * host CPUs. * + * If @flags includes VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES then libvirt + * will explicitly list all CPU features that are part of the host CPU, + * without this flag features that are part of the CPU model will not be + * listed. + * * Returns XML description of the computed CPU or NULL on error. */ char * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2daafa8..2ad236e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11056,12 +11056,12 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, { char *cpu = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL); if (virConnectBaselineCPUEnsureACL(conn) < 0) goto cleanup; - cpu = cpuBaselineXML(xmlCPUs, ncpus, NULL, 0); + cpu = cpuBaselineXML(xmlCPUs, ncpus, NULL, 0, flags); cleanup: return cpu; diff --git a/tests/cputest.c b/tests/cputest.c index 2e5f0cd..959cb9f 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -75,6 +75,7 @@ struct data { const char *modelsName; unsigned int nmodels; const char *preferred; + unsigned int flags; int result; }; @@ -330,7 +331,7 @@ cpuTestBaseline(const void *arg) if (!(cpus = cpuTestLoadMultiXML(data->arch, data->name, &ncpus))) goto cleanup; - baseline = cpuBaseline(cpus, ncpus, NULL, 0); + baseline = cpuBaseline(cpus, ncpus, NULL, 0, data->flags); if (data->result < 0) { virResetLastError(); if (!baseline) @@ -510,12 +511,12 @@ mymain(void) } #define DO_TEST(arch, api, name, host, cpu, \ - models, nmodels, preferred, result) \ + models, nmodels, preferred, flags, result) \ do { \ static struct data data = { \ arch, api, host, cpu, models, \ models == NULL ? NULL : #models, \ - nmodels, preferred, result \ + nmodels, preferred, flags, result \ }; \ if (cpuTestRun(name, &data) < 0) \ ret = -1; \ @@ -524,31 +525,31 @@ mymain(void) #define DO_TEST_COMPARE(arch, host, cpu, result) \ DO_TEST(arch, API_COMPARE, \ host "/" cpu " (" #result ")", \ - host, cpu, NULL, 0, NULL, result) + host, cpu, NULL, 0, NULL, 0, result) #define DO_TEST_UPDATE(arch, host, cpu, result) \ do { \ DO_TEST(arch, API_UPDATE, \ cpu " on " host, \ - host, cpu, NULL, 0, NULL, 0); \ + host, cpu, NULL, 0, NULL, 0, 0); \ DO_TEST_COMPARE(arch, host, host "+" cpu, result); \ } while (0) -#define DO_TEST_BASELINE(arch, name, result) \ +#define DO_TEST_BASELINE(arch, name, flags, result) \ DO_TEST(arch, API_BASELINE, name, NULL, "baseline-" name, \ - NULL, 0, NULL, result) + NULL, 0, NULL, flags, result) #define DO_TEST_HASFEATURE(arch, host, feature, result) \ DO_TEST(arch, API_HAS_FEATURE, \ host "/" feature " (" #result ")", \ - host, feature, NULL, 0, NULL, result) + host, feature, NULL, 0, NULL, 0, result) #define DO_TEST_GUESTDATA(arch, host, cpu, models, preferred, result) \ DO_TEST(arch, API_GUEST_DATA, \ host "/" cpu " (" #models ", pref=" #preferred ")", \ host, cpu, models, \ models == NULL ? 0 : sizeof(models) / sizeof(char *), \ - preferred, result) + preferred, 0, result) /* host to host comparison */ DO_TEST_COMPARE("x86", "host", "host", VIR_CPU_COMPARE_IDENTICAL); @@ -593,11 +594,12 @@ mymain(void) DO_TEST_UPDATE("x86", "host", "host-passthrough", VIR_CPU_COMPARE_IDENTICAL); /* computing baseline CPUs */ - DO_TEST_BASELINE("x86", "incompatible-vendors", -1); - DO_TEST_BASELINE("x86", "no-vendor", 0); - DO_TEST_BASELINE("x86", "some-vendors", 0); - DO_TEST_BASELINE("x86", "1", 0); - DO_TEST_BASELINE("x86", "2", 0); + DO_TEST_BASELINE("x86", "incompatible-vendors", 0, -1); + DO_TEST_BASELINE("x86", "no-vendor", 0, 0); + DO_TEST_BASELINE("x86", "some-vendors", 0, 0); + DO_TEST_BASELINE("x86", "1", 0, 0); + DO_TEST_BASELINE("x86", "2", 0, 0); + DO_TEST_BASELINE("x86", "3", VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, 0); /* CPU features */ DO_TEST_HASFEATURE("x86", "host", "vmx", YES); diff --git a/tests/cputestdata/x86-baseline-3-result.xml b/tests/cputestdata/x86-baseline-3-result.xml new file mode 100644 index 0000000..d196112 --- /dev/null +++ b/tests/cputestdata/x86-baseline-3-result.xml @@ -0,0 +1,35 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>Westmere</model> + <feature policy='require' name='lahf_lm'/> + <feature policy='require' name='lm'/> + <feature policy='require' name='nx'/> + <feature policy='require' name='syscall'/> + <feature policy='require' name='aes'/> + <feature policy='require' name='popcnt'/> + <feature policy='require' name='sse4.2'/> + <feature policy='require' name='sse4.1'/> + <feature policy='require' name='cx16'/> + <feature policy='require' name='ssse3'/> + <feature policy='require' name='pni'/> + <feature policy='require' name='sse2'/> + <feature policy='require' name='sse'/> + <feature policy='require' name='fxsr'/> + <feature policy='require' name='mmx'/> + <feature policy='require' name='clflush'/> + <feature policy='require' name='pse36'/> + <feature policy='require' name='pat'/> + <feature policy='require' name='cmov'/> + <feature policy='require' name='mca'/> + <feature policy='require' name='pge'/> + <feature policy='require' name='mtrr'/> + <feature policy='require' name='sep'/> + <feature policy='require' name='apic'/> + <feature policy='require' name='cx8'/> + <feature policy='require' name='mce'/> + <feature policy='require' name='pae'/> + <feature policy='require' name='msr'/> + <feature policy='require' name='tsc'/> + <feature policy='require' name='pse'/> + <feature policy='require' name='de'/> + <feature policy='require' name='fpu'/> +</cpu> diff --git a/tests/cputestdata/x86-baseline-3.xml b/tests/cputestdata/x86-baseline-3.xml new file mode 100644 index 0000000..7654a1d --- /dev/null +++ b/tests/cputestdata/x86-baseline-3.xml @@ -0,0 +1,7 @@ +<cpuTest> +<cpu> + <arch>x86_64</arch> + <model>Westmere</model> + <topology sockets='1' cores='2' threads='1'/> +</cpu> +</cpuTest> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8cafce4..a1d5ab4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6148,6 +6148,10 @@ static const vshCmdOptDef opts_cpu_baseline[] = { .flags = VSH_OFLAG_REQ, .help = N_("file containing XML CPU descriptions") }, + {.name = "features", + .type = VSH_OT_BOOL, + .help = N_("Show features that are part of the CPU model type") + }, {.name = NULL} }; @@ -6159,6 +6163,7 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) char *buffer; char *result = NULL; const char **list = NULL; + unsigned int flags = 0; int count = 0; xmlDocPtr xml = NULL; @@ -6168,6 +6173,9 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; + if (vshCommandOptBool(cmd, "model_features")) + flags |= VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES; + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) return false; @@ -6211,7 +6219,7 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) list[i] = vshStrdup(ctl, (const char *)xmlBufferContent(xml_buf)); } - result = virConnectBaselineCPU(ctl->conn, list, count, 0); + result = virConnectBaselineCPU(ctl->conn, list, count, flags); if (result) { vshPrint(ctl, "%s", result); diff --git a/tools/virsh.pod b/tools/virsh.pod index 3ff6da1..0ae5178 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -485,13 +485,16 @@ cell and the total free memory on the machine. Finally, with a numeric argument or with --cellno plus a cell number it will display the free memory for the specified cell only. -=item B<cpu-baseline> I<FILE> +=item B<cpu-baseline> I<FILE> [I<--features>] Compute baseline CPU which will be supported by all host CPUs given in <file>. The list of host CPUs is built by extracting all <cpu> elements from the <file>. Thus, the <file> can contain either a set of <cpu> elements separated by new lines or even a set of complete <capabilities> elements printed by -B<capabilities> command. +B<capabilities> command. If I<--features> is specified then the +resulting XML description will explicitly include all features that make +up the CPU, without this option features that are part of the CPU model +will not be listed in the XML description. =item B<cpu-compare> I<FILE> -- 1.7.10.4 -- Don Dugger "Censeo Toto nos in Kansa esse decisse." - D. Gale n0ano(a)n0ano.com Ph: 303/443-3786