Re: [libvirt] [PATCH v2 2/4] libxl: add support for CPUID features policy

On 07/03/2017 09:03 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, accept only "hostf-passthrough" > mode. > 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; > --- > Changes since v1: > - use new ("libxl") syntax to set only bits explicitly mentioned in > domain XML > --- > src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++--- > src/libxl/libxl_conf.h | 1 +- > 2 files changed, 74 insertions(+), 4 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index a0a53c2..0cf51a8 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -291,6 +291,44 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf) > return 0; > } > +static const char *libxlTranslateCPUFeature(const char *virCPUFeatureName) > +{ > + /* libxl use different names for some CPUID bits */ > + if (STREQ(virCPUFeatureName, "cx16")) > + return "cmpxchg16"; > + if (STREQ(virCPUFeatureName, "cid")) > + return "cntxid"; > + if (STREQ(virCPUFeatureName, "ds_cpl")) > + return "dscpl"; > + if (STREQ(virCPUFeatureName, "pclmuldq")) > + return "pclmulqdq"; > + if (STREQ(virCPUFeatureName, "pni")) > + return "sse3"; > + if (STREQ(virCPUFeatureName, "ht")) > + return "htt"; > + if (STREQ(virCPUFeatureName, "pn")) > + return "psn"; > + if (STREQ(virCPUFeatureName, "clflush")) > + return "clfsh"; > + if (STREQ(virCPUFeatureName, "sep")) > + return "sysenter"; > + if (STREQ(virCPUFeatureName, "cx8")) > + return "cmpxchg8"; > + if (STREQ(virCPUFeatureName, "nodeid_msr")) > + return "nodeid"; > + if (STREQ(virCPUFeatureName, "cr8legacy")) > + return "altmovcr8"; > + if (STREQ(virCPUFeatureName, "lahf_lm")) > + return "lahfsahf"; > + if (STREQ(virCPUFeatureName, "cmp_legacy")) > + return "cmplegacy"; > + if (STREQ(virCPUFeatureName, "fxsr_opt")) > + return "ffxsr"; > + if (STREQ(virCPUFeatureName, "pdpe1gb")) > + return "page1gb"; > + return virCPUFeatureName; > +} > + I don't have an alternative, but I see a mapping table becoming stale as new CPU features are added to libxl.

> static int > libxlMakeDomBuildInfo(virDomainDefPtr def, > libxl_ctx *ctx, > @@ -376,10 +414,18 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > def->features[VIR_DOMAIN_FEATURE_ACPI] == > VIR_TRISTATE_SWITCH_ON); > - if (caps && > - def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { > + if (caps && def->cpu) { > bool hasHwVirt = false; > bool svm = false, vmx = false; > + char xlCPU[32]; > + > + if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unsupported cpu mode '%s'"), > + virCPUModeTypeToString(def->cpu->mode)); > + return -1; > + } > + Although trivial, IMO this change should be in a separate patch where it will be easy to find.

> if (ARCH_IS_X86(def->os.arch)) { > vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"); > @@ -394,20 +440,43 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > case VIR_CPU_FEATURE_DISABLE: > case VIR_CPU_FEATURE_FORBID: > + snprintf(xlCPU, > + sizeof(xlCPU), > + "%s=0", > + libxlTranslateCPUFeature( > + def->cpu->features[i].name)); > + 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; > + } > 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"))) Spurious change. > hasHwVirt = false; > break; > case VIR_CPU_FEATURE_FORCE: > case VIR_CPU_FEATURE_REQUIRE: > + snprintf(xlCPU, > + sizeof(xlCPU), > + "%s=1", > + libxlTranslateCPUFeature( > + def->cpu->features[i].name)); > + 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: > break; > } > } > + libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); > } > - libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); This change also seems unrelated. Perhaps it can be combined with the change forbidding all but host_passthrough CPU model. > } > if (def->nsounds > 0) { > diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h > index 264df11..8d89ccd 100644 > --- a/src/libxl/libxl_conf.h > +++ b/src/libxl/libxl_conf.h > @@ -60,6 +60,7 @@ > # define LIBXL_DUMP_DIR LIBXL_LIB_DIR "/dump" > # define LIBXL_CHANNEL_DIR LIBXL_LIB_DIR "/channel/target" > # define LIBXL_BOOTLOADER_PATH "pygrub" > +# define LIBXL_DEFAULT_CPUID_REG_CONFIG "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" This doesn't appear to be used anywhere in the patch.