| Message ID | 20241028091856.2151603-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Fix module-handling use-aftere-free's | expand |
On 28.10.2024 10:18, Andrew Cooper wrote: > There's no need for microcode_init_cache() to be called exactly where it is in > __start_xen(). All that matters is it must be after xmalloc() is available > and before APs start up. > > As a consequence, microcode_init_cache() runs a little later on boot now. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -819,8 +819,9 @@ static int __init early_update_cache(const void *data, size_t len) > return rc; > } > > -int __init microcode_init_cache(struct boot_info *bi) > +static int __init cf_check microcode_init_cache(void) > { > + struct boot_info *bi = &xen_boot_info; I take it that this not becoming pointer-to-const is related to your reply to Roger elsewhere, where he asked for adding const, while you expect the const to need dropping again in the course of Daniel's work? Jan
On 28/10/2024 1:28 pm, Jan Beulich wrote: > On 28.10.2024 10:18, Andrew Cooper wrote: >> There's no need for microcode_init_cache() to be called exactly where it is in >> __start_xen(). All that matters is it must be after xmalloc() is available >> and before APs start up. >> >> As a consequence, microcode_init_cache() runs a little later on boot now. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -819,8 +819,9 @@ static int __init early_update_cache(const void *data, size_t len) >> return rc; >> } >> >> -int __init microcode_init_cache(struct boot_info *bi) >> +static int __init cf_check microcode_init_cache(void) >> { >> + struct boot_info *bi = &xen_boot_info; > I take it that this not becoming pointer-to-const is related to your reply to > Roger elsewhere, where he asked for adding const, while you expect the const > to need dropping again in the course of Daniel's work? It's not const because that wouldn't even compile here. But yes, it does get mutated by the end of the boot module work. ~Andrew
On 10/28/24 05:18, Andrew Cooper wrote: > There's no need for microcode_init_cache() to be called exactly where it is in > __start_xen(). All that matters is it must be after xmalloc() is available > and before APs start up. > > As a consequence, microcode_init_cache() runs a little later on boot now. > > No functional 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: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/arch/x86/cpu/microcode/core.c | 4 +++- > xen/arch/x86/include/asm/microcode.h | 1 - > xen/arch/x86/setup.c | 2 -- > 3 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index 8d916d891e6e..9a2cc631d2aa 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -819,8 +819,9 @@ static int __init early_update_cache(const void *data, size_t len) > return rc; > } > > -int __init microcode_init_cache(struct boot_info *bi) > +static int __init cf_check microcode_init_cache(void) > { > + struct boot_info *bi = &xen_boot_info; For context, the current function definition didn't have *bi const because it will become necessary later update state in bi. So it is natural for it to remain as such by getting the reference from global. Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 8d916d891e6e..9a2cc631d2aa 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -819,8 +819,9 @@ static int __init early_update_cache(const void *data, size_t len) return rc; } -int __init microcode_init_cache(struct boot_info *bi) +static int __init cf_check microcode_init_cache(void) { + struct boot_info *bi = &xen_boot_info; int rc = 0; if ( !ucode_ops.apply_microcode ) @@ -838,6 +839,7 @@ int __init microcode_init_cache(struct boot_info *bi) return rc; } +presmp_initcall(microcode_init_cache); /* BSP calls this function to parse ucode blob and then apply an update. */ static int __init early_microcode_update_cpu(void) diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h index a278773f8b5d..c62c131fd047 100644 --- a/xen/arch/x86/include/asm/microcode.h +++ b/xen/arch/x86/include/asm/microcode.h @@ -28,6 +28,5 @@ int microcode_update_one(void); struct boot_info; int early_microcode_init(struct boot_info *bi); -int microcode_init_cache(struct boot_info *bi); #endif /* ASM_X86__MICROCODE_H */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 177f4024abca..5db57e1b1af2 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1952,8 +1952,6 @@ void asmlinkage __init noreturn __start_xen(void) timer_init(); - microcode_init_cache(bi); /* Needs xmalloc() */ - tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */ calculate_raw_cpu_policy(); /* Needs microcode. No other dependenices. */
There's no need for microcode_init_cache() to be called exactly where it is in __start_xen(). All that matters is it must be after xmalloc() is available and before APs start up. As a consequence, microcode_init_cache() runs a little later on boot now. No functional 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: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/x86/cpu/microcode/core.c | 4 +++- xen/arch/x86/include/asm/microcode.h | 1 - xen/arch/x86/setup.c | 2 -- 3 files changed, 3 insertions(+), 4 deletions(-)