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

[Wang, Lei]

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.
>>
>> 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.
>>
>> 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.

>> +        ((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
>>
>>