Re: [Xen-devel] [PATCH] tools/xen-mceinj: support AMD

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.12 at 16:07, Christoph Egger <Christoph.Egger@xxxxxxx> wrote:
> #define LOGFILE stdout
> 
> int dump;
>+int opt_exception;
> struct xen_mc_msrinject msr_inj;
>+int cpu_is_amd;
>+int cpu_is_intel;

Albeit I realize that this isn't the case with the context code here,
let's not continue bad habits: The newly added variables should
be static and - as long as not precluded by their use - bool.

>+
> 
> static void Lprintf(const char *fmt, ...)
> {
>...
>         case MCi_type_MISC:
>             addr = MSR_IA32_MC0_CTL + (bank * 4) + type;
>             break;
>+        case MC4_type_MISC1:
>+            addr = 0xc0000408;
>+            break;
>+        case MC4_type_MISC2:
>+            addr = 0xc0000409;
>+            break;
>+        case MC4_type_MISC3:
>+            addr = 0xc000040a;
>+            break;
>         case MCi_type_CTL2:
>             addr = MSR_IA32_MC0_CTL2 + bank;
>             break;

What makes it only bank 4 being added here? This question also
applies to the hypervisor side patch you sent on Friday.

>-    sprintf(path, "/local/domain/%d/memory/target", domid);
>+    snprintf(path, sizeof(path), "/local/domain/%d/memory/target", domid);
 
While fine with me, this is completely unrelated.

>+static void cpuid(const unsigned int *input, unsigned int *regs)

While it makes no difference to the treatment of the parameter or
the generated code, this should still be "unsigned int regs[4]" for
documentation purposes.

>+{
>+    unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
>+    asm (
>+#ifdef __i386__
>+        "push %%ebx; push %%edx\n\t"
>+#else
>+        "push %%rbx; push %%rdx\n\t"
>+#endif
>+        "cpuid\n\t"
>+        "mov %%ebx,4(%4)\n\t"
>+        "mov %%edx,12(%4)\n\t"
>+#ifdef __i386__
>+        "pop %%edx; pop %%ebx\n\t"
>+#else
>+        "pop %%rdx; pop %%rbx\n\t"
>+#endif
>+        : "=a" (regs[0]), "=c" (regs[2])
>+        : "0" (input[0]), "1" (count), "S" (regs)
>+        : "memory" );
>+}

What did you clone this from? It re-introduces a bug long fixed in
libxc (use of push/pop here collides with the 64-bit red zone; see
tools/libxc/xc_cpuid_x86.c:cpuid() and its history).

>+static void cpuid_brand_get(char *str)
>+{
>+    unsigned int input[2] = { 0, 0 };
>+    unsigned int regs[4];
>+
>+    cpuid(input, regs);
>+
>+    *(uint32_t *)(str + 0) = regs[1];
>+    *(uint32_t *)(str + 4) = regs[3];
>+    *(uint32_t *)(str + 8) = regs[2];
>+    str[12] = '\0';
>+}

I believe that by way of using a suitably defined union you can
get away here without any type casts (which I would generally
expect the compiler to warn about, as I think [hope] that the
tools don't get built with -fno-strict-aliasing).

Also, the file use hypervisor coding style, so please follow that
in the adjustments you are doing (and in particular please don't
do any adjustments just to _remove_ that coding style).

>-    int type = MCE_SRAO_MEM;
>+    int type;
>...
>+    if (cpu_is_intel)
>+        type = INTEL_MCE_SRAO_MEM;

So on AMD "type" remains uninitialized unless the -t option was
used?

Jan


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