[BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Huang Ying]

Now, if we inject a fatal MCE into guest OS, for example Linux, Linux
will go panic and then reboot. But if we inject another MCE now,
system will reset directly instead of go panic firstly, because
MCG_STATUS.MCIP is set to 1 and not cleared after reboot. This is does
not follow the behavior in real hardware.

This patch fixes this via set IA32_MCG_STATUS to 0 during system reset.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 qemu-kvm-x86.c |    1 +
 1 file changed, 1 insertion(+)

--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
 #endif
     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);
+    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
 
     rc = kvm_set_msrs(env, msrs, n);
     if (rc == -1)

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Andi Kleen]

> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
>  #endif
>      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);
> +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);

Still need to keep EIPV and possibly RIPV, don't we?

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Avi Kivity]

On 01/05/2010 10:34 AM, Huang Ying wrote:
> Now, if we inject a fatal MCE into guest OS, for example Linux, Linux
> will go panic and then reboot. But if we inject another MCE now,
> system will reset directly instead of go panic firstly, because
> MCG_STATUS.MCIP is set to 1 and not cleared after reboot. This is does
> not follow the behavior in real hardware.
>
> This patch fixes this via set IA32_MCG_STATUS to 0 during system reset.
>
> Signed-off-by: Huang Ying<ying.huang@intel.com>
> ---
>   qemu-kvm-x86.c |    1 +
>   1 file changed, 1 insertion(+)
>
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
>   #endif
>       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);
> +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
>
>    

Not sure why you reset this in kvm_arch_load_regs().  Shouldn't this be 
in the cpu reset code?

-- 
error compiling committee.c: too many arguments to function

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Huang Ying]

Hi, Avi,

On Tue, 2010-01-05 at 18:50 +0800, Avi Kivity wrote:
> On 01/05/2010 10:34 AM, Huang Ying wrote:
> > Now, if we inject a fatal MCE into guest OS, for example Linux, Linux
> > will go panic and then reboot. But if we inject another MCE now,
> > system will reset directly instead of go panic firstly, because
> > MCG_STATUS.MCIP is set to 1 and not cleared after reboot. This is does
> > not follow the behavior in real hardware.
> >
> > This patch fixes this via set IA32_MCG_STATUS to 0 during system reset.
> >
> > Signed-off-by: Huang Ying<ying.huang@intel.com>
> > ---
> >   qemu-kvm-x86.c |    1 +
> >   1 file changed, 1 insertion(+)
> >
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
> >   #endif
> >       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);
> > +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
> >
> >    
> 
> Not sure why you reset this in kvm_arch_load_regs().  Shouldn't this be 
> in the cpu reset code?

I found kvm_arch_load_regs() is called by kvm_arch_cpu_reset(), which is
called by qemu_kvm_system_reset(). It is not in cpu reset path?

Best Regards,
Huang Ying

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Avi Kivity]

On 01/06/2010 09:05 AM, Huang Ying wrote:
> @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
>>>    #endif
>>>        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);
>>> +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
>>>
>>>
>>>        
>> Not sure why you reset this in kvm_arch_load_regs().  Shouldn't this be
>> in the cpu reset code?
>>      
> I found kvm_arch_load_regs() is called by kvm_arch_cpu_reset(), which is
> called by qemu_kvm_system_reset(). It is not in cpu reset path?
>    

It is, but it is also called from many other places, which could cause 
this msr to be zeroed.

A better solution is to allocate it a field in CPUState, load and save 
it in kvm_arch_*_regs, and zero it during reset.

-- 
error compiling committee.c: too many arguments to function

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Huang Ying]

On Wed, 2010-01-06 at 16:03 +0800, Avi Kivity wrote:
> On 01/06/2010 09:05 AM, Huang Ying wrote:
> > @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
> >>>    #endif
> >>>        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);
> >>> +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
> >>>
> >>>
> >>>        
> >> Not sure why you reset this in kvm_arch_load_regs().  Shouldn't this be
> >> in the cpu reset code?
> >>      
> > I found kvm_arch_load_regs() is called by kvm_arch_cpu_reset(), which is
> > called by qemu_kvm_system_reset(). It is not in cpu reset path?
> >    
> 
> It is, but it is also called from many other places, which could cause 
> this msr to be zeroed.
> 
> A better solution is to allocate it a field in CPUState, load and save 
> it in kvm_arch_*_regs, and zero it during reset.

Yes. You are right. I will fix this.

Best Regards,
Huang Ying

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Huang Ying]

On Tue, 2010-01-05 at 18:44 +0800, Andi Kleen wrote:
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
> >  #endif
> >      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);
> > +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
> 
> Still need to keep EIPV and possibly RIPV, don't we?

It appears that EIPV and RIPV is meaningless outside of MCE exception
handler. But I will try to check the real hardware behavior.

Best Regards,
Huang Ying

Re: [BUGFIX] MCE: Fix bug of IA32_MCG_STATUS after system reset

[Marcelo Tosatti]

On Wed, Jan 06, 2010 at 04:17:42PM +0800, Huang Ying wrote:
> On Wed, 2010-01-06 at 16:03 +0800, Avi Kivity wrote:
> > On 01/06/2010 09:05 AM, Huang Ying wrote:
> > > @@ -1015,6 +1015,7 @@ void kvm_arch_load_regs(CPUState *env)
> > >>>    #endif
> > >>>        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);
> > >>> +    set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0);
> > >>>
> > >>>
> > >>>        
> > >> Not sure why you reset this in kvm_arch_load_regs().  Shouldn't this be
> > >> in the cpu reset code?
> > >>      
> > > I found kvm_arch_load_regs() is called by kvm_arch_cpu_reset(), which is
> > > called by qemu_kvm_system_reset(). It is not in cpu reset path?
> > >    
> > 
> > It is, but it is also called from many other places, which could cause 
> > this msr to be zeroed.
> > 
> > A better solution is to allocate it a field in CPUState, load and save 
> > it in kvm_arch_*_regs, and zero it during reset.
> 
> Yes. You are right. I will fix this.

BTW, the MCE MSRs are not being migrated. Perhaps you'd like to fix that
while at it.