RE: [Xen-devel] [PATCH] DOM0: Adding MCA Loging support in DOM0

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Jan Beulich wrote:
>>>> "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> 09.06.09 11:30 >>>
>> xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote:
>>>>>> "Ke, Liping" <liping.ke@xxxxxxxxx> 09.06.09 07:10 >>>
>>>> This patch is to support MCA info logging in DOM0. When an MCE/CMCI
>>>> error happens (or by polling), the related error info will be sent
>>>> to DOM0 by XEN. This patch will help to fetch the xen-logged
>>>> information by hypercall and then convert XEN-format log into Linux
>>>> format MCELOG. So we can reuse current available mcelog tools for
>>>> native Linux now.
>>> 
>>> Could this patch please be re-worked to not needlessly touch non-Xen
>>> code? This would include a separate config option for the new code,
>>> while disabling the native sub-options (which at once would avoid
>>> the hacks you appear to need to make mce_amd.c build).
>> 
>> Jan, I think the native sub-options (X86_MCE_INTEL and
> X86_MCE_AMD) needs be enabled still.
>> When a MCE happens, and dom0 is effected, that MCE will be
> injected to dom0 as virtual MCE, and that requires to enable these
>> otions.
>> When a MCE happens and dom0 is not effected, that MCE will be
> sent to dom0 as vIRQ so that dom0 can log that event for
>> analysis.
> 
> Oh, okay, if the code is indeed needed, than that's fine. But
> the hacks needed
> to get mce_amd.c to compile look suspicious. As much as e.g. changing
> the defaults for the MCE Kconfig sub-options - those should be
> kept as is, and if
> the new code has any kind of dependency on them, the to-be-added new
> Kconfig option should express this accurately.

Maybe we should change the "add" to "copy back". That two code are from native 
kernel, but it is missed when copy the apic.c to apic-xen.c, we just add it 
back. (After all, these hunks just change xen specific code, apic-xen.c and 
mach-xen/asm/hw-irq.h)

> 
> Basically, behavior for a native kernel built from the same
> sources should not
> be modified at all.
> 
>> But yes, maybe following hunk can be done in xen-code to avoid 
>> needless touch. 
>> 
>> +
>> +    /*Register vIRQ handler for MCE LOG processing*/
>> +#if defined (CONFIG_XEN) && defined(CONFIG_X86_MCE_INTEL)
>> +    printk(KERN_DEBUG "MCE: bind virq for DOM0 Logging\n"); +   
>> bind_virq_for_mce(); +#endif
>> +
>>      return err;
>> 
>> And following code can be removed, although it may cause dom0's
>> needless polling. 
>> 
>> +#if defined (CONFIG_XEN) && defined(CONFIG_X86_MCE_INTEL)
>> +static int check_interval = 0; /* disable polling */ +#else
>> static int check_interval = 5 * 60; /* 5 minutes */ +#endif
>> +
> 
> Actually, these two hunks actually represent examples of what I'm not
> concerned about . 

Remove it to xen-specific file will help us in future for the PV_ops dom0.

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