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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/cpu/microcode/core.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 6a87390ab770..28cfeab75a81 100644
--- 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;
 
     /*
      * Cmdline parsing ensures this invariant holds, so that we don't end up
@@ -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;
 }
 
 int __init early_microcode_init(struct boot_info *bi)
-- 
2.39.5

On 10/28/24 05: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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

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@citrix.com>

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@suse.com>
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