| Message ID | 1517da3b-7d39-47b9-2714-d97dac217678@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl/CPUID: drop two more feature flag table entries | expand |
On Wed, Aug 23, 2023 at 09:21:26AM +0200, Jan Beulich wrote: > Two entries were left in place by d638fe233cb3 ("libxl: use the cpuid > feature names from cpufeatureset.h"), despite matching the generated > names. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Anthony PERARD <anthony.perard@citrix.com> > --- > Permitting "psn", "ia64, "cntxid", and "perfctr_*" when the hypervisor > doesn't even know of the bits (perhaps wrongly so) is kind of odd as > well. Permitting bits like "est", which the hypervisor knows of but > doesn't expose to guests, is also questionable. I think those are just aliases, to a specific bit in a bitmap. Even if we remove those aliases, it is still possible to instruct libxl to do something with those bits. So there presence isn't permission, I don't think. Looks like "est" is just an alias for "eist", so it doesn't seems useful to remove it either. Thanks,
On 23.08.2023 15:45, Anthony PERARD wrote: > On Wed, Aug 23, 2023 at 09:21:26AM +0200, Jan Beulich wrote: >> Two entries were left in place by d638fe233cb3 ("libxl: use the cpuid >> feature names from cpufeatureset.h"), despite matching the generated >> names. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks. >> --- >> Permitting "psn", "ia64, "cntxid", and "perfctr_*" when the hypervisor >> doesn't even know of the bits (perhaps wrongly so) is kind of odd as >> well. Permitting bits like "est", which the hypervisor knows of but >> doesn't expose to guests, is also questionable. > > I think those are just aliases, to a specific bit in a bitmap. Even if > we remove those aliases, it is still possible to instruct libxl to do > something with those bits. So there presence isn't permission, I don't > think. It's not permission, but recognizing the field name when its use then (in the best case) doesn't do anything is at least misleading. > Looks like "est" is just an alias for "eist", so it doesn't seems useful > to remove it either. Well, I'm effectively also questioning the exposure of "eist". Using INIT_FEATURE_NAMES here was likely okay as a quick first approach, but I think we want to limit what is recognized to what actually is useful in at least some cases (yet better would of course be to also not recognize e.g. HVM-only options when we're dealing with a PV guest). Jan
--- a/tools/libs/light/libxl_cpuid.c +++ b/tools/libs/light/libxl_cpuid.c @@ -260,7 +260,6 @@ int libxl_cpuid_parse_config(libxl_cpuid {"clfsh", 0x00000001, NA, CPUID_REG_EDX, 19, 1}, {"tm", 0x00000001, NA, CPUID_REG_EDX, 29, 1}, {"ia64", 0x00000001, NA, CPUID_REG_EDX, 30, 1}, - {"pbe", 0x00000001, NA, CPUID_REG_EDX, 31, 1}, {"arat", 0x00000006, NA, CPUID_REG_EAX, 2, 1}, @@ -279,7 +278,6 @@ int libxl_cpuid_parse_config(libxl_cpuid {"invtsc", 0x80000007, NA, CPUID_REG_EDX, 8, 1}, {"ppin", 0x80000008, NA, CPUID_REG_EBX, 23, 1}, - {"btc-no", 0x80000008, NA, CPUID_REG_EBX, 29, 1}, {"nc", 0x80000008, NA, CPUID_REG_ECX, 0, 8}, {"apicidsize", 0x80000008, NA, CPUID_REG_ECX, 12, 4},
Two entries were left in place by d638fe233cb3 ("libxl: use the cpuid feature names from cpufeatureset.h"), despite matching the generated names. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Permitting "psn", "ia64, "cntxid", and "perfctr_*" when the hypervisor doesn't even know of the bits (perhaps wrongly so) is kind of odd as well. Permitting bits like "est", which the hypervisor knows of but doesn't expose to guests, is also questionable. I wouldn't really call this a bug fix (the entries are simply redundant, but nothing bad would happen with them there), hence no Fixes: tag.