WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [patch 4/4]Enable CMCI (Corrected Machine Check Error In

On Tuesday 13 January 2009 03:12:51 Ke, Liping wrote:
> 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?

When I defined the API, I already knew that 5 is not enough for Intel.
So I made struct mc_info large enough to keep multiple entities in any
combination. I expected from you to fill struct mcinfo_extended two or
three times into struct mc_info  the same way as you do with
struct mcinfo_bank.   There's no (additional) allocation needed.

From your description I just read, that you don't know this marshalling 
technique.


> > 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
> };

Your proposed change fixes the alignment problem, but it doesn't answer my
question: Why is mc_apicid needed at all since the CPU core is already
identified by mc_coreid ?

Christoph



-- 
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34
85609 Dornach b. München
 
Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis München
Registergericht München, HRB Nr. 43632


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