RE: [Xen-devel] [PATCH] X86 MCE: Add SRAR handler

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> On 11.10.11 at 10:15, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> Jan Beulich wrote:
>>>>>> On 08.10.11 at 10:29, "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>
>>>>>> wrote:
>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>> Sent: Friday, September 30, 2011 3:25 PM
>>>>> To: Liu, Jinsong; Jiang, Yunhong
>>>>> Cc: keir.xen@xxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxx
>>>>> Subject: RE: [Xen-devel] [PATCH] X86 MCE: Add SRAR handler
>>>>> 
>>>>>>>> On 30.09.11 at 04:51, "Jiang, Yunhong"
>>>>>>>> <yunhong.jiang@xxxxxxxxx> wrote:
>>>>> 
>>>>>> 
>>>>>>> -----Original Message-----
>>>>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>>>> This made me look at the current source, and there I see in
>>>>>>> mce_urgent_action() 
>>>>>>> 
>>>>>>>     if ( !(gstatus & MCG_STATUS_RIPV) && !guest_mode(regs))
>>>>>>> return -1; 
>>>>>>> 
>>>>>>> which I think should say ... _EIPV and use || instead. Thoughts?
>>>>>> 
>>>>>> I think this code means, if the error happens in hypervisor mode
>>>>>> (i.e. !guest_mode()), and RIPV indicate the RIP in stack can't be
>>>>>> restarted, we have to panic.
>>>>> 
>>>>> Then the guest_mode() check still lacks an extra check of EIPV,
>>>>> like 
>>>>> 
>>>>>      if ( !(gstatus & MCG_STATUS_RIPV) &&
>>>>>           (!(gstatus & MCG_STATUS_EIPV) || !guest_mode(regs)))
>>>>> return -1; 
>>>>> 
>>>> 
>>>> The RIPV is not related to the EIPV. RIPV means the context saved
>>>> in the stack can't be restarted anymore. According to the SDM, RIPV
>>>> means "execution can be restarted reliably at the instruction
>>>> pointed to by the instruction pointer pushed on the stack". It's
>>>> not about error happened synchronously or asynchronously. The
>>>> point is, if the program is running in hypervisor context, and
>>>> RIPV tells us that the program can't be restarted, we can't do
>>>> anything but panic, because we can't switch context while we are
>>>> in xen. So this code have nothing to do with EIPV.
>>> 
>>> I continue to disagree (including the statement in your other
>>> response): RIPV only tells us whether we can resume, not in which
>>> context the error occurred. EIPV tells us whether, by looking at the
>>> saved registers, we can determine the context that the error
>>> occurred in. Since with !RIPV we have to determine in what context
>>> the error occurred in order to decide whether to panic or just kill
>>> a guest, we can't ignore EIPV (and if it's not set we have to
>>> assume the worst case, since even if the registers indicate guest
>>> mode the error may have occurred in hypervisor context or accessing
>>> hypervisor structures [consider e.g. a data load error during a GDT
>>> access]). 
>>> 
>>> Jan
>> 
>> Yes, I agree EIPV=0 may indicate async error, but I think your
>> solution *overkilled* most cases (i.e. the real guest instruction
>> fetch error). 
>> 
>> Our idea is,
>>   * xen mce would flush prefetched instruction so we can delay
>> handle it until if real need; 
>>   * a h/w error will not disappear, but if it was not being
>> *consumed*, it's OK for system keep going (like SRAO error which do
>> not need s/w handle immediately); 
>> 
>> Suppose an async instruction fetch error (RIVP=EIVP=0), triggered at
>> guest context but instruction prefetch hypervisor context. The
>>   scenario is, * at xen mce, the prefetched instruction has been
>> flushed. xen mce handler needn't panic, instead it mark the page as
>> broken page, then trigger vmce to guest;
> 
> If the prefetch was from Xen space (only in guest context),
> delivering a vMCE to the guest is pointless (and perhaps confusing to
> the guest). 
> 

Yes, exactly. how about delay handle it as:
* at mce isr
        if ( !(gstatus & MCG_STATUS_RIPV) && !guest_mode(regs))
                xen panic;
* at mce softirq
        if ( (srar error) && (EIPV ==0) && (broken page owned by hypervisor) )
                xen panic;

>>   * guest may kill app, kernel thread, guest itself, or whatever;
>> 
>> The error is still an error, w/ 2 possibilities in the future:
>>   1. it may not be consumed as an SRAR error, system keep going, h/w
>> mechanism may detect a SRAO error (i.e. memroy scrub) at some time
>> point and handled then; 
>>   2. it may be consumed at some time point and a SRAR error
>>    triggered again. At this time, 1). if srar occurred at hypervisor
>>    context, xen will panic. or, 2). if srar occurred at guest
>> context, xen kill the guest as a malicious one (as what the 2nd
>> patch do), and move the page to broken page list; 
>> 
>> Considering the rare possibility of the above case, I think it's
>> acceptable to handle it in this way. Thoughts?
> 
> You're only discussing instruction fetches (which can be discarded),
> but you're not covering the other example I gave (GDT access from
> guest context - just like this is a ring-0 operations from the paging
> unit's pov, this ought to be an out-of-context operation from MCE's
> perspective). 
> 
> Jan

That would be data load error (EIPV=1), a sync error.

Thanks,
Jinsong


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel