| Message ID | 1564654971-31328-11-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve late microcode loading | expand |
On 01.08.2019 12:22, Chao Gao wrote: > --- a/xen/arch/x86/microcode.c > +++ b/xen/arch/x86/microcode.c > @@ -277,6 +277,9 @@ static long do_microcode_update(void *_info) > if ( error ) > info->error = error; > > + if ( microcode_ops->end_update ) > + microcode_ops->end_update(); > + > info->cpu = cpumask_next(info->cpu, &cpu_online_map); > if ( info->cpu < nr_cpu_ids ) > return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info); This being the only change in this file - don't you also need to alter the early ucode loading path? > @@ -631,11 +622,19 @@ static int start_update(void) > return 0; > } > > +static void end_update(void) > +{ > +#if CONFIG_HVM > + svm_host_osvw_init(); > +#endif > +} Instead of leaving an empty function in the !HVM case, ... > static const struct microcode_ops microcode_amd_ops = { > .cpu_request_microcode = cpu_request_microcode, > .collect_cpu_info = collect_cpu_info, > .apply_microcode = apply_microcode, > .start_update = start_update, > + .end_update = end_update, ... could you please leave this pointer uninitialized (i.e. NULL) in that case? Jan
On Fri, Aug 02, 2019 at 03:21:55PM +0000, Jan Beulich wrote: >On 01.08.2019 12:22, Chao Gao wrote: >> --- a/xen/arch/x86/microcode.c >> +++ b/xen/arch/x86/microcode.c >> @@ -277,6 +277,9 @@ static long do_microcode_update(void *_info) >> if ( error ) >> info->error = error; >> >> + if ( microcode_ops->end_update ) >> + microcode_ops->end_update(); >> + >> info->cpu = cpumask_next(info->cpu, &cpu_online_map); >> if ( info->cpu < nr_cpu_ids ) >> return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info); > >This being the only change in this file - don't you also need to >alter the early ucode loading path? Yes. I should have. > >> @@ -631,11 +622,19 @@ static int start_update(void) >> return 0; >> } >> >> +static void end_update(void) >> +{ >> +#if CONFIG_HVM >> + svm_host_osvw_init(); >> +#endif >> +} > >Instead of leaving an empty function in the !HVM case, ... > >> static const struct microcode_ops microcode_amd_ops = { >> .cpu_request_microcode = cpu_request_microcode, >> .collect_cpu_info = collect_cpu_info, >> .apply_microcode = apply_microcode, >> .start_update = start_update, >> + .end_update = end_update, > >... could you please leave this pointer uninitialized (i.e. >NULL) in that case? Will do. Thanks Chao
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index bfb0afb..082b29c 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -277,6 +277,9 @@ static long do_microcode_update(void *_info) if ( error ) info->error = error; + if ( microcode_ops->end_update ) + microcode_ops->end_update(); + info->cpu = cpumask_next(info->cpu, &cpu_online_map); if ( info->cpu < nr_cpu_ids ) return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info); diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c index 83ed8f9..3d1505d 100644 --- a/xen/arch/x86/microcode_amd.c +++ b/xen/arch/x86/microcode_amd.c @@ -600,10 +600,6 @@ static int cpu_request_microcode(const void *buf, size_t bufsize) xfree(mc_amd); out: -#if CONFIG_HVM - svm_host_osvw_init(); -#endif - /* * In some cases we may return an error even if processor's microcode has * been updated. For example, the first patch in a container file is loaded @@ -617,13 +613,8 @@ static int start_update(void) { #if CONFIG_HVM /* - * We assume here that svm_host_osvw_init() will be called on each cpu (from - * cpu_request_microcode()). - * - * Note that if collect_cpu_info() returns an error then - * cpu_request_microcode() will not invoked thus leaving OSVW bits not - * updated. Currently though collect_cpu_info() will not fail on processors - * supporting OSVW so we will not deal with this possibility. + * svm_host_osvw_init() will be called on each cpu by calling '.end_update' + * in common code. */ svm_host_osvw_reset(); #endif @@ -631,11 +622,19 @@ static int start_update(void) return 0; } +static void end_update(void) +{ +#if CONFIG_HVM + svm_host_osvw_init(); +#endif +} + static const struct microcode_ops microcode_amd_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .start_update = start_update, + .end_update = end_update, .free_patch = free_patch, .compare_patch = compare_patch, .match_cpu = match_cpu, diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 35223eb..c8d2c4f 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -24,6 +24,7 @@ struct microcode_ops { int (*collect_cpu_info)(struct cpu_signature *csig); int (*apply_microcode)(void); int (*start_update)(void); + void (*end_update)(void); void (*free_patch)(void *mc); bool (*match_cpu)(const struct microcode_patch *patch); enum microcode_match_result (*compare_patch)(
Introduce a vendor hook, .end_update, for svm_host_osvw_init(). The hook function is called on each cpu after loading an update. It is a preparation for spliting out apply_microcode() from cpu_request_microcode(). Signed-off-by: Chao Gao <chao.gao@intel.com> --- Changes in v8: - new --- xen/arch/x86/microcode.c | 3 +++ xen/arch/x86/microcode_amd.c | 21 ++++++++++----------- xen/include/asm-x86/microcode.h | 1 + 3 files changed, 14 insertions(+), 11 deletions(-)