Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature

[David Edmondson <dme@dme.org>]

On Tuesday, 2021-01-19 at 10:20:56 -05, Eduardo Habkost wrote:

> Hi,
>
> Thanks for the patch.  Getting rid of special -feature/+feature
> behavior was in our TODO list for a long time.
>
> On Tue, Jan 19, 2021 at 02:22:06PM +0000, David Edmondson wrote:
>> "Minus" features are applied after "plus" features, so ensure that a
>> later "plus" feature causes an earlier "minus" feature to be removed.
>> 
>> This has no effect on the existing "-feature,feature=on" backward
>> compatibility code (which warns and turns the feature off).
>
> If we are changing behavior, why not change behavior of
> "-feature,feature=on" at the same time?  This would allow us to
> get rid of plus_features/minus_features completely and just make
> +feature/-feature synonyms to feature=on/feature=off.

Okay, I'll do that.

Given that there have been warnings associated with
"-feature,feature=on" for a while, changing that behaviour seems
acceptable.

Would the same be true for changing "-feature,+feature"? (i.e. what this
patch does) Really: can this just be changed, or does there have to be
some period where the behaviour stays the same with a warning?

>> 
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  target/i386/cpu.c                   | 33 +++++++++++++++++++++++------
>>  tests/qtest/test-x86-cpuid-compat.c |  8 +++----
>>  2 files changed, 30 insertions(+), 11 deletions(-)
>> 
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 35459a38bb..13f58ef183 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4750,13 +4750,32 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
>>          GlobalProperty *prop;
>>  
>>          /* Compatibility syntax: */
>> -        if (featurestr[0] == '+') {
>> -            plus_features = g_list_append(plus_features,
>> -                                          g_strdup(featurestr + 1));
>> -            continue;
>> -        } else if (featurestr[0] == '-') {
>> -            minus_features = g_list_append(minus_features,
>> -                                           g_strdup(featurestr + 1));
>> +        if (featurestr[0] == '+' || featurestr[0] == '-') {
>> +            const char *feat = featurestr + 1;
>> +            GList **remove, **add;
>> +            GList *val;
>> +
>> +            if (featurestr[0] == '+') {
>> +                remove = &minus_features;
>> +                add = &plus_features;
>> +            } else {
>> +                remove = &plus_features;
>> +                add = &minus_features;
>> +            }
>> +
>> +            val = g_list_find_custom(*remove, feat, compare_string);
>> +            if (val) {
>> +                char *data = val->data;
>> +
>> +                *remove = g_list_remove(*remove, data);
>> +                g_free(data);
>> +            }
>> +
>> +            val = g_list_find_custom(*add, feat, compare_string);
>> +            if (!val) {
>> +                *add = g_list_append(*add, g_strdup(feat));
>> +            }
>
> I believe we'll be able to get rid of plus_features/minus_features
> completely if we remove compatibility of "-feature,feature=on".
> But if we don't, wouldn't it be simpler to replace
> plus_features/minus_features with a single list, appending items
> in the order they appear?

Will investigate.

>> +
>>              continue;
>>          }
>>  
>> diff --git a/tests/qtest/test-x86-cpuid-compat.c b/tests/qtest/test-x86-cpuid-compat.c
>> index 7ca1883a29..6824d2b13e 100644
>> --- a/tests/qtest/test-x86-cpuid-compat.c
>> +++ b/tests/qtest/test-x86-cpuid-compat.c
>> @@ -171,18 +171,18 @@ static void test_plus_minus_subprocess(void)
>>      char *path;
>>  
>>      /* Rules:
>> -     * 1)"-foo" overrides "+foo"
>> +     * 1) The later of "+foo" or "-foo" wins
>>       * 2) "[+-]foo" overrides "foo=..."
>>       * 3) Old feature names with underscores (e.g. "sse4_2")
>>       *    should keep working
>>       *
>> -     * Note: rules 1 and 2 are planned to be removed soon, and
>> -     * should generate a warning.
>> +     * Note: rule 2 is planned to be removed soon, and should generate
>> +     * a warning.
>>       */
>>      qtest_start("-cpu pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on");
>>      path = get_cpu0_qom_path();
>>  
>> -    g_assert_false(qom_get_bool(path, "fpu"));
>> +    g_assert_true(qom_get_bool(path, "fpu"));
>>      g_assert_false(qom_get_bool(path, "mce"));
>>      g_assert_true(qom_get_bool(path, "cx8"));
>>  
>> -- 
>> 2.29.2
>> 
>
> -- 
> Eduardo

dme.
-- 
They must have taken my marbles away.