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

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

-- 
Eduardo

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