RE: [Xen-devel] [patch 4/4]Enable CMCI (Corrected Machine Check Error Interrupt) for Intel CPUs

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

Hi, Christoph
Please see our comments below

Christoph Egger wrote:
> On Friday 19 December 2008 07:19:16 Ke, Liping wrote:
>> Hi, all
>> 
>> Patch 4 is the main patch for CMCI enabling in XEN. It adds the CMCI
>> interrupt handler, new common CMCI/MCA init process,CMCI owner judge
>> algorithm when bring_up CPUs, CPU on/offline  and  polling
>> mechanisms, etc 
>> 
>> Thanks & Regards,
>> Criping
> 
> This patch changes the public API. There's no need to change
> struct mcinfo_extended. The marshalling technique allows to use
> this structure as often as needed. Please undo this change.

Since Intel extended msrs' number is bigger than AMD, and we can't use pointer 
and allocate memory in mca handler,
so we extended it from 5 -> 10. We think it should not impact any of your usage.

Else, we can change boundary 5->0, use a globally allocated pointer instead. 
But it seems not that necessary.
How do you think about?

> 
> struct mcinfo_global is also changed. Why can't mc_coreid not be
> filled with the apicid ? Adding the apicid field breaks the alignment
> causing troubles on 32bit-guest-on-64bit-hypervisor.
We plan to extend the apic_id to 32 bit since 8 bit is not enough for new 
apic_id.
Besides, for this problem, before sending the patch, we actually talked about 
it. Seems original structure is not 32/64 alligned.
How about below changes?

struct mcinfo_global {
    struct mcinfo_common common; 4 byte

    /* running domain at the time in error (most likely the impacted one) */
    uint16_t mc_domid; 2 byte
    uint32_t mc_socketid; /* physical socket of the physical core */ 4 byte
    uint16_t mc_coreid; /* physical impacted core */ 2 byte
    uint8_t  mc_apicid; ---change it to 4 byte
    uint16_t mc_core_threadid; /* core thread of physical core */ 2 byte
    uint16_t mc_vcpuid; /* virtual cpu scheduled for mc_domid */ 2 byte
    uint64_t mc_gstatus; /* global status */ 8 byte
    uint32_t mc_flags; 4 byte
};


Change to 
------------------------>>>>>

struct mcinfo_global {
    struct mcinfo_common common; 4 byte

    /* running domain at the time in error (most likely the impacted one) */
    uint32_t mc_socketid; /* physical socket of the physical core */ 4 byte
-----------------------------------------------------------------
    uint16_t mc_domid; 2 byte
    uint16_t mc_coreid; /* physical impacted core */ 2 byte
    uint32_t  mc_apicid; ---change it to 4 byte
-------------------------------------------------------------------
    uint16_t mc_core_threadid; /* core thread of physical core */ 2 byte
    uint16_t mc_vcpuid; /* virtual cpu scheduled for mc_domid */ 2 byte
    uint32_t mc_flags; 4 byte
--------------------------------------------------------------------------
    uint64_t mc_gstatus; /* global status */ 8 byte
};

Thanks & Regards,
Criping




> 
> Christoph


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