docs/man/xl.cfg.5.pod.in | 22 +-- tools/libs/light/libxl_cpuid.c | 246 ++++++++++----------------------- tools/xl/xl_parse.c | 3 + 3 files changed, 78 insertions(+), 193 deletions(-)
The current implementation in libxl_cpuid_parse_config() requires
keeping a list of cpuid feature bits that should be mostly in sync
with the contents of cpufeatureset.h.
Avoid such duplication by using the automatically generated list of
cpuid features in INIT_FEATURE_NAMES in order to map feature names to
featureset bits, and then translate from featureset bits into cpuid
leaf, subleaf, register tuple.
Note that the full contents of the previous cpuid translation table
can't be removed. That's because some feature names allowed by libxl
are not described in the featuresets, or because naming has diverged
and the previous nomenclature is preserved for compatibility reasons.
Should result in no functional change observed by callers, albeit some
new cpuid features will be available as a result of the change.
While there constify cpuid_flags name field.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
There's currently no table to map from featureset word into cpuid
leaf, so open-code one (feature_to_cpuid). Otherwise we would need to
parse the comments in cpufeatureset.h in order to obtain the
leaf/subleaf/register each word maps into.
One possible way of improving would be to use a cpu_policy and
featureset in libxl_cpuid_parse_config(), but that requires adding an
xc_cpu_policy_t object to libxl_domain_build_info, and the proposed
patch is already an improvement over the current status.
---
docs/man/xl.cfg.5.pod.in | 22 +--
tools/libs/light/libxl_cpuid.c | 246 ++++++++++-----------------------
tools/xl/xl_parse.c | 3 +
3 files changed, 78 insertions(+), 193 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 24ac92718288..c5c5b8716521 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2010,24 +2010,12 @@ proccount procpkg stepping
=back
-List of keys taking a character:
+List of keys taking a character can be found in the public header file
+L<arch-x86/cpufeatureset.h|http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,cpufeatureset.h.html>
-=over 4
-
-3dnow 3dnowext 3dnowprefetch abm acpi adx aes altmovcr8 apic arat avx avx2
-avx512-4fmaps avx512-4vnniw avx512bw avx512cd avx512dq avx512er avx512f
-avx512ifma avx512pf avx512vbmi avx512vl bmi1 bmi2 clflushopt clfsh clwb cmov
-cmplegacy cmpxchg16 cmpxchg8 cmt cntxid dca de ds dscpl dtes64 erms est extapic
-f16c ffxsr fma fma4 fpu fsgsbase fxsr hle htt hypervisor ia64 ibs invpcid
-invtsc lahfsahf lm lwp mca mce misalignsse mmx mmxext monitor movbe mpx msr
-mtrr nodeid nx ospke osvw osxsave pae page1gb pat pbe pcid pclmulqdq pdcm
-perfctr_core perfctr_nb pge pku popcnt pse pse36 psn rdrand rdseed rdtscp rtm
-sha skinit smap smep smx ss sse sse2 sse3 sse4.1 sse4.2 sse4_1 sse4_2 sse4a
-ssse3 svm svm_decode svm_lbrv svm_npt svm_nrips svm_pausefilt svm_tscrate
-svm_vmcbclean syscall sysenter tbm tm tm2 topoext tsc tsc-deadline tsc_adjust
-umip vme vmx wdt x2apic xop xsave xtpr
-
-=back
+Note that C<clflush> is described as an option that takes a value, and that
+takes precedence over the C<clflush> flag in C<cpufeatureset.h>. The feature
+flag must be referenced as C<clfsh>.
=back
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index f5ce9f97959c..0c7ffff416fe 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -14,6 +14,8 @@
#include "libxl_internal.h"
+#include <xen/lib/x86/cpu-policy.h>
+
int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
{
return !libxl_cpuid_policy_list_length(pl);
@@ -51,7 +53,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
* Used for the static structure describing all features.
*/
struct cpuid_flags {
- char* name;
+ const char* name;
uint32_t leaf;
uint32_t subleaf;
int reg;
@@ -81,6 +83,14 @@ static libxl_cpuid_policy_list cpuid_find_match(libxl_cpuid_policy_list *list,
return *list + i;
}
+static int search_feature(const void *a, const void *b)
+{
+ const char *key = a;
+ const char *feat = ((struct { const char *n; } *)b)->n;
+
+ return strncmp(key, feat, strlen(key)) ?: strlen(key) - strlen(feat);
+}
+
/* parse a single key=value pair and translate it into the libxc
* used interface using 32-characters strings for each register.
* Will overwrite earlier entries and thus can be called multiple
@@ -101,208 +111,37 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
{"proccount", 0x00000001, NA, CPUID_REG_EBX, 16, 8},
{"localapicid", 0x00000001, NA, CPUID_REG_EBX, 24, 8},
- {"sse3", 0x00000001, NA, CPUID_REG_ECX, 0, 1},
- {"pclmulqdq", 0x00000001, NA, CPUID_REG_ECX, 1, 1},
- {"dtes64", 0x00000001, NA, CPUID_REG_ECX, 2, 1},
- {"monitor", 0x00000001, NA, CPUID_REG_ECX, 3, 1},
- {"dscpl", 0x00000001, NA, CPUID_REG_ECX, 4, 1},
- {"vmx", 0x00000001, NA, CPUID_REG_ECX, 5, 1},
- {"smx", 0x00000001, NA, CPUID_REG_ECX, 6, 1},
{"est", 0x00000001, NA, CPUID_REG_ECX, 7, 1},
- {"tm2", 0x00000001, NA, CPUID_REG_ECX, 8, 1},
- {"ssse3", 0x00000001, NA, CPUID_REG_ECX, 9, 1},
{"cntxid", 0x00000001, NA, CPUID_REG_ECX, 10, 1},
- {"fma", 0x00000001, NA, CPUID_REG_ECX, 12, 1},
{"cmpxchg16", 0x00000001, NA, CPUID_REG_ECX, 13, 1},
- {"xtpr", 0x00000001, NA, CPUID_REG_ECX, 14, 1},
- {"pdcm", 0x00000001, NA, CPUID_REG_ECX, 15, 1},
- {"pcid", 0x00000001, NA, CPUID_REG_ECX, 17, 1},
- {"dca", 0x00000001, NA, CPUID_REG_ECX, 18, 1},
/* Linux uses sse4_{1,2}. Keep sse4.{1,2} for compatibility */
- {"sse4_1", 0x00000001, NA, CPUID_REG_ECX, 19, 1},
{"sse4.1", 0x00000001, NA, CPUID_REG_ECX, 19, 1},
- {"sse4_2", 0x00000001, NA, CPUID_REG_ECX, 20, 1},
{"sse4.2", 0x00000001, NA, CPUID_REG_ECX, 20, 1},
- {"x2apic", 0x00000001, NA, CPUID_REG_ECX, 21, 1},
- {"movbe", 0x00000001, NA, CPUID_REG_ECX, 22, 1},
- {"popcnt", 0x00000001, NA, CPUID_REG_ECX, 23, 1},
- {"tsc-deadline", 0x00000001, NA, CPUID_REG_ECX, 24, 1},
{"aes", 0x00000001, NA, CPUID_REG_ECX, 25, 1},
- {"xsave", 0x00000001, NA, CPUID_REG_ECX, 26, 1},
- {"osxsave", 0x00000001, NA, CPUID_REG_ECX, 27, 1},
- {"avx", 0x00000001, NA, CPUID_REG_ECX, 28, 1},
- {"f16c", 0x00000001, NA, CPUID_REG_ECX, 29, 1},
- {"rdrand", 0x00000001, NA, CPUID_REG_ECX, 30, 1},
- {"hypervisor", 0x00000001, NA, CPUID_REG_ECX, 31, 1},
-
- {"fpu", 0x00000001, NA, CPUID_REG_EDX, 0, 1},
- {"vme", 0x00000001, NA, CPUID_REG_EDX, 1, 1},
- {"de", 0x00000001, NA, CPUID_REG_EDX, 2, 1},
- {"pse", 0x00000001, NA, CPUID_REG_EDX, 3, 1},
- {"tsc", 0x00000001, NA, CPUID_REG_EDX, 4, 1},
- {"msr", 0x00000001, NA, CPUID_REG_EDX, 5, 1},
- {"pae", 0x00000001, NA, CPUID_REG_EDX, 6, 1},
- {"mce", 0x00000001, NA, CPUID_REG_EDX, 7, 1},
+
{"cmpxchg8", 0x00000001, NA, CPUID_REG_EDX, 8, 1},
- {"apic", 0x00000001, NA, CPUID_REG_EDX, 9, 1},
{"sysenter", 0x00000001, NA, CPUID_REG_EDX, 11, 1},
- {"mtrr", 0x00000001, NA, CPUID_REG_EDX, 12, 1},
- {"pge", 0x00000001, NA, CPUID_REG_EDX, 13, 1},
- {"mca", 0x00000001, NA, CPUID_REG_EDX, 14, 1},
- {"cmov", 0x00000001, NA, CPUID_REG_EDX, 15, 1},
- {"pat", 0x00000001, NA, CPUID_REG_EDX, 16, 1},
- {"pse36", 0x00000001, NA, CPUID_REG_EDX, 17, 1},
{"psn", 0x00000001, NA, CPUID_REG_EDX, 18, 1},
{"clfsh", 0x00000001, NA, CPUID_REG_EDX, 19, 1},
- {"ds", 0x00000001, NA, CPUID_REG_EDX, 21, 1},
- {"acpi", 0x00000001, NA, CPUID_REG_EDX, 22, 1},
- {"mmx", 0x00000001, NA, CPUID_REG_EDX, 23, 1},
- {"fxsr", 0x00000001, NA, CPUID_REG_EDX, 24, 1},
- {"sse", 0x00000001, NA, CPUID_REG_EDX, 25, 1},
- {"sse2", 0x00000001, NA, CPUID_REG_EDX, 26, 1},
- {"ss", 0x00000001, NA, CPUID_REG_EDX, 27, 1},
- {"htt", 0x00000001, NA, CPUID_REG_EDX, 28, 1},
{"tm", 0x00000001, NA, CPUID_REG_EDX, 29, 1},
{"ia64", 0x00000001, NA, CPUID_REG_EDX, 30, 1},
{"pbe", 0x00000001, NA, CPUID_REG_EDX, 31, 1},
{"arat", 0x00000006, NA, CPUID_REG_EAX, 2, 1},
- {"fsgsbase", 0x00000007, 0, CPUID_REG_EBX, 0, 1},
- {"tsc_adjust", 0x00000007, 0, CPUID_REG_EBX, 1, 1},
- {"bmi1", 0x00000007, 0, CPUID_REG_EBX, 3, 1},
- {"hle", 0x00000007, 0, CPUID_REG_EBX, 4, 1},
- {"avx2", 0x00000007, 0, CPUID_REG_EBX, 5, 1},
- {"smep", 0x00000007, 0, CPUID_REG_EBX, 7, 1},
- {"bmi2", 0x00000007, 0, CPUID_REG_EBX, 8, 1},
- {"erms", 0x00000007, 0, CPUID_REG_EBX, 9, 1},
- {"invpcid", 0x00000007, 0, CPUID_REG_EBX, 10, 1},
- {"rtm", 0x00000007, 0, CPUID_REG_EBX, 11, 1},
- {"cmt", 0x00000007, 0, CPUID_REG_EBX, 12, 1},
- {"mpx", 0x00000007, 0, CPUID_REG_EBX, 14, 1},
- {"avx512f", 0x00000007, 0, CPUID_REG_EBX, 16, 1},
- {"avx512dq", 0x00000007, 0, CPUID_REG_EBX, 17, 1},
- {"rdseed", 0x00000007, 0, CPUID_REG_EBX, 18, 1},
- {"adx", 0x00000007, 0, CPUID_REG_EBX, 19, 1},
- {"smap", 0x00000007, 0, CPUID_REG_EBX, 20, 1},
- {"avx512-ifma", 0x00000007, 0, CPUID_REG_EBX, 21, 1},
- {"clflushopt", 0x00000007, 0, CPUID_REG_EBX, 23, 1},
- {"clwb", 0x00000007, 0, CPUID_REG_EBX, 24, 1},
- {"proc-trace", 0x00000007, 0, CPUID_REG_EBX, 25, 1},
- {"avx512pf", 0x00000007, 0, CPUID_REG_EBX, 26, 1},
- {"avx512er", 0x00000007, 0, CPUID_REG_EBX, 27, 1},
- {"avx512cd", 0x00000007, 0, CPUID_REG_EBX, 28, 1},
- {"sha", 0x00000007, 0, CPUID_REG_EBX, 29, 1},
- {"avx512bw", 0x00000007, 0, CPUID_REG_EBX, 30, 1},
- {"avx512vl", 0x00000007, 0, CPUID_REG_EBX, 31, 1},
-
- {"prefetchwt1", 0x00000007, 0, CPUID_REG_ECX, 0, 1},
- {"avx512-vbmi", 0x00000007, 0, CPUID_REG_ECX, 1, 1},
- {"umip", 0x00000007, 0, CPUID_REG_ECX, 2, 1},
- {"pku", 0x00000007, 0, CPUID_REG_ECX, 3, 1},
- {"ospke", 0x00000007, 0, CPUID_REG_ECX, 4, 1},
- {"avx512-vbmi2", 0x00000007, 0, CPUID_REG_ECX, 6, 1},
- {"cet-ss", 0x00000007, 0, CPUID_REG_ECX, 7, 1},
- {"gfni", 0x00000007, 0, CPUID_REG_ECX, 8, 1},
- {"vaes", 0x00000007, 0, CPUID_REG_ECX, 9, 1},
- {"vpclmulqdq", 0x00000007, 0, CPUID_REG_ECX, 10, 1},
- {"avx512-vnni", 0x00000007, 0, CPUID_REG_ECX, 11, 1},
- {"avx512-bitalg",0x00000007, 0, CPUID_REG_ECX, 12, 1},
- {"avx512-vpopcntdq",0x00000007,0,CPUID_REG_ECX, 14, 1},
- {"rdpid", 0x00000007, 0, CPUID_REG_ECX, 22, 1},
- {"cldemote", 0x00000007, 0, CPUID_REG_ECX, 25, 1},
- {"pks", 0x00000007, 0, CPUID_REG_ECX, 31, 1},
-
- {"avx512-4vnniw",0x00000007, 0, CPUID_REG_EDX, 2, 1},
- {"avx512-4fmaps",0x00000007, 0, CPUID_REG_EDX, 3, 1},
- {"fsrm", 0x00000007, 0, CPUID_REG_EDX, 4, 1},
- {"avx512-vp2intersect",0x00000007,0,CPUID_REG_EDX,8, 1},
- {"srbds-ctrl", 0x00000007, 0, CPUID_REG_EDX, 9, 1},
- {"md-clear", 0x00000007, 0, CPUID_REG_EDX, 10, 1},
- {"serialize", 0x00000007, 0, CPUID_REG_EDX, 14, 1},
- {"tsxldtrk", 0x00000007, 0, CPUID_REG_EDX, 16, 1},
- {"cet-ibt", 0x00000007, 0, CPUID_REG_EDX, 20, 1},
- {"avx512-fp16", 0x00000007, 0, CPUID_REG_EDX, 23, 1},
- {"ibrsb", 0x00000007, 0, CPUID_REG_EDX, 26, 1},
- {"stibp", 0x00000007, 0, CPUID_REG_EDX, 27, 1},
- {"l1d-flush", 0x00000007, 0, CPUID_REG_EDX, 28, 1},
- {"arch-caps", 0x00000007, 0, CPUID_REG_EDX, 29, 1},
- {"core-caps", 0x00000007, 0, CPUID_REG_EDX, 30, 1},
- {"ssbd", 0x00000007, 0, CPUID_REG_EDX, 31, 1},
-
- {"avx-vnni", 0x00000007, 1, CPUID_REG_EAX, 4, 1},
- {"avx512-bf16", 0x00000007, 1, CPUID_REG_EAX, 5, 1},
- {"fzrm", 0x00000007, 1, CPUID_REG_EAX, 10, 1},
- {"fsrs", 0x00000007, 1, CPUID_REG_EAX, 11, 1},
- {"fsrcs", 0x00000007, 1, CPUID_REG_EAX, 12, 1},
- {"wrmsrns", 0x00000007, 1, CPUID_REG_EAX, 19, 1},
- {"avx-ifma", 0x00000007, 1, CPUID_REG_EAX, 23, 1},
-
- {"avx-vnni-int8",0x00000007, 1, CPUID_REG_EDX, 4, 1},
- {"avx-ne-convert",0x00000007, 1, CPUID_REG_EDX, 5, 1},
- {"cet-sss", 0x00000007, 1, CPUID_REG_EDX, 18, 1},
-
- {"intel-psfd", 0x00000007, 2, CPUID_REG_EDX, 0, 1},
- {"ipred-ctrl", 0x00000007, 2, CPUID_REG_EDX, 1, 1},
- {"rrsba-ctrl", 0x00000007, 2, CPUID_REG_EDX, 2, 1},
- {"ddp-ctrl", 0x00000007, 2, CPUID_REG_EDX, 3, 1},
- {"bhi-ctrl", 0x00000007, 2, CPUID_REG_EDX, 4, 1},
- {"mcdt-no", 0x00000007, 2, CPUID_REG_EDX, 5, 1},
-
{"lahfsahf", 0x80000001, NA, CPUID_REG_ECX, 0, 1},
{"cmplegacy", 0x80000001, NA, CPUID_REG_ECX, 1, 1},
- {"svm", 0x80000001, NA, CPUID_REG_ECX, 2, 1},
- {"extapic", 0x80000001, NA, CPUID_REG_ECX, 3, 1},
{"altmovcr8", 0x80000001, NA, CPUID_REG_ECX, 4, 1},
- {"abm", 0x80000001, NA, CPUID_REG_ECX, 5, 1},
- {"sse4a", 0x80000001, NA, CPUID_REG_ECX, 6, 1},
- {"misalignsse", 0x80000001, NA, CPUID_REG_ECX, 7, 1},
- {"3dnowprefetch",0x80000001, NA, CPUID_REG_ECX, 8, 1},
- {"osvw", 0x80000001, NA, CPUID_REG_ECX, 9, 1},
- {"ibs", 0x80000001, NA, CPUID_REG_ECX, 10, 1},
- {"xop", 0x80000001, NA, CPUID_REG_ECX, 11, 1},
- {"skinit", 0x80000001, NA, CPUID_REG_ECX, 12, 1},
- {"wdt", 0x80000001, NA, CPUID_REG_ECX, 13, 1},
- {"lwp", 0x80000001, NA, CPUID_REG_ECX, 15, 1},
- {"fma4", 0x80000001, NA, CPUID_REG_ECX, 16, 1},
{"nodeid", 0x80000001, NA, CPUID_REG_ECX, 19, 1},
- {"tbm", 0x80000001, NA, CPUID_REG_ECX, 21, 1},
- {"topoext", 0x80000001, NA, CPUID_REG_ECX, 22, 1},
{"perfctr_core", 0x80000001, NA, CPUID_REG_ECX, 23, 1},
{"perfctr_nb", 0x80000001, NA, CPUID_REG_ECX, 24, 1},
- {"syscall", 0x80000001, NA, CPUID_REG_EDX, 11, 1},
- {"nx", 0x80000001, NA, CPUID_REG_EDX, 20, 1},
- {"mmxext", 0x80000001, NA, CPUID_REG_EDX, 22, 1},
- {"ffxsr", 0x80000001, NA, CPUID_REG_EDX, 25, 1},
- {"page1gb", 0x80000001, NA, CPUID_REG_EDX, 26, 1},
- {"rdtscp", 0x80000001, NA, CPUID_REG_EDX, 27, 1},
- {"lm", 0x80000001, NA, CPUID_REG_EDX, 29, 1},
- {"3dnowext", 0x80000001, NA, CPUID_REG_EDX, 30, 1},
- {"3dnow", 0x80000001, NA, CPUID_REG_EDX, 31, 1},
-
{"procpkg", 0x00000004, 0, CPUID_REG_EAX, 26, 6},
{"invtsc", 0x80000007, NA, CPUID_REG_EDX, 8, 1},
- {"clzero", 0x80000008, NA, CPUID_REG_EBX, 0, 1},
- {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
- {"wbnoinvd", 0x80000008, NA, CPUID_REG_EBX, 9, 1},
- {"ibpb", 0x80000008, NA, CPUID_REG_EBX, 12, 1},
- {"ibrs", 0x80000008, NA, CPUID_REG_EBX, 14, 1},
- {"amd-stibp", 0x80000008, NA, CPUID_REG_EBX, 15, 1},
- {"ibrs-always", 0x80000008, NA, CPUID_REG_EBX, 16, 1},
- {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17, 1},
- {"ibrs-fast", 0x80000008, NA, CPUID_REG_EBX, 18, 1},
- {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19, 1},
- {"no-lmsl", 0x80000008, NA, CPUID_REG_EBX, 20, 1},
{"ppin", 0x80000008, NA, CPUID_REG_EBX, 23, 1},
- {"amd-ssbd", 0x80000008, NA, CPUID_REG_EBX, 24, 1},
- {"virt-ssbd", 0x80000008, NA, CPUID_REG_EBX, 25, 1},
- {"ssb-no", 0x80000008, NA, CPUID_REG_EBX, 26, 1},
- {"psfd", 0x80000008, NA, CPUID_REG_EBX, 28, 1},
{"btc-no", 0x80000008, NA, CPUID_REG_EBX, 29, 1},
- {"ibpb-ret", 0x80000008, NA, CPUID_REG_EBX, 30, 1},
{"nc", 0x80000008, NA, CPUID_REG_ECX, 0, 8},
{"apicidsize", 0x80000008, NA, CPUID_REG_ECX, 12, 4},
@@ -316,22 +155,55 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
{"svm_pausefilt",0x8000000a, NA, CPUID_REG_EDX, 10, 1},
{"lfence+", 0x80000021, NA, CPUID_REG_EAX, 2, 1},
- {"nscb", 0x80000021, NA, CPUID_REG_EAX, 6, 1},
- {"auto-ibrs", 0x80000021, NA, CPUID_REG_EAX, 8, 1},
- {"cpuid-user-dis", 0x80000021, NA, CPUID_REG_EAX, 17, 1},
{"maxhvleaf", 0x40000000, NA, CPUID_REG_EAX, 0, 8},
{NULL, 0, NA, CPUID_REG_INV, 0, 0}
};
+ static const struct feature_name {
+ const char *name;
+ unsigned int bit;
+ } features[] = INIT_FEATURE_NAMES;
+ /*
+ * NB: if we switch to using a cpu_policy derived object instead of a
+ * libxl_cpuid_policy_list we could get rid of the featureset -> cpuid leaf
+ * conversion table and use a featureset directly as we have conversions
+ * to/from featureset and cpu_policy.
+ */
+ static const struct {
+ uint32_t leaf, subleaf;
+ unsigned int reg;
+ } feature_to_cpuid[] = {
+ {0x00000001, NA, CPUID_REG_EDX},
+ {0x00000001, NA, CPUID_REG_ECX},
+ {0x80000001, NA, CPUID_REG_EDX},
+ {0x80000001, NA, CPUID_REG_ECX},
+ {0x0000000D, 1, CPUID_REG_EAX},
+ {0x00000007, 0, CPUID_REG_EBX},
+ {0x00000007, 0, CPUID_REG_ECX},
+ {0x80000007, NA, CPUID_REG_EDX},
+ {0x80000008, NA, CPUID_REG_EBX},
+ {0x00000007, 0, CPUID_REG_EDX},
+ {0x00000007, 1, CPUID_REG_EAX},
+ {0x80000021, NA, CPUID_REG_EAX},
+ {0x00000007, 1, CPUID_REG_EBX},
+ {0x00000007, 2, CPUID_REG_EDX},
+ {0x00000007, 1, CPUID_REG_ECX},
+ {0x00000007, 1, CPUID_REG_EDX},
+ { NA, NA, CPUID_REG_INV}, /* MSR_ARCH_CAPS.eax */
+ { NA, NA, CPUID_REG_INV}, /* MSR_ARCH_CAPS.edx */
+ };
#undef NA
char *sep, *val, *endptr;
int i;
const struct cpuid_flags *flag;
+ struct cpuid_flags f;
struct xc_xend_cpuid *entry;
unsigned long num;
char flags[33], *resstr;
+ BUILD_BUG_ON(ARRAY_SIZE(feature_to_cpuid) != FEATURESET_NR_ENTRIES);
+
sep = strchr(str, '=');
if (sep == NULL) {
return 1;
@@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
break;
}
+ if (flag->name == NULL) {
+ const struct feature_name *feat;
+ /* Provide a NUL terminated feature name to the search helper. */
+ char *name = strndup(str, sep - str);
+
+ if (name == NULL)
+ return 4;
+
+ feat = bsearch(name, features, ARRAY_SIZE(features), sizeof(features[0]),
+ search_feature);
+ free(name);
+
+ if (feat != NULL) {
+ f.name = feat->name;
+ f.leaf = feature_to_cpuid[feat->bit / 32].leaf;
+ f.subleaf = feature_to_cpuid[feat->bit / 32].subleaf;
+ f.reg = feature_to_cpuid[feat->bit / 32].reg;
+ f.bit = feat->bit % 32;
+ f.length = 1,
+ flag = &f;
+ }
+ }
if (flag->name == NULL) {
return 2;
}
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 1f6f47daf4e1..80b189dea0c1 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2620,6 +2620,9 @@ skip_usbdev:
case 3:
errstr = "illegal CPUID value (must be: [0|1|x|k|s])";
break;
+ case 4:
+ errstr = "out of memory";
+ break;
default:
errstr = "unknown error";
break;
--
2.40.0
On Mon, Jun 05, 2023 at 12:36:57PM +0200, Roger Pau Monne wrote:
> The current implementation in libxl_cpuid_parse_config() requires
> keeping a list of cpuid feature bits that should be mostly in sync
> with the contents of cpufeatureset.h.
>
> Avoid such duplication by using the automatically generated list of
> cpuid features in INIT_FEATURE_NAMES in order to map feature names to
> featureset bits, and then translate from featureset bits into cpuid
> leaf, subleaf, register tuple.
>
> Note that the full contents of the previous cpuid translation table
> can't be removed. That's because some feature names allowed by libxl
> are not described in the featuresets, or because naming has diverged
> and the previous nomenclature is preserved for compatibility reasons.
>
> Should result in no functional change observed by callers, albeit some
> new cpuid features will be available as a result of the change.
I've looked at the removed lists, and some cpuid flag name might be
missing with this patch.
When looking in "libxl_cpuid.c" and
tools/include/xen/lib/x86/cpuid-autogen.h (INIT_FEATURE_NAMES), I found
that some flags removed from libxl_cpuid.c seems to be missing with this
patch:
"sse4_1"
"sse4_2"
"tsc_adjust"
"cmt"
I did the same with the list removed from the man page, and those
flags are missing (some were probably also missing before, so probably
not a problem:
"avx512ifma"
"avx512vbmi"
"cmt"
"sse4_1"
"sse4_2"
"tsc_adjust"
So, at least for the first list, is it a problem? Or did I failed to
check them properly?
(It seems that "cmt" or "tsc_adjust" for example comes from libvirt,
90b2a267c19c ("libxl: add more cpuid flags handling"))
> While there constify cpuid_flags name field.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 24ac92718288..c5c5b8716521 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2010,24 +2010,12 @@ proccount procpkg stepping
>
> =back
>
> -List of keys taking a character:
> +List of keys taking a character can be found in the public header file
> +L<arch-x86/cpufeatureset.h|http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,cpufeatureset.h.html>
https:// ;-)
And this probably want to be "xenbits.xenproject.org"
Also, I think there's maybe an issue with this link. Maybe someone can
assume that "TM1" takes a character, but that flags I think would get
rejected, issue with upper case vs lower case. Then, if we deal with
the casing, we still have feature like "AVX512_IFMA", which would only
be recognize if written "avx512-ifma", so issue with "-" vs "_".
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index f5ce9f97959c..0c7ffff416fe 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
> if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
> break;
> }
> + if (flag->name == NULL) {
> + const struct feature_name *feat;
> + /* Provide a NUL terminated feature name to the search helper. */
> + char *name = strndup(str, sep - str);
> +
> + if (name == NULL)
> + return 4;
out-of-memory are usually fatal in libxl. Any reason to use `strndup`
instead of `libxl__strndup`? I guess we don't have any libxl_ctx, so we
can't use the libxl function.
So, instead of returning an arbitrary integer that isn't returned yet by
the function, could you return ERROR_NOMEM?
I wonder if it would be possible to use a buffer on the stack instead,
but it might not be worth the effort to find the right size.
Thanks,
--
Anthony PERARD
On Tue, Jun 13, 2023 at 11:44:51AM +0100, Anthony PERARD wrote:
> On Mon, Jun 05, 2023 at 12:36:57PM +0200, Roger Pau Monne wrote:
> > The current implementation in libxl_cpuid_parse_config() requires
> > keeping a list of cpuid feature bits that should be mostly in sync
> > with the contents of cpufeatureset.h.
> >
> > Avoid such duplication by using the automatically generated list of
> > cpuid features in INIT_FEATURE_NAMES in order to map feature names to
> > featureset bits, and then translate from featureset bits into cpuid
> > leaf, subleaf, register tuple.
> >
> > Note that the full contents of the previous cpuid translation table
> > can't be removed. That's because some feature names allowed by libxl
> > are not described in the featuresets, or because naming has diverged
> > and the previous nomenclature is preserved for compatibility reasons.
> >
> > Should result in no functional change observed by callers, albeit some
> > new cpuid features will be available as a result of the change.
>
> I've looked at the removed lists, and some cpuid flag name might be
> missing with this patch.
>
> When looking in "libxl_cpuid.c" and
> tools/include/xen/lib/x86/cpuid-autogen.h (INIT_FEATURE_NAMES), I found
> that some flags removed from libxl_cpuid.c seems to be missing with this
> patch:
> "sse4_1"
> "sse4_2"
> "tsc_adjust"
> "cmt"
Hm, I got confused with the _ vs - notation, and with cmt I wrongly
deleted it I guess.
> I did the same with the list removed from the man page, and those
> flags are missing (some were probably also missing before, so probably
> not a problem:
> "avx512ifma"
> "avx512vbmi"
Those two where already missing, so I'm not planning to add those.
> "cmt"
> "sse4_1"
> "sse4_2"
> "tsc_adjust"
Those are an oversight and will be added back to cpuid_flags array.
>
> So, at least for the first list, is it a problem? Or did I failed to
> check them properly?
> (It seems that "cmt" or "tsc_adjust" for example comes from libvirt,
> 90b2a267c19c ("libxl: add more cpuid flags handling"))
>
> > While there constify cpuid_flags name field.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 24ac92718288..c5c5b8716521 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2010,24 +2010,12 @@ proccount procpkg stepping
> >
> > =back
> >
> > -List of keys taking a character:
> > +List of keys taking a character can be found in the public header file
> > +L<arch-x86/cpufeatureset.h|http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,cpufeatureset.h.html>
>
> https:// ;-)
>
> And this probably want to be "xenbits.xenproject.org"
>
> Also, I think there's maybe an issue with this link. Maybe someone can
> assume that "TM1" takes a character, but that flags I think would get
> rejected, issue with upper case vs lower case. Then, if we deal with
> the casing, we still have feature like "AVX512_IFMA", which would only
> be recognize if written "avx512-ifma", so issue with "-" vs "_".
Right, it's not perfect, but I don't see much better way if we want to
keep the documentation in sync.
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index f5ce9f97959c..0c7ffff416fe 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
> > if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
> > break;
> > }
> > + if (flag->name == NULL) {
> > + const struct feature_name *feat;
> > + /* Provide a NUL terminated feature name to the search helper. */
> > + char *name = strndup(str, sep - str);
> > +
> > + if (name == NULL)
> > + return 4;
>
> out-of-memory are usually fatal in libxl. Any reason to use `strndup`
> instead of `libxl__strndup`? I guess we don't have any libxl_ctx, so we
> can't use the libxl function.
Yes, even NO_GC requires a CTX.
> So, instead of returning an arbitrary integer that isn't returned yet by
> the function, could you return ERROR_NOMEM?
That would be my preference, but the function already returns
(arbitrary) integers as error codes, so I'm not sure whether
ERROR_NOMEM won't alias with one of the existing error codes.
> I wonder if it would be possible to use a buffer on the stack instead,
> but it might not be worth the effort to find the right size.
Hm, I could in theory try to do that, as feature names tend to be
small, however I think the implementation would be fragile, as the
length of the feature name is based on user input (iow: we would do an
alloca() using a user provided size).
Thanks, Roger.
On Tue, Jun 13, 2023 at 01:01:47PM +0200, Roger Pau Monné wrote:
> On Tue, Jun 13, 2023 at 11:44:51AM +0100, Anthony PERARD wrote:
> > On Mon, Jun 05, 2023 at 12:36:57PM +0200, Roger Pau Monne wrote:
> > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > > index 24ac92718288..c5c5b8716521 100644
> > > --- a/docs/man/xl.cfg.5.pod.in
> > > +++ b/docs/man/xl.cfg.5.pod.in
> > > @@ -2010,24 +2010,12 @@ proccount procpkg stepping
> > >
> > > =back
> > >
> > > -List of keys taking a character:
> > > +List of keys taking a character can be found in the public header file
> > > +L<arch-x86/cpufeatureset.h|http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,cpufeatureset.h.html>
> >
> > https:// ;-)
> >
> > And this probably want to be "xenbits.xenproject.org"
> >
> > Also, I think there's maybe an issue with this link. Maybe someone can
> > assume that "TM1" takes a character, but that flags I think would get
> > rejected, issue with upper case vs lower case. Then, if we deal with
> > the casing, we still have feature like "AVX512_IFMA", which would only
> > be recognize if written "avx512-ifma", so issue with "-" vs "_".
>
> Right, it's not perfect, but I don't see much better way if we want to
> keep the documentation in sync.
I guess we can start with the change you proposed.
Then, maybe adding that the keys needs to be lower case with dash
instead of underscore might be helpful in the man page.
And if we really feel generous, we could just handle both syntax, and
have the function apply the necessary transformation, lower case and
s/_/-/.
> > > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > > index f5ce9f97959c..0c7ffff416fe 100644
> > > --- a/tools/libs/light/libxl_cpuid.c
> > > +++ b/tools/libs/light/libxl_cpuid.c
> > > @@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
> > > if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
> > > break;
> > > }
> > > + if (flag->name == NULL) {
> > > + const struct feature_name *feat;
> > > + /* Provide a NUL terminated feature name to the search helper. */
> > > + char *name = strndup(str, sep - str);
> > > +
> > > + if (name == NULL)
> > > + return 4;
> > So, instead of returning an arbitrary integer that isn't returned yet by
> > the function, could you return ERROR_NOMEM?
>
> That would be my preference, but the function already returns
> (arbitrary) integers as error codes, so I'm not sure whether
> ERROR_NOMEM won't alias with one of the existing error codes.
All libxl error code are negative, so we should be fine.
Cheers,
--
Anthony PERARD
On 05.06.2023 12:36, Roger Pau Monne wrote:
> @@ -51,7 +53,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
> * Used for the static structure describing all features.
> */
> struct cpuid_flags {
> - char* name;
> + const char* name;
Nit: Would you mind also moving * to its designated position at this
occasion?
> @@ -81,6 +83,14 @@ static libxl_cpuid_policy_list cpuid_find_match(libxl_cpuid_policy_list *list,
> return *list + i;
> }
>
> +static int search_feature(const void *a, const void *b)
> +{
> + const char *key = a;
> + const char *feat = ((struct { const char *n; } *)b)->n;
Urgh - what a cast. There's no connection at all between this and
struct feature_name in libxl_cpuid_parse_config(). I think you want
to move that type declaration out of the function, to also use it
here. But if not, comments on both sides are going to be necessary
imo.
> + return strncmp(key, feat, strlen(key)) ?: strlen(key) - strlen(feat);
Why not simply
return strcmp(key, feat);
? Both names are nul-terminated, first and foremost because
otherwise using strlen() wouldn't be appropriate either.
> @@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
> if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
> break;
> }
> + if (flag->name == NULL) {
> + const struct feature_name *feat;
> + /* Provide a NUL terminated feature name to the search helper. */
> + char *name = strndup(str, sep - str);
> +
> + if (name == NULL)
> + return 4;
> +
> + feat = bsearch(name, features, ARRAY_SIZE(features), sizeof(features[0]),
> + search_feature);
> + free(name);
> +
> + if (feat != NULL) {
> + f.name = feat->name;
> + f.leaf = feature_to_cpuid[feat->bit / 32].leaf;
> + f.subleaf = feature_to_cpuid[feat->bit / 32].subleaf;
> + f.reg = feature_to_cpuid[feat->bit / 32].reg;
> + f.bit = feat->bit % 32;
> + f.length = 1,
> + flag = &f;
Nit: Too deep indentation throughout here.
Jan
On Mon, Jun 05, 2023 at 12:36:57PM +0200, Roger Pau Monne wrote:
> @@ -81,6 +83,14 @@ static libxl_cpuid_policy_list cpuid_find_match(libxl_cpuid_policy_list *list,
> return *list + i;
> }
>
> +static int search_feature(const void *a, const void *b)
> +{
> + const char *key = a;
> + const char *feat = ((struct { const char *n; } *)b)->n;
The cast should be (const struct { const char *n; } *) to not drop the
const.
Thanks, Roger.
© 2016 - 2024 Red Hat, Inc.