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

[Avi Kivity <avi@redhat.com>]

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