Re: [PATCH -v2] Add savevm/loadvm support for MCE

[Huang Ying <ying.huang@intel.com>]

On Tue, 2010-03-02 at 16:09 +0800, Jan Kiszka wrote:
> Huang Ying wrote:
> > MCE registers are saved/load into/from CPUState in
> > kvm_arch_save/load_regs. To simulate the MCG_STATUS clearing upon
> > reset, MSR_MCG_STATUS is set to 0 for KVM_PUT_RESET_STATE.
> > 
> > v2:
> > 
> >  - Rebased on new CPU registers save/load framework.
> 
> Yep, much closer. :)
> 
> > 
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > ---
> >  qemu-kvm-x86.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -979,11 +979,33 @@ void kvm_arch_load_regs(CPUState *env, i
> >          set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME, env->system_time_msr);
> >          set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
> >      }
> > +#ifdef KVM_CAP_MCE
> > +    if (env->mcg_cap && level == KVM_PUT_RESET_STATE) {
> > +        /*
> > +         * MCG_STATUS should reset to 0 after reset, while other MCE
> > +         * registers should be unchanged
> > +         */
> > +        set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
> 
> For the sake of consistency, just write mcg_status here (it's properly
> updated in cpu_reset).

OK.

> > +    }
> > +#endif
> >  
> >      rc = kvm_set_msrs(env, msrs, n);
> >      if (rc == -1)
> >          perror("kvm_set_msrs FAILED");
> >  
> > +#ifdef KVM_CAP_MCE
> > +    if (env->mcg_cap && level == KVM_PUT_FULL_STATE) {
> > +        n = 0;
> > +        set_msr_entry(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
> > +        set_msr_entry(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
> 
> You can move this block up, reusing the kvm_set_msrs above. But...
> 
> > +        for (i = 0; i < (env->mcg_cap & 0xff); i++)
> 
> ...this requires some care. We have space for writing up to 100
> registers in our msrs array. You may have to extend it unless this
> number is much smaller in reality.

The default number of MCE banks is 10, this means 42 entries. So I think
it is safer to use another kvm_set_msrs. And the stack space is limited
too.

> > +            set_msr_entry(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
> > +        rc = kvm_set_msrs(env, msrs, n);
> > +        if (rc == -1)
> > +            perror("kvm_set_msrs FAILED");
> > +    }
> > +#endif
> > +
> >      if (level >= KVM_PUT_RESET_STATE) {
> >          kvm_arch_load_mpstate(env);
> >          kvm_load_lapic(env);
> > @@ -1155,6 +1177,27 @@ void kvm_arch_save_regs(CPUState *env)
> >                  return;
> >          }
> >      }
> > +
> > +#ifdef KVM_CAP_MCE
> > +    if (env->mcg_cap) {
> 
> No need to check for msg_cap, the kernel will ignore unknown MSRs.

And some error message will be printed in kernel log. Is it OK?

> > +        msrs[0].index = MSR_MCG_STATUS;
> > +        msrs[1].index = MSR_MCG_CTL;
> > +        n = (env->mcg_cap & 0xff) * 4;
> > +        for (i = 0; i < n; i++)
> 
> Same are above, we may run out of array space.
> 
> > +            msrs[2 + i].index = MSR_MC0_CTL + i;
> > +
> > +        rc = kvm_get_msrs(env, msrs, n + 2);
> > +        if (rc == -1)
> > +            perror("kvm_get_msrs FAILED");
> > +        else {
> > +            env->mcg_status = msrs[0].data;
> > +            env->mcg_ctl = msrs[1].data;
> > +            for (i = 0; i < n; i++)
> > +                env->mce_banks[i] = msrs[2 + i].data;
> > +        }
> 
> Please split this block into setup and MSR transfer, and then merge it
> into the existing MSR readout to avoid calling kvm_get_msrs twice.

I will do that if the stack space is not an issue.

Best Regards,
Huang Ying