[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] libs/light: use the cpuid feature names from cpufeatureset.h
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |