[libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy
Jim Fehlig
jfehlig at suse.com
Thu Jan 4 00:00:10 UTC 2018
On 12/19/2017 06:19 AM, Joao Martins wrote:
> On 12/13/2017 07:09 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, only "host-passthrough" mode is
>> accepted.
>> 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>
>
> This is quite neat Marek :)
>
> I have one comment towards the end, suggesting an alternative to a static
> translation table.
>
>> ---
>> Changes since v3:
>> - compile fix (one more s/libxlTranslateCPUFeature/xenTranslateCPUFeature/)
>> Changes since v2:
>> - drop spurious changes
>> - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by
>> xenconfig driver
>> Changes since v1:
>> - use new ("libxl") syntax to set only bits explicitly mentioned in
>> domain XML
>> ---
>> src/libxl/libxl_conf.c | 35 +++++++++++++++++++++++++++++++++--
>> src/xenconfig/xen_xl.c | 34 ++++++++++++++++++++++++++++++++++
>> src/xenconfig/xen_xl.h | 2 ++
>> 3 files changed, 69 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 184610966..5eae6d10f 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -50,6 +50,7 @@
>> #include "secret_util.h"
>> #include "cpu/cpu.h"
>> #include "xen_common.h"
>> +#include "xen_xl.h"
>>
>>
>> #define VIR_FROM_THIS VIR_FROM_LIBXL
>> @@ -357,6 +358,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>> bool hasHwVirt = false;
>> int nested_hvm = -1;
>> bool svm = false, vmx = false;
>> + char xlCPU[32];
>>
>> if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> @@ -379,15 +381,44 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>> case VIR_CPU_FEATURE_DISABLE:
>> case VIR_CPU_FEATURE_FORBID:
>> 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"))) {
>> nested_hvm = 0;
>> + continue;
>> + }
>> +
>> + snprintf(xlCPU,
>> + sizeof(xlCPU),
>> + "%s=0",
>> + xenTranslateCPUFeature(
>> + def->cpu->features[i].name,
>> + false));
>> + 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_FORCE:
>> case VIR_CPU_FEATURE_REQUIRE:
>> 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"))) {
>> nested_hvm = 1;
>> + continue;
>> + }
>> +
>> + snprintf(xlCPU,
>> + sizeof(xlCPU),
>> + "%s=1",
>> + xenTranslateCPUFeature(
>> + def->cpu->features[i].name, false));
>> + 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:
>> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
>> index 317c7a08d..356ca3a4b 100644
>> --- a/src/xenconfig/xen_xl.c
>> +++ b/src/xenconfig/xen_xl.c
>> @@ -225,6 +225,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
>> return 0;
>> }
>>
>> +/*
>> + * Translate CPU feature name from libvirt to libxl (from_libxl=false) or from
>> + * libxl to libvirt (from_libxl=true).
>> + */
>> +const char *
>> +xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
>> +{
>> + static const char *translation_table[][2] = {
>> + /* libvirt name, libxl name */
>> + { "cx16", "cmpxchg16" },
>> + { "cid", "cntxid" },
>> + { "ds_cpl", "dscpl" },
>> + { "pclmuldq", "pclmulqdq" },
>> + { "pni", "sse3" },
>> + { "ht", "htt" },
>> + { "pn", "psn" },
>> + { "clflush", "clfsh" },
>> + { "sep", "sysenter" },
>> + { "cx8", "cmpxchg8" },
>> + { "nodeid_msr", "nodeid" },
>> + { "cr8legacy", "altmovcr8" },
>> + { "lahf_lm", "lahfsahf" },
>> + { "cmp_legacy", "cmplegacy" },
>> + { "fxsr_opt", "ffxsr" },
>> + { "pdpe1gb", "page1gb" },
>> + };
>> + size_t i;
>> +
>> + for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++)
>> + if (STREQ(translation_table[i][from_libxl], feature_name))
>> + return translation_table[i][!from_libxl];
>> + return feature_name;
>> +}
>> +
>>
>
> Cc'ing Jim as he may have some thoughts on the direction of this.
>
> What happens if the admin decides to change /usr/share/libvirt/cpu_map.xml?
Is changing existing content likely/practical?
> Also you can also add new leafs/features to cpu_map.xml
Right. And I suppose the table in libxl_cpuid.c can grow too. And in cases where
they differ we'd need to update the static table, which we'll probably only
remember to do when receiving bug reports. So I like the idea of making this
more dynamic, but I apparently don't have enough brain power today to understand
your suggestion :-).
> Maybe an idea instead is to have a table with all leafs that libxl has hardcoded
> (i.e. cpuid_flags array on
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_cpuid.c;hb=HEAD#l92).
Where would this table reside? In src/cpu/?
> Then adding a new cpu driver routine to look for cpuid data based on feature
> name (and the reverse too). Finally you would populate this translation table at
> driver load time, or you could even get away without a translation table (but
> would be a little more inefficient?).
I'm having difficulty understanding how this provides a dynamic solution.
Wouldn't the table you mention above need extended anytime the hardcoded one in
libxl was extended? Which would probably only happen after receiving a bug
report? :-)
Hmm, the more I think about it, the more I convince myself that knowing libvirt
and libxl use different names for a CPU feature is static information. I'm
afraid I don't have any better ideas beyond Marek's neat trick.
Regards,
Jim
>
>> static int
>> xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
>> diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h
>> index dd963268e..68f332aca 100644
>> --- a/src/xenconfig/xen_xl.h
>> +++ b/src/xenconfig/xen_xl.h
>> @@ -33,4 +33,6 @@ virDomainDefPtr xenParseXL(virConfPtr conn,
>>
>> virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr);
>>
>> +const char *xenTranslateCPUFeature(const char *feature_name, bool from_libxl);
>> +
>> #endif /* __VIR_XEN_XL_H__ */
>>
>
More information about the libvir-list
mailing list