| Message ID | 20200713174436.41070-3-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for env->user_features | expand |
On Tue, Jul 14, 2020 at 01:44:36AM +0800, Xiaoyao Li wrote: > Features unavailable due to absent of their dependent features should > not be added to env->user_features. env->user_features only contains the > feature explicity specified with -feature/+feature by user. > > Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism") > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> Paolo, do you remember why that line existed? It doesn't make sense to me. There are exactly 2 lines of code reading user_features, and both of them are inside x86_cpu_expand_features() above this hunk. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target/i386/cpu.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 9812d5747f35..fb1de1bd6165 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) > unavailable_features & env->user_features[d->to.index], > "This feature depends on other features that were not requested"); > > - env->user_features[d->to.index] |= unavailable_features; > env->features[d->to.index] &= ~unavailable_features; > } > } > -- > 2.18.4 >
On 13/07/20 20:48, Eduardo Habkost wrote: > On Tue, Jul 14, 2020 at 01:44:36AM +0800, Xiaoyao Li wrote: >> Features unavailable due to absent of their dependent features should >> not be added to env->user_features. env->user_features only contains the >> feature explicity specified with -feature/+feature by user. >> >> Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism") >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > Paolo, do you remember why that line existed? It doesn't make > sense to me. > > There are exactly 2 lines of code reading user_features, and both > of them are inside x86_cpu_expand_features() above this hunk. I think it was just to be safe in case in the future something else adds features automatically, in the same way as the cpu->max_features case: env->features[w] |= x86_cpu_get_supported_feature_word(w, cpu->migratable) & ~env->user_features[w] & ~feature_word_info[w].no_autoenable_flags; It would prevent the unavailable dependent features from being added. But yeah, it would just be enough to place it above this hunk. Paolo > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >> --- >> target/i386/cpu.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 9812d5747f35..fb1de1bd6165 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) >> unavailable_features & env->user_features[d->to.index], >> "This feature depends on other features that were not requested"); >> >> - env->user_features[d->to.index] |= unavailable_features; >> env->features[d->to.index] &= ~unavailable_features; >> } >> } >> -- >> 2.18.4 >> >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 9812d5747f35..fb1de1bd6165 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) unavailable_features & env->user_features[d->to.index], "This feature depends on other features that were not requested"); - env->user_features[d->to.index] |= unavailable_features; env->features[d->to.index] &= ~unavailable_features; } }
Features unavailable due to absent of their dependent features should not be added to env->user_features. env->user_features only contains the feature explicity specified with -feature/+feature by user. Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism") Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- target/i386/cpu.c | 1 - 1 file changed, 1 deletion(-)