[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+featu
From: |
David Edmondson |
Subject: |
Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature |
Date: |
Tue, 19 Jan 2021 16:27:56 +0000 |
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.
- [RFC PATCH 0/2] x86 CPU feature +/- fiddling and +kvm-no-defaults, David Edmondson, 2021/01/19
- [RFC PATCH 2/2] target/i386: Add "-cpu +kvm-no-defaults", David Edmondson, 2021/01/19
- [RFC PATCH 1/2] hw/i386: -cpu model, -feature, +feature should enable feature, David Edmondson, 2021/01/19
- Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Eduardo Habkost, 2021/01/19
- Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature,
David Edmondson <=
- Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Eduardo Habkost, 2021/01/19
- Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Igor Mammedov, 2021/01/20
- Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, David Edmondson, 2021/01/20
- Re: [External] : Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Daniel P . Berrangé, 2021/01/20
- Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, David Edmondson, 2021/01/20
- Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Eduardo Habkost, 2021/01/20
- Re: [RFC PATCH 1/2] hw/i386: -cpu model,-feature,+feature should enable feature, Igor Mammedov, 2021/01/20
- [PATCH] docs/system: Deprecate `-cpu ...,+feature,-feature` syntax, Eduardo Habkost, 2021/01/20
- Re: [PATCH] docs/system: Deprecate `-cpu ..., +feature, -feature` syntax, David Edmondson, 2021/01/20
- Re: [PATCH] docs/system: Deprecate `-cpu ...,+feature,-feature` syntax, Daniel P . Berrangé, 2021/01/21