Re: [Xen-devel] [PATCH] x86, amd_ucode: Support multiple container files appended together

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 6/20/2014 3:11 AM, Jan Beulich wrote:
> > > > On 19.06.14 at 17:14, <aravind.gopalakrishnan@xxxxxxx> wrote:
> > @@ -124,6 +127,25 @@ static bool_t verify_patch_size(uint32_t patch_size)
> >       return (patch_size <= max_size);
> >   }
> >   
> > +static void find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
> > +                              unsigned int current_cpu_id,
> > +                              unsigned int *equiv_cpu_id)
> > +{
> > +    unsigned int i;
> > +
> > +    if ( !equiv_cpu_table )
> > +        return;
> > +
> > +    for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ )
> > +    {
> > +        if ( current_cpu_id == equiv_cpu_table[i].installed_cpu )
> > +        {
> > +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> > +            break;
> > +        }
> > +    }
> > +}
> Please have this function return a found/not-found indication
> rather than having the callers rely on *equiv_cpu_id transitioning
> from zero to no-zero. That way you can also drop the respective
> initializers in the callers.
Ok, Will modify this.
> > @@ -272,13 +294,16 @@ static int get_ucode_from_buffer_amd(
> >   
> >   static int install_equiv_cpu_table(
> >       struct microcode_amd *mc_amd,
> > -    const uint32_t *buf,
> > -    size_t *offset)
> > +    const uint8_t *data,
> > +    size_t *tot_size,
> > +    size_t *curr_offset)
> >   {
> > +    size_t off = *curr_offset;
> > +    uint32_t *buf = (uint32_t *) &data[off];
> I realize there are may casts there in this file. But please don't add
> more of them without a strict need (replacing existing ones would
> also be much appreciated). In the case here, if "data" was "const
> void *" you'd get away with just "data + off". Furthermore there's
> no reason for having the one-time use variable "off" here - just
> use *offset directly.
Ok, I'll change this per your suggestion.
About replacing the existing casts, I'll generate a separate patch for it..

> > +        if ( header[0] == UCODE_MAGIC &&
> > +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> > +            break;
> > +
> > +        size = header[1] + SECTION_HDR_SIZE;
> > +        *offset += size;
> > +        size_left -= size;
> Don't you think you should first check for size_left >= size here?
Yep. Thanks for pointing this out. I'll add it.

> > @@ -334,7 +389,30 @@ static int cpu_request_microcode(int cpu, const void *buf, 
> > size_t bufsize)
> >           goto out;
> >       }
> >   
> > -    error = install_equiv_cpu_table(mc_amd, buf, &offset);
> > +    /*
> > +     * Multiple container file support:
> > +     * 1. check if this container file has equiv_cpu_id match
> > +     * 2. If not, fast-fwd to next container file
> > +     */
> > +    while ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size,
> > +                                             &offset)) == 0 )
> > +    {
> > +        find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> > +                          &equiv_cpu_id);
> > +        if ( equiv_cpu_id )
> > +            break;
> > +
> > +        error = container_fast_forward(buf, (bufsize - offset), &offset);
> > +        if ( error )
> > +        {
> > +            printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container 
> > file\n"
> > +                   "microcode: Failed to update patch level. "
> > +                   "Current lvl:%#x\n", cpu, uci->cpu_sig.rev);
> > +            error = -EINVAL;
> Either you use the error value returned from container_fast_forward(),
> or you make that function simply return a bool_t.
Hmm, Yep. I'll fix this.

> > @@ -379,8 +457,16 @@ static int cpu_request_microcode(int cpu, const void *buf, 
> > size_t bufsize)
> >           save_error = get_ucode_from_buffer_amd(
> >               mc_amd, buf, bufsize, &applied_offset);
> >   
> > +        /*
> > +         * If there happens to be multiple container files and if patch
> > +         * update succeeded on first container itself, a stale error val
> > +         * is returned from get_ucode_from_buffer_amd. So, force
> > +         * error = 0 here as we have already succeeded in the update.
> > +         */
> >           if ( save_error )
> >               error = save_error;
> > +        else
> > +            error = 0;
> First of all I don't think this change is logically a part of $subject (as
> it also alters behavior for single container files). With that the
> question then is why the change is really correct. And if it was really
> correct, you could just drop the if() instead.
The change is needed (as stated in the comments) because we end up 
returning err val (EINVAL) like so-
DEBUG: cpu_request_microcode :  no save_err;error=-22

even though update succeeded-
(XEN) microcode: CPU0 found a matching microcode update with version 
0x600063d (current=0x6000626)
(XEN) microcode: CPU0 updated from revision 0x6000626 to 0x600063d

That said, I also understand your point that we can just drop the if(). 
So, I'll change this as per your suggestion.
The fact that we have to make this change as a _consequence_ of adding 
multiple container file support,
might explain why it was done later right?
I can make this a separate patch as well. Let me know what you think.

Thanks,
-Aravind.

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