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