| Message ID | 20230615154834.959-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 15.06.2023 17:48, Alejandro Vallejo wrote: > The code currently assumes all microcode handlers are set or none are. That > won't be the case in a future patch, as apply_microcode() may not be set > while the others are. Hence, this patch allows reading the microcode > revision even if updating it is unavailable. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 15/06/2023 4:48 pm, Alejandro Vallejo wrote: > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index e65af4b82e..df7e1df870 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -750,11 +750,12 @@ __initcall(microcode_init); > @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, > break; > } > > + if ( ucode_ops.collect_cpu_info ) > + ucode_ops.collect_cpu_info(); > + I still think this wants to be the other side of "ucode loading fully unavailable", just below. I'm confident it will result in easier-to-follow logic. ~Andrew > if ( !ucode_ops.apply_microcode ) > { > printk(XENLOG_WARNING "Microcode loading not available\n"); >
On 19.06.2023 17:49, Andrew Cooper wrote: > On 15/06/2023 4:48 pm, Alejandro Vallejo wrote: >> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c >> index e65af4b82e..df7e1df870 100644 >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -750,11 +750,12 @@ __initcall(microcode_init); >> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, >> break; >> } >> >> + if ( ucode_ops.collect_cpu_info ) >> + ucode_ops.collect_cpu_info(); >> + > > I still think this wants to be the other side of "ucode loading fully > unavailable", just below. > > I'm confident it will result in easier-to-follow logic. Yet wouldn't that be against the purpose of obtaining the ucode revision even if loading isn't possible? Jan
On 19/06/2023 4:58 pm, Jan Beulich wrote: > On 19.06.2023 17:49, Andrew Cooper wrote: >> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote: >>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c >>> index e65af4b82e..df7e1df870 100644 >>> --- a/xen/arch/x86/cpu/microcode/core.c >>> +++ b/xen/arch/x86/cpu/microcode/core.c >>> @@ -750,11 +750,12 @@ __initcall(microcode_init); >>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, >>> break; >>> } >>> >>> + if ( ucode_ops.collect_cpu_info ) >>> + ucode_ops.collect_cpu_info(); >>> + >> I still think this wants to be the other side of "ucode loading fully >> unavailable", just below. >> >> I'm confident it will result in easier-to-follow logic. > Yet wouldn't that be against the purpose of obtaining the ucode > revision even if loading isn't possible? No. The logical order of checks is: if ( !ops.apply ) return; // Fully unavailable collect_cpu_info(); if ( rev == ~0 || !can_load ) return; // Loading unavailable // search for blob to load This form has fewer misleading NULL checks, and doesn't get printk(XENLOG_WARNING "Microcode loading not available\n"); after successful microcode actions. ~Andrew
On 19.06.2023 18:06, Andrew Cooper wrote: > On 19/06/2023 4:58 pm, Jan Beulich wrote: >> On 19.06.2023 17:49, Andrew Cooper wrote: >>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote: >>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c >>>> index e65af4b82e..df7e1df870 100644 >>>> --- a/xen/arch/x86/cpu/microcode/core.c >>>> +++ b/xen/arch/x86/cpu/microcode/core.c >>>> @@ -750,11 +750,12 @@ __initcall(microcode_init); >>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, >>>> break; >>>> } >>>> >>>> + if ( ucode_ops.collect_cpu_info ) >>>> + ucode_ops.collect_cpu_info(); >>>> + >>> I still think this wants to be the other side of "ucode loading fully >>> unavailable", just below. >>> >>> I'm confident it will result in easier-to-follow logic. >> Yet wouldn't that be against the purpose of obtaining the ucode >> revision even if loading isn't possible? > > No. The logical order of checks is: > > if ( !ops.apply ) > return; // Fully unavailable > > collect_cpu_info(); > > if ( rev == ~0 || !can_load ) > return; // Loading unavailable > > // search for blob to load > > > This form has fewer misleading NULL checks, and doesn't get > printk(XENLOG_WARNING "Microcode loading not available\n"); after > successful microcode actions. But from the earlier version and from what I've seen in patches 1-4 so far, I expect patch 5 will introduce a case with ops.apply being NULL but ops.collect being non-NULL. Otherwise I don't see why patch 2 does what it does (sacrificing cf_clobber, albeit likely not really intentionally). Jan
On 19.06.2023 18:10, Jan Beulich wrote: > On 19.06.2023 18:06, Andrew Cooper wrote: >> On 19/06/2023 4:58 pm, Jan Beulich wrote: >>> On 19.06.2023 17:49, Andrew Cooper wrote: >>>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote: >>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c >>>>> index e65af4b82e..df7e1df870 100644 >>>>> --- a/xen/arch/x86/cpu/microcode/core.c >>>>> +++ b/xen/arch/x86/cpu/microcode/core.c >>>>> @@ -750,11 +750,12 @@ __initcall(microcode_init); >>>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, >>>>> break; >>>>> } >>>>> >>>>> + if ( ucode_ops.collect_cpu_info ) >>>>> + ucode_ops.collect_cpu_info(); >>>>> + >>>> I still think this wants to be the other side of "ucode loading fully >>>> unavailable", just below. >>>> >>>> I'm confident it will result in easier-to-follow logic. >>> Yet wouldn't that be against the purpose of obtaining the ucode >>> revision even if loading isn't possible? >> >> No. The logical order of checks is: >> >> if ( !ops.apply ) >> return; // Fully unavailable >> >> collect_cpu_info(); >> >> if ( rev == ~0 || !can_load ) >> return; // Loading unavailable >> >> // search for blob to load >> >> >> This form has fewer misleading NULL checks, and doesn't get >> printk(XENLOG_WARNING "Microcode loading not available\n"); after >> successful microcode actions. > > But from the earlier version and from what I've seen in patches 1-4 > so far, I expect patch 5 will introduce a case with ops.apply being > NULL but ops.collect being non-NULL. Otherwise I don't see why patch > 2 does what it does (sacrificing cf_clobber, albeit likely not really > intentionally). As expected with patch 5 ops.apply can be NULL without indicating "fully unavailable". collect_cpu_info being NULL could be taken as indicator of "fully unavailable", though. Jan
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index e65af4b82e..df7e1df870 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -750,11 +750,12 @@ __initcall(microcode_init); /* Load a cached update to current cpu */ int microcode_update_one(void) { + if ( ucode_ops.collect_cpu_info ) + alternative_vcall(ucode_ops.collect_cpu_info); + if ( !ucode_ops.apply_microcode ) return -EOPNOTSUPP; - alternative_vcall(ucode_ops.collect_cpu_info); - return microcode_update_cpu(NULL); } @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map, break; } + if ( ucode_ops.collect_cpu_info ) + ucode_ops.collect_cpu_info(); + if ( !ucode_ops.apply_microcode ) { printk(XENLOG_WARNING "Microcode loading not available\n"); @@ -868,8 +872,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();
The code currently assumes all microcode handlers are set or none are. That won't be the case in a future patch, as apply_microcode() may not be set while the others are. Hence, this patch allows reading the microcode revision even if updating it is unavailable. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v3: * Hunks taken from v2/patch4 (Jan) --- xen/arch/x86/cpu/microcode/core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)