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

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

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