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 <[email protected]>
Hmm, yes, I think this is correct now (but the mapping had to persist earlier
on, likely years ago). So
Acked-by: Jan Beulich <[email protected]>
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
