Re: [PATCH] libs/light: use the cpuid feature names from cpufeatureset.h

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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