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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

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.