| Message ID | 1475040669-29085-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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.
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
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".
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
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.
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.
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 {