| Message ID | 20221219144533.19836-4-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: load microcode earlier on boot CPU | expand |
Hi Sergey, On 19/12/2022 14:45, Sergey Dyasli wrote: > Call early_microcode_init() straight after multiboot modules become > accessible. Modify it to load the ucode directly from the blob bypassing > populating microcode_cache because xmalloc is still not available at > that point during Xen boot. > > Introduce early_microcode_init_cache() for populating microcode_cache. > It needs to rescan the modules in order to find the new virtual address > of the ucode blob because it changes during the boot process, e.g. > from 0x00000000010802fc to 0xffff83204dac52fc. > > While at it, drop alternative_vcall() from early_microcode_init() since > it's not useful in an __init fuction. Can you explain in the commit message why you need to load the microcode early? E.g. is it a nice feature to have or a real issue? If the latter, then I think we will need to consider the patches for backport. Cheers,
On 19/12/2022 4:57 pm, Julien Grall wrote: > Hi Sergey, > > On 19/12/2022 14:45, Sergey Dyasli wrote: >> Call early_microcode_init() straight after multiboot modules become >> accessible. Modify it to load the ucode directly from the blob bypassing >> populating microcode_cache because xmalloc is still not available at >> that point during Xen boot. >> >> Introduce early_microcode_init_cache() for populating microcode_cache. >> It needs to rescan the modules in order to find the new virtual address >> of the ucode blob because it changes during the boot process, e.g. >> from 0x00000000010802fc to 0xffff83204dac52fc. >> >> While at it, drop alternative_vcall() from early_microcode_init() since >> it's not useful in an __init fuction. > > Can you explain in the commit message why you need to load the > microcode early? E.g. is it a nice feature to have or a real issue? > > If the latter, then I think we will need to consider the patches for > backport. Microcode loading should be as early as possible. Linux does it even before setting up the console (which is a bit too early IMO). Xen currently loads microcode half way through BSP boot, because there's a inappropriate dependency on needing xmalloc(). This is what Sergey is addressing with this series. I'm working on addressing the TODO in the penultimate hunk of this patch (resolving some major abuse with with the multiboot module structures), which will let us load microcode even earlier. A consequence of this (relatively) late loading is that we've got a tangle of feature enumeration logic where cpu_has_* doesn't fully work before ucode load, and we've got a lot of ad-hoc logic which is fragile. So no - there's not a specific bug driving this, but a lot of cleanup that I've been wanting to do since before speculation came along. ~Andrew
On 19/12/2022 17:33, Andrew Cooper wrote: > On 19/12/2022 4:57 pm, Julien Grall wrote: >> Hi Sergey, >> >> On 19/12/2022 14:45, Sergey Dyasli wrote: >>> Call early_microcode_init() straight after multiboot modules become >>> accessible. Modify it to load the ucode directly from the blob bypassing >>> populating microcode_cache because xmalloc is still not available at >>> that point during Xen boot. >>> >>> Introduce early_microcode_init_cache() for populating microcode_cache. >>> It needs to rescan the modules in order to find the new virtual address >>> of the ucode blob because it changes during the boot process, e.g. >>> from 0x00000000010802fc to 0xffff83204dac52fc. >>> >>> While at it, drop alternative_vcall() from early_microcode_init() since >>> it's not useful in an __init fuction. >> >> Can you explain in the commit message why you need to load the >> microcode early? E.g. is it a nice feature to have or a real issue? >> >> If the latter, then I think we will need to consider the patches for >> backport. > > Microcode loading should be as early as possible. Linux does it even > before setting up the console (which is a bit too early IMO). > > Xen currently loads microcode half way through BSP boot, because there's > a inappropriate dependency on needing xmalloc(). This is what Sergey is > addressing with this series. > > I'm working on addressing the TODO in the penultimate hunk of this patch > (resolving some major abuse with with the multiboot module structures), > which will let us load microcode even earlier. > > A consequence of this (relatively) late loading is that we've got a > tangle of feature enumeration logic where cpu_has_* doesn't fully work > before ucode load, and we've got a lot of ad-hoc logic which is fragile. > > > So no - there's not a specific bug driving this, but a lot of cleanup > that I've been wanting to do since before speculation came along. Thanks for the clarification! Cheers,
On 19.12.2022 15:45, Sergey Dyasli wrote: > Call early_microcode_init() straight after multiboot modules become > accessible. Modify it to load the ucode directly from the blob bypassing > populating microcode_cache because xmalloc is still not available at > that point during Xen boot. > > Introduce early_microcode_init_cache() for populating microcode_cache. > It needs to rescan the modules in order to find the new virtual address > of the ucode blob because it changes during the boot process, e.g. > from 0x00000000010802fc to 0xffff83204dac52fc. > > While at it, drop alternative_vcall() from early_microcode_init() since > it's not useful in an __init fuction. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 19/12/2022 2:45 pm, Sergey Dyasli wrote: > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 6bb5bc7c84..2d7c815e0a 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > relocated = true; > @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > init_IRQ(); > > - microcode_grab_module(module_map, mbi); > - > timer_init(); > > - early_microcode_init(); > + early_microcode_init_cache(module_map, mbi); microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ Can fix on commit. ~Andrew
On 20.12.2022 15:50, Andrew Cooper wrote: > On 19/12/2022 2:45 pm, Sergey Dyasli wrote: >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 6bb5bc7c84..2d7c815e0a 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> relocated = true; >> @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> init_IRQ(); >> >> - microcode_grab_module(module_map, mbi); >> - >> timer_init(); >> >> - early_microcode_init(); >> + early_microcode_init_cache(module_map, mbi); > > microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ > > Can fix on commit. Are you merely after the added comment, or is the omission of the early_ prefix also meaningful in some way? Jan
On 20/12/2022 3:18 pm, Jan Beulich wrote: > On 20.12.2022 15:50, Andrew Cooper wrote: >> On 19/12/2022 2:45 pm, Sergey Dyasli wrote: >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index 6bb5bc7c84..2d7c815e0a 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> relocated = true; >>> @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) >>> >>> init_IRQ(); >>> >>> - microcode_grab_module(module_map, mbi); >>> - >>> timer_init(); >>> >>> - early_microcode_init(); >>> + early_microcode_init_cache(module_map, mbi); >> microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ >> >> Can fix on commit. > Are you merely after the added comment, or is the omission of the early_ > prefix also meaningful in some way? This isn't "early_microcode" and frankly wasn't "early" to begin with. Caching the blob can happen at any time after the heap is set up, so should not have anything like "early" in its name. The comment is just to make it easier in the future to figure out how to rearrange __start_xen(). ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 85c05e480d..04b5d346ab 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -27,6 +27,7 @@ #include <xen/err.h> #include <xen/guest_access.h> #include <xen/init.h> +#include <xen/multiboot.h> #include <xen/param.h> #include <xen/spinlock.h> #include <xen/stop_machine.h> @@ -198,7 +199,8 @@ void __init microcode_scan_module( bootstrap_map(NULL); } } -void __init microcode_grab_module( + +static void __init microcode_grab_module( unsigned long *module_map, const multiboot_info_t *mbi) { @@ -732,10 +734,54 @@ int microcode_update_one(void) return microcode_update_cpu(NULL); } +static int __init early_update_cache(const void *data, size_t len) +{ + int rc = 0; + struct microcode_patch *patch; + + if ( !data ) + return -ENOMEM; + + patch = parse_blob(data, len); + if ( IS_ERR(patch) ) + { + printk(XENLOG_WARNING "Parsing microcode blob error %ld\n", + PTR_ERR(patch)); + return PTR_ERR(patch); + } + + if ( !patch ) + return -ENOENT; + + spin_lock(µcode_mutex); + rc = microcode_update_cache(patch); + spin_unlock(µcode_mutex); + ASSERT(rc); + + return rc; +} + +int __init early_microcode_init_cache(unsigned long *module_map, + const struct multiboot_info *mbi) +{ + int rc = 0; + + if ( ucode_scan ) + /* Need to rescan the modules because they might have been relocated */ + microcode_scan_module(module_map, mbi); + + if ( ucode_mod.mod_end ) + rc = early_update_cache(bootstrap_map(&ucode_mod), + ucode_mod.mod_end); + else if ( ucode_blob.size ) + rc = early_update_cache(ucode_blob.data, ucode_blob.size); + + return rc; +} + /* BSP calls this function to parse ucode blob and then apply an update. */ static int __init early_microcode_update_cpu(void) { - int rc = 0; const void *data = NULL; size_t len; struct microcode_patch *patch; @@ -754,7 +800,7 @@ static int __init early_microcode_update_cpu(void) if ( !data ) return -ENOMEM; - patch = parse_blob(data, len); + patch = ucode_ops.cpu_request_microcode(data, len, false); if ( IS_ERR(patch) ) { printk(XENLOG_WARNING "Parsing microcode blob error %ld\n", @@ -765,15 +811,11 @@ static int __init early_microcode_update_cpu(void) if ( !patch ) return -ENOENT; - spin_lock(µcode_mutex); - rc = microcode_update_cache(patch); - spin_unlock(µcode_mutex); - ASSERT(rc); - - return microcode_update_one(); + return microcode_update_cpu(patch); } -int __init early_microcode_init(void) +int __init early_microcode_init(unsigned long *module_map, + const struct multiboot_info *mbi) { const struct cpuinfo_x86 *c = &boot_cpu_data; int rc = 0; @@ -797,7 +839,9 @@ int __init early_microcode_init(void) return -ENODEV; } - alternative_vcall(ucode_ops.collect_cpu_info); + microcode_grab_module(module_map, mbi); + + ucode_ops.collect_cpu_info(); if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h index 3b0234e9fa..170481d257 100644 --- a/xen/arch/x86/include/asm/microcode.h +++ b/xen/arch/x86/include/asm/microcode.h @@ -6,6 +6,8 @@ #include <public/xen.h> +struct multiboot_info; + struct cpu_signature { /* CPU signature (CPUID.1.EAX). */ unsigned int sig; @@ -21,7 +23,10 @@ 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 early_microcode_init(unsigned long *module_map, + const struct multiboot_info *mbi); +int early_microcode_init_cache(unsigned long *module_map, + const struct multiboot_info *mbi); int microcode_update_one(void); #endif /* ASM_X86__MICROCODE_H */ diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index ae470ea12f..ae0dd3915a 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -44,9 +44,6 @@ void *bootstrap_map(const module_t *mod); int xen_in_range(unsigned long mfn); -void microcode_grab_module( - unsigned long *, const multiboot_info_t *); - extern uint8_t kbd_shift_flags; #ifdef NDEBUG diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 6bb5bc7c84..2d7c815e0a 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1178,6 +1178,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) mod[i].reserved = 0; } + /* + * TODO: load ucode earlier once multiboot modules become accessible + * at an earlier stage. + */ + early_microcode_init(module_map, mbi); + if ( xen_phys_start ) { relocated = true; @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) init_IRQ(); - microcode_grab_module(module_map, mbi); - timer_init(); - early_microcode_init(); + early_microcode_init_cache(module_map, mbi); tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
Call early_microcode_init() straight after multiboot modules become accessible. Modify it to load the ucode directly from the blob bypassing populating microcode_cache because xmalloc is still not available at that point during Xen boot. Introduce early_microcode_init_cache() for populating microcode_cache. It needs to rescan the modules in order to find the new virtual address of the ucode blob because it changes during the boot process, e.g. from 0x00000000010802fc to 0xffff83204dac52fc. While at it, drop alternative_vcall() from early_microcode_init() since it's not useful in an __init fuction. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v1 --> v2: - Don't call microcode_grab_module() the second time, use microcode_scan_module() instead - Use forward declaration of struct multiboot_info - Don't use alternative calls - Rename early_microcode_update_cache() to early_update_cache() and move it around a bit --- xen/arch/x86/cpu/microcode/core.c | 66 +++++++++++++++++++++++----- xen/arch/x86/include/asm/microcode.h | 7 ++- xen/arch/x86/include/asm/setup.h | 3 -- xen/arch/x86/setup.c | 10 +++-- 4 files changed, 68 insertions(+), 18 deletions(-)