| Message ID | 20220820092533.29420-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make pat and mtrr independent from each other | expand |
On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote: > The Cyrix cpu specific MTRR function cyrix_set_all() will never be > called, as the struct mtrr_ops set_all() callback will only be called > in the use_intel() case, which would require the use_intel_if member > of struct mtrr_ops to be set, which isn't the case for Cyrix. Doing some git archeology: So the commit which added mtrr_aps_delayed_init is d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init") from 2009. The IPI callback before it, looked like this: static void ipi_handler(void *info) { #ifdef CONFIG_SMP struct set_mtrr_data *data = info; unsigned long flags; local_irq_save(flags); atomic_dec(&data->count); while (!atomic_read(&data->gate)) cpu_relax(); /* The master has cleared me to execute */ if (data->smp_reg != ~0U) { mtrr_if->set(data->smp_reg, data->smp_base, data->smp_size, data->smp_type); } else { mtrr_if->set_all(); ^^^^^^^^^ and that else branch would call ->set_all() on Cyrix too. Suresh's patch changed it to do: - } else { + } else if (mtrr_aps_delayed_init) { + /* + * Initialize the MTRRs inaddition to the synchronisation. + */ mtrr_if->set_all(); BUT below in the set_mtrr() call, it did: /* * HACK! * We use this same function to initialize the mtrrs on boot. * The state of the boot cpu's mtrrs has been saved, and we want * to replicate across all the APs. * If we're doing that @reg is set to something special... */ if (reg != ~0U) mtrr_if->set(reg, base, size, type); else if (!mtrr_aps_delayed_init) mtrr_if->set_all(); ^^^ and that would be the Cyrix case. But then 192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous") came and "cleaned" all up by removing the "HACK" and doing ->set_all() only in the rendezvous handler: + } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) { mtrr_if->set_all(); } Which begs the question: why doesn't the second part of the else test match on Cyrix? The "|| !cpu_online(smp_processor_id())" case. If only we had a Cyrix machine somewhere...
On 25.08.22 12:31, Borislav Petkov wrote: > On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote: >> The Cyrix cpu specific MTRR function cyrix_set_all() will never be >> called, as the struct mtrr_ops set_all() callback will only be called >> in the use_intel() case, which would require the use_intel_if member >> of struct mtrr_ops to be set, which isn't the case for Cyrix. > > Doing some git archeology: > > So the commit which added mtrr_aps_delayed_init is > > d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init") > > from 2009. > > The IPI callback before it, looked like this: > > static void ipi_handler(void *info) > { > #ifdef CONFIG_SMP > struct set_mtrr_data *data = info; > unsigned long flags; > > local_irq_save(flags); > > atomic_dec(&data->count); > while (!atomic_read(&data->gate)) > cpu_relax(); > > /* The master has cleared me to execute */ > if (data->smp_reg != ~0U) { > mtrr_if->set(data->smp_reg, data->smp_base, > data->smp_size, data->smp_type); > } else { > mtrr_if->set_all(); > ^^^^^^^^^ > > and that else branch would call ->set_all() on Cyrix too. > > Suresh's patch changed it to do: > > - } else { > + } else if (mtrr_aps_delayed_init) { > + /* > + * Initialize the MTRRs inaddition to the synchronisation. > + */ > mtrr_if->set_all(); > > BUT below in the set_mtrr() call, it did: > > /* > * HACK! > * We use this same function to initialize the mtrrs on boot. > * The state of the boot cpu's mtrrs has been saved, and we want > * to replicate across all the APs. > * If we're doing that @reg is set to something special... > */ > if (reg != ~0U) > mtrr_if->set(reg, base, size, type); > else if (!mtrr_aps_delayed_init) > mtrr_if->set_all(); > ^^^ > > and that would be the Cyrix case. > > But then > > 192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous") > > came and "cleaned" all up by removing the "HACK" and doing ->set_all() > only in the rendezvous handler: > > + } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) { > mtrr_if->set_all(); > } > > Which begs the question: why doesn't the second part of the else test > match on Cyrix? The "|| !cpu_online(smp_processor_id())" case. > > If only we had a Cyrix machine somewhere... > You are missing one aspect here: there is no call path for Cyrix CPUs using reg == ~0U. So the condition of the "else if" will never be evaluated with Cyrix. Juergen
On 25.08.22 12:31, Borislav Petkov wrote: > On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote: >> The Cyrix cpu specific MTRR function cyrix_set_all() will never be >> called, as the struct mtrr_ops set_all() callback will only be called >> in the use_intel() case, which would require the use_intel_if member >> of struct mtrr_ops to be set, which isn't the case for Cyrix. > > Doing some git archeology: > > So the commit which added mtrr_aps_delayed_init is > > d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init") > > from 2009. > > The IPI callback before it, looked like this: > > static void ipi_handler(void *info) > { > #ifdef CONFIG_SMP > struct set_mtrr_data *data = info; > unsigned long flags; > > local_irq_save(flags); > > atomic_dec(&data->count); > while (!atomic_read(&data->gate)) > cpu_relax(); > > /* The master has cleared me to execute */ > if (data->smp_reg != ~0U) { > mtrr_if->set(data->smp_reg, data->smp_base, > data->smp_size, data->smp_type); > } else { > mtrr_if->set_all(); > ^^^^^^^^^ > > and that else branch would call ->set_all() on Cyrix too. > > Suresh's patch changed it to do: > > - } else { > + } else if (mtrr_aps_delayed_init) { > + /* > + * Initialize the MTRRs inaddition to the synchronisation. > + */ > mtrr_if->set_all(); > > BUT below in the set_mtrr() call, it did: > > /* > * HACK! > * We use this same function to initialize the mtrrs on boot. > * The state of the boot cpu's mtrrs has been saved, and we want > * to replicate across all the APs. > * If we're doing that @reg is set to something special... > */ > if (reg != ~0U) > mtrr_if->set(reg, base, size, type); > else if (!mtrr_aps_delayed_init) > mtrr_if->set_all(); > ^^^ > > and that would be the Cyrix case. > > But then > > 192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous") > > came and "cleaned" all up by removing the "HACK" and doing ->set_all() > only in the rendezvous handler: > > + } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) { > mtrr_if->set_all(); > } > > Which begs the question: why doesn't the second part of the else test > match on Cyrix? The "|| !cpu_online(smp_processor_id())" case. > > If only we had a Cyrix machine somewhere... > Maybe the alternative reasoning is much faster to understand: if the Cyrix set_all() could be called, the AMD and Centaur ones would be callable, too. Those being called would result in a NULL deref, so why should we keep the Cyrix one? Juergen
On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote: > Maybe the alternative reasoning is much faster to understand: if the > Cyrix set_all() could be called, the AMD and Centaur ones would be callable, > too. Right. > Those being called would result in a NULL deref, so why should we keep > the Cyrix one? I know you're eager to remove dead code - I'd love that too. But before we do that, we need to find out whether some Cyrix hw out there would not need this. I know, I know, they should've complained by now ... maybe they have but we haven't heard about it. What it most likely looks like is that those machines - a commit from before git commit 8fbdcb188e31ac901e216b466b97e90e8b057daa Author: Dave Jones <davej@suse.de> Date: Wed Aug 14 21:14:22 2002 -0700 [PATCH] Modular x86 MTRR driver. talks about +/* + * On Cyrix 6x86(MX) and M II the ARR3 is special: it has connection + * with the SMM (System Management Mode) mode. So we need the following: + * Check whether SMI_LOCK (CCR3 bit 0) is set + * if it is set, write a warning message: ARR3 cannot be changed! + * (it cannot be changed until the next processor reset) which sounds like old rust. And which no one uses or such machines are long dead already. Wikipedia says: https://en.wikipedia.org/wiki/Cyrix_6x86 "The Cyrix 6x86 is a line of sixth-generation, 32-bit x86 microprocessors designed and released by Cyrix in 1995..." So I'm thinking removing it would be ok...
On 25.08.22 13:42, Borislav Petkov wrote: > On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote: >> Maybe the alternative reasoning is much faster to understand: if the >> Cyrix set_all() could be called, the AMD and Centaur ones would be callable, >> too. > > Right. > >> Those being called would result in a NULL deref, so why should we keep >> the Cyrix one? > > I know you're eager to remove dead code - I'd love that too. But before > we do that, we need to find out whether some Cyrix hw out there would > not need this. Back to reasoning #1. Only the use_intel() case calls the code in question with reg == ~0. And use_intel() is clearly not Cyrix. Juergen
diff --git a/arch/x86/kernel/cpu/mtrr/cyrix.c b/arch/x86/kernel/cpu/mtrr/cyrix.c index ca670919b561..c77d3b0a5bf2 100644 --- a/arch/x86/kernel/cpu/mtrr/cyrix.c +++ b/arch/x86/kernel/cpu/mtrr/cyrix.c @@ -234,42 +234,8 @@ static void cyrix_set_arr(unsigned int reg, unsigned long base, post_set(); } -typedef struct { - unsigned long base; - unsigned long size; - mtrr_type type; -} arr_state_t; - -static arr_state_t arr_state[8] = { - {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, - {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL} -}; - -static unsigned char ccr_state[7] = { 0, 0, 0, 0, 0, 0, 0 }; - -static void cyrix_set_all(void) -{ - int i; - - prepare_set(); - - /* the CCRs are not contiguous */ - for (i = 0; i < 4; i++) - setCx86(CX86_CCR0 + i, ccr_state[i]); - for (; i < 7; i++) - setCx86(CX86_CCR4 + i, ccr_state[i]); - - for (i = 0; i < 8; i++) { - cyrix_set_arr(i, arr_state[i].base, - arr_state[i].size, arr_state[i].type); - } - - post_set(); -} - static const struct mtrr_ops cyrix_mtrr_ops = { .vendor = X86_VENDOR_CYRIX, - .set_all = cyrix_set_all, .set = cyrix_set_arr, .get = cyrix_get_arr, .get_free_region = cyrix_get_free_region,
The Cyrix cpu specific MTRR function cyrix_set_all() will never be called, as the struct mtrr_ops set_all() callback will only be called in the use_intel() case, which would require the use_intel_if member of struct mtrr_ops to be set, which isn't the case for Cyrix. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- arch/x86/kernel/cpu/mtrr/cyrix.c | 34 -------------------------------- 1 file changed, 34 deletions(-)