Re: pvops microcode support for AMD FAM >= 15

[Boris Ostrovsky <boris.ostrovsky@amd.com>]

O Thu, Dec 06, 2012 at 11:13:09AM +0000, Jan Beulich wrote:
> >>> On 06.12.12 at 11:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > (Putting debian-kernel to bcc, since I don't imagine they are interested
> > in the details of this discussion, I'll reraise the result with the
> > Debian Xen maintainer when we have one)
> > 
> > On Wed, 2012-12-05 at 17:53 +0000, Ian Campbell wrote:
> >> On Wed, 2012-12-05 at 17:27 +0000, Boris Ostrovsky wrote:
> >> > On 12/05/2012 12:02 PM, Jan Beulich wrote:
> >> > > But all of this shouldn't lead to equivalent ID mismatches, should
> >> > > it? It ought to simply find nothing to update...
> >> > 
> >> > 
> >> > The patch file (/lib/firmware/amd-ucode/microcode_amd_fam15h.bin) may 
> >> > contain more than one patch. The driver goes over this file patch by 
> >> > patch and tries to see whether to apply it.
> >> > 
> >> > I think what happened in Ian's case was that the patch file contained 
> >> > two patches --- one for this processor (ID 6012) and another for a 
> >> > different processor (ID 6101). (Both are family 15h but different revs).
> >> > 
> >> > The driver applied the first patch on core 0. Then, on core 1, the code 
> >> > tried the first patch (at file offset 60) and noticed that it is already 
> >> > applied. So it continued to the next patch (at offset 2660) which is not 
> >> > meant for this processor, thus generating the "does not match" message.
> > 
> > OOI what would have happened if the two patches were in the opposite
> > order? Would CPU0 have seen the ID 6101 patch first and aborted?
> 
> That would work well.
> 
> The problem is that cpu_request_microcode() returns the result
> of its last call to microcode_fits(), no matter whether a prior one
> already returned success (>= 0).
> 
> Something like
> 
>                                                    &offset)) == 0 )
>      {
>          error = microcode_fits(mc_amd, cpu);
> -        if (error <= 0)
> +        if (error < 0)
> +            error = 0;
> +        if (error == 0)
>              continue;
>  
>          error = apply_microcode(cpu);
> 
> would apparently be needed. Or we could of course make
> microcode_fits() return a bool_t in the first place.
> 
> But then again it would probably be nice to indeed return
> failure from cpu_request_microcode() if _no_ suitable
> microcode was found at all. Question is whether one blob
> can contain more than one update for a given equivalent ID.
> If not, bailing from the loop even if microcode_fits() returns
> zero might be the right solution (and presumably the latter
> then shouldn't return zero when no equivalent ID was
> found).

I would argue that cpu_request_microcode() should not return an error if no suitable microcode is available. In most cases BIOS will already have the right version of microcode and so the driver not loading anything is really considered a normal scenario. One could even say that being able to load the microcode in the driver indicates stale BIOS and so *that* is the error. I am not suggesting returning an error on that but perhaps raising log level from KERN_INFO to KERN_WARN.

As for whether a container can have more than one update --- typically no but I would like to be able to support this.

> 
> But no matter what solution we pick, we need to review this
> carefully in the context of the earlier regressions we had in
> this area.


I will probably have a patch for review either later today or tomorrow --- I need to test more patch file configurations.

-boris