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@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
