Re: [PATCH 08/10] x86/ucode: Use bootstrap_unmap() in early_microcode_load()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.10.2024 10:18, Andrew Cooper wrote:
> If bootstrap_map() has provided a mapping, we must free it when done.  Failing
> to do so may cause a spurious failure for unrelated logic later.
> 
> Inserting a bootstrap_unmap() here does not break the use of ucode_{blob,mod}
> any more than they already are.
> 
> Add a printk noting when we didn't find a microcode patch.  It's at debug
> level, because this is the expected case on AMD Client systems, and SDPs, but
> for people trying to figure out why microcode loading isn't work, it might be
> helpful.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Hmm, yes, I think this is correct now (but the mapping had to persist earlier
on, likely years ago). So
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
However, as a nit, ...

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -823,6 +823,7 @@ static int __init early_microcode_load(struct boot_info 
> *bi)
>      size_t size;
>      struct microcode_patch *patch;
>      int idx = opt_mod_idx;
> +    int rc = 0;

... the initializer doesn't appear to be needed here; all paths ...

> @@ -878,15 +879,24 @@ static int __init early_microcode_load(struct boot_info 
> *bi)
>      patch = ucode_ops.cpu_request_microcode(data, size, false);
>      if ( IS_ERR(patch) )
>      {
> -        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
> -               PTR_ERR(patch));
> -        return PTR_ERR(patch);
> +        rc = PTR_ERR(patch);
> +        printk(XENLOG_WARNING "Microcode: Parse error %d\n", rc);
> +        goto unmap;
>      }
>  
>      if ( !patch )
> -        return -ENOENT;
> +    {
> +        printk(XENLOG_DEBUG "Microcode: No suitable patch found\n");
> +        rc = -ENOENT;
> +        goto unmap;
> +    }
> +
> +    rc = microcode_update_cpu(patch, 0);
>  
> -    return microcode_update_cpu(patch, 0);
> + unmap:
> +    bootstrap_unmap();
> +
> +    return rc;
>  }

... reliably set the variable. (I don't recall what our conclusion was as to
Misra possibly considering such unnecessary initializers dead code.)

Jan