[PATCH] target-i386: fix losing XCR0 processor state component bits

[PATCH] target-i386: fix losing XCR0 processor state component bits

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Commit 96193c22a "target-i386: Move xsave component mask to features array"
leverages features array to handle XCR0 processor state component bits, 
however, it introduces a regression:

warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]

My desktop doesn't have enough advance features, so just X87,SSE,AVX 
warnings are splat when I boot a guest.

The get migratable flags logic in x86_cpu_filter_features() path will 
filter out the feature flags which are unsupported and unmigratable. 
However, the bits of XCR0 processor state component featureword don't 
have feat_names, and some features like SSE/AVX etc have feat_names in 
CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.

This patch fix it by don't filter out XCR0 processor state components 
bits though they don't have feat_names just as before commit 96193c22ab3.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 target-i386/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ad09246..9d24eff 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
         r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
                                                     wi->cpuid_ecx,
                                                     wi->cpuid_reg);
+        if ((w == FEAT_XSAVE_COMP_LO) ||
+            (w == FEAT_XSAVE_COMP_HI)) {
+            return r;
+        }
     } else if (tcg_enabled()) {
         r = wi->tcg_features;
     } else {
-- 
1.9.1

[Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Commit 96193c22a "target-i386: Move xsave component mask to features array"
leverages features array to handle XCR0 processor state component bits, 
however, it introduces a regression:

warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]

My desktop doesn't have enough advance features, so just X87,SSE,AVX 
warnings are splat when I boot a guest.

The get migratable flags logic in x86_cpu_filter_features() path will 
filter out the feature flags which are unsupported and unmigratable. 
However, the bits of XCR0 processor state component featureword don't 
have feat_names, and some features like SSE/AVX etc have feat_names in 
CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.

This patch fix it by don't filter out XCR0 processor state components 
bits though they don't have feat_names just as before commit 96193c22ab3.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 target-i386/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ad09246..9d24eff 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
         r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
                                                     wi->cpuid_ecx,
                                                     wi->cpuid_reg);
+        if ((w == FEAT_XSAVE_COMP_LO) ||
+            (w == FEAT_XSAVE_COMP_HI)) {
+            return r;
+        }
     } else if (tcg_enabled()) {
         r = wi->tcg_features;
     } else {
-- 
1.9.1

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 07:31, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Commit 96193c22a "target-i386: Move xsave component mask to features array"
> leverages features array to handle XCR0 processor state component bits, 
> however, it introduces a regression:
> 
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
> 
> My desktop doesn't have enough advance features, so just X87,SSE,AVX 
> warnings are splat when I boot a guest.
> 
> The get migratable flags logic in x86_cpu_filter_features() path will 
> filter out the feature flags which are unsupported and unmigratable. 
> However, the bits of XCR0 processor state component featureword don't 
> have feat_names, and some features like SSE/AVX etc have feat_names in 
> CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.
> 
> This patch fix it by don't filter out XCR0 processor state components 
> bits though they don't have feat_names just as before commit 96193c22ab3.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  target-i386/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ad09246..9d24eff 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>          r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                      wi->cpuid_ecx,
>                                                      wi->cpuid_reg);
> +        if ((w == FEAT_XSAVE_COMP_LO) ||
> +            (w == FEAT_XSAVE_COMP_HI)) {
> +            return r;
> +        }
>      } else if (tcg_enabled()) {
>          r = wi->tcg_features;
>      } else {
> 

I think the right place to add the test is x86_cpu_get_migratable_flags.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 07:31, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Commit 96193c22a "target-i386: Move xsave component mask to features array"
> leverages features array to handle XCR0 processor state component bits, 
> however, it introduces a regression:
> 
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
> 
> My desktop doesn't have enough advance features, so just X87,SSE,AVX 
> warnings are splat when I boot a guest.
> 
> The get migratable flags logic in x86_cpu_filter_features() path will 
> filter out the feature flags which are unsupported and unmigratable. 
> However, the bits of XCR0 processor state component featureword don't 
> have feat_names, and some features like SSE/AVX etc have feat_names in 
> CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.
> 
> This patch fix it by don't filter out XCR0 processor state components 
> bits though they don't have feat_names just as before commit 96193c22ab3.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  target-i386/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ad09246..9d24eff 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>          r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                      wi->cpuid_ecx,
>                                                      wi->cpuid_reg);
> +        if ((w == FEAT_XSAVE_COMP_LO) ||
> +            (w == FEAT_XSAVE_COMP_HI)) {
> +            return r;
> +        }
>      } else if (tcg_enabled()) {
>          r = wi->tcg_features;
>      } else {
> 

I think the right place to add the test is x86_cpu_get_migratable_flags.

Paolo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Wanpeng Li]

2016-09-28 15:54 GMT+08:00 Paolo Bonzini <bonzini@gnu.org>:
[...]
> I think the right place to add the test is x86_cpu_get_migratable_flags.

I just sent out v2 to handle this, thanks for pointing out.

Regards,
Wanpeng Li

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Wanpeng Li]

2016-09-28 15:54 GMT+08:00 Paolo Bonzini <bonzini@gnu.org>:
[...]
> I think the right place to add the test is x86_cpu_get_migratable_flags.

I just sent out v2 to handle this, thanks for pointing out.

Regards,
Wanpeng Li

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 09:54:27AM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 07:31, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng.li@hotmail.com>
> > 
> > Commit 96193c22a "target-i386: Move xsave component mask to features array"
> > leverages features array to handle XCR0 processor state component bits, 
> > however, it introduces a regression:
> > 
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
> > 
> > My desktop doesn't have enough advance features, so just X87,SSE,AVX 
> > warnings are splat when I boot a guest.

Oops. I assume this is reproducible only using "-cpu host"?

> > 
> > The get migratable flags logic in x86_cpu_filter_features() path will 
> > filter out the feature flags which are unsupported and unmigratable. 
> > However, the bits of XCR0 processor state component featureword don't 
> > have feat_names, and some features like SSE/AVX etc have feat_names in 
> > CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.
> > 
> > This patch fix it by don't filter out XCR0 processor state components 
> > bits though they don't have feat_names just as before commit 96193c22ab3.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> > ---
> >  target-i386/cpu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ad09246..9d24eff 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> >          r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> >                                                      wi->cpuid_ecx,
> >                                                      wi->cpuid_reg);
> > +        if ((w == FEAT_XSAVE_COMP_LO) ||
> > +            (w == FEAT_XSAVE_COMP_HI)) {
> > +            return r;
> > +        }
> >      } else if (tcg_enabled()) {
> >          r = wi->tcg_features;
> >      } else {
> > 
> 
> I think the right place to add the test is x86_cpu_get_migratable_flags.

This can be fixed by adding actual property names to the
FEAT_XSAVE_COMP_* bits. This way we will be able to report
meaningful names to management in case GET_SUPPORTED_CPUID says a
given xsave component is not supported yet, and will ensure we
correctly treat still-unknown xsave components as unmigratable.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 09:54:27AM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 07:31, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng.li@hotmail.com>
> > 
> > Commit 96193c22a "target-i386: Move xsave component mask to features array"
> > leverages features array to handle XCR0 processor state component bits, 
> > however, it introduces a regression:
> > 
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
> > warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
> > 
> > My desktop doesn't have enough advance features, so just X87,SSE,AVX 
> > warnings are splat when I boot a guest.

Oops. I assume this is reproducible only using "-cpu host"?

> > 
> > The get migratable flags logic in x86_cpu_filter_features() path will 
> > filter out the feature flags which are unsupported and unmigratable. 
> > However, the bits of XCR0 processor state component featureword don't 
> > have feat_names, and some features like SSE/AVX etc have feat_names in 
> > CPUID.01H:EDX, CPUID.01H:ECX, so they are treated as unsupported.
> > 
> > This patch fix it by don't filter out XCR0 processor state components 
> > bits though they don't have feat_names just as before commit 96193c22ab3.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> > ---
> >  target-i386/cpu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ad09246..9d24eff 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2156,6 +2156,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> >          r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> >                                                      wi->cpuid_ecx,
> >                                                      wi->cpuid_reg);
> > +        if ((w == FEAT_XSAVE_COMP_LO) ||
> > +            (w == FEAT_XSAVE_COMP_HI)) {
> > +            return r;
> > +        }
> >      } else if (tcg_enabled()) {
> >          r = wi->tcg_features;
> >      } else {
> > 
> 
> I think the right place to add the test is x86_cpu_get_migratable_flags.

This can be fixed by adding actual property names to the
FEAT_XSAVE_COMP_* bits. This way we will be able to report
meaningful names to management in case GET_SUPPORTED_CPUID says a
given xsave component is not supported yet, and will ensure we
correctly treat still-unknown xsave components as unmigratable.

-- 
Eduardo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 16:57, Eduardo Habkost wrote:\
> This can be fixed by adding actual property names to the
> FEAT_XSAVE_COMP_* bits. This way we will be able to report
> meaningful names to management in case GET_SUPPORTED_CPUID says a
> given xsave component is not supported yet, and will ensure we
> correctly treat still-unknown xsave components as unmigratable.

Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
marshals and unmarshals the registers out and back into the xsave data,
so that unknown features are indeed unmigratable.

But are the property names necessary?  It makes no sense to
enable/disable XSAVE components separately from the other CPUID bits
that enable them.  Could we just mark all unknown features as
unmigratable without giving them names?

Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 16:57, Eduardo Habkost wrote:\
> This can be fixed by adding actual property names to the
> FEAT_XSAVE_COMP_* bits. This way we will be able to report
> meaningful names to management in case GET_SUPPORTED_CPUID says a
> given xsave component is not supported yet, and will ensure we
> correctly treat still-unknown xsave components as unmigratable.

Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
marshals and unmarshals the registers out and back into the xsave data,
so that unknown features are indeed unmigratable.

But are the property names necessary?  It makes no sense to
enable/disable XSAVE components separately from the other CPUID bits
that enable them.  Could we just mark all unknown features as
unmigratable without giving them names?

Paolo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 05:01:01PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 16:57, Eduardo Habkost wrote:\
> > This can be fixed by adding actual property names to the
> > FEAT_XSAVE_COMP_* bits. This way we will be able to report
> > meaningful names to management in case GET_SUPPORTED_CPUID says a
> > given xsave component is not supported yet, and will ensure we
> > correctly treat still-unknown xsave components as unmigratable.
> 
> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> marshals and unmarshals the registers out and back into the xsave data,
> so that unknown features are indeed unmigratable.
> 
> But are the property names necessary?  It makes no sense to
> enable/disable XSAVE components separately from the other CPUID bits
> that enable them.  Could we just mark all unknown features as
> unmigratable without giving them names?

We could, as we don't really need to make them configurable. But
giving them names will also allow us to return more useful data
to libvirt in case GET_SUPPORTED_CPUID returns some bits as
unsupported. The new CPU runnability/comparison APIs are all
based on property names.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 05:01:01PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 16:57, Eduardo Habkost wrote:\
> > This can be fixed by adding actual property names to the
> > FEAT_XSAVE_COMP_* bits. This way we will be able to report
> > meaningful names to management in case GET_SUPPORTED_CPUID says a
> > given xsave component is not supported yet, and will ensure we
> > correctly treat still-unknown xsave components as unmigratable.
> 
> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> marshals and unmarshals the registers out and back into the xsave data,
> so that unknown features are indeed unmigratable.
> 
> But are the property names necessary?  It makes no sense to
> enable/disable XSAVE components separately from the other CPUID bits
> that enable them.  Could we just mark all unknown features as
> unmigratable without giving them names?

We could, as we don't really need to make them configurable. But
giving them names will also allow us to return more useful data
to libvirt in case GET_SUPPORTED_CPUID returns some bits as
unsupported. The new CPU runnability/comparison APIs are all
based on property names.

-- 
Eduardo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 17:05, Eduardo Habkost wrote:
> > Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > marshals and unmarshals the registers out and back into the xsave data,
> > so that unknown features are indeed unmigratable.
> > 
> > But are the property names necessary?  It makes no sense to
> > enable/disable XSAVE components separately from the other CPUID bits
> > that enable them.  Could we just mark all unknown features as
> > unmigratable without giving them names?
>
> We could, as we don't really need to make them configurable. But
> giving them names will also allow us to return more useful data
> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> unsupported. The new CPU runnability/comparison APIs are all
> based on property names.

The names could, or perhaps should, be obtained also from
x86_ext_save_areas (apart from the legacy x87 and sse components which
are guaranteed to be there).  Basically property names such as "avx"
trigger both the regular CPUID bits and the XSAVE components.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 17:05, Eduardo Habkost wrote:
> > Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > marshals and unmarshals the registers out and back into the xsave data,
> > so that unknown features are indeed unmigratable.
> > 
> > But are the property names necessary?  It makes no sense to
> > enable/disable XSAVE components separately from the other CPUID bits
> > that enable them.  Could we just mark all unknown features as
> > unmigratable without giving them names?
>
> We could, as we don't really need to make them configurable. But
> giving them names will also allow us to return more useful data
> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> unsupported. The new CPU runnability/comparison APIs are all
> based on property names.

The names could, or perhaps should, be obtained also from
x86_ext_save_areas (apart from the legacy x87 and sse components which
are guaranteed to be there).  Basically property names such as "avx"
trigger both the regular CPUID bits and the XSAVE components.

Paolo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 17:05, Eduardo Habkost wrote:
> > > Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > > marshals and unmarshals the registers out and back into the xsave data,
> > > so that unknown features are indeed unmigratable.
> > > 
> > > But are the property names necessary?  It makes no sense to
> > > enable/disable XSAVE components separately from the other CPUID bits
> > > that enable them.  Could we just mark all unknown features as
> > > unmigratable without giving them names?
> >
> > We could, as we don't really need to make them configurable. But
> > giving them names will also allow us to return more useful data
> > to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> > unsupported. The new CPU runnability/comparison APIs are all
> > based on property names.
> 
> The names could, or perhaps should, be obtained also from
> x86_ext_save_areas (apart from the legacy x87 and sse components which
> are guaranteed to be there).  Basically property names such as "avx"
> trigger both the regular CPUID bits and the XSAVE components.

Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
it is more useful to say "avx" is unsupported by the host, than
something like "xsave-component-ymm".

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 17:05, Eduardo Habkost wrote:
> > > Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > > marshals and unmarshals the registers out and back into the xsave data,
> > > so that unknown features are indeed unmigratable.
> > > 
> > > But are the property names necessary?  It makes no sense to
> > > enable/disable XSAVE components separately from the other CPUID bits
> > > that enable them.  Could we just mark all unknown features as
> > > unmigratable without giving them names?
> >
> > We could, as we don't really need to make them configurable. But
> > giving them names will also allow us to return more useful data
> > to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> > unsupported. The new CPU runnability/comparison APIs are all
> > based on property names.
> 
> The names could, or perhaps should, be obtained also from
> x86_ext_save_areas (apart from the legacy x87 and sse components which
> are guaranteed to be there).  Basically property names such as "avx"
> trigger both the regular CPUID bits and the XSAVE components.

Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
it is more useful to say "avx" is unsupported by the host, than
something like "xsave-component-ymm".

-- 
Eduardo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 17:59, Eduardo Habkost wrote:
> On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 28/09/2016 17:05, Eduardo Habkost wrote:
>>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
>>>> marshals and unmarshals the registers out and back into the xsave data,
>>>> so that unknown features are indeed unmigratable.
>>>>
>>>> But are the property names necessary?  It makes no sense to
>>>> enable/disable XSAVE components separately from the other CPUID bits
>>>> that enable them.  Could we just mark all unknown features as
>>>> unmigratable without giving them names?
>>>
>>> We could, as we don't really need to make them configurable. But
>>> giving them names will also allow us to return more useful data
>>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
>>> unsupported. The new CPU runnability/comparison APIs are all
>>> based on property names.
>>
>> The names could, or perhaps should, be obtained also from
>> x86_ext_save_areas (apart from the legacy x87 and sse components which
>> are guaranteed to be there).  Basically property names such as "avx"
>> trigger both the regular CPUID bits and the XSAVE components.
> 
> Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> it is more useful to say "avx" is unsupported by the host, than
> something like "xsave-component-ymm".

So instead of looking at wi->feat_names[i] we could have a wrapper that
returns the name and special cases xsave components words?  This should
be used in both x86_cpu_get_migratable_flags and
report_unavailable_features, but not elsewhere as far as I could see.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Paolo Bonzini]

On 28/09/2016 17:59, Eduardo Habkost wrote:
> On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 28/09/2016 17:05, Eduardo Habkost wrote:
>>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
>>>> marshals and unmarshals the registers out and back into the xsave data,
>>>> so that unknown features are indeed unmigratable.
>>>>
>>>> But are the property names necessary?  It makes no sense to
>>>> enable/disable XSAVE components separately from the other CPUID bits
>>>> that enable them.  Could we just mark all unknown features as
>>>> unmigratable without giving them names?
>>>
>>> We could, as we don't really need to make them configurable. But
>>> giving them names will also allow us to return more useful data
>>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
>>> unsupported. The new CPU runnability/comparison APIs are all
>>> based on property names.
>>
>> The names could, or perhaps should, be obtained also from
>> x86_ext_save_areas (apart from the legacy x87 and sse components which
>> are guaranteed to be there).  Basically property names such as "avx"
>> trigger both the regular CPUID bits and the XSAVE components.
> 
> Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> it is more useful to say "avx" is unsupported by the host, than
> something like "xsave-component-ymm".

So instead of looking at wi->feat_names[i] we could have a wrapper that
returns the name and special cases xsave components words?  This should
be used in both x86_cpu_get_migratable_flags and
report_unavailable_features, but not elsewhere as far as I could see.

Paolo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 06:07:13PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 17:59, Eduardo Habkost wrote:
> > On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 28/09/2016 17:05, Eduardo Habkost wrote:
> >>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> >>>> marshals and unmarshals the registers out and back into the xsave data,
> >>>> so that unknown features are indeed unmigratable.
> >>>>
> >>>> But are the property names necessary?  It makes no sense to
> >>>> enable/disable XSAVE components separately from the other CPUID bits
> >>>> that enable them.  Could we just mark all unknown features as
> >>>> unmigratable without giving them names?
> >>>
> >>> We could, as we don't really need to make them configurable. But
> >>> giving them names will also allow us to return more useful data
> >>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> >>> unsupported. The new CPU runnability/comparison APIs are all
> >>> based on property names.
> >>
> >> The names could, or perhaps should, be obtained also from
> >> x86_ext_save_areas (apart from the legacy x87 and sse components which
> >> are guaranteed to be there).  Basically property names such as "avx"
> >> trigger both the regular CPUID bits and the XSAVE components.
> > 
> > Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> > it is more useful to say "avx" is unsupported by the host, than
> > something like "xsave-component-ymm".
> 
> So instead of looking at wi->feat_names[i] we could have a wrapper that
> returns the name and special cases xsave components words?  This should
> be used in both x86_cpu_get_migratable_flags and
> report_unavailable_features, but not elsewhere as far as I could see.

Sounds good to me. I will do it.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 06:07:13PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/09/2016 17:59, Eduardo Habkost wrote:
> > On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 28/09/2016 17:05, Eduardo Habkost wrote:
> >>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> >>>> marshals and unmarshals the registers out and back into the xsave data,
> >>>> so that unknown features are indeed unmigratable.
> >>>>
> >>>> But are the property names necessary?  It makes no sense to
> >>>> enable/disable XSAVE components separately from the other CPUID bits
> >>>> that enable them.  Could we just mark all unknown features as
> >>>> unmigratable without giving them names?
> >>>
> >>> We could, as we don't really need to make them configurable. But
> >>> giving them names will also allow us to return more useful data
> >>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> >>> unsupported. The new CPU runnability/comparison APIs are all
> >>> based on property names.
> >>
> >> The names could, or perhaps should, be obtained also from
> >> x86_ext_save_areas (apart from the legacy x87 and sse components which
> >> are guaranteed to be there).  Basically property names such as "avx"
> >> trigger both the regular CPUID bits and the XSAVE components.
> > 
> > Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> > it is more useful to say "avx" is unsupported by the host, than
> > something like "xsave-component-ymm".
> 
> So instead of looking at wi->feat_names[i] we could have a wrapper that
> returns the name and special cases xsave components words?  This should
> be used in both x86_cpu_get_migratable_flags and
> report_unavailable_features, but not elsewhere as far as I could see.

Sounds good to me. I will do it.

-- 
Eduardo

Re: [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 01:13:32PM -0300, Eduardo Habkost wrote:
> On Wed, Sep 28, 2016 at 06:07:13PM +0200, Paolo Bonzini wrote:
> > On 28/09/2016 17:59, Eduardo Habkost wrote:
> > > On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> > >> On 28/09/2016 17:05, Eduardo Habkost wrote:
> > >>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > >>>> marshals and unmarshals the registers out and back into the xsave data,
> > >>>> so that unknown features are indeed unmigratable.
> > >>>>
> > >>>> But are the property names necessary?  It makes no sense to
> > >>>> enable/disable XSAVE components separately from the other CPUID bits
> > >>>> that enable them.  Could we just mark all unknown features as
> > >>>> unmigratable without giving them names?
> > >>>
> > >>> We could, as we don't really need to make them configurable. But
> > >>> giving them names will also allow us to return more useful data
> > >>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> > >>> unsupported. The new CPU runnability/comparison APIs are all
> > >>> based on property names.
> > >>
> > >> The names could, or perhaps should, be obtained also from
> > >> x86_ext_save_areas (apart from the legacy x87 and sse components which
> > >> are guaranteed to be there).  Basically property names such as "avx"
> > >> trigger both the regular CPUID bits and the XSAVE components.
> > > 
> > > Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> > > it is more useful to say "avx" is unsupported by the host, than
> > > something like "xsave-component-ymm".
> > 
> > So instead of looking at wi->feat_names[i] we could have a wrapper that
> > returns the name and special cases xsave components words?  This should
> > be used in both x86_cpu_get_migratable_flags and
> > report_unavailable_features, but not elsewhere as far as I could see.
> 
> Sounds good to me. I will do it.

I changed my mind: this will require making a few changes on
ext_save_areas and I want to fix this regression as soon as
possible. I will submit a fix to the regression that adds a
migratable_flags field to FeatureWordInfo, and then implement
your suggestion as part of v2 of my
query-cpu-definitions/unavailable-features series.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: fix losing XCR0 processor state component bits

[Eduardo Habkost]

On Wed, Sep 28, 2016 at 01:13:32PM -0300, Eduardo Habkost wrote:
> On Wed, Sep 28, 2016 at 06:07:13PM +0200, Paolo Bonzini wrote:
> > On 28/09/2016 17:59, Eduardo Habkost wrote:
> > > On Wed, Sep 28, 2016 at 05:09:46PM +0200, Paolo Bonzini wrote:
> > >> On 28/09/2016 17:05, Eduardo Habkost wrote:
> > >>>> Hmm, right.  Even though XSAVE could be migrated as a blob, QEMU
> > >>>> marshals and unmarshals the registers out and back into the xsave data,
> > >>>> so that unknown features are indeed unmigratable.
> > >>>>
> > >>>> But are the property names necessary?  It makes no sense to
> > >>>> enable/disable XSAVE components separately from the other CPUID bits
> > >>>> that enable them.  Could we just mark all unknown features as
> > >>>> unmigratable without giving them names?
> > >>>
> > >>> We could, as we don't really need to make them configurable. But
> > >>> giving them names will also allow us to return more useful data
> > >>> to libvirt in case GET_SUPPORTED_CPUID returns some bits as
> > >>> unsupported. The new CPU runnability/comparison APIs are all
> > >>> based on property names.
> > >>
> > >> The names could, or perhaps should, be obtained also from
> > >> x86_ext_save_areas (apart from the legacy x87 and sse components which
> > >> are guaranteed to be there).  Basically property names such as "avx"
> > >> trigger both the regular CPUID bits and the XSAVE components.
> > > 
> > > Yes, this makes sense. If XSTATE_YMM_BIT is missing, for example,
> > > it is more useful to say "avx" is unsupported by the host, than
> > > something like "xsave-component-ymm".
> > 
> > So instead of looking at wi->feat_names[i] we could have a wrapper that
> > returns the name and special cases xsave components words?  This should
> > be used in both x86_cpu_get_migratable_flags and
> > report_unavailable_features, but not elsewhere as far as I could see.
> 
> Sounds good to me. I will do it.

I changed my mind: this will require making a few changes on
ext_save_areas and I want to fix this regression as soon as
possible. I will submit a fix to the regression that adds a
migratable_flags field to FeatureWordInfo, and then implement
your suggestion as part of v2 of my
query-cpu-definitions/unavailable-features series.

-- 
Eduardo