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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 6/24/2014 2:23 AM, Jan Beulich wrote:
> > --- a/xen/arch/x86/microcode_amd.c
> > +++ b/xen/arch/x86/microcode_amd.c
> > @@ -29,6 +29,17 @@
> >   
> >   #define pr_debug(x...) ((void)0)
> >   
> > +#define CONT_HDR_SIZE           12
> > +#define SECTION_HDR_SIZE        8
> > +#define AMD_UCODE_APPLY_SUCCESS 0
> > +/*
> > + * Currently, there are two AMD container files on
> > + *
> > https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode
> > + * So, this is max number of container binaries that can be appended
> > + * together and packaged along with initrd.
> > + */
> > +#define AMD_MAX_CONT_APPEND     2
> So as soon as a 3rd one appears the code will need to be changed
> again? Pretty bad approach I would say.
Yeah. I thought about it too. But if a new container comes up, then it 
will still take
very little effort to change this macro here.

The idea was that I could use this for catching the corner case.

Anyway, I have got a different way to handle that. (so this macro def 
will go as a result)
(explanation below...)

> > @@ -124,30 +135,43 @@ static bool_t verify_patch_size(uint32_t patch_size)
> >       return (patch_size <= max_size);
> >   }
> >   
> > -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> > +static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
> > +                              unsigned int current_cpu_id,
> > +                              unsigned int *equiv_cpu_id)
> >   {
> > -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> > -    const struct microcode_header_amd *mc_header = mc_amd->mpb;
> > -    const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
> > -    unsigned int current_cpu_id;
> > -    unsigned int equiv_cpu_id = 0x0;
> >       unsigned int i;
> > +    unsigned int found = 0;
> This is your return value - why is it "unsigned int" when the function
> returns bool_t? But the variable is pointless anyway, since ...
>
> >   
> > -    /* We should bind the task to the CPU */
> > -    BUG_ON(cpu != raw_smp_processor_id());
> > -
> > -    current_cpu_id = cpuid_eax(0x00000001);
> > +    if ( !equiv_cpu_table )
> > +        return 0;
> >   
> >       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;
> > +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> > +            found = 1;
> >               break;
> >           }
> >       }
> >   
> > -    if ( !equiv_cpu_id )
> > +    return found;
> > +}
> ... rather than break-ing from the loop you could just "return 1"
> there.
Fixed this.

> > @@ -211,7 +235,7 @@ static int apply_microcode(int cpu)
> >   
> >       uci->cpu_sig.rev = rev;
> >   
> > -    return 0;
> > +    return AMD_UCODE_APPLY_SUCCESS;
> Definitely not: This function _has to_ return zero (due to its use in
> the initializer of microcode_amd_ops), i.e. someone altering the
> definition of AMD_UCODE_APPLY_SUCCESS (e.g. making it a
> boolean) would break things.
So, the main reason behind introducing this macro was that-
For single containers, in cases of success, the last value assigned for 
'error'
is from 'apply_microcode' (above). And unless there is a 'save_error', 
this is the value
returned.
I wanted to make it clear that we are returning this value for multiple 
containers too (on success)
I understand your concern, So I have changed it to return 0.

> > @@ -236,7 +260,15 @@ static int get_ucode_from_buffer_amd(
> >       mpbuf = (const struct mpbhdr *)&bufp[off];
> >       if ( mpbuf->type != UCODE_UCODE_TYPE )
> >       {
> > -        printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
> > +        /*
> > +         * We will hit this condition only if an update has succeeded
> > +         * but there is still another container file being parsed.
> > +         * In that case, there is no need of this ERR message to be
> > +         * printed.
> > +         */
> > +        if ( mpbuf->type != UCODE_MAGIC )
> > +            printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
> The more often I look at it, the less convinced am I that this is okay:
> UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
> that the field matches up with the next block signature. Either don't
> abuse the field, or make clear via comment why it is done this way.
Would this be better? -
if ( *(const uint32_t *)buf != UCODE_MAGIC )

I can still let the above comment be there (or modify it to make it more 
clear?)

> > @@ -272,14 +304,13 @@ 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 void *data,
> > +    size_t *curr_offset)
> Is there any strong reason to rename "offset" to "curr_offset", ...
No. Have fixed this.

> >   {
> > +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
> >       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
> >   
> > -    /* No more data */
> > -    if ( mpbuf->len + 12 >= *offset )
> > -        return -EINVAL;
> Iirc you and Boris agreed that this check is pointless _here_. But I
> doubt it can be removed without replacement elsewhere.
For single containers, this check made some sense earlier as we verify 
to see there is *some*
data beyond the equivalent_table structure.
Say, mpbuf->len=0 and we return error val; Due to the fact that we have 
already advanced *offset,
cases when we reach EOF or *offset goes over bufsize is handled in 
container_fast_forward
function.

For multiple containers, we will always have at least two such container 
headers and hence,
mpbuf->len + 12 is always less than total_size

If first container for some reason is corrupted and exposes 
mpbuf->len=0,  we return EINVAL
and forward to next container.
(This is infact one reason to  advance *offset earlier. See below)

Now, if the last container were to have mpbuf->len=0,
As Boris mentioned on earlier thread, we will
continue because 'if (0+12 >= tot_size) ' is false.
Here too, we will return EINVAL.

Again, advancing *offset early allows to workaround these issues.
And this check can be removed as a result.

> > +    *curr_offset += mpbuf->len + CONT_HDR_SIZE;     /* add header length */
> >   
> >       if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
> >       {
> > @@ -303,7 +334,30 @@ static int install_equiv_cpu_table(
> >       memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len);
> >       mc_amd->equiv_cpu_table_size = mpbuf->len;
> >   
> > -    *offset = mpbuf->len + 12;	/* add header length */
> ... and to move this assignment up?
So, If for some reason this function returns error val for the _first_ 
container;
Then *offset would still be 0.
And 'container_fast_forward' will not advance it as both 'MAGIC' and 
'UCODE_EQUIV_CPU_TABLE_TYPE' checks match.
We will be stuck in this loop.

Hence, I moved the assignment up.
But, this is also introducing some issues that I failed to handle here.
Have fixed it now. (See below)

> > +    return 0;
> > +}
> > +
> > +static int container_fast_forward(const void *data, size_t size_left, size_t 
> > *offset)
> > +{
> > +    size_t size;
> > +    uint32_t *header;
> > +
> > +    while ( size_left )
> > +    {
> > +        header = (uint32_t *) (data + *offset);
> Pointless (and wrong, as it discards the const qualifier) cast.
Ok. I have removed this cast (and as a consequence *header) entirely.
Also need -
if ( size_left < 0 ) check. Reason-
With offset value being advanced aggressively in 'install_equiv_cpu_table',
It could be that we go over the bufsize. This situation will be caught here.


> > +        if ( header[0] == UCODE_MAGIC &&
> > +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> Are these two valid if size_left < SECTION_HDR_SIZE?
Fixed this.

> > +            break;
> > +
> > +        size = header[1] + SECTION_HDR_SIZE;
> > +        *offset += size;
> > +
> > +        if ( size_left >= size )
> > +            size_left -= size;
> > +    }
> And if size_left < size we're continuing the loop (perhaps indefinitely)?
Fixed this.
Instead of above check, I inverted the relation and saved an indentation 
level.

> > @@ -334,7 +393,33 @@ 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 ( containers_processed <= AMD_MAX_CONT_APPEND )
> Leaving aside the point made above regarding this limit, you set the
> limit to 2 but then allow 3 iterations?
Have modified this to while ( offset < bufsize )

> > @@ -379,7 +464,15 @@ 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 ( save_error )
> > +        /*
> > +         * If there happens to be multiple container files and if patch
> > +         * update succeeded on an earlier container itself, a stale error
> > +         * val is returned from get_ucode_from_buffer_amd. Since we already
> > +         * succeeded in patch application, return as success itself
> > +         */
> > +        if ( (containers_processed < AMD_MAX_CONT_APPEND) && error )
> > +            error = AMD_UCODE_APPLY_SUCCESS;
> > +        else if ( save_error )
> >               error = save_error;
> This still seems wrong to me: For one, you don't ever try to apply
> a patch from another container if one succeeded (since the looking
> for containers sits in a separate loop prior to the one looking into
> the container, and there's no path back to the top of the first loop).
This is because there can be only one container that has the valid patch.

> Further I again don't follow the conditions you use:
> containers_processed >= AMD_MAX_CONT_APPEND should have
> got dealt with (if at all possible) much earlier (after the first loop),
> and the "&& error" looks superfluous as long as
> AMD_UCODE_APPLY_SUCCESS == 0. Which finally gets us back to
> the same point made above - this function again has to return zero
> on success, no matter what AMD_UCODE_APPLY_SUCCESS is
> defined to.
Ok, I have removed AMD_MAX_CONT_APPEND altogether now, and changed it to-
if ( offset < bufsize )
  error = 0;

From the loop: while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
we will always have (offset == bufsize) if the patch happens to be on 
the last container file (even as apply_microcode succeeds)
So, the modified check should work fine. While at the same time does not 
change behavior of single container files.
Btw, we also need a similar check here:
  if ( save_error )
    {
        if ( (error != save_error) && (offset < bufsize) ) {
             error = 0;
        }
        xfree(mc_amd);
        uci->mc.mc_amd = mc_old;
    }

Reason:
By the time 'microcode_init' runs, we have already updated the patch 
level on all (currently) running cpus.
So, get_ucode_from_buffer_amd will return -EINVAL due to -
if ( mpbuf->type != UCODE_UCODE_TYPE )

Only this time, there is no 'applied_offset' as well.
So, 'save_error' = 1. But error == -EINVAL.
This situation can only occur in the case of multiple container files 
and only when update succeeded with
the first container file itself. Hence, this check is also necessary to 
return 0 for success.
I'll await your comments before sending out a V3.

Thanks,
-Aravind.


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