[PATCH 0/2] Fix MCE not cleared on reset

[PATCH 0/2] Fix MCE not cleared on reset

[Avi Kivity]

Injecting an MCE will often panic the guest kernel.  After reset, however,
a new MCE will not be recognized because kvm thinks the first one is still
being processed.

Fix by exposing the MCE MSRs to userspace, which will clear them on reset.

Avi Kivity (2):
  KVM: Expose MCE control MSRs to userspace
  KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST

 Documentation/kvm/api.txt |    4 ++++
 arch/x86/kvm/x86.c        |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)

[PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Avi Kivity]

Userspace needs to reset and save/restore these MSRs.

The MCE banks are not exposed since their number varies from vcpu to vcpu.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7070b41..1e12cc5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -744,6 +744,8 @@ static unsigned num_msrs_to_save;
 
 static u32 emulated_msrs[] = {
 	MSR_IA32_MISC_ENABLE,
+	MSR_IA32_MCG_STATUS,
+	MSR_IA32_MCG_CTL,
 };
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
-- 
1.7.1

[PATCH 2/2] KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 Documentation/kvm/api.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index d9b00f1..179cc51 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -126,6 +126,10 @@ user fills in the size of the indices array in nmsrs, and in return
 kvm adjusts nmsrs to reflect the actual number of msrs and fills in
 the indices array with their numbers.
 
+Note: if kvm indicates supports MCE (KVM_CAP_MCE), then the MCE bank MSRs are
+not returned in the MSR list, as different vcpus can have a different number
+of banks, as set via the KVM_X86_SETUP_MCE ioctl.
+
 4.4 KVM_CHECK_EXTENSION
 
 Capability: basic
-- 
1.7.1

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Huang Ying]

Hi, Avi,

On Wed, 2010-07-07 at 19:09 +0800, Avi Kivity wrote:
> Userspace needs to reset and save/restore these MSRs.
> 
> The MCE banks are not exposed since their number varies from vcpu to vcpu.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/x86.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7070b41..1e12cc5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -744,6 +744,8 @@ static unsigned num_msrs_to_save;
>  
>  static u32 emulated_msrs[] = {
>  	MSR_IA32_MISC_ENABLE,
> +	MSR_IA32_MCG_STATUS,
> +	MSR_IA32_MCG_CTL,

We need only clear MSR_IA32_MCG_STATUS during reset, but should not
clear MSR_IA32_MCG_CTL.

>  };
>  
>  static int set_efer(struct kvm_vcpu *vcpu, u64 efer)

Best Regards,
Huang Ying

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Avi Kivity]

On 07/08/2010 05:07 AM, Huang Ying wrote:
>
>>   static u32 emulated_msrs[] = {
>>   	MSR_IA32_MISC_ENABLE,
>> +	MSR_IA32_MCG_STATUS,
>> +	MSR_IA32_MCG_CTL,
>>      
> We need only clear MSR_IA32_MCG_STATUS during reset, but should not
> clear MSR_IA32_MCG_CTL.
>
>    

Why not?

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

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Huang Ying]

On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote:
> On 07/08/2010 05:07 AM, Huang Ying wrote:
> >
> >>   static u32 emulated_msrs[] = {
> >>   	MSR_IA32_MISC_ENABLE,
> >> +	MSR_IA32_MCG_STATUS,
> >> +	MSR_IA32_MCG_CTL,
> >>      
> > We need only clear MSR_IA32_MCG_STATUS during reset, but should not
> > clear MSR_IA32_MCG_CTL.
> >
> >    
> 
> Why not?

According to Intel 64 and IA32 Architectures Software Developer's Manual
(SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across
reset. Except we need some special processing for MSR_IA32_MCG_STATUS.

And if we clear MSR_IA32_MCG_CTL, the machine check reporting is
disabled according to SDM Vol 3A, section 15.3.1.3

Best Regards,
Huang Ying

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Avi Kivity]

On 07/08/2010 11:03 AM, Huang Ying wrote:
> On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote:
>    
>> On 07/08/2010 05:07 AM, Huang Ying wrote:
>>      
>>>        
>>>>    static u32 emulated_msrs[] = {
>>>>    	MSR_IA32_MISC_ENABLE,
>>>> +	MSR_IA32_MCG_STATUS,
>>>> +	MSR_IA32_MCG_CTL,
>>>>
>>>>          
>>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not
>>> clear MSR_IA32_MCG_CTL.
>>>
>>>
>>>        
>> Why not?
>>      
> According to Intel 64 and IA32 Architectures Software Developer's Manual
> (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across
> reset.


Well, my copy says:

     Machine-Check Architecture   Pwr up or Reset:  Undefined          
INIT: Unchanged

So it seems we're free to clear it.  But probably the best thing would 
be to have the bios initialize it to disabled state?

Note, in any case we have to expose the MSRs for live migration.

> Except we need some special processing for MSR_IA32_MCG_STATUS.
>    

What do you have in mind?

> And if we clear MSR_IA32_MCG_CTL, the machine check reporting is
> disabled according to SDM Vol 3A, section 15.3.1.3
>    

Won't the kernel reenable MCE?  In my testing, the sequence 
MCE-reset-MCE worked after the patch (whereas it would fail without it).

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

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Huang Ying]

On Thu, 2010-07-08 at 16:16 +0800, Avi Kivity wrote:
> On 07/08/2010 11:03 AM, Huang Ying wrote:
> > On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote:
> >    
> >> On 07/08/2010 05:07 AM, Huang Ying wrote:
> >>      
> >>>        
> >>>>    static u32 emulated_msrs[] = {
> >>>>    	MSR_IA32_MISC_ENABLE,
> >>>> +	MSR_IA32_MCG_STATUS,
> >>>> +	MSR_IA32_MCG_CTL,
> >>>>
> >>>>          
> >>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not
> >>> clear MSR_IA32_MCG_CTL.
> >>>
> >>>
> >>>        
> >> Why not?
> >>      
> > According to Intel 64 and IA32 Architectures Software Developer's Manual
> > (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across
> > reset.
> 
> 
> Well, my copy says:
> 
>      Machine-Check Architecture   Pwr up or Reset:  Undefined          
> INIT: Unchanged

If my understanding is correct. Pwr up or Reset is cold reboot and INIT
is warm reboot. So MCE can be logged after the warm reboot.

> So it seems we're free to clear it.  But probably the best thing would 
> be to have the bios initialize it to disabled state?
> 
> Note, in any case we have to expose the MSRs for live migration.
> 
> > Except we need some special processing for MSR_IA32_MCG_STATUS.
> >    
> 
> What do you have in mind?

MSR_IA32_MCG_STATUS need to be cleared across reboot.

> > And if we clear MSR_IA32_MCG_CTL, the machine check reporting is
> > disabled according to SDM Vol 3A, section 15.3.1.3
> >    
> 
> Won't the kernel reenable MCE?  In my testing, the sequence 
> MCE-reset-MCE worked after the patch (whereas it would fail without it).

Yes. Because kernel will reenable it. But if we only clear
MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too.

Best Regards,
Huang Ying

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Avi Kivity]

On 07/08/2010 11:47 AM, Huang Ying wrote:
> On Thu, 2010-07-08 at 16:16 +0800, Avi Kivity wrote:
>    
>> On 07/08/2010 11:03 AM, Huang Ying wrote:
>>      
>>> On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote:
>>>
>>>        
>>>> On 07/08/2010 05:07 AM, Huang Ying wrote:
>>>>
>>>>          
>>>>>
>>>>>            
>>>>>>     static u32 emulated_msrs[] = {
>>>>>>     	MSR_IA32_MISC_ENABLE,
>>>>>> +	MSR_IA32_MCG_STATUS,
>>>>>> +	MSR_IA32_MCG_CTL,
>>>>>>
>>>>>>
>>>>>>              
>>>>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not
>>>>> clear MSR_IA32_MCG_CTL.
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> Why not?
>>>>
>>>>          
>>> According to Intel 64 and IA32 Architectures Software Developer's Manual
>>> (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across
>>> reset.
>>>        
>>
>> Well, my copy says:
>>
>>       Machine-Check Architecture   Pwr up or Reset:  Undefined
>> INIT: Unchanged
>>      
> If my understanding is correct. Pwr up or Reset is cold reboot and INIT
> is warm reboot. So MCE can be logged after the warm reboot.
>    

Yes.  But we're allowed to clear it on a real RESET, just not on INIT.

Our INIT handing is probably broken in this area.

>> So it seems we're free to clear it.  But probably the best thing would
>> be to have the bios initialize it to disabled state?
>>
>> Note, in any case we have to expose the MSRs for live migration.
>>
>>      
>>> Except we need some special processing for MSR_IA32_MCG_STATUS.
>>>
>>>        
>> What do you have in mind?
>>      
> MSR_IA32_MCG_STATUS need to be cleared across reboot.
>    

Ok.

>>> And if we clear MSR_IA32_MCG_CTL, the machine check reporting is
>>> disabled according to SDM Vol 3A, section 15.3.1.3
>>>
>>>        
>> Won't the kernel reenable MCE?  In my testing, the sequence
>> MCE-reset-MCE worked after the patch (whereas it would fail without it).
>>      
> Yes. Because kernel will reenable it. But if we only clear
> MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too.
>    

What happens if we reboot into a kernel that doesn't enable MCE?

I guess it doesn't matter: the new kernel will keep cr4.mce cleared and 
thus MCE will be blocked.

I'd like to keep the patch as is, so live migration works for MCE (we'll 
need to add bank support).  I think there's no problem clearing _CTL on 
reset.  If there is, we can patch qemu not to clear the MSR.  Is that 
acceptable?

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

Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace

[Huang Ying]

Sorry for late.

On Thu, 2010-07-08 at 17:12 +0800, Avi Kivity wrote:
[..snip..]
> >>> And if we clear MSR_IA32_MCG_CTL, the machine check reporting is
> >>> disabled according to SDM Vol 3A, section 15.3.1.3
> >>>
> >>>        
> >> Won't the kernel reenable MCE?  In my testing, the sequence
> >> MCE-reset-MCE worked after the patch (whereas it would fail without it).
> >>      
> > Yes. Because kernel will reenable it. But if we only clear
> > MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too.
> >    
> 
> What happens if we reboot into a kernel that doesn't enable MCE?
> 
> I guess it doesn't matter: the new kernel will keep cr4.mce cleared and 
> thus MCE will be blocked.
> 
> I'd like to keep the patch as is, so live migration works for MCE (we'll 
> need to add bank support).  I think there's no problem clearing _CTL on 
> reset.  If there is, we can patch qemu not to clear the MSR.  Is that 
> acceptable?

Yes. At least for Linux that is OK. Don't know whether Windows will set
_CTL during boot.

Best Regards,
Huang Ying

Re: [PATCH 0/2] Fix MCE not cleared on reset

[Marcelo Tosatti]

On Wed, Jul 07, 2010 at 02:09:37PM +0300, Avi Kivity wrote:
> Injecting an MCE will often panic the guest kernel.  After reset, however,
> a new MCE will not be recognized because kvm thinks the first one is still
> being processed.
> 
> Fix by exposing the MCE MSRs to userspace, which will clear them on reset.
> 
> Avi Kivity (2):
>   KVM: Expose MCE control MSRs to userspace
>   KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST
> 
>  Documentation/kvm/api.txt |    4 ++++
>  arch/x86/kvm/x86.c        |    2 ++
>  2 files changed, 6 insertions(+), 0 deletions(-)

Applied, thanks.