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@invisiblethingslab.com>
---
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 1846109..7760c2b 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",
+ 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:
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 317c7a0..356ca3a 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;
+}
+
static int
xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h
index dd96326..68f332a 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__ */
--
git-series 0.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listConvert 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@invisiblethingslab.com>
---
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;
+}
+
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__ */
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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@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? Also
you can also add new leafs/features to cpu_map.xml
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).
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?).
> 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__ */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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__ */
>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jan 03, 2018 at 05:00:10PM -0700, Jim Fehlig wrote:
> On 12/19/2017 06:19 AM, Joao Martins wrote:
> > On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
> > > +/*
> > > + * 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? :-)
Probably... And worse thing about such table is it needs to contain all
entries from said libxl_cpuid.c. My solution require only listing
entries with mismatching names.
Alternative would be to not use "new libxl syntax", but "old xend
syntax" (which is still supported by libxl). The later allow to list
specific bits instead of feature names. That was implemented in v1 of
this patch series[1]... The problem with that approach is currently libvirt
does not expose API for lookup of individual features, but only to
construct full CPU and then enumerate its CPUID. That means it isn't
feasible for the current approach (mode='host-passthrough', then
enable/disable individual features). See discussion on v1.
Adding such API to libvirt cpu driver is beyond my knowledge of libvirt
code and available time :/
> 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
[1] https://www.redhat.com/archives/libvir-list/2017-June/msg01292.html
--
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?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/04/2018 12:43 AM, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 03, 2018 at 05:00:10PM -0700, Jim Fehlig wrote:
>> On 12/19/2017 06:19 AM, Joao Martins wrote:
>>> On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
>>>> +/*
>>>> + * 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? :-)
>
> Probably... And worse thing about such table is it needs to contain all
> entries from said libxl_cpuid.c. My solution require only listing
> entries with mismatching names.
>
/nods and it would be a gigantic table most likely.
> Alternative would be to not use "new libxl syntax", but "old xend
> syntax" (which is still supported by libxl). The later allow to list
> specific bits instead of feature names. That was implemented in v1 of
> this patch series[1]... The problem with that approach is currently libvirt
> does not expose API for lookup of individual features, but only to
> construct full CPU and then enumerate its CPUID.
I kinda liked your xend version, provided we could lookup the feature bits as
you are hinting here.
> That means it isn't
> feasible for the current approach (mode='host-passthrough', then
> enable/disable individual features). See discussion on v1.
> Adding such API to libvirt cpu driver is beyond my knowledge of libvirt
> code and available time :/
>
The main reason I suggesting this out was because this patch was hardcoding
libvirt feature names. Maybe it's not an issue :)
Joao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/04/2018 12:00 AM, Jim Fehlig wrote:
> 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@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?
>
Not sure TBH. But I would imagine for every new feature, or if admin wants to
adjust mnemonics with say Linux. But this is guess work of what the people do
nowadays. But I guess the question is whether these feature names are part of
the API or are expected to change.
>> 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/?
>
Not sure, but would be placed preferably in src/xenconfig or src/libxl/.
>> 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.
>
I probably didn't explain myself correctly.
What I mentioned above was to have the feature bits as common denominator as
opposed to feature names (that currently vary between libvirt and libxl). And
the array we import from libxl declares both feature name and bits (or just
resorting to xend syntax). So for each libxl feature bits we declare, we look
for the right libvirt feature name.
> 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__ */
>>>
>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.