H. Peter Anvin wrote:
>"Maciej W. Rozycki" wrote:
>...
>> It might be viable just to delete the test altogether, though and just
>> trap #GP(0) on the MSR access. For the sake of simplicity. If a problem
>> with a system ever arizes, we may handle it then.
>>
>> Note that we still have to choose appropriate vendor-specific PeMo
>> handling and an event for the NMI watchdog anyway.
>>
>
>Right... if that is the case then it seems reasonable.
No, poking into MSRs not explicitly defined on the current CPU is
inherently unsafe. I have several x86 CPU data sheets here in front
of me which say the same thing: "Don't write to undocumented MSRs."
You cannot assume that every single x86 out there stays clear of
all Intel-defined MSRs. Intel has also expanded this set over time:
older designs may not even have known about the APIC_BASE MSR.
No, the problem we're facing here is a phase ordering issue.
There are actually several bugs lurking here:
1. identify_cpu() (and more importantly get_cpu_vendor()) is called
very late (init/main.c's check_bugs()), while we obviously need
that info much earlier. Thus, boot_cpu_data.x86_vendor is still
uninitialised (zero by .bss) when we read it for the purpose of
setting up the local APIC.
2. include/asm-i386/processor.h #defines X86_VENDOR_INTEL as 0.
Thus, we may accidentally believe that the CPU is an Intel, if
we read x86_vendor before identify_cpu() had done its deed.
This explains why Petr Vandrovec's testing on K7 succeeded (the
APIC-enabling code mistook his K7 for a P6). Change Intel's
vendor id to something non-zero and you'll see that on
UP P6 the local APIC doesn't get initialised.
3. init/main.c calls time_init() before check_bugs() and thus
identify_cpu(). time_init() calls dodgy_tsc() which calls
get_cpu_vendor(). However, get_cpu_vendor() only works on
CPUID-enabled CPUs, so dodgy_tsc()'s call to init_cyrix()
isn't guaranteed to happen. (init_cyrix() will be called later,
but that may be too late.)
Also, time_init() checks cpu_has_tsc, but since this is before
identify_cpu() has done its thing, the capabilities are still
the raw ones from head.S's CPUID calls.
4. The cpu detection code rewrite in 2.4.0-test<something>
introduced a bug. The capabilities field was expanded from
one to four words, which changed the starting offset of
the vendor_id string. But the offsets in head.S weren't
updated, causing head.S's initial vendor_id data to be stored
in the wrong place. This hasn't been noticed yet since
identify_cpu() does this work again correctly, but it does
mean that calling get_cpu_vendor() before identify_cpu()
doesn't work any more.
Below is a patch against 2.4.1-ac5 which fixes CPU detection
as far as the UP_APIC stuff is concerned. I had to add a
get_cpu_vendor() call to detect_init_APIC(), so that our
detection code gets a chance to work. I also fixed the field
ordering and offsets in processor.h and head.S. The resulting
kernel works ok on my UP P6.
(Petr: can you check that it still works on your K7?)
Ideally, identify_cpu() should be run before init_apic_mappings(),
but my attempts to do so has so far had some weird side-effects
(lost interrupts, incorrect bogomips, apparently stuck watchdog,
and keyboard timeouts), so I won't touch that stuff.
/Mikael
--- linux-2.4.1-ac5/arch/i386/kernel/apic.c.~1~ Thu Feb 8 02:01:39 2001
+++ linux-2.4.1-ac5/arch/i386/kernel/apic.c Thu Feb 8 05:28:21 2001
@@ -388,21 +388,23 @@
{
u32 h, l, dummy, features;
- if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
- printk("No APIC support for non-Intel processors.\n");
- return -1;
- }
+ get_cpu_vendor(&boot_cpu_data);
- if (boot_cpu_data.x86 < 5) {
- printk("No local APIC on pre-Pentium Intel processors.\n");
- return -1;
+ switch (boot_cpu_data.x86_vendor) {
+ case X86_VENDOR_AMD:
+ if (boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model > 1)
+ break;
+ goto no_apic;
+ case X86_VENDOR_INTEL:
+ if (boot_cpu_data.x86 == 6 ||
+ (boot_cpu_data.x86 == 5 && cpu_has_apic))
+ break;
+ goto no_apic;
+ default:
+ goto no_apic;
}
if (!cpu_has_apic) {
- if (boot_cpu_data.x86 == 5) {
- printk("APIC turned off by hardware.\n");
- return -1;
- }
/*
* Some BIOSes disable the local APIC in the
* APIC_BASE MSR. This can only be done in
@@ -434,6 +436,10 @@
printk("Found and enabled local APIC!\n");
return 0;
+
+no_apic:
+ printk("No local APIC present or hardware disabled\n");
+ return -1;
}
void __init init_apic_mappings(void)
--- linux-2.4.1-ac5/arch/i386/kernel/head.S.~1~ Tue Dec 12 13:57:57 2000
+++ linux-2.4.1-ac5/arch/i386/kernel/head.S Thu Feb 8 05:31:12 2001
@@ -33,8 +33,8 @@
#define X86_MASK CPU_PARAMS+3
#define X86_HARD_MATH CPU_PARAMS+6
#define X86_CPUID CPU_PARAMS+8
-#define X86_CAPABILITY CPU_PARAMS+12
-#define X86_VENDOR_ID CPU_PARAMS+16
+#define X86_VENDOR_ID CPU_PARAMS+12
+#define X86_CAPABILITY CPU_PARAMS+28
/*
* swapper_pg_dir is the main page directory, address 0x00101000
--- linux-2.4.1-ac5/include/asm-i386/processor.h.~1~ Thu Feb 8 02:01:44 2001
+++ linux-2.4.1-ac5/include/asm-i386/processor.h Thu Feb 8 05:29:59 2001
@@ -39,8 +39,8 @@
char hard_math;
char rfu;
int cpuid_level; /* Maximum supported CPUID level, -1=no CPUID */
- __u32 x86_capability[NCAPINTS];
char x86_vendor_id[16];
+ __u32 x86_capability[NCAPINTS];
char x86_model_id[64];
int x86_cache_size; /* in KB - valid for CPUS which support this
call */
@@ -54,15 +54,17 @@
unsigned long pgtable_cache_sz;
};
-#define X86_VENDOR_INTEL 0
-#define X86_VENDOR_CYRIX 1
-#define X86_VENDOR_AMD 2
-#define X86_VENDOR_UMC 3
-#define X86_VENDOR_NEXGEN 4
-#define X86_VENDOR_CENTAUR 5
-#define X86_VENDOR_RISE 6
-#define X86_VENDOR_TRANSMETA 7
-#define X86_VENDOR_UNKNOWN 0xff
+enum {
+ X86_VENDOR_UNKNOWN = 0, /* zero must be the "unknown" case */
+ X86_VENDOR_INTEL,
+ X86_VENDOR_CYRIX,
+ X86_VENDOR_AMD,
+ X86_VENDOR_UMC,
+ X86_VENDOR_NEXGEN,
+ X86_VENDOR_CENTAUR,
+ X86_VENDOR_RISE,
+ X86_VENDOR_TRANSMETA,
+};
/*
* capabilities of CPUs
Mikael Pettersson wrote:
>
> H. Peter Anvin wrote:
>
> >"Maciej W. Rozycki" wrote:
> >...
> >> It might be viable just to delete the test altogether, though and just
> >> trap #GP(0) on the MSR access. For the sake of simplicity. If a problem
> >> with a system ever arizes, we may handle it then.
> >>
> >> Note that we still have to choose appropriate vendor-specific PeMo
> >> handling and an event for the NMI watchdog anyway.
> >
> >Right... if that is the case then it seems reasonable.
>
> No, poking into MSRs not explicitly defined on the current CPU is
> inherently unsafe. I have several x86 CPU data sheets here in front
> of me which say the same thing: "Don't write to undocumented MSRs."
> You cannot assume that every single x86 out there stays clear of
> all Intel-defined MSRs. Intel has also expanded this set over time:
> older designs may not even have known about the APIC_BASE MSR.
>
You misread me. "In that case it seems reasonable to do vendor-specific
detection."
-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
On Thu, 8 Feb 2001, Mikael Pettersson wrote:
> No, poking into MSRs not explicitly defined on the current CPU is
> inherently unsafe. I have several x86 CPU data sheets here in front
> of me which say the same thing: "Don't write to undocumented MSRs."
Your point is right -- the problem are not undefined MSRs (that raise
#GP(0) on an access) but undocumented ones, sigh...
> You cannot assume that every single x86 out there stays clear of
> all Intel-defined MSRs. Intel has also expanded this set over time:
> older designs may not even have known about the APIC_BASE MSR.
Intel is actually sane -- you get #GP(0) for this MSR on P5. Others
might not and there are less cluefull vendors out there.
> 1. identify_cpu() (and more importantly get_cpu_vendor()) is called
[...]
> 2. include/asm-i386/processor.h #defines X86_VENDOR_INTEL as 0.
[...]
> 3. init/main.c calls time_init() before check_bugs() and thus
[...]
> 4. The cpu detection code rewrite in 2.4.0-test<something>
[...]
Yes, there are more problems as well. I'm working on it -- you may want
to look at the preliminary patch I sent here on Monday (strangely enough,
nobody out of linux-kernel seemed to be interested so far). Next version
should be available later this week -- the main problem with moving
identify_cpu() earlier are other cpu_data fields that get initialized
later, so more code needs to be actually rewritten.
> Ideally, identify_cpu() should be run before init_apic_mappings(),
> but my attempts to do so has so far had some weird side-effects
> (lost interrupts, incorrect bogomips, apparently stuck watchdog,
> and keyboard timeouts), so I won't touch that stuff.
You see. I'm going to move identify_cpu() very early anyway, but this
need a careful code review.
Maciej
--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +