| Message ID | 20230724165235.25262-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 24.07.2023 18:52, Alejandro Vallejo wrote: > 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> Hmm. > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s", While we have tentatively agreed to adjust what _will_ be emitted by default (subject to suitable justification in that change's description), such a patch is yet to be sent. As it stands this message will be invisible by default. > + "HW toggle"); With the comment talking about hypervisors, what is this string supposed to tell an observer of the message in a log file? Jan
On 25.07.2023 08:40, Jan Beulich wrote: > On 24.07.2023 18:52, Alejandro Vallejo wrote: >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s", > > While we have tentatively agreed to adjust what _will_ be emitted by > default (subject to suitable justification in that change's > description), such a patch is yet to be sent. As it stands this message > will be invisible by default. > >> + "HW toggle"); > > With the comment talking about hypervisors, what is this string supposed > to tell an observer of the message in a log file? Looking at patch 3 I get the impression that you put here the wrong of the two messages there. Jan
On Tue, Jul 25, 2023 at 08:40:31AM +0200, Jan Beulich wrote: > On 24.07.2023 18:52, Alejandro Vallejo wrote: > > 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> > > Hmm. > > > --- a/xen/arch/x86/cpu/microcode/core.c > > +++ b/xen/arch/x86/cpu/microcode/core.c > > @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s", > > While we have tentatively agreed to adjust what _will_ be emitted by > default (subject to suitable justification in that change's > description), such a patch is yet to be sent. Ugh, that was indeed mistagged. Sorry about that. I touched several parts of this patch shortly before sending it and it crept in by mistake. > As it stands this message > will be invisible by default. Arguably, that's not necessarily a bad thing. The fact that microcode cannot be updated is expected behaviour and it makes little sense to warn about it. If only because they should already be aware of it through their agreement with their provider. The case I can think of where a warning would be sensible is where the system has a microcode blob more recent than the currently installed revision. This is something the admin may want to be aware of in order to pester their provider for updates. In the common case the machine won't even need such an update, so sending unconditional warnings per boot seems unwarranted. > > > + "HW toggle"); > > With the comment talking about hypervisors, what is this string supposed > to tell an observer of the message in a log file? > > Jan Nothing good. As you noticed later, that's the wrong substring off the last patch and should've been "rev = ~0" Thanks, Alejandro
On Tue, Jul 25, 2023 at 01:50:52PM +0100, Alejandro Vallejo wrote: > On Tue, Jul 25, 2023 at 08:40:31AM +0200, Jan Beulich wrote: > > > --- a/xen/arch/x86/cpu/microcode/core.c > > > +++ b/xen/arch/x86/cpu/microcode/core.c > > > @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s", > > > [snip] > > > As it stands this message > > will be invisible by default. > Arguably, that's not necessarily a bad thing. The fact that microcode > cannot be updated is expected behaviour and it makes little sense to warn > about it. If only because they should already be aware of it through their > agreement with their provider. > > The case I can think of where a warning would be sensible is where the > system has a microcode blob more recent than the currently installed > revision. This is something the admin may want to be aware of in order to > pester their provider for updates. In the common case the machine won't > even need such an update, so sending unconditional warnings per boot seems > unwarranted. Actually, the previous message probably ought to be an INFO too. It's an unconditional warning on old AMD and anything non AMD/Intel for no good reason. Alejandro
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index bec8b55db2..71e3944cf2 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s", + "HW toggle"); + 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();