Re: RFC: [2.6 patch] better i386 CPU selection

Jeff Garzik
2003-09-13T18:22:07Z

On Sat, Sep 13, 2003 at 05:11:49PM +0100, Dave Jones wrote:
>
> > In 2.4 selecting e.g. M486 has the semantics to get a kernel that runs
> > on a 486 and above.
> > In 2.6 selecting M486 means that only the 486 is supported.
>
> What are you basing this on ? This seems bogus to me.
> Last I checked, I could for eg, boot a 386 kernel on an Athlon.

If you know that you're only booting on a 486, why include all the junk
needed solely for later processors?

>
> > +config CPU_ONLY_K7
> > + bool
> > + depends on CPU_K7 && !CPU_INTEL && !CPU_K6 && !CPU_K8 && !X86_ELAN && !CPU_CRUSOE && !CPU_WINCHIP && !CPU_CYRIXIII && !CPU_VIAC3_2
> > +
> > +config CPU_ONLY_K8
> > + bool
> > + depends on CPU_K8 && !CPU_INTEL && !CPU_K6 && !CPU_K7 && !X86_ELAN && !CPU_CRUSOE && !CPU_WINCHIP && !CPU_CYRIXIII && !CPU_VIAC3_2
> > +
> > +config CPU_ONLY_WINCHIP
> > + bool
> > + depends on CPU_WINCHIP && !CPU_INTEL && !CPU_K6 && !CPU_K7 && !CPU_K8 && !X86_ELAN && !CPU_CRUSOE && !CPU_CYRIXIII && !CPU_VIAC3_2
> > + default y
>
> These are hurrendous. Each time we add support for a new CPU we
> have to update each and every one of these. This won't fly,
> someone *WILL* miss one. We've had this sort of thing happen before,
> and its an accident waiting to happen.

Agreed

> > --- linux-2.6.0-test5-cpu/arch/i386/mm/init.c.old 2003-09-13 14:18:04.000000000 +0200
> > +++ linux-2.6.0-test5-cpu/arch/i386/mm/init.c 2003-09-13 14:23:26.000000000 +0200
> > @@ -436,8 +436,12 @@
> > if (!mem_map)
> > BUG();
> > #endif
> > -
> > +
> > +#ifdef CONFIG_CPU_686
> > bad_ppro = ppro_with_ram_bug();
> > +#else
> > + bad_ppro = 0;
> > +#endif
> >
>
> If we boot a 386 kernel on a ppro with that bug, this goes bang.

Echo my first comment. I think it's fair to make this stuff
fine-grained -- as long as present behavior is preserved. So IMO
fine-grained "I really want this cpu, and this cpu only" stuff really
requires a dependency on CONFIG_EMBEDDED. When !CONFIG_EMBEDDED, for
example, it would define CONFIG_CPU_686 to ensure the required 686
pieces were in place.

I like the general direction of Adrian's patch, and think that moving in
this direction will provide a lot of hidden maintenance _benefits_ after
the initial pain... But. Adrian's patch is a tough thing to get right,
and IMO requires a lot of testing.

> > static void __init init_ifs(void)
> > {
> > +
> > +#if defined(CONFIG_CPU_K6)
> > amd_init_mtrr();
> > +#endif
> > +
> > +#if defined(CONFIG_CPU_586)
> > cyrix_init_mtrr();
> > +#endif
> > +
> > +#if defined(CONFIG_CPU_WINCHIP) || defined(CONFIG_CPU_CYRIXIII) || defined(CONFIG_CPU_VIAC3_2)
> > centaur_init_mtrr();
> > +#endif
> > +
>
> For the handful of bytes saved in the mtrr driver, I'm more concerned
> about ifdef noise, and the fact that we don't have a compile once-run
> everywhere MTRR driver anymore unless you pick your options right

The arch/i386/kernel/cpu stuff is so modular, code like the above just
screams for an ->init_mtrr() hook in there. drivers/char/hw_random.c
(portably) depends on VIA RNG's xstore instruction, which implies a
dependency on code and settings in arch/i386/*. So, there's nothing
fundamentally wrong with sticking your fingers in there, IMO...

Jeff