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@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.
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:
signature.asc
Description: OpenPGP digital signature
