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

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(-)

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));
>

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.

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