diff mbox series

[v5,2/4] x86/microcode: Ignore microcode loading interface for revision = -1

Message ID 20230629152656.12655-3-alejandro.vallejo@cloud.com (mailing list archive)
State Superseded
Headers show
Series Prevent attempting updates known to fail | expand

Commit Message

Alejandro Vallejo June 29, 2023, 3:26 p.m. UTC 
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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v5:
  * Style fix. Brace position.
---
 xen/arch/x86/cpu/microcode/core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Andrew Cooper July 5, 2023, 2:13 p.m. UTC | #1 
On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index bec8b55db2..b620e3bfa6 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -867,10 +867,22 @@ int __init early_microcode_init(unsigned long *module_map,
>          return -ENODEV;
>      }
>  
> -    microcode_grab_module(module_map, mbi);
> -
>      ucode_ops.collect_cpu_info();
>  
> +    /*
> +     * 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 )
> +    {
> +        printk(XENLOG_WARNING "Microcode loading disabled\n");

XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
virt\n"

It's normal (and not a warning) when running under other hypervisors,
and just "loading disabled" is too little information.

Happy to fix on commit.  Everything else looks ok.

~Andrew
Jan Beulich July 5, 2023, 2:24 p.m. UTC | #2 
On 05.07.2023 16:13, Andrew Cooper wrote:
> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>> index bec8b55db2..b620e3bfa6 100644
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -867,10 +867,22 @@ int __init early_microcode_init(unsigned long *module_map,
>>          return -ENODEV;
>>      }
>>  
>> -    microcode_grab_module(module_map, mbi);
>> -
>>      ucode_ops.collect_cpu_info();
>>  
>> +    /*
>> +     * 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 )
>> +    {
>> +        printk(XENLOG_WARNING "Microcode loading disabled\n");
> 
> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
> virt\n"
> 
> It's normal (and not a warning) when running under other hypervisors,

Except that INFO won't be visible by default in release configurations.

Jan

> and just "loading disabled" is too little information.
> 
> Happy to fix on commit.  Everything else looks ok.
> 
> ~Andrew
Andrew Cooper July 5, 2023, 2:28 p.m. UTC | #3 
On 05/07/2023 3:24 pm, Jan Beulich wrote:
> On 05.07.2023 16:13, Andrew Cooper wrote:
>> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>> index bec8b55db2..b620e3bfa6 100644
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -867,10 +867,22 @@ int __init early_microcode_init(unsigned long *module_map,
>>>          return -ENODEV;
>>>      }
>>>  
>>> -    microcode_grab_module(module_map, mbi);
>>> -
>>>      ucode_ops.collect_cpu_info();
>>>  
>>> +    /*
>>> +     * 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 )
>>> +    {
>>> +        printk(XENLOG_WARNING "Microcode loading disabled\n");
>> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
>> virt\n"
>>
>> It's normal (and not a warning) when running under other hypervisors,
> Except that INFO won't be visible by default in release configurations.

Well that's not a bug with microcode then, is it...

I can't believe I'm having to say no to emitting messages at the wrong
log level to work around a bug with selecting the default log level in
the first place.

~Andrew
Jan Beulich July 5, 2023, 2:34 p.m. UTC | #4 
On 05.07.2023 16:28, Andrew Cooper wrote:
> On 05/07/2023 3:24 pm, Jan Beulich wrote:
>> On 05.07.2023 16:13, Andrew Cooper wrote:
>>> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>>> index bec8b55db2..b620e3bfa6 100644
>>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>>> @@ -867,10 +867,22 @@ int __init early_microcode_init(unsigned long *module_map,
>>>>          return -ENODEV;
>>>>      }
>>>>  
>>>> -    microcode_grab_module(module_map, mbi);
>>>> -
>>>>      ucode_ops.collect_cpu_info();
>>>>  
>>>> +    /*
>>>> +     * 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 )
>>>> +    {
>>>> +        printk(XENLOG_WARNING "Microcode loading disabled\n");
>>> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
>>> virt\n"
>>>
>>> It's normal (and not a warning) when running under other hypervisors,
>> Except that INFO won't be visible by default in release configurations.
> 
> Well that's not a bug with microcode then, is it...
> 
> I can't believe I'm having to say no to emitting messages at the wrong
> log level to work around a bug with selecting the default log level in
> the first place.

Hmm, I think not emitting info level messages is quite right. If I'm
not mistaken we emit various others at warning level to "account" for
this (you might instead call it "to work around this").

Furthermore, as to your suggestions here: What would you expect patch
4 to do? Emit yet another message, instead of having both conditions
fold to just one of them?

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index bec8b55db2..b620e3bfa6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -867,10 +867,22 @@  int __init early_microcode_init(unsigned long *module_map,
         return -ENODEV;
     }
 
-    microcode_grab_module(module_map, mbi);
-
     ucode_ops.collect_cpu_info();
 
+    /*
+     * 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 )
+    {
+        printk(XENLOG_WARNING "Microcode loading disabled\n");
+        ucode_ops.apply_microcode = NULL;
+        return -ENODEV;
+    }
+
+    microcode_grab_module(module_map, mbi);
+
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();