Re: [libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy

On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote: > Convert CPU features policy into libxl cpuid policy settings. Use new > ("libxl") syntax, which allow to enable/disable specific bits, using > host CPU as a base. For this reason, only "host-passthrough" mode is > accepted. > Libxl do not have distinction between "force" and "required" policy > (there is only "force") and also between "forbid" and "disable" (there > is only "disable"). So, merge them appropriately. If anything, "require" > and "forbid" should be enforced outside of specific driver. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com&gt; This is quite neat Marek :) I have one comment towards the end, suggesting an alternative to a static translation table. > --- > Changes since v3: > - compile fix (one more s/libxlTranslateCPUFeature/xenTranslateCPUFeature/) > Changes since v2: > - drop spurious changes > - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by > xenconfig driver > Changes since v1: > - use new ("libxl") syntax to set only bits explicitly mentioned in > domain XML > --- > src/libxl/libxl_conf.c | 35 +++++++++++++++++++++++++++++++++-- > src/xenconfig/xen_xl.c | 34 ++++++++++++++++++++++++++++++++++ > src/xenconfig/xen_xl.h | 2 ++ > 3 files changed, 69 insertions(+), 2 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index 184610966..5eae6d10f 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -50,6 +50,7 @@ > #include "secret_util.h" > #include "cpu/cpu.h" > #include "xen_common.h" > +#include "xen_xl.h" > > > #define VIR_FROM_THIS VIR_FROM_LIBXL > @@ -357,6 +358,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > bool hasHwVirt = false; > int nested_hvm = -1; > bool svm = false, vmx = false; > + char xlCPU[32]; > > if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > @@ -379,15 +381,44 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > case VIR_CPU_FEATURE_DISABLE: > case VIR_CPU_FEATURE_FORBID: > if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > - (svm && STREQ(def->cpu->features[i].name, "svm"))) > + (svm && STREQ(def->cpu->features[i].name, "svm"))) { > nested_hvm = 0; > + continue; > + } > + > + snprintf(xlCPU, > + sizeof(xlCPU), > + "%s=0", > + xenTranslateCPUFeature( > + def->cpu->features[i].name, > + false)); > + if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unsupported cpu feature '%s'"), > + def->cpu->features[i].name); > + return -1; > + } > break; > > case VIR_CPU_FEATURE_FORCE: > case VIR_CPU_FEATURE_REQUIRE: > if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > - (svm && STREQ(def->cpu->features[i].name, "svm"))) > + (svm && STREQ(def->cpu->features[i].name, "svm"))) { > nested_hvm = 1; > + continue; > + } > + > + snprintf(xlCPU, > + sizeof(xlCPU), > + "%s=1", > + xenTranslateCPUFeature( > + def->cpu->features[i].name, false)); > + if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unsupported cpu feature '%s'"), > + def->cpu->features[i].name); > + return -1; > + } > break; > case VIR_CPU_FEATURE_OPTIONAL: > case VIR_CPU_FEATURE_LAST: > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c > index 317c7a08d..356ca3a4b 100644 > --- a/src/xenconfig/xen_xl.c > +++ b/src/xenconfig/xen_xl.c > @@ -225,6 +225,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) > return 0; > } > > +/* > + * Translate CPU feature name from libvirt to libxl (from_libxl=false) or from > + * libxl to libvirt (from_libxl=true). > + */ > +const char * > +xenTranslateCPUFeature(const char *feature_name, bool from_libxl) > +{ > + static const char *translation_table[][2] = { > + /* libvirt name, libxl name */ > + { "cx16", "cmpxchg16" }, > + { "cid", "cntxid" }, > + { "ds_cpl", "dscpl" }, > + { "pclmuldq", "pclmulqdq" }, > + { "pni", "sse3" }, > + { "ht", "htt" }, > + { "pn", "psn" }, > + { "clflush", "clfsh" }, > + { "sep", "sysenter" }, > + { "cx8", "cmpxchg8" }, > + { "nodeid_msr", "nodeid" }, > + { "cr8legacy", "altmovcr8" }, > + { "lahf_lm", "lahfsahf" }, > + { "cmp_legacy", "cmplegacy" }, > + { "fxsr_opt", "ffxsr" }, > + { "pdpe1gb", "page1gb" }, > + }; > + size_t i; > + > + for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++) > + if (STREQ(translation_table[i][from_libxl], feature_name)) > + return translation_table[i][!from_libxl]; > + return feature_name; > +} > + > Cc'ing Jim as he may have some thoughts on the direction of this. What happens if the admin decides to change /usr/share/libvirt/cpu_map.xml?

Also you can also add new leafs/features to cpu_map.xml

Maybe an idea instead is to have a table with all leafs that libxl has hardcoded (i.e. cpuid_flags array on http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_cpuid...).

Then adding a new cpu driver routine to look for cpuid data based on feature name (and the reverse too). Finally you would populate this translation table at driver load time, or you could even get away without a translation table (but would be a little more inefficient?).

> static int > xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) > diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h > index dd963268e..68f332aca 100644 > --- a/src/xenconfig/xen_xl.h > +++ b/src/xenconfig/xen_xl.h > @@ -33,4 +33,6 @@ virDomainDefPtr xenParseXL(virConfPtr conn, > > virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr); > > +const char *xenTranslateCPUFeature(const char *feature_name, bool from_libxl); > + > #endif /* __VIR_XEN_XL_H__ */ >