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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 6/26/2014 11:01 AM, Boris Ostrovsky wrote:
>
>             +
>             +        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;
>             +        }
>             +    }
>             +
>
>
>         I just realized that I don't understand something: if you have
>         two merged container files, both having an entry for current
>         processor in the equivalence table but only the second file
>         having the actual patch (or at least the patch with higher
>         version), will we get to load that patch?
>
>
>     We will not come across such a situation:
>     One container has patch files for families 10h,11h,12h,14h and the
>     other has patches for 15h.
>     So there will be an equiv_cpu_id match in (at least and) only
>     *one* of the containers.
>     (If there ever are patches for 16h, then I suspect it will have a
>     separate container of it's own. So there will not be any clashes
>     with older containers)
>
>     Besides, the patches themselves are not incremental, i.e;
>     If there is a newer patch, then it handles all errata/bugs that
>     were handled by the previous patch and then some.
>     Which means, you will not have a container that has two patches
>     for the same processor.
>     So, *strictly speaking*, even the loop to
>     get_ucode_from_buffer_amd() till the end of buffer is not
>     necessary once we
>     have already applied a patch.
>
>     But, to change this behavior now will need more logic rework..
>
>
> I don't think there is a whole lot of work/testing (especially since I 
> am not the one doing it ;-)) --- you just need to skip header and 
> equivalence table of the next container.
>
> if ( mpbuf->type != UCODE_UCODE_TYPE )
> {
>      if ( *(const uint32_t *)buf == UCODE_MAGIC )
>      {
>          struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[2]; // or [1]?
Neither will work as we will need to first advance buf 4 bytes.. (but I 
get your point)
Regardless, this is not necessary.
Let me clarify (below)
>          *offset += mpbuf->len + CONT_HDR_SIZE + 4;  // I think 4, to skip MAGIC
>                                                      // and next word.
>          // Now we've fast-forwarded past header and eq. table
>          return 0; // or, goto beginning?
>      }
> }
>
> Will that work?

But we don't actually need to parse the second container for reasons 
given in the previous thread.

Example scenarios-
1. Order of containers in initrd: f15h container, container for 
remaining families.
   - equiv_cpu_id match for first container (surely you are on a f15h model)
     - parse this container, find correct patch and try to apply
     - No need to parse second container as it will not have patches for 
f15h.
   - no match for 1st container, but match on 2nd, then current 
processor is in set [f10h,f14h]
     - parse this container, find correct patch and try to apply
   - no match on both.
    - borked containers. So, abort.

2. Order of containers: container for families from f10h-f14h; container 
for f15h
   - equiv_cpu_id match for 1st container (surely you are on some family 
in the set of [f10h, f14h])
     - Parse container, find patch, try to apply
     - No need to parse second container as it will not have patches for 
f10h-f14h
  - found match on 2nd, then current processor is a f15h model,
     - parse container, find patch, try to apply
  - no match
     - borked container. So abort.

The process(,scripts) to create containers ensure that we only have 
patches for f10h-f14h on 'microcode_amd.bin'
and patches for f15h models (could be different model/stepping values) 
on microcode_amd_fam15h.bin

So it is safe to assume that we we only have a matching equiv_id in only 
one of the containers.

-Aravind.