Hi,

Thanks your comments,

On Fri, 14 Oct 2016 16:23:40 +0200 (CEST)
s...@openbsd.org wrote:
> On Fri, 14 Oct 2016, YASUOKA Masahiko wrote:
>> I'm working on NEC Express5800/R110h-1 (dmesg is attached).  On this
>> machine, our kernel panics with following message.
>> 
>>   cpu0 at mainbus0panic: cpu at apic id 0 already attached?
>> 
>> This seems to happen since x2APIC on the machine is enabled by BIOS
>> and the kernel doesn't assume that.  The diff makes the kernel use
>> x2APIC if it is enabled by BIOS.
>> 
>> ok?
> 
> the code looks ok, but ...
(snip)
> ... I would leave the comment here why we don't enable it on all hardware 
> that supports it.

I reverted the comment and also tweaked white space.

Index: sys/arch/amd64/amd64/lapic.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
retrieving revision 1.44
diff -u -p -r1.44 lapic.c
--- sys/arch/amd64/amd64/lapic.c        22 Jun 2016 01:12:38 -0000      1.44
+++ sys/arch/amd64/amd64/lapic.c        17 Oct 2016 01:24:50 -0000
@@ -170,59 +170,57 @@ lapic_map(paddr_t lapic_base)
        int s;
        pt_entry_t *pte;
        vaddr_t va;
+       u_int64_t msr;
 
-       /*
-        * On real hardware, x2apic must only be enabled if interrupt remapping
-        * is also enabled. See 10.12.7 of the SDM vol 3.
-        * On hypervisors, this is not necessary. Hypervisors can implement
-        * x2apic support even if the host CPU does not support it.
-        * Until we support interrupt remapping, use x2apic only if the
-        * hypervisor flag is also set.
-        */
-       if ((cpu_ecxfeature&CPUIDECX_X2APIC) && (cpu_ecxfeature&CPUIDECX_HV)) {
-               u_int64_t msr;
-
-               disable_intr();
-               s = lapic_tpr;
-
-               msr = rdmsr(MSR_APICBASE);
-               msr |= APICBASE_ENABLE_X2APIC;
-               wrmsr(MSR_APICBASE, msr);
+       disable_intr();
+       s = lapic_tpr;
+
+       msr = rdmsr(MSR_APICBASE);
 
+       if (ISSET(msr, APICBASE_ENABLE_X2APIC) ||
+           (ISSET(cpu_ecxfeature, CPUIDECX_HV) &&
+           ISSET(cpu_ecxfeature, CPUIDECX_X2APIC))) {
+                /*
+                 * On real hardware, x2apic must only be enabled if interrupt
+                 * remapping is also enabled. See 10.12.7 of the SDM vol 3.
+                 * On hypervisors, this is not necessary. Hypervisors can
+                 * implement x2apic support even if the host CPU does not
+                 * support it.  Until we support interrupt remapping, use
+                 * x2apic only if the hypervisor flag is also set or it is
+                 * enabled by BIOS.
+                 */
+               if (!ISSET(msr, APICBASE_ENABLE_X2APIC)) {
+                       msr |= APICBASE_ENABLE_X2APIC;
+                       wrmsr(MSR_APICBASE, msr);
+               }
                lapic_readreg = x2apic_readreg;
                lapic_writereg = x2apic_writereg;
 #ifdef MULTIPROCESSOR
                x86_ipi = x2apic_ipi;
 #endif
                x2apic_enabled = 1;
-
                codepatch_call(CPTAG_EOI, &x2apic_eoi);
 
                lapic_writereg(LAPIC_TPRI, s);
-               enable_intr();
+       } else {
+               /*
+                * Map local apic.  If we have a local apic, it's safe to
+                * assume we're on a 486 or better and can use invlpg and
+                * non-cacheable PTE's
+                *
+                * Whap the PTE "by hand" rather than calling pmap_kenter_pa
+                * because the latter will attempt to invoke TLB shootdown
+                * code just as we might have changed the value of
+                * cpu_number()..
+                */
+               va = (vaddr_t)&local_apic;
+               pte = kvtopte(va);
+               *pte = lapic_base | PG_RW | PG_V | PG_N | PG_G | pg_nx;
+               invlpg(va);
 
-               return;
+               lapic_tpr = s;
        }
 
-       va = (vaddr_t)&local_apic;
-
-       disable_intr();
-       s = lapic_tpr;
-
-       /*
-        * Map local apic.  If we have a local apic, it's safe to assume
-        * we're on a 486 or better and can use invlpg and non-cacheable PTE's
-        *
-        * Whap the PTE "by hand" rather than calling pmap_kenter_pa because
-        * the latter will attempt to invoke TLB shootdown code just as we
-        * might have changed the value of cpu_number()..
-        */
-
-       pte = kvtopte(va);
-       *pte = lapic_base | PG_RW | PG_V | PG_N | PG_G | pg_nx;
-       invlpg(va);
-
-       lapic_tpr = s;
        enable_intr();
 }
 

Reply via email to