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

[Jim Fehlig]

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 at 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.

>   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.

Regards,
Jim