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 <[email protected]> >>> --- >>> 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
signature.asc
Description: OpenPGP digital signature
