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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 6/27/2014 5:50 AM, Jan Beulich wrote:
> > > > On 25.06.14 at 21:34, <aravind.gopalakrishnan@xxxxxxx> wrote:
> > This patch adds support for parsing through multiple AMD container
> > binaries concatenated together. It is a feature already present in Linux.
> > Link to linux patch:
> > http://lkml.kernel.org/r/1370463236-2115-3-git-send-email-jacob.shin@xxxxxxx
> >
> > Other changes introduced:
> >   - Define HDR_SIZE's explicitly for code clarity.
> >
> > Changes in V3
> >   - Change approach that used AMD_MAX_CONT_APPEND to handle edge case
> >   - No need of a 'found' variable
> >   - return 0 for success and not AMD_UCODE_APPLY_SUCCESS as someone
> >     could just as easily break things by redefining it as bool
> >   - No need of 'header' in container_fast_forward
> >   - Handle more corner cases in container_fast_forward
> >   - Fix code style issues
> Can you please get used to put this revision log after the first ---
> marker? It doesn't belong in ti actual commit message.
Okay.

> > @@ -272,14 +293,13 @@ static int get_ucode_from_buffer_amd(
> >   
> >   static int install_equiv_cpu_table(
> >       struct microcode_amd *mc_amd,
> > -    const uint32_t *buf,
> > +    const void *data,
> >       size_t *offset)
> >   {
> > +    uint32_t *buf = (uint32_t *) (data + *offset);
> I know I complained about this or a similar pointless cast for the
> previous version.
Yes, I thought I'll handle it as a separate cleanup patch as it was not 
directly
related to the subject of the patch. But it's a small change, so I'll do 
this and include
it in the commit message as well..

> > +static int container_fast_forward(const void *data, size_t size_left, size_t 
> > *offset)
> > +{
> > +    size_t size;
> > +
> > +    while ( size_left )
> > +    {
> > +        if ( size_left < SECTION_HDR_SIZE )
> > +            return -EINVAL;
> > +
> > +        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
> > +             *(const uint32_t *)(data + *offset + 4) ==
> > +                                 UCODE_EQUIV_CPU_TABLE_TYPE )
> > +            break;
> > +
> > +        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;
> Now these three casts+derefs surely warrant a "const uint32_t *"
> variable, making the resulting code quite a bit easier to read.
Okay. Will just use the bits in V2 with a const qualifier

> > +        if ( size_left < size )
> > +            return -EINVAL;
> > +
> > +        size_left -= size;
> > +        *offset += size;
> Endless loop if size ends up being zero? (If you do such verifications
> at all, you should perhaps be as strict as possible,
Sorry about that (Again). Will fix.


> i.e. if size is
> required to be a multiple of 4 [which I think it is], you should also
> check that.)
I don't think that's a rule per-se.
#define F16H_MPB_MAX_SIZE 3458
Which is indivisible by 4.

> > @@ -334,7 +384,40 @@ 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 ( offset < bufsize )
> > +    {
> > +        error = install_equiv_cpu_table(mc_amd, buf, &offset);
> > +
> > +        if ( !error &&
> > +             find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> > +                               &equiv_cpu_id) )
> > +                break;
> > +
> > +        /*
> > +         * Could happen as we advance 'offset' early
> > +         * in install_equiv_cpu_table
> > +         */
> > +        if ( offset > bufsize )
> > +        {
> > +            printk(KERN_ERR "microcode: Microcode buffer overrun\n");
> > +            return -EINVAL;
> > +        }
> > +
> > +        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);
> > +            goto out;
> > +        }
> So the immediately preceding error path used just "return" - why
> are they different (and I'm not asking about the printk())?
Ok, Shall use one or the other.. (Probably the goto as I see that's the 
favored one in this function)

> > @@ -379,12 +462,35 @@ 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
> Do you perhaps mean bogus instead of stale?
Yes. Wrong word. Will fix.

> > +         * val is returned from get_ucode_from_buffer_amd. Since we already
> > +         * succeeded in patch application, force error = 0
> > +         */
> > +        if ( offset < bufsize )
> > +            error = 0;
> > +        else if ( save_error )
> >               error = save_error;
> And still my question stands: Since you don't look at further containers
> if you found a match and successfully applied the updated, why is this
> change needed (or is the comment saying the wrong thing)?
Maybe the comment needs to be more verbose(?)

Yes, we found a match and yes, we applied the patch successfully.
But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
and return -EINVAL
which is assigned to the variable 'error'
(Assuming ofc that there is a second container there which we don't need 
to parse because
we have already succeeded in patch application)

This is what I wanted to convey from

"astale  bogus error val is returned from get_ucode_from_buffer_amd."

But, we need to return 0 on success; which is why this change is needed 
here..
> >       }
> >   
> >       if ( save_error )
> >       {
> > +        /*
> > +         * By the time 'microcode_init' runs, we have already updated the
> > +         * patch level on all (currently) running cpus.
> > +         * But, get_ucode_from_buffer_amd will return -EINVAL as
> > +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
> > +         * Multiple containers are present && update succeeded with the
> > +         * first container file itself.
> > +         *
> > +         * Only this time, there is no 'applied_offset' as well.
> > +         * So, 'save_error' = 1. But error = -EINVAL.
> > +         * Hence, this check is necessary to return 0 for success.
> > +         */
> > +        if ( (error != save_error) && (offset < bufsize) )
> > +            error = 0;
> Same for this change/comment.
Hmm.. I'm having trouble trying to re-word this comment then..

Given the situation where  - we have already applied the patch update after
'microcode_presmp_init' and 'microcode_resume_cpu';
   |
   v
Now 'microcode_init' runs and calls into 'cpu_request_microcode';
   |
   v
We use 1st while loop to find_equiv_cpu_id() and match it with the container
   |
   v
For this particular case, we assume it's a match on the 1st container; 
so break
   |
   v
Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
   |
   v
At some point, it will find the correct patch; but this time there is no 
need to update
   |
   v
The behavior is now similar to what I have described above. i.e
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
and return -EINVAL
which is assigned to the variable 'error'
   |
   v
But, now (as stated in the comment..)

* Only this time, there is no 'applied_offset' as well.
+         * So, 'save_error' = 1. But error = -EINVAL.

   |
   v
And since we need to return 0 for success, this change is needed here.

Thanks,
-Aravind

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