[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 0/6] Support for new CPU model SapphireRapids
From: |
Wang, Lei |
Subject: |
Re: [PATCH v3 0/6] Support for new CPU model SapphireRapids |
Date: |
Tue, 7 Feb 2023 10:50:56 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0 Thunderbird/102.6.1 |
On 2/2/2023 7:05 PM, Igor Mammedov wrote:
> On Fri, 6 Jan 2023 00:38:20 -0800
> Lei Wang <lei4.wang@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.wang@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.wang@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.wang@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.wang@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
>