|
From: | Xiaoyao Li |
Subject: | Re: [PATCH v3 4/6] i386: Mask and report unavailable multi-bit feature values |
Date: | Thu, 9 Feb 2023 11:29:06 +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/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
[Prev in Thread] | Current Thread | [Next in Thread] |