Some hypervisors report ~0 as the microcode revision to mean "don't issue
microcode updates". Ignore the microcode loading interface in that case.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2:
  * New addition
---
 xen/arch/x86/cpu/microcode/core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 892bcec901..4f60d96d98 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -874,6 +874,21 @@ int __init early_microcode_init(unsigned long *module_map,
         break;
     }
 
+    if ( ucode_ops.collect_cpu_info )
+        ucode_ops.collect_cpu_info();
+
+    /*
+     * This is a special case for virtualized Xen. Some hypervisors
+     * deliberately report a microcode revision of -1 to mean that they
+     * will not accept microcode updates. We take the hint and ignore the
+     * microcode interface in that case.
+     */
+    if ( this_cpu(cpu_sig).rev == ~0 )
+    {
+        this_cpu(cpu_sig) = (struct cpu_signature){ 0 };
+        ucode_ops = (struct microcode_ops){ 0 };
+    }
+
     if ( !ucode_ops.apply_microcode )
     {
         printk(XENLOG_WARNING "Microcode loading not available\n");
@@ -882,8 +897,6 @@ int __init early_microcode_init(unsigned long *module_map,
 
     microcode_grab_module(module_map, mbi);
 
-    ucode_ops.collect_cpu_info();
-
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
-- 
2.34.1

On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index 892bcec901..4f60d96d98 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -874,6 +874,21 @@ int __init early_microcode_init(unsigned long *module_map,
>          break;
>      }
>  
> +    if ( ucode_ops.collect_cpu_info )
> +        ucode_ops.collect_cpu_info();
> +
> +    /*
> +     * This is a special case for virtualized Xen.

I'm not sure this first sentence is useful.  I'd just start with "Some
hypervisors ..."

>  Some hypervisors
> +     * deliberately report a microcode revision of -1 to mean that they
> +     * will not accept microcode updates. We take the hint and ignore the
> +     * microcode interface in that case.
> +     */
> +    if ( this_cpu(cpu_sig).rev == ~0 )
> +    {
> +        this_cpu(cpu_sig) = (struct cpu_signature){ 0 };
> +        ucode_ops = (struct microcode_ops){ 0 };

I think we want to retain XENPF_get_ucode_revision's ability to see this ~0.

As with the following patch, we want to retain the ability to query, so
leave cpu_sig alone and only remove the apply_microcode hook.  In turn,
that probably means this wants to be an else if in the next clause down.

Moving it down also means you can drop the check for collect_cpu_info,
because it's a mandatory hook if ucode_ops was filled in.

~Andrew

> +    }
> +
>      if ( !ucode_ops.apply_microcode )
>      {
>          printk(XENLOG_WARNING "Microcode loading not available\n");