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

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

[Huang Ying]

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.

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);
+    }
+#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);
+        for (i = 0; i < (env->mcg_cap & 0xff); i++)
+            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) {
+        msrs[0].index = MSR_MCG_STATUS;
+        msrs[1].index = MSR_MCG_CTL;
+        n = (env->mcg_cap & 0xff) * 4;
+        for (i = 0; i < n; i++)
+            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;
+        }
+    }
+#endif
+
     kvm_arch_save_mpstate(env);
     kvm_save_lapic(env);
     kvm_get_vcpu_events(env);

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]

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

> +    }
> +#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.

> +            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.

> +        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.

> +    }
> +#endif
> +
>      kvm_arch_save_mpstate(env);
>      kvm_save_lapic(env);
>      kvm_get_vcpu_events(env);
> 
> 

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Huang Ying]

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4176 bytes --]

Huang Ying wrote:
> 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.

It's as safe as assuming 100 entries is enough - with or without the
base entries (which are only 12 if I can count correctly). Unless you
can exclude that the number of banks will make things blow up, add a
check. And I don't see a need for two runs yet.

> 
>>> +            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?

Nope, good point.

> 
>>> +        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.
> 

As I said, the space issue need to be address independent of the
question one vs. two runs. When there are really KBytes required, go for
qemu_malloc.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]