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

[Christoph Egger <Christoph.Egger@xxxxxxx>]

On 12/14/11 13:42, Jan Beulich wrote:
> > > > 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?
This little structure eliminates the uses buf_pos[x] throughout the
code which is much easier to read and understand.

> If anything, this should be data[] (and no need for const).
Then it is data[]

> > @@ -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.
done (assuming you mean 'hdr').

> > +    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'.
done.

> > +    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).
done.

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

> >          printk(KERN_ERR "microcode: error! Wrong "
> > -               "microcode patch file magic\n");
> > +            "microcode patch file magic\n");
> Why are you still mis-adjusting indentation here?
oh...

> > +    mc_amd = xmalloc_bytes(sizeof(*mc_amd));
> As said during the first review round - this ought to be
>
>      mc_amd = xmalloc(struct microcode_amd);
sorry, didn't realize that it was not only relevant to the shown line
of code.

> > +    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
> > +        buf, bufsize,&offset)) == 0 )
> Bad indentation.
Fixed.

> > +    ASSERT(src != NULL);
> Pointless.
Maybe. Maybe not. Anyway, I removed it.

> > +        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).
Fixed.

New version attached.
Signed-off-by: Christoph Egger <Christoph.Egger@xxxxxxx>

> Jan

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Attachment: xen_microcode_fam15.diff
Description: xen_microcode_fam15.diff

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