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.edmond...@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
