Re: [Xen-devel] [PATCH] microcode fix

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.12.11 at 11:17, Christoph Egger <Christoph.Egger@xxxxxxx> wrote:
>+struct mpbhdr {
>+    uint32_t type;
>+    uint32_t len;
>+    const uint8_t data;

What's this? If anything, this should be data[] (and no need for const).

>@@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
>     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>     uint32_t rev;
>     struct microcode_amd *mc_amd = uci->mc.mc_amd;
>+    struct microcode_header_amd *hdr = mc_amd->mpb;

Using mc_amd here, but ...
 
>     /* We should bind the task to the CPU */
>     BUG_ON(raw_smp_processor_id() != cpu);
> 
>     if ( mc_amd == NULL )
>         return -EINVAL;

... the NULL check is only here.

>+    if ( mc_amd->mpb == NULL )
>+        return -EINVAL;

And quite obviously you should preferably use the local variable
here...

>-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
>+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);

... and here.

>+    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
>+           (unsigned long)bufsize, mpbuf->len, off);

The cast is pointless; to avoid it, use %zu as format string or declare
the parameter as 'unsigned long'.

>+    if (mc_amd->mpb_size < mpbuf->len) {
>+        xfree(mc_amd->mpb);
>+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
>+        mc_amd->mpb_size = mpbuf->len;

NULL check missing here (and in such event the clearing of
->mpb_size).

>+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);

Unnecessary cast.

>         printk(KERN_ERR "microcode: error! Wrong "
>-               "microcode patch file magic\n");
>+            "microcode patch file magic\n");

Why are you still mis-adjusting indentation here?

>+    mc_amd = xmalloc_bytes(sizeof(*mc_amd));

As said during the first review round - this ought to be

    mc_amd = xmalloc(struct microcode_amd);

>+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
>+        buf, bufsize, &offset)) == 0 )

Bad indentation.

>+    ASSERT(src != NULL);

Pointless.

>+        if (mc_amd != NULL) {

While this NULL check is needed, ...

>+            if (mc_amd->equiv_cpu_table)

... this and ...

>+                xfree(mc_amd->equiv_cpu_table);
>+            if (mc_amd->mpb)

... this are pointless.

>+                xfree(mc_amd->mpb);
>+            xfree(mc_amd);
>+        }
>+
>+        mc_amd = xmalloc(struct microcode_amd);

    uci->mc.mc_amd = mc_amd;

>         if ( !mc_amd )

(as was the case in the original code). No need to do this once in the
success path and once in the error one.

>+    uci->mc.mc_amd = mc_amd = NULL;
>+    return -ENOMEM;

Even if it was necessary to keep it this way, initializing a local variable
immediately before returning is bogus (and calling for a compiler
warning and hence a build failure due to -Werror sooner or later).

Jan


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