Re: [Xen-devel] [PATCH 1/3] xen/mce: Add mcelog support for Xen platform (v2)

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

Konrad Rzeszutek Wilk wrote:
> On Fri, May 25, 2012 at 05:56:56PM +0000, Liu, Jinsong wrote:
>> Konrad Rzeszutek Wilk wrote:
>>>>>>>> -static struct miscdevice mce_chrdev_device = {
>>>>>>>> +struct miscdevice mce_chrdev_device = {
>>>>>>>>        MISC_MCELOG_MINOR,
>>>>>>>>        "mcelog",
>>>>>>>>        &mce_chrdev_ops,
>>>>>>> 
>>>>>>> You're still reusing those - pls, define your own 'struct
>>>>>>> miscdevice mce_chrdev_device' in drivers/xen/ or somewhere
>>>>>>> convenient and your own mce_chrdev_ops. The only thing you
>>>>>>> should be touching in arch/x86/.../mcheck/ is the export of
>>>>>>> MISC_MCELOG_MINOR. 
>>>>>>> 
>>>>>>> Thanks.
>>>>>> 
>>>>>> I'm *not* reuse native code.
>>>>>> I have defined 'struct miscdevice xen_mce_chrdev_device' in
>>>>>> drivers/xen, and I also implement xen_mce_chrdev_ops, they are
>>>>>> all xen-self-contained. 
>>>>>> 
>>>>>> The patch just redirect native mce_chrdev_device to
>>>>>> xen_mce_chrdev_device when running under xen environment.
>>>>>> It didn't change any native code (except just cancel
>>>>>> mce_chrdev_device 'static'), and will not break native logic.
>>>>> 
>>>>> Why are you doing that?
>>>>> 
>>>>> Why don't you do
>>>>> 
>>>>>   misc_register(&xen_mce_chrdev_device);
>>>>> 
>>>>> in xen_early_init_mcelog() ?
>>>>> 
>>>>> This way there'll be no arch/x86/ dependencies at all.
>>>> 
>>>> The reason is, if we do so, it would be covered by native
>>>> misc_register(&mce_chrdev_device) later when native kernel init
>>>> (xen init first and then start native kernel).
>>> 
>>> Won't the second registration (so the original one) of the major
>>> fail? So the mce_log would just error out since somebody already
>>> registered?
>> 
>> No, that would be device confliction, the 2nd register return as
>> -EBUSY and un-predicetable result. 
> 
> And the existing code does not actually check the 'misc_register'
> return value? Ah yes. Perhaps then a fix to
> arch/x86/kernel/cpu/mcheck/mce.c to do the proper de-registration if
> 'misc_register' fails? 

It's weird. From code point of view, it indeed not check return value so it 
should go silently. mce.c didn't do misc_deregister.

> 
> Or just set 'mce_disabled=1' in the bootup of Xen, similar to
> how lguest.c does it?
> 
>> I test it in your way, mcelog fail to fetch any error log.
> 
> And that is b/c of? What exactly?

I don't know exactly what's the reason, but test again still fail.

Thanks,
Jinsong



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel