On 2/2/2023 7:05 PM, Igor Mammedov wrote: > On Fri, 6 Jan 2023 00:38:20 -0800 > Lei Wang <lei4.w...@intel.com> wrote: > >> This series aims to add a new CPU model SapphireRapids, and tries to >> address the problem stated in >> https://lore.kernel.org/all/20220812055751.14553-1-lei4.w...@intel.com/T/#mcf67dbd1ad37c65d7988c36a2b267be9afd2fb30, >> so that named CPU model can define its own AMX values, and QEMU won't >> pass the wrong AMX values to KVM in future platforms if they have >> different values supported. >> >> The original patch is >> https://lore.kernel.org/all/20220812055751.14553-1-lei4.w...@intel.com/T/#u. > > MultiBitFeatureInfo looks like an interesting > idea but among fixing whatever issues this has atm, > you'd probably need to introduce a new qdev_bitfield property > infrastructure so that such features could be treated like any > other qdev properties. > Also when MultiBitFeatureInfo is added, one should convert all > other usecases where it's applicable (not only for new code) > so that we would end up with consolidated approach instead of > zoo mess. > > I'm not sure all MultiBitFeatureInfo complexity is necessary > just for adding a new CPU model, I'd rather use ad hoc approach > that we were using before for non boolean features.
Hi, Igor. I do not quite understand what does the "ad hoc approach" mean, currently if we specify a multi-bit non-boolean CPUID value which is different from the host value to CPU model, e.g., consider the following scenario: - KVM **ONLY** supports value 5 (101) and, - QEMU user want to pass value 3 (011) to it, and follow the current logic: uint64_t unavailable_features = requested_features & ~host_feat; then: 1. The warning message will be messy and not intuitive: requested_features bit 1 is 1 and host_feat bit 1 is 0, so it will warn on this non-sense bit. 2. Some CPUID bits will "leak" into the final CPUID passed to KVM: requested_features bit 0 is 1 and host_feat bit 0 is also 1, so it will pass this CPUID bit to host, the request_features value is 3 (011), finally we get 1 (001), this is not we want. Am I understanding it correctly? > > And then try to develop MultiBitFeatureInfo & co as a separate > series to demonstrate how much it will improve current > cpu models definitions. > > PS: > 'make check-acceptance' are broken with this > >> --- >> >> Changelog: >> >> v3: >> - Rebase on the latest QEMU (d1852caab131ea898134fdcea8c14bc2ee75fbe9). >> - v2: >> https://lore.kernel.org/qemu-devel/20221102085256.81139-1-lei4.w...@intel.com/ >> >> v2: >> - Fix when passing all zeros of AMX-related CPUID, QEMU will warn >> unsupported. >> - Remove unnecessary function definition and make code cleaner. >> - Fix some typos. >> - v1: >> https://lore.kernel.org/qemu-devel/20221027020036.373140-1-lei4.w...@intel.com/T/#t >> >> >> Lei Wang (6): >> i386: Introduce FeatureWordInfo for AMX CPUID leaf 0x1D and 0x1E >> i386: Remove unused parameter "uint32_t bit" in >> feature_word_description() >> i386: Introduce new struct "MultiBitFeatureInfo" for multi-bit >> features >> i386: Mask and report unavailable multi-bit feature values >> i386: Initialize AMX CPUID leaves with corresponding env->features[] >> leaves >> i386: Add new CPU model SapphireRapids >> >> target/i386/cpu-internal.h | 11 ++ >> target/i386/cpu.c | 311 +++++++++++++++++++++++++++++++++++-- >> target/i386/cpu.h | 16 ++ >> 3 files changed, 322 insertions(+), 16 deletions(-) >> >> >> base-commit: d1852caab131ea898134fdcea8c14bc2ee75fbe9 >