Still hoping that someone can reveiw this patch. On Fri, Aug 02, 2013 at 01:08:19PM -0600, n0ano wrote: > > 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@xxxxxxxxx> > > --- > [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@xxxxxxxxx > Ph: 303/443-3786 -- Don Dugger "Censeo Toto nos in Kansa esse decisse." - D. Gale n0ano@xxxxxxxxx Ph: 303/443-3786 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list