* [PATCH 0/2] Fixes for env->user_features
@ 2020-07-13 17:44 Xiaoyao Li
2020-07-13 17:44 ` [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model Xiaoyao Li
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Xiaoyao Li @ 2020-07-13 17:44 UTC (permalink / raw)
To: Eduardo Habkost, Paolo Bonzini, Richard Henderson; +Cc: Xiaoyao Li, qemu-devel
Patch 1 fixes the env->features set by versioned CPU model.
Patch 2 fixed the env->features set by unavailable_features due to
feature_dependencies[] checking.
Xiaoyao Li (2):
i368/cpu: Clear env->user_features after loading versioned CPU model
i386/cpu: Don't add unavailable_features to env->user_features
target/i386/cpu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
2.18.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model
2020-07-13 17:44 [PATCH 0/2] Fixes for env->user_features Xiaoyao Li
@ 2020-07-13 17:44 ` Xiaoyao Li
2020-07-13 18:44 ` Eduardo Habkost
2020-07-13 17:44 ` [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features Xiaoyao Li
2020-07-13 18:08 ` [PATCH 0/2] Fixes for env->user_features no-reply
2 siblings, 1 reply; 7+ messages in thread
From: Xiaoyao Li @ 2020-07-13 17:44 UTC (permalink / raw)
To: Eduardo Habkost, Paolo Bonzini, Richard Henderson
Cc: Xiaoyao Li, qemu-devel, Chenyi Qiang
Features defined in versioned CPU model are recorded in env->user_features
since they are updated as property. It's unwated because they are not
user specified.
Simply clear env->user_features as a fix. It won't clear user specified
features because user specified features are filled to
env->user_features later in x86_cpu_expand_features().
Cc: Chenyi Qiang <chenyi.qiang@intel.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
target/i386/cpu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1e5123251d74..9812d5747f35 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5159,6 +5159,12 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
x86_cpu_apply_version_props(cpu, model);
+
+ /* Properties in versioned CPU model are not user specified features.
+ * We can simply clear env->user_features here since it will be filled later
+ * in x86_cpu_expand_features() based on plus_features and minus_features.
+ */
+ memset(&env->user_features, 0, sizeof(env->user_features));
}
#ifndef CONFIG_USER_ONLY
--
2.18.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features
2020-07-13 17:44 [PATCH 0/2] Fixes for env->user_features Xiaoyao Li
2020-07-13 17:44 ` [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model Xiaoyao Li
@ 2020-07-13 17:44 ` Xiaoyao Li
2020-07-13 18:48 ` Eduardo Habkost
2020-07-13 18:08 ` [PATCH 0/2] Fixes for env->user_features no-reply
2 siblings, 1 reply; 7+ messages in thread
From: Xiaoyao Li @ 2020-07-13 17:44 UTC (permalink / raw)
To: Eduardo Habkost, Paolo Bonzini, Richard Henderson; +Cc: Xiaoyao Li, qemu-devel
Features unavailable due to absent of their dependent features should
not be added to env->user_features. env->user_features only contains the
feature explicity specified with -feature/+feature by user.
Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism")
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
target/i386/cpu.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9812d5747f35..fb1de1bd6165 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
unavailable_features & env->user_features[d->to.index],
"This feature depends on other features that were not requested");
- env->user_features[d->to.index] |= unavailable_features;
env->features[d->to.index] &= ~unavailable_features;
}
}
--
2.18.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fixes for env->user_features
2020-07-13 17:44 [PATCH 0/2] Fixes for env->user_features Xiaoyao Li
2020-07-13 17:44 ` [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model Xiaoyao Li
2020-07-13 17:44 ` [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features Xiaoyao Li
@ 2020-07-13 18:08 ` no-reply
2 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2020-07-13 18:08 UTC (permalink / raw)
To: xiaoyao.li; +Cc: qemu-devel, pbonzini, xiaoyao.li, ehabkost, rth
Patchew URL: https://patchew.org/QEMU/20200713174436.41070-1-xiaoyao.li@intel.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
TEST iotest-qcow2: 022
TEST check-unit: tests/test-char
**
ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
TEST iotest-qcow2: 024
TEST iotest-qcow2: 025
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=4939dead4d0f403ea41adb4c514af82e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zqji5law/src/docker-src.2020-07-13-13.53.16.24512:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=4939dead4d0f403ea41adb4c514af82e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zqji5law/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 14m59.851s
user 0m9.044s
The full log is available at
http://patchew.org/logs/20200713174436.41070-1-xiaoyao.li@intel.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model
2020-07-13 17:44 ` [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model Xiaoyao Li
@ 2020-07-13 18:44 ` Eduardo Habkost
0 siblings, 0 replies; 7+ messages in thread
From: Eduardo Habkost @ 2020-07-13 18:44 UTC (permalink / raw)
To: Xiaoyao Li; +Cc: Chenyi Qiang, Paolo Bonzini, qemu-devel, Richard Henderson
On Tue, Jul 14, 2020 at 01:44:35AM +0800, Xiaoyao Li wrote:
> Features defined in versioned CPU model are recorded in env->user_features
> since they are updated as property. It's unwated because they are not
> user specified.
>
> Simply clear env->user_features as a fix. It won't clear user specified
> features because user specified features are filled to
> env->user_features later in x86_cpu_expand_features().
>
> Cc: Chenyi Qiang <chenyi.qiang@intel.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> target/i386/cpu.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1e5123251d74..9812d5747f35 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5159,6 +5159,12 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
> object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
>
> x86_cpu_apply_version_props(cpu, model);
> +
> + /* Properties in versioned CPU model are not user specified features.
> + * We can simply clear env->user_features here since it will be filled later
> + * in x86_cpu_expand_features() based on plus_features and minus_features.
> + */
> + memset(&env->user_features, 0, sizeof(env->user_features));
> }
>
> #ifndef CONFIG_USER_ONLY
> --
> 2.18.4
>
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features
2020-07-13 17:44 ` [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features Xiaoyao Li
@ 2020-07-13 18:48 ` Eduardo Habkost
2020-07-27 12:01 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2020-07-13 18:48 UTC (permalink / raw)
To: Xiaoyao Li; +Cc: Paolo Bonzini, qemu-devel, Richard Henderson
On Tue, Jul 14, 2020 at 01:44:36AM +0800, Xiaoyao Li wrote:
> Features unavailable due to absent of their dependent features should
> not be added to env->user_features. env->user_features only contains the
> feature explicity specified with -feature/+feature by user.
>
> Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism")
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Paolo, do you remember why that line existed? It doesn't make
sense to me.
There are exactly 2 lines of code reading user_features, and both
of them are inside x86_cpu_expand_features() above this hunk.
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> target/i386/cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9812d5747f35..fb1de1bd6165 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> unavailable_features & env->user_features[d->to.index],
> "This feature depends on other features that were not requested");
>
> - env->user_features[d->to.index] |= unavailable_features;
> env->features[d->to.index] &= ~unavailable_features;
> }
> }
> --
> 2.18.4
>
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features
2020-07-13 18:48 ` Eduardo Habkost
@ 2020-07-27 12:01 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2020-07-27 12:01 UTC (permalink / raw)
To: Eduardo Habkost, Xiaoyao Li; +Cc: qemu-devel, Richard Henderson
On 13/07/20 20:48, Eduardo Habkost wrote:
> On Tue, Jul 14, 2020 at 01:44:36AM +0800, Xiaoyao Li wrote:
>> Features unavailable due to absent of their dependent features should
>> not be added to env->user_features. env->user_features only contains the
>> feature explicity specified with -feature/+feature by user.
>>
>> Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism")
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>
> Paolo, do you remember why that line existed? It doesn't make
> sense to me.
>
> There are exactly 2 lines of code reading user_features, and both
> of them are inside x86_cpu_expand_features() above this hunk.
I think it was just to be safe in case in the future something else adds
features automatically, in the same way as the cpu->max_features case:
env->features[w] |=
x86_cpu_get_supported_feature_word(w, cpu->migratable) &
~env->user_features[w] &
~feature_word_info[w].no_autoenable_flags;
It would prevent the unavailable dependent features from being added.
But yeah, it would just be enough to place it above this hunk.
Paolo
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
>> ---
>> target/i386/cpu.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 9812d5747f35..fb1de1bd6165 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6370,7 +6370,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>> unavailable_features & env->user_features[d->to.index],
>> "This feature depends on other features that were not requested");
>>
>> - env->user_features[d->to.index] |= unavailable_features;
>> env->features[d->to.index] &= ~unavailable_features;
>> }
>> }
>> --
>> 2.18.4
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-27 12:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 17:44 [PATCH 0/2] Fixes for env->user_features Xiaoyao Li
2020-07-13 17:44 ` [PATCH 1/2] i368/cpu: Clear env->user_features after loading versioned CPU model Xiaoyao Li
2020-07-13 18:44 ` Eduardo Habkost
2020-07-13 17:44 ` [PATCH 2/2] i386/cpu: Don't add unavailable_features to env->user_features Xiaoyao Li
2020-07-13 18:48 ` Eduardo Habkost
2020-07-27 12:01 ` Paolo Bonzini
2020-07-13 18:08 ` [PATCH 0/2] Fixes for env->user_features no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.