| Message ID | 20230106083826.5384-5-lei4.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for new CPU model SapphireRapids | expand |
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. > + ((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 > >
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 >> >>
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. >>> 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. >>> + ((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 >>> >>>
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 >>>> >>>> >
On 2/9/2023 12:21 PM, Wang, Lei wrote: >>>>> 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. > passing value 0 to KVM is valid, is not the reason why the handling can be skipped. The correct reason is that when value is 0, it means the multi-bit feature is not enabled/requested. It's always legal and doesn't need any handling. So it can be just skipped.
On 2/9/2023 1:59 PM, Xiaoyao Li wrote: > On 2/9/2023 12:21 PM, Wang, Lei wrote: >>>>>> 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. >> > > passing value 0 to KVM is valid, is not the reason why the handling can be skipped. > > The correct reason is that when value is 0, it means the multi-bit feature is > not enabled/requested. It's always legal and doesn't need any handling. So it > can be just skipped. Currently, we can say yes, but I doubt if there will be a multi-bit field in the future that only accepts the non-zero value specified.
On 2/9/2023 2:15 PM, Wang, Lei wrote: > On 2/9/2023 1:59 PM, Xiaoyao Li wrote: >> On 2/9/2023 12:21 PM, Wang, Lei wrote: >>>>>>> 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. >>> >> >> passing value 0 to KVM is valid, is not the reason why the handling can be skipped. >> >> The correct reason is that when value is 0, it means the multi-bit feature is >> not enabled/requested. It's always legal and doesn't need any handling. So it >> can be just skipped. > > Currently, we can say yes, but I doubt if there will be a multi-bit field in the > future that only accepts the non-zero value specified. If so, then the multi-bit field cannot fall into the default handling category and it requires its own special handling callback. It's also the reason I asked "What's the behavior of default ? you need to call out it clearly." This patch should define well the behavior of default handler and describe it in the commit message. Moreover, it's better to comment it as well above the implementation of mark_unavailable_multi_bit_default()
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) && + ((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); }
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(+)