On Mon, Jul 17, 2017 at 03:57:17PM -0600, Jim Fehlig wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
This is one reason why v1 used name->bit mapping of libvirt. But it
required "old" (xend) config syntax and had a side effect of setting all
bits of specified CPU model (yes, it required CPU model specified in
XML).
> > 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.
Ok.
> > 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.
Ah, indeed, leftover of v1.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
