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