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(-)
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,
--
2.35.3On 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
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...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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
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
© 2016 - 2026 Red Hat, Inc.