[Qemu-devel] [PATCH 0/3] target-i386: kvm: Use env->mcg_cap when setting up MCE

[Qemu-devel] [PATCH 0/3] target-i386: kvm: Use env->mcg_cap when setting up MCE

[Eduardo Habkost]

Instead of overwriting env->mcg_cap, make kvm_arch_init_vcpu(),
use the value already set at the CPU object when initializing
MCE.

Except for the new "unsupported MCG_CAPS bits" warning, this
patch doesn't change any of the existing QEMU behavior. The
previous code set env->mcg_cap to:
  (MCE_CAP_DEF & ioctl(KVM_X86_GET_MCE_CAP_SUPPORTED)) | MCE_BANKS_DEF
and the new code still keeps it exactly the same, as env->mcg_cap
is already initialized as MCE_CAP_DEF|MCE_BANKS_DEF at
mce_init().

This will allow us to change mce_init() in the future, to
implement different defaults depending on CPU model, machine-type
or command-line parameters.

Eduardo Habkost (3):
  target-i386: kvm: Abort if MCE bank count is not supported by host
  target-i386: kvm: Use env->mcg_cap when setting up MCE
  target-i386: kvm: Print warning when clearing mcg_cap bits

 target-i386/cpu.h |  2 ++
 target-i386/kvm.c | 22 ++++++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] target-i386: kvm: Abort if MCE bank count is not supported by host

[Eduardo Habkost]

Instead of silently changing the number of banks in mcg_cap based
on kvm_get_mce_cap_supported(), abort initialization if the host
doesn't support MCE_BANKS_DEF banks.

Note that MCE_BANKS_DEF was always 10 since it was introduced in
QEMU, and Linux always returned 32 at KVM_CAP_MCE since
KVM_CAP_MCE was introduced, so no behavior is being changed and
the error can't be triggered by any Linux version. The point of
the new check is to ensure we won't silently change the bank
count if we change MCE_BANKS_DEF or make the bank count
configurable in the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2a9953b..ee7bc69 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,11 +784,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return ret;
         }
 
-        if (banks > MCE_BANKS_DEF) {
-            banks = MCE_BANKS_DEF;
+        if (MCE_BANKS_DEF > banks) {
+            error_report("kvm: Unsupported MCE bank count: %d > %d\n",
+                         MCE_BANKS_DEF, banks);
+            return -ENOTSUP;
         }
+
         mcg_cap &= MCE_CAP_DEF;
-        mcg_cap |= banks;
+        mcg_cap |= MCE_BANKS_DEF;
         ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
         if (ret < 0) {
             fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/3] target-i386: kvm: Abort if MCE bank count is not supported by host

[Paolo Bonzini]

On 25/11/2015 16:49, Eduardo Habkost wrote:
> Instead of silently changing the number of banks in mcg_cap based
> on kvm_get_mce_cap_supported(), abort initialization if the host
> doesn't support MCE_BANKS_DEF banks.
> 
> Note that MCE_BANKS_DEF was always 10 since it was introduced in
> QEMU, and Linux always returned 32 at KVM_CAP_MCE since
> KVM_CAP_MCE was introduced, so no behavior is being changed and
> the error can't be triggered by any Linux version. The point of
> the new check is to ensure we won't silently change the bank
> count if we change MCE_BANKS_DEF or make the bank count
> configurable in the future.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/kvm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 2a9953b..ee7bc69 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -784,11 +784,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
>              return ret;
>          }
>  
> -        if (banks > MCE_BANKS_DEF) {
> -            banks = MCE_BANKS_DEF;
> +        if (MCE_BANKS_DEF > banks) {
> +            error_report("kvm: Unsupported MCE bank count: %d > %d\n",
> +                         MCE_BANKS_DEF, banks);

Yoda conditions?

        if (banks < MCE_BANKS_DEF) {
            error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = %d)",
                         MCE_BANKS_DEF, banks);

Paolo

> +            return -ENOTSUP;
>          }
> +
>          mcg_cap &= MCE_CAP_DEF;
> -        mcg_cap |= banks;
> +        mcg_cap |= MCE_BANKS_DEF;
>          ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
>          if (ret < 0) {
>              fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
> 

Re: [Qemu-devel] [PATCH 1/3] target-i386: kvm: Abort if MCE bank count is not supported by host

[Eduardo Habkost]

On Wed, Nov 25, 2015 at 05:46:38PM +0100, Paolo Bonzini wrote:
> 
> 
> On 25/11/2015 16:49, Eduardo Habkost wrote:
> > Instead of silently changing the number of banks in mcg_cap based
> > on kvm_get_mce_cap_supported(), abort initialization if the host
> > doesn't support MCE_BANKS_DEF banks.
> > 
> > Note that MCE_BANKS_DEF was always 10 since it was introduced in
> > QEMU, and Linux always returned 32 at KVM_CAP_MCE since
> > KVM_CAP_MCE was introduced, so no behavior is being changed and
> > the error can't be triggered by any Linux version. The point of
> > the new check is to ensure we won't silently change the bank
> > count if we change MCE_BANKS_DEF or make the bank count
> > configurable in the future.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/kvm.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 2a9953b..ee7bc69 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -784,11 +784,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >              return ret;
> >          }
> >  
> > -        if (banks > MCE_BANKS_DEF) {
> > -            banks = MCE_BANKS_DEF;
> > +        if (MCE_BANKS_DEF > banks) {
> > +            error_report("kvm: Unsupported MCE bank count: %d > %d\n",
> > +                         MCE_BANKS_DEF, banks);
> 
> Yoda conditions?
> 
>         if (banks < MCE_BANKS_DEF) {
>             error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = %d)",
>                          MCE_BANKS_DEF, banks);

This was on purpose, because MCE_BANKS_DEF is replaced by
(env->mcg_caps & MCG_CAPS_COUNT_MASK) in the next patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] target-i386: kvm: Abort if MCE bank count is not supported by host

[Paolo Bonzini]

On 25/11/2015 18:26, Eduardo Habkost wrote:
>> > Yoda conditions?
>> > 
>> >         if (banks < MCE_BANKS_DEF) {
>> >             error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = %d)",
>> >                          MCE_BANKS_DEF, banks);
> This was on purpose, because MCE_BANKS_DEF is replaced by
> (env->mcg_caps & MCG_CAPS_COUNT_MASK) in the next patch.

Yeah, I noticed it later.

Paolo

[Qemu-devel] [PATCH 2/3] target-i386: kvm: Use env->mcg_cap when setting up MCE

[Eduardo Habkost]

When setting up MCE, instead of using the MCE_*_DEF macros
directly, just filter the existing env->mcg_cap value.

As env->mcg_cap is already initialized as
MCE_CAP_DEF|MCE_BANKS_DEF at target-i386/cpu.c:mce_init(), this
doesn't change any behavior. But it will allow us to change
mce_init() in the future, to implement different defaults
depending on CPU model, machine-type or command-line parameters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.h |  2 ++
 target-i386/kvm.c | 11 ++++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fc4a605..84edfd0 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -286,6 +286,8 @@
 #define MCE_CAP_DEF     (MCG_CTL_P|MCG_SER_P)
 #define MCE_BANKS_DEF   10
 
+#define MCG_CAP_BANKS_MASK 0xff
+
 #define MCG_STATUS_RIPV (1ULL<<0)   /* restart ip valid */
 #define MCG_STATUS_EIPV (1ULL<<1)   /* ip points to correct instruction */
 #define MCG_STATUS_MCIP (1ULL<<2)   /* machine check in progress */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ee7bc69..d63a85b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,21 +784,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return ret;
         }
 
-        if (MCE_BANKS_DEF > banks) {
+        if ((env->mcg_cap & MCG_CAP_BANKS_MASK) > banks) {
             error_report("kvm: Unsupported MCE bank count: %d > %d\n",
-                         MCE_BANKS_DEF, banks);
+                         (int)(env->mcg_cap & MCG_CAP_BANKS_MASK), banks);
             return -ENOTSUP;
         }
 
-        mcg_cap &= MCE_CAP_DEF;
-        mcg_cap |= MCE_BANKS_DEF;
-        ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
+        env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
+        ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
         if (ret < 0) {
             fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
             return ret;
         }
-
-        env->mcg_cap = mcg_cap;
     }
 
     qemu_add_vm_change_state_handler(cpu_update_state, env);
-- 
2.1.0

[Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Eduardo Habkost]

Instead of silently clearing mcg_cap bits when the host doesn't
support them, print a warning when doing that.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index d63a85b..446bdfc 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -774,7 +774,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
            (CPUID_MCE | CPUID_MCA)
         && kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
-        uint64_t mcg_cap;
+        uint64_t mcg_cap, unsupported_caps;
         int banks;
         int ret;
 
@@ -790,6 +790,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return -ENOTSUP;
         }
 
+        unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
+        if (unsupported_caps) {
+            error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64 "\n",
+                         unsupported_caps);
+        }
+
         env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
         ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
         if (ret < 0) {
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Paolo Bonzini]

On 25/11/2015 16:49, Eduardo Habkost wrote:
> Instead of silently clearing mcg_cap bits when the host doesn't
> support them, print a warning when doing that.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/kvm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index d63a85b..446bdfc 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -774,7 +774,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
>             (CPUID_MCE | CPUID_MCA)
>          && kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
> -        uint64_t mcg_cap;
> +        uint64_t mcg_cap, unsupported_caps;
>          int banks;
>          int ret;
>  
> @@ -790,6 +790,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>              return -ENOTSUP;
>          }
>  
> +        unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
> +        if (unsupported_caps) {
> +            error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64 "\n",

\n should not be at end of error_report.

Fixed and applied.

Paolo

> +                         unsupported_caps);
> +        }
> +
>          env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
>          ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
>          if (ret < 0) {
> 

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Eduardo Habkost]

On Wed, Nov 25, 2015 at 05:45:20PM +0100, Paolo Bonzini wrote:
> On 25/11/2015 16:49, Eduardo Habkost wrote:
> > Instead of silently clearing mcg_cap bits when the host doesn't
> > support them, print a warning when doing that.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/kvm.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index d63a85b..446bdfc 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -774,7 +774,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> >             (CPUID_MCE | CPUID_MCA)
> >          && kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
> > -        uint64_t mcg_cap;
> > +        uint64_t mcg_cap, unsupported_caps;
> >          int banks;
> >          int ret;
> >  
> > @@ -790,6 +790,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >              return -ENOTSUP;
> >          }
> >  
> > +        unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
> > +        if (unsupported_caps) {
> > +            error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64 "\n",
> 
> \n should not be at end of error_report.
> 
> Fixed and applied.

MCG_CAP_BANKS_MASK is defined by patch 2/3. Have you applied the
whole series?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Paolo Bonzini]

On 25/11/2015 18:29, Eduardo Habkost wrote:
>>> > >  
>>> > > +        unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
>>> > > +        if (unsupported_caps) {
>>> > > +            error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64 "\n",
>> > 
>> > \n should not be at end of error_report.
>> > 
>> > Fixed and applied.
> MCG_CAP_BANKS_MASK is defined by patch 2/3. Have you applied the
> whole series?

Yes, of course.

Paolo

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Borislav Petkov]

On Wed, Nov 25, 2015 at 01:49:49PM -0200, Eduardo Habkost wrote:
> Instead of silently clearing mcg_cap bits when the host doesn't
> support them, print a warning when doing that.

Why the host? Why would we want there to be any relation between the MCA
capabilities of the host and what qemu is emulating?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Paolo Bonzini]

On 25/11/2015 18:21, Borislav Petkov wrote:
>> Instead of silently clearing mcg_cap bits when the host doesn't
>> > support them, print a warning when doing that.
> Why the host? Why would we want there to be any relation between the MCA
> capabilities of the host and what qemu is emulating?

He means the hypervisor. :)

Paolo

Re: [Qemu-devel] [PATCH 3/3] target-i386: kvm: Print warning when clearing mcg_cap bits

[Borislav Petkov]

On Wed, Nov 25, 2015 at 06:29:25PM +0100, Paolo Bonzini wrote:
> On 25/11/2015 18:21, Borislav Petkov wrote:
> >> Instead of silently clearing mcg_cap bits when the host doesn't
> >> > support them, print a warning when doing that.
> > Why the host? Why would we want there to be any relation between the MCA
> > capabilities of the host and what qemu is emulating?
> 
> He means the hypervisor. :)

Ah, ok. :)

Then they look good to me, a step in the right direction.

Acked-by: Borislav Petkov <bp@suse.de>

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.