[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 invisiblethingslab com>
> > ---
> > 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]