Messages in this thread | | | Date | Thu, 25 Aug 2022 12:31:11 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function |
| |
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...
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |