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