Re: [PATCH v3 4/6] i386: Mask and report unavailable multi-bit feature values

[Wang, Lei]

On 2/9/2023 11:29 AM, Xiaoyao Li wrote:
> On 2/9/2023 9:04 AM, Wang, Lei wrote:
>> On 2/6/2023 3:43 PM, Yuan Yao wrote:
>>> On Fri, Jan 06, 2023 at 12:38:24AM -0800, Lei Wang wrote:
>>>> Some feature words, e.g., feature words in AMX-related CPUID leaf 0x1D and
>>>> 0x1E are not bit-wise but multiple bits represents one value. Handle this
>>>> situation when the values specified are not the same as which are reported
>>>> by KVM. The handling includes:
>>>>
>>>>   - The responsibility of masking bits and giving warnings are delegated to
>>>>     the feature enabler. A framework is also provided to enable this.
>>>>   - To simplify the initialization, a default function is provided if the
>>>>     the function is not specified.
> 
> What's the behavior of default ? you need to call out it clearly.
> 
>>>> The reason why delegating this responsibility rather than just marking
>>>> them as zeros when they are not same is because different multi-bit
>>>> features may have different logic, which is case by case, for example:
>>>>
>>>>   1. CPUID.0x14_0x1:EBX[15:0]. Even though it's multi-bits field, it's a
>>>>      bitmap and each bit represents a separate capability.
>>>>
>>>>   2. CPUID.0x14_0x1:EAX[2:0] represents the number of configurable Address
>>>>      Ranges. 3 bits as a whole to represent a integer value. It means the
>>>>      maximum capability of HW. If KVM reports M, then M to 0 is legal
>>>>      value to configure (because KVM can emulate each value correctly).
>>>>
>>>>   3. CPUID.0x1D_0x1:EAX[31:16] represents palette 1 bytes_per_tile. 16 bits
>>>>      as a whole represent an integer value. It's not like case 2 and SW
>>>>      needs to configure the same value as reported. Because it's not
>>>>      possible for SW to configure to a different value and KVM cannot
>>>>      emulate it.
>>>>
>>>> So marking them blindly as zeros is incorrect, and delegating this
>>>> responsibility can let each multi-bit feature have its own way to mask 
>>>> bits.
> 
> you can first describe there are such 3 cases of multi-bit features and they
> need different handling for checking whether configured value is supported by
> KVM or not. So introduce a handling callback function that each multi-bit
> feature can implement their own. Meanwhile, provide a default handling 
> callback
> that handles case x: when configured value is not the same as the one reported
> by KVM, clearing it to zero to mark it as unavailable.

OK, will rephrase the commit message.

> 
>>>> Signed-off-by: Lei Wang <lei4.wang@intel.com>
>>>> ---
>>>>   target/i386/cpu-internal.h |  2 ++
>>>>   target/i386/cpu.c          | 36 ++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
>>>> index 66b3d66cb4..83c7b53926 100644
>>>> --- a/target/i386/cpu-internal.h
>>>> +++ b/target/i386/cpu-internal.h
>>>> @@ -30,6 +30,8 @@ typedef struct MultiBitFeatureInfo {
>>>>       uint64_t mask;
>>>>       unsigned high_bit_position;
>>>>       unsigned low_bit_position;
>>>> +    void (*mark_unavailable_multi_bit)(X86CPU *cpu, FeatureWord w, int 
>>>> index,
>>>> +                                       const char *verbose_prefix);
>>>>   } MultiBitFeatureInfo;
>>>>
>>>>   typedef struct FeatureWordInfo {
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index 88aa780566..e638a31d34 100644
>>>> --- a/target/i386/cpu.c
>>>> +++ b/target/i386/cpu.c
>>>> @@ -4377,6 +4377,28 @@ static bool x86_cpu_have_filtered_features(X86CPU 
>>>> *cpu)
>>>>       return false;
>>>>   }
>>>>
>>>> +static void mark_unavailable_multi_bit_default(X86CPU *cpu, FeatureWord w,
>>>> +                                               int index,
>>>> +                                               const char *verbose_prefix)
>>>> +{
>>>> +    FeatureWordInfo *f = &feature_word_info[w];
>>>> +    g_autofree char *feat_word_str = feature_word_description(f);
>>>> +    uint64_t host_feat = x86_cpu_get_supported_feature_word(w, false);
>>>> +    MultiBitFeatureInfo mf = f->multi_bit_features[index];
>>>> +
>>>> +    if ((cpu->env.features[w] & mf.mask) &&
>>>
>>> With this checking bits are all 0 but covered by mf.mask's range are 
>>> skippped,
>>> even if they're different from the host_feat, please check whether it's 
>>> desried
>>> behavior.
>>
>> This is the intended design because there are quite a number of multi-bit 
>> CPUIDs
>> should support passing all 0 to them.
> 
> you didn't answer the question. The question is why the handling can be 
> skipped
> when the value of multi-bit feature is 0.

I think the default function should handle the most common case, which is,
passing all 0 multi-bits to KVM is valid and shouldn't be warned. E.g, when we
are using some earlier CPU models which doesn't support AMX, we shouldn't be
warned that all 0 values don't match the CPUID reported by KVM.

> 
>>>> +        ((cpu->env.features[w] ^ host_feat) & mf.mask)) {
>>>> +        if (!cpu->force_features) {
>>>> +            cpu->env.features[w] &= ~mf.mask;
>>>> +        }
>>>> +        cpu->filtered_features[w] |= mf.mask;
>>>> +        if (verbose_prefix)
>>>> +            warn_report("%s: %s.%s [%u:%u]", verbose_prefix, 
>>>> feat_word_str,
>>>> +                        mf.feat_name, mf.high_bit_position,
>>>> +                        mf.low_bit_position);
>>>> +    }
>>>> +}
>>>> +
>>>>   static void mark_unavailable_features(X86CPU *cpu, FeatureWord w, 
>>>> uint64_t
>>>> mask,
>>>>                                         const char *verbose_prefix)
>>>>   {
>>>> @@ -6442,6 +6464,20 @@ static void x86_cpu_filter_features(X86CPU *cpu, 
>>>> bool
>>>> verbose)
>>>>               x86_cpu_get_supported_feature_word(w, false);
>>>>           uint64_t requested_features = env->features[w];
>>>>           uint64_t unavailable_features = requested_features & ~host_feat;
>>>> +        FeatureWordInfo f = feature_word_info[w];
>>>> +        int i;
>>>> +
>>>> +        for (i = 0; i < f.num_multi_bit_features; i++) {
>>>> +            MultiBitFeatureInfo mf = f.multi_bit_features[i];
>>>> +            if (mf.mark_unavailable_multi_bit) {
>>>> +                mf.mark_unavailable_multi_bit(cpu, w, i, prefix);
>>>> +            } else {
>>>> +                mark_unavailable_multi_bit_default(cpu, w, i, prefix);
>>>> +            }
>>>> +
>>>> +            unavailable_features &= ~mf.mask;
>>>> +        }
>>>> +
>>>>           mark_unavailable_features(cpu, w, unavailable_features, prefix);
>>>>       }
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>