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(-)
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(-)
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
> -----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
© 2016 - 2024 Red Hat, Inc.