Re: [Xen-devel] [PATCH] x86/ucode: Improve error handling and container file processing on AMD

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.12 at 19:28, Boris Ostrovsky <boris.ostrovsky@xxxxxxx> wrote:
> Do not report error when a patch is not appplicable to current processor,
> simply skip it and move on to next patch in container file.
> 
> Process container file to the end instead of stopping at the first
> applicable patch.
> 
> Log the fact that a patch has been applied at KERN_WARNING level, add
> CPU number to debug messages.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxx>
> ---
>  xen/arch/x86/microcode_amd.c |   69 
> +++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index 7a54001..bb5968e 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -88,13 +88,13 @@ static int collect_cpu_info(int cpu, struct cpu_signature 
> *csig)
>  
>      rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
>  
> -    printk(KERN_DEBUG "microcode: collect_cpu_info: patch_id=%#x\n",
> -           csig->rev);
> +    printk(KERN_DEBUG "microcode: CPU%d collect_cpu_info: patch_id=%#x\n",
> +           cpu, csig->rev);
>  
>      return 0;
>  }
>  
> -static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>  {
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      const struct microcode_header_amd *mc_header = mc_amd->mpb;
> @@ -125,11 +125,16 @@ static int microcode_fits(const struct microcode_amd 
> *mc_amd, int cpu)
>          printk(KERN_DEBUG "microcode: CPU%d patch does not match "
>                 "(patch is %x, cpu base id is %x) \n",
>                 cpu, mc_header->processor_rev_id, equiv_cpu_id);
> -        return -EINVAL;
> +        return 0;
>      }
>  
>      if ( mc_header->patch_id <= uci->cpu_sig.rev )
> +    {
> +        printk(KERN_DEBUG "microcode: CPU%d patching is not needed: "
> +               "patch provides level 0x%x, cpu is at 0x%x \n",

Did you not notice that all the other messages now use %#x?

Also, I'm not really convinced we need this added verbosity. I
personally tend to run all my systems with "loglvl=all", and the
amount of output produced by the code in this file made me
change that for just the one AMD machine I have that is new
enough to support microcode loading.

Minimally I'd expect nothing to be printed here if the two
versions match.

> +               cpu, mc_header->patch_id, uci->cpu_sig.rev);
>          return 0;
> +    }
>  
>      printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
>             "update with version %#x (current=%#x)\n",
> @@ -173,7 +178,7 @@ static int apply_microcode(int cpu)
>          return -EIO;
>      }
>  
> -    printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n",
> +    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to 
> %#x\n",
>             cpu, uci->cpu_sig.rev, hdr->patch_id);
>  
>      uci->cpu_sig.rev = rev;
> @@ -181,7 +186,7 @@ static int apply_microcode(int cpu)
>      return 0;
>  }
>  
> -static int get_next_ucode_from_buffer_amd(
> +static int get_ucode_from_buffer_amd(
>      struct microcode_amd *mc_amd,
>      const void *buf,
>      size_t bufsize,
> @@ -194,8 +199,12 @@ static int get_next_ucode_from_buffer_amd(
>      off = *offset;
>  
>      /* No more data */
> -    if ( off >= bufsize )
> -        return 1;
> +    if ( off >= bufsize ) 
> +    {
> +        printk(KERN_ERR "microcode: error! "
> +               "ucode buffer overrun\n");

All on one line please (and the "error!" probably is superfluous).

Also, is printing this really correct when off == bufsize? Or can
this not happen at all?

> +        return -EINVAL;
> +    }
>  
>      mpbuf = (const struct mpbhdr *)&bufp[off];
>      if ( mpbuf->type != UCODE_UCODE_TYPE )
> @@ -205,8 +214,8 @@ static int get_next_ucode_from_buffer_amd(
>          return -EINVAL;
>      }
>  
> -    printk(KERN_DEBUG "microcode: size %zu, block size %u, offset %zu\n",
> -           bufsize, mpbuf->len, off);
> +    printk(KERN_DEBUG "microcode: CPU%d size %zu, block size %u, offset 
> %zu\n",
> +           raw_smp_processor_id(), bufsize, mpbuf->len, off);
>  
>      if ( (off + mpbuf->len) > bufsize )
>      {
> @@ -278,8 +287,8 @@ static int cpu_request_microcode(int cpu, const void 
> *buf, size_t bufsize)
>  {
>      struct microcode_amd *mc_amd, *mc_old;
>      size_t offset = bufsize;
> +    size_t last_offset, applied_offset = 0;
>      int error = 0;
> -    int ret;
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>  
>      /* We should bind the task to the CPU */
> @@ -321,33 +330,32 @@ static int cpu_request_microcode(int cpu, const void 
> *buf, size_t bufsize)
>       */
>      mc_amd->mpb = NULL;
>      mc_amd->mpb_size = 0;
> -    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize,
> -                                                  &offset)) == 0 )
> +    last_offset = offset;
> +    while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
> +                                               &offset)) == 0 )
>      {
> -        error = microcode_fits(mc_amd, cpu);
> -        if (error <= 0)
> -            continue;
> +        if ( microcode_fits(mc_amd, cpu) )
> +            if ( apply_microcode(cpu) == 0 )

Fold the two if()-s into one, please.

But then again you lose the return value of apply_microcode()
here, which is wrong.

> +                applied_offset = last_offset;
>  
> -        error = apply_microcode(cpu);
> -        if (error == 0)
> -        {
> -            error = 1;
> +        last_offset = offset;
> +
> +        if ( offset >= bufsize )
>              break;
> -        }
>      }
>  
> -    if ( ret < 0 )
> -        error = ret;
> -
>      /* On success keep the microcode patch for
>       * re-apply on resume.
>       */
> -    if ( error == 1 )
> +    if ( applied_offset != 0 )
>      {
> -        xfree(mc_old);
> -        error = 0;
> +        error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
> +                                          &applied_offset);
> +        if (error == 0)
> +            xfree(mc_old);
>      }
> -    else
> +
> +    if ( applied_offset == 0 || error != 0 )
>      {
>          xfree(mc_amd);
>          uci->mc.mc_amd = mc_old;
> @@ -364,10 +372,9 @@ static int microcode_resume_match(int cpu, const void 
> *mc)
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>      const struct microcode_amd *src = mc;
> -    int res = microcode_fits(src, cpu);
>  
> -    if ( res <= 0 )
> -        return res;
> +    if ( microcode_fits(src, cpu) == 0 )

microcode_fits() now returning bool_t clearly asks for using ! instead
of == 0 here.

Jan

> +        return 0;
>  
>      if ( src != mc_amd )
>      {
> -- 
> 1.7.10.4



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