Patchwork [v3,12/21] kvm: x86: Drop MCE MSRs write back restrictions

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 4, 2011, 8:32 a.m.
Message ID <2c5efab40d7088353c5147c41fb13b44a944679c.1294129949.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/77421/
State New
Headers show

Comments

Jan Kiszka - Jan. 4, 2011, 8:32 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

There is no need to restrict writing back MCE MSRs to reset or full
state updates as setting their values has no side effects.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Huang Ying <ying.huang@intel.com>
---
 target-i386/kvm.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
Huang Ying - Jan. 5, 2011, 6:42 a.m.
On Tue, 2011-01-04 at 16:32 +0800, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> There is no need to restrict writing back MCE MSRs to reset or full
> state updates as setting their values has no side effects.

Sorry for late.

The MCE MSRs contents is sticky for warm reset except MCG_STATUS, so
their content should be kept.  And the following sequence may set
uncorrected value in MCE registers.

savevm -> loadvm -> (OS clear MCE registers) -> reset -> (MCE registers
has new (uncorrected) value)

Best Regards,
Huang Ying

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Huang Ying <ying.huang@intel.com>
> ---
>  target-i386/kvm.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 8267655..1789bff 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -863,14 +863,10 @@ static int kvm_put_msrs(CPUState *env, int level)
>      if (env->mcg_cap) {
>          int i;
>  
> -        if (level == KVM_PUT_RESET_STATE) {
> -            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
> -        } else if (level == KVM_PUT_FULL_STATE) {
> -            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
> -            kvm_msr_entry_set(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
> -            for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
> -                kvm_msr_entry_set(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
> -            }
> +        kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
> +        kvm_msr_entry_set(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
> +        for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
> +            kvm_msr_entry_set(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
>          }
>      }
>  #endif
Jan Kiszka - Jan. 5, 2011, 8:07 a.m.
Am 05.01.2011 07:42, Huang Ying wrote:
> On Tue, 2011-01-04 at 16:32 +0800, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> There is no need to restrict writing back MCE MSRs to reset or full
>> state updates as setting their values has no side effects.
> 
> Sorry for late.

Don't worry.

> 
> The MCE MSRs contents is sticky for warm reset except MCG_STATUS, so
> their content should be kept.  And the following sequence may set
> uncorrected value in MCE registers.
> 
> savevm -> loadvm -> (OS clear MCE registers) -> reset -> (MCE registers
> has new (uncorrected) value)

Sorry, I can't follow. Unless I miss some subtle detail, the question is
not when we transfer the mcg_* CPUState fields to the kernel, but when
and how we manipulate them in user space, e.g. on reset. Where are those
fields touched incorrectly between get and put msrs so that we cannot
write them back?

Jan
Huang Ying - Jan. 5, 2011, 8:33 a.m.
On Wed, 2011-01-05 at 16:07 +0800, Jan Kiszka wrote:
> Am 05.01.2011 07:42, Huang Ying wrote:
> > On Tue, 2011-01-04 at 16:32 +0800, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> There is no need to restrict writing back MCE MSRs to reset or full
> >> state updates as setting their values has no side effects.
> > 
> > Sorry for late.
> 
> Don't worry.
> 
> > 
> > The MCE MSRs contents is sticky for warm reset except MCG_STATUS, so
> > their content should be kept.  And the following sequence may set
> > uncorrected value in MCE registers.
> > 
> > savevm -> loadvm -> (OS clear MCE registers) -> reset -> (MCE registers
> > has new (uncorrected) value)
> 
> Sorry, I can't follow. Unless I miss some subtle detail, the question is
> not when we transfer the mcg_* CPUState fields to the kernel, but when
> and how we manipulate them in user space, e.g. on reset. Where are those
> fields touched incorrectly between get and put msrs so that we cannot
> write them back?

If my understanding is correct, MSRs are not saved to user space
(env->mce_banks) during reset in current code.  So if all MCE MSRs are
restored to kernel, their user space contents from previous loadvm may
be put into kernel after reset.

Best Regards,
Huang Ying

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 8267655..1789bff 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -863,14 +863,10 @@  static int kvm_put_msrs(CPUState *env, int level)
     if (env->mcg_cap) {
         int i;
 
-        if (level == KVM_PUT_RESET_STATE) {
-            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
-        } else if (level == KVM_PUT_FULL_STATE) {
-            kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
-            kvm_msr_entry_set(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
-            for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
-                kvm_msr_entry_set(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
-            }
+        kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
+        kvm_msr_entry_set(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl);
+        for (i = 0; i < (env->mcg_cap & 0xff) * 4; i++) {
+            kvm_msr_entry_set(&msrs[n++], MSR_MC0_CTL + i, env->mce_banks[i]);
         }
     }
 #endif