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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]