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

[Eduardo Habkost]

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.

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

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