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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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