Re: [PATCH] x86, amd_ucode: Skip microcode updates for final levels

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/07/15 17:23, Aravind Gopalakrishnan wrote:
> Some of older[Fam10h] systems require that the microcode versions
> that it comes up with should not be updated by the microcode driver.
> Otherwise, system hangs are known to occur.
>
> In this patch, we check for those microcode versions and abort the
> update process if existing microcode level is already applied by
> the BIOS.
>
> A linux version of the patch has already made it into tip-
> http://marc.info/?l=linux-kernel&m=143703405627170
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
>  xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index f79b397..c958a47 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -347,6 +347,30 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
>      return 0;
>  }
>  

Please include the same comment as the Linux patch, explaining that
these microcode versions can't be updated from.

I would also like to see some documentation from AMD concerning this.

> +static unsigned int final_levels[] = {
> +    0x01000098,
> +    0x0100009f,
> +    0x010000af,
> +    0
> +};
> +
> +static bool_t check_final_patch_levels(int cpu)
> +{
> +    /*
> +     * Check the current patch levels on the cpu. If they are equal to
> +     * any of the 'final_levels', then we should not update the microcode
> +     * patch on the cpu as system will hang otherwise.
> +     */
> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> +    int i;

unsigned

> +
> +    for ( i = 0; final_levels[i]; i++ )

ARRAY_SIZE(), and drop the 0 on the end of the list.

> +        if ( uci->cpu_sig.rev == final_levels[i] )
> +            return 1;
> +
> +    return 0;
> +}
> +
>  static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>  {
>      struct microcode_amd *mc_amd, *mc_old;
> @@ -369,6 +393,13 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>          goto out;
>      }
>  
> +    if ( check_final_patch_levels(cpu) )
> +    {
> +        pr_debug("microcode: Cannot update microcode patch on the cpu as we hit a final level\n");

pr_debug() is compiled out completely.  I would suggest
printk(XENLOG_INFO instead.

~Andrew

> +        error = -EPERM;
> +        goto out;
> +    }
> +
>      mc_amd = xmalloc(struct microcode_amd);
>      if ( !mc_amd )
>      {