Message ID | 20200601145755.3661-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [for-4.14] x86/ucode: Fix errors with start/end_update() | expand |
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 01 June 2020 15:58 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu <wl@xen.org>; > Roger Pau Monné <roger.pau@citrix.com>; Paul Durrant <paul@xen.org> > Subject: [PATCH for-4.14] x86/ucode: Fix errors with start/end_update() > > c/s 9267a439c "x86/ucode: Document the behaviour of the microcode_ops hooks" > identified several poor behaviours of the start_update()/end_update_percpu() > hooks. > > AMD have subsequently confirmed that OSVW don't, and are not expected to, > change across a microcode load, rendering all of this complexity unecessary. > > Instead of fixing up the logic to not leave the OSVW state reset in a number > of corner cases, delete the logic entirely. This in turn allows for the > removal of the poorly-named 'start_update' parameter to > microcode_update_one(). > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Paul Durrant <paul@xen.org> > > This wants backporting to 4.13, hence considering for 4.14 at this point. Release-acked-by: Paul Durrant <paul@xen.org> > --- > xen/arch/x86/acpi/power.c | 2 +- > xen/arch/x86/cpu/microcode/amd.c | 17 ----------------- > xen/arch/x86/cpu/microcode/core.c | 33 +++------------------------------ > xen/arch/x86/cpu/microcode/private.h | 14 -------------- > xen/arch/x86/smpboot.c | 2 +- > xen/include/asm-x86/microcode.h | 2 +- > 6 files changed, 6 insertions(+), 64 deletions(-) > > diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c > index 0cda362045..dfffe08e18 100644 > --- a/xen/arch/x86/acpi/power.c > +++ b/xen/arch/x86/acpi/power.c > @@ -287,7 +287,7 @@ static int enter_state(u32 state) > console_end_sync(); > watchdog_enable(); > > - microcode_update_one(true); > + microcode_update_one(); > > if ( !recheck_cpu_features(0) ) > panic("Missing previously available feature(s)\n"); > diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c > index 3f0969e70d..11e24637e7 100644 > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -395,26 +395,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz > return patch; > } > > -#ifdef CONFIG_HVM > -static int start_update(void) > -{ > - /* > - * svm_host_osvw_init() will be called on each cpu by calling '.end_update' > - * in common code. > - */ > - svm_host_osvw_reset(); > - > - return 0; > -} > -#endif > - > const struct microcode_ops amd_ucode_ops = { > .cpu_request_microcode = cpu_request_microcode, > .collect_cpu_info = collect_cpu_info, > .apply_microcode = apply_microcode, > -#ifdef CONFIG_HVM > - .start_update = start_update, > - .end_update_percpu = svm_host_osvw_init, > -#endif > .compare_patch = compare_patch, > }; > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index d879d28787..18ebc07b13 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -546,9 +546,6 @@ static int do_microcode_update(void *patch) > else > ret = secondary_thread_fn(); > > - if ( microcode_ops->end_update_percpu ) > - microcode_ops->end_update_percpu(); > - > return ret; > } > > @@ -620,16 +617,6 @@ static long microcode_update_helper(void *data) > } > spin_unlock(µcode_mutex); > > - if ( microcode_ops->start_update ) > - { > - ret = microcode_ops->start_update(); > - if ( ret ) > - { > - microcode_free_patch(patch); > - goto put; > - } > - } > - > cpumask_clear(&cpu_callin_map); > atomic_set(&cpu_out, 0); > atomic_set(&cpu_updated, 0); > @@ -728,28 +715,14 @@ static int __init microcode_init(void) > __initcall(microcode_init); > > /* Load a cached update to current cpu */ > -int microcode_update_one(bool start_update) > +int microcode_update_one(void) > { > - int err; > - > if ( !microcode_ops ) > return -EOPNOTSUPP; > > microcode_ops->collect_cpu_info(); > > - if ( start_update && microcode_ops->start_update ) > - { > - err = microcode_ops->start_update(); > - if ( err ) > - return err; > - } > - > - err = microcode_update_cpu(NULL); > - > - if ( microcode_ops->end_update_percpu ) > - microcode_ops->end_update_percpu(); > - > - return err; > + return microcode_update_cpu(NULL); > } > > /* BSP calls this function to parse ucode blob and then apply an update. */ > @@ -790,7 +763,7 @@ static int __init early_microcode_update_cpu(void) > spin_unlock(µcode_mutex); > ASSERT(rc); > > - return microcode_update_one(true); > + return microcode_update_one(); > } > > int __init early_microcode_init(void) > diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h > index dc5c7a81ae..c00ba590d1 100644 > --- a/xen/arch/x86/cpu/microcode/private.h > +++ b/xen/arch/x86/cpu/microcode/private.h > @@ -46,20 +46,6 @@ struct microcode_ops { > int (*apply_microcode)(const struct microcode_patch *patch); > > /* > - * Optional. If provided and applicable to the specific update attempt, > - * is run once by the initiating CPU. Returning an error will abort the > - * load attempt. > - */ > - int (*start_update)(void); > - > - /* > - * Optional. If provided, called on every CPU which completes a microcode > - * load. May be called in the case of some errors, and not others. May > - * be called even if start_update() wasn't. > - */ > - void (*end_update_percpu)(void); > - > - /* > * Given two patches, are they both applicable to the current CPU, and is > * new a higher revision than old? > */ > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c > index 13b3dade9c..f878a00760 100644 > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -369,7 +369,7 @@ void start_secondary(void *unused) > > initialize_cpu_data(cpu); > > - microcode_update_one(false); > + microcode_update_one(); > > /* > * If MSR_SPEC_CTRL is available, apply Xen's default setting and discard > diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h > index 9da63f992e..3b0234e9fa 100644 > --- a/xen/include/asm-x86/microcode.h > +++ b/xen/include/asm-x86/microcode.h > @@ -22,6 +22,6 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig); > void microcode_set_module(unsigned int idx); > int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len); > int early_microcode_init(void); > -int microcode_update_one(bool start_update); > +int microcode_update_one(void); > > #endif /* ASM_X86__MICROCODE_H */ > -- > 2.11.0
On Mon, Jun 01, 2020 at 03:57:55PM +0100, Andrew Cooper wrote: > c/s 9267a439c "x86/ucode: Document the behaviour of the microcode_ops hooks" > identified several poor behaviours of the start_update()/end_update_percpu() > hooks. > > AMD have subsequently confirmed that OSVW don't, and are not expected to, > change across a microcode load, rendering all of this complexity unecessary. Is there a reference to some AMD PM or similar document that can be added here for completeness? > Instead of fixing up the logic to not leave the OSVW state reset in a number > of corner cases, delete the logic entirely. This in turn allows for the > removal of the poorly-named 'start_update' parameter to > microcode_update_one(). > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 01/06/2020 16:48, Roger Pau Monné wrote: > On Mon, Jun 01, 2020 at 03:57:55PM +0100, Andrew Cooper wrote: >> c/s 9267a439c "x86/ucode: Document the behaviour of the microcode_ops hooks" >> identified several poor behaviours of the start_update()/end_update_percpu() >> hooks. >> >> AMD have subsequently confirmed that OSVW don't, and are not expected to, >> change across a microcode load, rendering all of this complexity unecessary. > Is there a reference to some AMD PM or similar document that can be > added here for completeness? Not at the moment. (I'm attempting to solve this...) > >> Instead of fixing up the logic to not leave the OSVW state reset in a number >> of corner cases, delete the logic entirely. This in turn allows for the >> removal of the poorly-named 'start_update' parameter to >> microcode_update_one(). >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, ~Andrew
On 01.06.2020 16:57, Andrew Cooper wrote: > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -395,26 +395,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz > return patch; > } > > -#ifdef CONFIG_HVM > -static int start_update(void) > -{ > - /* > - * svm_host_osvw_init() will be called on each cpu by calling '.end_update' > - * in common code. > - */ > - svm_host_osvw_reset(); > - > - return 0; > -} > -#endif > - > const struct microcode_ops amd_ucode_ops = { > .cpu_request_microcode = cpu_request_microcode, > .collect_cpu_info = collect_cpu_info, > .apply_microcode = apply_microcode, > -#ifdef CONFIG_HVM > - .start_update = start_update, > - .end_update_percpu = svm_host_osvw_init, As a result the function can (and imo should) become static. Preferably with that Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c index 0cda362045..dfffe08e18 100644 --- a/xen/arch/x86/acpi/power.c +++ b/xen/arch/x86/acpi/power.c @@ -287,7 +287,7 @@ static int enter_state(u32 state) console_end_sync(); watchdog_enable(); - microcode_update_one(true); + microcode_update_one(); if ( !recheck_cpu_features(0) ) panic("Missing previously available feature(s)\n"); diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 3f0969e70d..11e24637e7 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -395,26 +395,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz return patch; } -#ifdef CONFIG_HVM -static int start_update(void) -{ - /* - * svm_host_osvw_init() will be called on each cpu by calling '.end_update' - * in common code. - */ - svm_host_osvw_reset(); - - return 0; -} -#endif - const struct microcode_ops amd_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, -#ifdef CONFIG_HVM - .start_update = start_update, - .end_update_percpu = svm_host_osvw_init, -#endif .compare_patch = compare_patch, }; diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index d879d28787..18ebc07b13 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -546,9 +546,6 @@ static int do_microcode_update(void *patch) else ret = secondary_thread_fn(); - if ( microcode_ops->end_update_percpu ) - microcode_ops->end_update_percpu(); - return ret; } @@ -620,16 +617,6 @@ static long microcode_update_helper(void *data) } spin_unlock(µcode_mutex); - if ( microcode_ops->start_update ) - { - ret = microcode_ops->start_update(); - if ( ret ) - { - microcode_free_patch(patch); - goto put; - } - } - cpumask_clear(&cpu_callin_map); atomic_set(&cpu_out, 0); atomic_set(&cpu_updated, 0); @@ -728,28 +715,14 @@ static int __init microcode_init(void) __initcall(microcode_init); /* Load a cached update to current cpu */ -int microcode_update_one(bool start_update) +int microcode_update_one(void) { - int err; - if ( !microcode_ops ) return -EOPNOTSUPP; microcode_ops->collect_cpu_info(); - if ( start_update && microcode_ops->start_update ) - { - err = microcode_ops->start_update(); - if ( err ) - return err; - } - - err = microcode_update_cpu(NULL); - - if ( microcode_ops->end_update_percpu ) - microcode_ops->end_update_percpu(); - - return err; + return microcode_update_cpu(NULL); } /* BSP calls this function to parse ucode blob and then apply an update. */ @@ -790,7 +763,7 @@ static int __init early_microcode_update_cpu(void) spin_unlock(µcode_mutex); ASSERT(rc); - return microcode_update_one(true); + return microcode_update_one(); } int __init early_microcode_init(void) diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index dc5c7a81ae..c00ba590d1 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -46,20 +46,6 @@ struct microcode_ops { int (*apply_microcode)(const struct microcode_patch *patch); /* - * Optional. If provided and applicable to the specific update attempt, - * is run once by the initiating CPU. Returning an error will abort the - * load attempt. - */ - int (*start_update)(void); - - /* - * Optional. If provided, called on every CPU which completes a microcode - * load. May be called in the case of some errors, and not others. May - * be called even if start_update() wasn't. - */ - void (*end_update_percpu)(void); - - /* * Given two patches, are they both applicable to the current CPU, and is * new a higher revision than old? */ diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 13b3dade9c..f878a00760 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -369,7 +369,7 @@ void start_secondary(void *unused) initialize_cpu_data(cpu); - microcode_update_one(false); + microcode_update_one(); /* * If MSR_SPEC_CTRL is available, apply Xen's default setting and discard diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 9da63f992e..3b0234e9fa 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -22,6 +22,6 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig); void microcode_set_module(unsigned int idx); int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len); int early_microcode_init(void); -int microcode_update_one(bool start_update); +int microcode_update_one(void); #endif /* ASM_X86__MICROCODE_H */
c/s 9267a439c "x86/ucode: Document the behaviour of the microcode_ops hooks" identified several poor behaviours of the start_update()/end_update_percpu() hooks. AMD have subsequently confirmed that OSVW don't, and are not expected to, change across a microcode load, rendering all of this complexity unecessary. Instead of fixing up the logic to not leave the OSVW state reset in a number of corner cases, delete the logic entirely. This in turn allows for the removal of the poorly-named 'start_update' parameter to microcode_update_one(). Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Paul Durrant <paul@xen.org> This wants backporting to 4.13, hence considering for 4.14 at this point. --- xen/arch/x86/acpi/power.c | 2 +- xen/arch/x86/cpu/microcode/amd.c | 17 ----------------- xen/arch/x86/cpu/microcode/core.c | 33 +++------------------------------ xen/arch/x86/cpu/microcode/private.h | 14 -------------- xen/arch/x86/smpboot.c | 2 +- xen/include/asm-x86/microcode.h | 2 +- 6 files changed, 6 insertions(+), 64 deletions(-)