Re: [Xen-devel] [PATCH V2] x86, amd_ucode: Safeguard against #GP

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.05.14 at 20:24, <aravind.gopalakrishnan@xxxxxxx> wrote:
> When HW tries to load a corrupted patch, it generates #GP
> and hangs the system. Use wrmsr_safe instead so that we
> fail to load microcode gracefully.

Leaving aside the question of whether this is needed at all (see
Andrew's earlier reply), a couple of mechanical comments:

> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -178,32 +178,39 @@ static int apply_microcode(int cpu)
>      uint32_t rev;
>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>      struct microcode_header_amd *hdr;
> +    int error = -EINVAL;
>  
>      /* We should bind the task to the CPU */
>      BUG_ON(raw_smp_processor_id() != cpu);
>  
>      if ( mc_amd == NULL )
> -        return -EINVAL;
> +        goto apply_err1;

Why?

>  
>      hdr = mc_amd->mpb;
>      if ( hdr == NULL )
> -        return -EINVAL;
> +        goto apply_err1;

And again, why?

>  
>      spin_lock_irqsave(&microcode_update_lock, flags);
>  
> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
> +    error = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>  
>      /* get patch id after patching */
>      rdmsrl(MSR_AMD_PATCHLEVEL, rev);
>  
>      spin_unlock_irqrestore(&microcode_update_lock, flags);
>  
> +    /* Catch HW patch application failure */
> +    if ( error ) {

Coding style.

> +        printk(KERN_ERR "microcode: CPU%d ucode patch application failed HW 
> tests. "
> +               "HW returned #GP\n", cpu);
> +        goto apply_err2;

No need to print two messages.

> +    }
> +
>      /* check current patch id and patch's id for match */
>      if ( rev != hdr->patch_id )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed\n", cpu, hdr->patch_id, rev);
> -        return -EIO;
> +        error = -EIO;
> +        goto apply_err2;

And with the above, this change isn't warranted too.

> @@ -212,6 +219,12 @@ static int apply_microcode(int cpu)
>      uci->cpu_sig.rev = rev;
>  
>      return 0;
> +
> +apply_err2:
> +    printk(KERN_ERR "microcode: CPU%d update from revision "
> +           "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
> +apply_err1:
> +    return error;

While as a consequence this change will go away too, for the
future - if you add any labels, they ought to be indented by at
least on space.

Jan


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