| Message ID | 20230327194126.3573997-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Fixes to bootstrap_map() handling | expand |
On 27.03.2023 21:41, Andrew Cooper wrote: > It is not valid to retain a bootstrap_map() across returning back to > __start_xen(), but various pointers get stashed across calls. Same comment here, and ... > Begin to address this by folding early_update_cache() into it's single caller, > rearranging the exit path to always invalidate the mapping. ... this looks to lack editing after copy-and-paste from the earlier patch. > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -804,45 +804,12 @@ int __init microcode_init_cache(unsigned long *module_map, > return rc; > } > > -/* BSP calls this function to parse ucode blob and then apply an update. */ > -static int __init early_microcode_update_cpu(void) > -{ > - const void *data = NULL; > - size_t len; > - struct microcode_patch *patch; > - > - if ( ucode_blob.size ) > - { > - len = ucode_blob.size; > - data = ucode_blob.data; > - } > - else if ( ucode_mod.mod_end ) > - { > - len = ucode_mod.mod_end; > - data = bootstrap_map(&ucode_mod); > - } > - > - if ( !data ) > - return -ENOMEM; Like in the earlier patch this looks to be lost. > - patch = ucode_ops.cpu_request_microcode(data, len, false); > - if ( IS_ERR(patch) ) > - { > - printk(XENLOG_WARNING "Parsing microcode blob error %ld\n", > - PTR_ERR(patch)); > - return PTR_ERR(patch); > - } > - > - if ( !patch ) > - return -ENOENT; > - > - return microcode_update_cpu(patch); > -} > - > int __init early_microcode_init(unsigned long *module_map, > const struct multiboot_info *mbi) > { > const struct cpuinfo_x86 *c = &boot_cpu_data; > + struct microcode_patch *patch; > + struct ucode_mod_blob blob = {}; > int rc = 0; > > switch ( c->x86_vendor ) > @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map, > > ucode_ops.collect_cpu_info(); > > - if ( ucode_mod.mod_end || ucode_blob.size ) > - rc = early_microcode_update_cpu(); > + if ( ucode_blob.data ) > + { > + blob = ucode_blob; > + } > + else if ( ucode_mod.mod_end ) > + { > + blob.data = bootstrap_map(&ucode_mod); > + blob.size = ucode_mod.mod_end; > + } I wonder whether the order of if/else-if being different between microcode_init_cache() and here (also before your change) is meaningful in any way. I would prefer if the checking was always done in the same order, if I can talk you into re-arranging here and/or in the earlier patch. Jan
On 28/03/2023 3:06 pm, Jan Beulich wrote: > On 27.03.2023 21:41, Andrew Cooper wrote: >> Begin to address this by folding early_update_cache() into it's single caller, >> rearranging the exit path to always invalidate the mapping. > ... this looks to lack editing after copy-and-paste from the earlier patch. Will fix. >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map, >> >> ucode_ops.collect_cpu_info(); >> >> - if ( ucode_mod.mod_end || ucode_blob.size ) >> - rc = early_microcode_update_cpu(); >> + if ( ucode_blob.data ) >> + { >> + blob = ucode_blob; >> + } >> + else if ( ucode_mod.mod_end ) >> + { >> + blob.data = bootstrap_map(&ucode_mod); >> + blob.size = ucode_mod.mod_end; >> + } > I wonder whether the order of if/else-if being different between > microcode_init_cache() and here (also before your change) is meaningful > in any way. I would prefer if the checking was always done in the same > order, if I can talk you into re-arranging here and/or in the earlier > patch. It does matter, yes (well - certainly in patch 2). (Although I see a .size -> .data typo in the moved code, which I need to fix.) However, both these chains are deleted in patch 5, so I'm going to keep patches 2 and 3 as close to pure code movement as I can. ~Andrew
On 28.03.2023 17:11, Andrew Cooper wrote: > On 28/03/2023 3:06 pm, Jan Beulich wrote: >> On 27.03.2023 21:41, Andrew Cooper wrote: >>> --- a/xen/arch/x86/cpu/microcode/core.c >>> +++ b/xen/arch/x86/cpu/microcode/core.c >>> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map, >>> >>> ucode_ops.collect_cpu_info(); >>> >>> - if ( ucode_mod.mod_end || ucode_blob.size ) >>> - rc = early_microcode_update_cpu(); >>> + if ( ucode_blob.data ) >>> + { >>> + blob = ucode_blob; >>> + } >>> + else if ( ucode_mod.mod_end ) >>> + { >>> + blob.data = bootstrap_map(&ucode_mod); >>> + blob.size = ucode_mod.mod_end; >>> + } >> I wonder whether the order of if/else-if being different between >> microcode_init_cache() and here (also before your change) is meaningful >> in any way. I would prefer if the checking was always done in the same >> order, if I can talk you into re-arranging here and/or in the earlier >> patch. > > It does matter, yes (well - certainly in patch 2). (Although I see a > .size -> .data typo in the moved code, which I need to fix.) > > However, both these chains are deleted in patch 5, so I'm going to keep > patches 2 and 3 as close to pure code movement as I can. Right - having seen the last patch, I'm certainly okay with this. Jan
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 3d23e3ed7ee4..4d2a896fe78d 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -804,45 +804,12 @@ int __init microcode_init_cache(unsigned long *module_map, return rc; } -/* BSP calls this function to parse ucode blob and then apply an update. */ -static int __init early_microcode_update_cpu(void) -{ - const void *data = NULL; - size_t len; - struct microcode_patch *patch; - - if ( ucode_blob.size ) - { - len = ucode_blob.size; - data = ucode_blob.data; - } - else if ( ucode_mod.mod_end ) - { - len = ucode_mod.mod_end; - data = bootstrap_map(&ucode_mod); - } - - if ( !data ) - return -ENOMEM; - - patch = ucode_ops.cpu_request_microcode(data, len, false); - if ( IS_ERR(patch) ) - { - printk(XENLOG_WARNING "Parsing microcode blob error %ld\n", - PTR_ERR(patch)); - return PTR_ERR(patch); - } - - if ( !patch ) - return -ENOENT; - - return microcode_update_cpu(patch); -} - int __init early_microcode_init(unsigned long *module_map, const struct multiboot_info *mbi) { const struct cpuinfo_x86 *c = &boot_cpu_data; + struct microcode_patch *patch; + struct ucode_mod_blob blob = {}; int rc = 0; switch ( c->x86_vendor ) @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map, ucode_ops.collect_cpu_info(); - if ( ucode_mod.mod_end || ucode_blob.size ) - rc = early_microcode_update_cpu(); + if ( ucode_blob.data ) + { + blob = ucode_blob; + } + else if ( ucode_mod.mod_end ) + { + blob.data = bootstrap_map(&ucode_mod); + blob.size = ucode_mod.mod_end; + } + + if ( !blob.data ) + return 0; + + patch = ucode_ops.cpu_request_microcode(blob.data, blob.size, false); + if ( IS_ERR(patch) ) + { + rc = PTR_ERR(patch); + printk(XENLOG_WARNING "Parsing microcode blob error %d\n", rc); + goto out; + } + + if ( !patch ) + { + rc = -ENOENT; + goto out; + } + + rc = microcode_update_cpu(patch); + + out: + bootstrap_map(NULL); return rc; }
It is not valid to retain a bootstrap_map() across returning back to __start_xen(), but various pointers get stashed across calls. Begin to address this by folding early_update_cache() into it's single caller, rearranging the exit path to always invalidate the mapping. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/arch/x86/cpu/microcode/core.c | 70 +++++++++++++++---------------- 1 file changed, 33 insertions(+), 37 deletions(-)