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

["Ke, Liping" <liping.ke@xxxxxxxxx>]

Hi, Jan

Thanks a lot for the detailed review!
I resend the verified patch according to the below agreement.

Regards,
Criping

Jan Beulich wrote:
>>>> --- a/arch/x86_64/Kconfig  Tue Jun 09 09:58:11 2009 +0800
>>>> +++ b/arch/x86_64/Kconfig  Wed Jun 10 14:12:55 2009 +0800 @@ -472,7
>>>> +472,6 @@ 
>>>> 
>>>> config X86_MCE
>>>>    bool "Machine check support" if EMBEDDED
>>>> -  depends on !X86_64_XEN
>>>>    default y
>>>>    help
>>>>       Include a machine check error handler to report hardware
>>>> errors. 
>>> 
>>> Shouldn't this rather change the dependency to XEN_PRIVILEGED_GUEST?
>>> 
>> Native depends on this config too, so have about change it like this
>> depends on   !X86_64_XEN || ( X86_64_XEN && XEN_PRIVILEGED_GUEST)
> 
> Oh, right. But then why not simply using !XEN_UNPRIVILEGED_GUEST?

Yes, it's very nice.

> 
>>>> @@ -483,7 +482,7 @@
>>>> config X86_MCE_INTEL
>>>>    bool "Intel MCE features"
>>>>    depends on X86_MCE && X86_LOCAL_APIC
>>>> -  default y
>>>> +  default n
>>>>    help
>>>>       Additional support for intel specific MCE features such as
>>>>       the thermal monitor.
>>> 
>>> This hunk should go.
>> 
>> This option will not appear under XEN, only for native?
>> depends on X86_MCE && X86_LOCAL_APIC && !X86_64_XEN
> 
> Whether it appears is not the question - you're changing the default
> value for no good reason.
> 
Yes. I need not change the default value now, it is true only in 
Native. We will not build those two specifc files.

>>>> 
>>>> +config X86_64_XEN_MCE
>>>> +  def_bool y
>>>> +  depends on X86_64_XEN && (X86_MCE_INTEL || X86_MCE_AMD)
>>>> +
>>> 
>>> I wonder if this wouldn't better be named X86_XEN_MCE (for
>>> consistency with a potential 32-bit implementation).
>>> 
>> 
>> This option will depends on X86_64_XEN && X86_MCE.
>> Yes, we could rename it. Yet this config file is under X86_64 sub
>> dir, also, seems 32bit system will not have mca support?
> 
> They do - see arch/i386/kernel/cpu/mcheck/.

Actually I mean 32 bit system might have rare hardware support. But
for consistency, yes, it's better to rename it as X86_XEN_MCE.
> 
> Jan

Attachment: intel_mce_dom0.patch
Description: intel_mce_dom0.patch

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