On Sat, Sep 06, 2014 at 04:04:49PM -0400, John Baldwin wrote: > On Friday, September 05, 2014 10:44:05 AM John Baldwin wrote: > > On Friday, September 05, 2014 4:43:05 am Konstantin Belousov wrote: > > > There is one weird detail, not touched by your patch. Amd64 resume > > > path calls initializecpu(), while i386 does not. I do not see any > > > use for the call, the reload of CRX registers by trampoline/resumectx > > > should already set everything to working state. E.g., enabling XMM > > > in CR4 after fpu state is restored looks strange. > > > > I can test that. > > I looked at this, and I actually think calling initializecpu() is correct. > It is true that it will set bits in CR4 that are already set, but it also > does vendor-specific initialization (e.g. init_amd() and init_via() on amd64 > set MSRs, and on i386 there is a lot more of those, though most of the > CPUs with extra settings probably do not support SMP or run in ACPI systems). > We could either save and restore those various vendor-specific MSRs and > registers in suspend/resume, or just call initializecpu() to set their > values. > The latter approach seems simpler, so I chose that route. In immediate > terms, > it should fix enabling PG_NX in MSR_EFER on i386 + PAE resume, but it also > makes the AP startup case simpler and closer to the amd64 code. I also moved > some code that set MSRs out of printcpuinfo() in identcpu.c and into > initializecpu() instead. Finally, I split initializecpucache() out similar > to > how it is split on amd64. Ok.
Two very minor comments below.
>
> Ah, I see one bug I will fix in my tree, pc98's machdep.c needs the change to
> call initializecpucache().
>
> --- //depot/vendor/freebsd/src/sys/i386/i386/initcpu.c
> +++ //depot/user/jhb/acpipci/i386/i386/initcpu.c
> @@ -59,6 +59,12 @@
> static void init_6x86(void);
> #endif /* I486_CPU */
>
> +#if defined(I586_CPU) && defined(CPU_WT_ALLOC)
> +static void enable_K5_wt_alloc(void);
> +static void enable_K6_wt_alloc(void);
> +static void enable_K6_2_wt_alloc(void);
> +#endif
> +
> #ifdef I686_CPU
> static void init_6x86MX(void);
> static void init_ppro(void);
> @@ -527,6 +533,8 @@
> intr_restore(saveintr);
> }
>
> +static int ppro_apic_used = -1;
> +
> static void
> init_ppro(void)
> {
> @@ -535,9 +543,29 @@
> /*
> * Local APIC should be disabled if it is not going to be used.
> */
> - apicbase = rdmsr(MSR_APICBASE);
> - apicbase &= ~APICBASE_ENABLED;
> - wrmsr(MSR_APICBASE, apicbase);
> + if (ppro_apic_used != 1) {
> + apicbase = rdmsr(MSR_APICBASE);
> + apicbase &= ~APICBASE_ENABLED;
> + wrmsr(MSR_APICBASE, apicbase);
> + ppro_apic_used = 0;
> + }
> +}
> +
> +/*
> + * If the local APIC is going to be used after being disabled above,
> + * re-enable it and don't disable it in the future.
> + */
> +void
> +ppro_reenable_apic(void)
> +{
> + u_int64_t apicbase;
> +
> + if (ppro_apic_used == 0) {
> + apicbase = rdmsr(MSR_APICBASE);
> + apicbase |= APICBASE_ENABLED;
> + wrmsr(MSR_APICBASE, apicbase);
> + ppro_apic_used = 1;
> + }
> }
>
> /*
> @@ -646,20 +674,6 @@
> }
> #endif
>
> -/*
> - * Initialize CR4 (Control register 4) to enable SSE instructions.
> - */
> -void
> -enable_sse(void)
> -{
> -#if defined(CPU_ENABLE_SSE)
> - if ((cpu_feature & CPUID_XMM) && (cpu_feature & CPUID_FXSR)) {
> - load_cr4(rcr4() | CR4_FXSR | CR4_XMM);
> - cpu_fxsr = hw_instruction_sse = 1;
> - }
> -#endif
> -}
> -
> extern int elf32_nxstack;
>
> void
> @@ -692,6 +706,27 @@
> #ifdef I586_CPU
> case CPU_586:
> switch (cpu_vendor_id) {
> + case CPU_VENDOR_AMD:
> +#ifdef CPU_WT_ALLOC
> + if (((cpu_id & 0x0f0) > 0) &&
> + ((cpu_id & 0x0f0) < 0x60) &&
> + ((cpu_id & 0x00f) > 3))
> + enable_K5_wt_alloc();
> + else if (((cpu_id & 0x0f0) > 0x80) ||
> + (((cpu_id & 0x0f0) == 0x80) &&
> + (cpu_id & 0x00f) > 0x07))
> + enable_K6_2_wt_alloc();
> + else if ((cpu_id & 0x0f0) > 0x50)
> + enable_K6_wt_alloc();
> +#endif
> + if ((cpu_id & 0xf0) == 0xa0)
> + /*
> + * Make sure the TSC runs through
> + * suspension, otherwise we can't use
> + * it as timecounter
> + */
> + wrmsr(0x1900, rdmsr(0x1900) | 0x20ULL);
> + break;
Move of the code to initialize CPU from identcpu() to initializecpu()
seems to be a fix on its own. Since you are moving the fragments
around, would you mind start using CPUID_MODEL/STEPPING etc constants ?
The code was ancient, probably predating the defines.
> case CPU_VENDOR_CENTAUR:
> init_winchip();
> break;
> @@ -762,7 +797,17 @@
> default:
> break;
> }
> - enable_sse();
> +#if defined(CPU_ENABLE_SSE)
> + if ((cpu_feature & CPUID_XMM) && (cpu_feature & CPUID_FXSR)) {
> + load_cr4(rcr4() | CR4_FXSR | CR4_XMM);
> + cpu_fxsr = hw_instruction_sse = 1;
> + }
> +#endif
> +}
> +
> +void
> +initializecpucache(void)
> +{
>
> /*
> * CPUID with %eax = 1, %ebx returns
> @@ -839,7 +884,7 @@
> * Enable write allocate feature of AMD processors.
> * Following two functions require the Maxmem variable being set.
> */
> -void
> +static void
> enable_K5_wt_alloc(void)
> {
> u_int64_t msr;
> @@ -885,7 +930,7 @@
> }
> }
>
> -void
> +static void
> enable_K6_wt_alloc(void)
> {
> quad_t size;
> @@ -945,7 +990,7 @@
> intr_restore(saveintr);
> }
>
> -void
> +static void
> enable_K6_2_wt_alloc(void)
> {
> quad_t size;
> --- //depot/vendor/freebsd/src/sys/i386/i386/machdep.c
> +++ //depot/user/jhb/acpipci/i386/i386/machdep.c
> @@ -2753,6 +2753,7 @@
> setidt(IDT_GP, &IDTVEC(prot), SDT_SYS386TGT, SEL_KPL,
> GSEL(GCODE_SEL, SEL_KPL));
> initializecpu(); /* Initialize CPU registers */
> + initializecpucache();
>
> /* make an initial tss so cpu can get interrupt stack on syscall! */
> /* Note: -16 is so we can grow the trapframe if we came from vm86 */
> --- //depot/vendor/freebsd/src/sys/i386/i386/mp_machdep.c
> +++ //depot/user/jhb/acpipci/i386/i386/mp_machdep.c
> @@ -745,25 +745,15 @@
> /* set up CPU registers and state */
> cpu_setregs();
>
> + /* set up SSE/NX registers */
> + initializecpu();
> +
> /* set up FPU state on the AP */
> npxinit();
>
> - /* set up SSE registers */
> - enable_sse();
> -
> if (cpu_ops.cpu_init)
> cpu_ops.cpu_init();
>
> -#ifdef PAE
> - /* Enable the PTE no-execute bit. */
> - if ((amd_feature & AMDID_NX) != 0) {
> - uint64_t msr;
> -
> - msr = rdmsr(MSR_EFER) | EFER_NXE;
> - wrmsr(MSR_EFER, msr);
> - }
> -#endif
> -
> /* A quick check from sanity claus */
> cpuid = PCPU_GET(cpuid);
> if (PCPU_GET(apic_id) != lapic_id()) {
> @@ -1528,6 +1518,7 @@
> } else {
> npxresume(&susppcbs[cpu]->sp_fpususpend);
> pmap_init_pat();
> + initializecpu();
> PCPU_SET(switchtime, 0);
> PCPU_SET(switchticks, ticks);
>
> --- //depot/vendor/freebsd/src/sys/i386/include/md_var.h
> +++ //depot/user/jhb/acpipci/i386/include/md_var.h
> @@ -99,14 +99,9 @@
> void dump_add_page(vm_paddr_t);
> void dump_drop_page(vm_paddr_t);
> void finishidentcpu(void);
> -#if defined(I586_CPU) && defined(CPU_WT_ALLOC)
> -void enable_K5_wt_alloc(void);
> -void enable_K6_wt_alloc(void);
> -void enable_K6_2_wt_alloc(void);
> -#endif
> -void enable_sse(void);
> void fillw(int /*u_short*/ pat, void *base, size_t cnt);
> void initializecpu(void);
> +void initializecpucache(void);
> void i686_pagezero(void *addr);
> void sse2_pagezero(void *addr);
> void init_AMD_Elan_sc520(void);
> @@ -114,6 +109,7 @@
> int isa_nmi(int cd);
> vm_paddr_t kvtop(void *addr);
> void panicifcpuunsupported(void);
> +void ppro_reenable_apic(void);
> void printcpuinfo(void);
> void setidt(int idx, alias_for_inthand_t *func, int typ, int dpl, int
> selec);
> int user_dbreg_trap(void);
> --- //depot/vendor/freebsd/src/sys/i386/xen/mp_machdep.c
> +++ //depot/user/jhb/acpipci/i386/xen/mp_machdep.c
> @@ -602,17 +602,8 @@
> npxinit();
> #if 0
>
> - /* set up SSE registers */
> - enable_sse();
> -#endif
> -#if 0 && defined(PAE)
> - /* Enable the PTE no-execute bit. */
> - if ((amd_feature & AMDID_NX) != 0) {
> - uint64_t msr;
> -
> - msr = rdmsr(MSR_EFER) | EFER_NXE;
> - wrmsr(MSR_EFER, msr);
> - }
> + /* set up SSE/NX registers */
I suggest removing 'registers' from the comment above.
> + initializecpu();
> #endif
> #if 0
> /* A quick check from sanity claus */
> --- //depot/vendor/freebsd/src/sys/x86/x86/identcpu.c
> +++ //depot/user/jhb/acpipci/x86/x86/identcpu.c
> @@ -405,30 +405,11 @@
> break;
> case 0x5a0:
> strcat(cpu_model, "Geode LX");
> - /*
> - * Make sure the TSC runs through suspension,
> - * otherwise we can't use it as timecounter
> - */
> - wrmsr(0x1900, rdmsr(0x1900) | 0x20ULL);
> break;
> default:
> strcat(cpu_model, "Unknown");
> break;
> }
> -#if defined(I586_CPU) && defined(CPU_WT_ALLOC)
> - if ((cpu_id & 0xf00) == 0x500) {
> - if (((cpu_id & 0x0f0) > 0)
> - && ((cpu_id & 0x0f0) < 0x60)
> - && ((cpu_id & 0x00f) > 3))
> - enable_K5_wt_alloc();
> - else if (((cpu_id & 0x0f0) > 0x80)
> - || (((cpu_id & 0x0f0) == 0x80)
> - && (cpu_id & 0x00f) > 0x07))
> - enable_K6_2_wt_alloc();
> - else if ((cpu_id & 0x0f0) > 0x50)
> - enable_K6_wt_alloc();
> - }
> -#endif
> #else
> if ((cpu_id & 0xf00) == 0xf00)
> strcat(cpu_model, "AMD64 Processor");
> --- //depot/vendor/freebsd/src/sys/x86/x86/local_apic.c
> +++ //depot/user/jhb/acpipci/x86/x86/local_apic.c
> @@ -55,7 +55,6 @@
> #include <vm/pmap.h>
>
> #include <x86/apicreg.h>
> -#include <machine/cpu.h>
> #include <machine/cputypes.h>
> #include <machine/frame.h>
> #include <machine/intr_machdep.h>
> @@ -1331,9 +1330,6 @@
> apic_init(void *dummy __unused)
> {
> struct apic_enumerator *enumerator;
> -#ifndef __amd64__
> - uint64_t apic_base;
> -#endif
> int retval, best;
>
> /* We only support built in local APICs. */
> @@ -1375,12 +1371,7 @@
> * CPUs during early startup. We need to turn the local APIC back
> * on on such CPUs now.
> */
> - if (cpu == CPU_686 && cpu_vendor_id == CPU_VENDOR_INTEL &&
> - (cpu_id & 0xff0) == 0x610) {
> - apic_base = rdmsr(MSR_APICBASE);
> - apic_base |= APICBASE_ENABLED;
> - wrmsr(MSR_APICBASE, apic_base);
> - }
> + ppro_reenable_apic();
> #endif
>
> /* Probe the CPU's in the system. */
>
> --
> John Baldwin
pgpNVnFqQE1jt.pgp
Description: PGP signature
