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

[Jim Fehlig]

On 07/17/2017 03:57 PM, 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 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.

Sorry, I forgot to mention that this series should also include a patch for 
domXML <-> xl.cfg conversions, plus a xlconfigtest :-). IMO, the libxl CPUID 
format is sufficient for now.

Regards,
Jim