From owner-svn-src-all@FreeBSD.ORG Tue Sep 9 08:58:20 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A2185700; Tue, 9 Sep 2014 08:58:20 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D92D56EF; Tue, 9 Sep 2014 08:58:19 +0000 (UTC) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s898wDTa079223 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 9 Sep 2014 11:58:13 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s898wDTa079223 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s898wDVG079222; Tue, 9 Sep 2014 11:58:13 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 9 Sep 2014 11:58:13 +0300 From: Konstantin Belousov To: John Baldwin Subject: Re: svn commit: r270850 - in head/sys: i386/i386 i386/include i386/isa x86/acpica Message-ID: <20140909085813.GC2737@kib.kiev.ua> References: <201408301748.s7UHmc6H059701@svn.freebsd.org> <20140905084305.GN2737@kib.kiev.ua> <201409051044.05853.jhb@freebsd.org> <1432043.pIBGVXe1sj@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="fsTlqAwC4TOVu2P/" Content-Disposition: inline In-Reply-To: <1432043.pIBGVXe1sj@ralph.baldwin.cx> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Sep 2014 08:58:20 -0000 --fsTlqAwC4TOVu2P/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > I can test that. >=20 > 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 am= d64 > 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 syste= ms). > We could either save and restore those various vendor-specific MSRs and= =20 > registers in suspend/resume, or just call initializecpu() to set their va= lues. =20 > The latter approach seems simpler, so I chose that route. In immediate t= erms,=20 > it should fix enabling PG_NX in MSR_EFER on i386 + PAE resume, but it als= o=20 > makes the AP startup case simpler and closer to the amd64 code. I also m= oved=20 > some code that set MSRs out of printcpuinfo() in identcpu.c and into=20 > initializecpu() instead. Finally, I split initializecpucache() out simil= ar to=20 > how it is split on amd64. Ok. Two very minor comments below. >=20 > Ah, I see one bug I will fix in my tree, pc98's machdep.c needs the chang= e to > call initializecpucache(). >=20 > --- //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 */ > =20 > +#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); > } > =20 > +static int ppro_apic_used =3D -1; > + > static void > init_ppro(void) > { > @@ -535,9 +543,29 @@ > /* > * Local APIC should be disabled if it is not going to be used. > */ > - apicbase =3D rdmsr(MSR_APICBASE); > - apicbase &=3D ~APICBASE_ENABLED; > - wrmsr(MSR_APICBASE, apicbase); > + if (ppro_apic_used !=3D 1) { > + apicbase =3D rdmsr(MSR_APICBASE); > + apicbase &=3D ~APICBASE_ENABLED; > + wrmsr(MSR_APICBASE, apicbase); > + ppro_apic_used =3D 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 =3D=3D 0) { > + apicbase =3D rdmsr(MSR_APICBASE); > + apicbase |=3D APICBASE_ENABLED; > + wrmsr(MSR_APICBASE, apicbase); > + ppro_apic_used =3D 1; > + } > } > =20 > /* > @@ -646,20 +674,6 @@ > } > #endif > =20 > -/* > - * 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 =3D hw_instruction_sse =3D 1; > - } > -#endif > -} > - > extern int elf32_nxstack; > =20 > 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) =3D=3D 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) =3D=3D 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 =3D hw_instruction_sse =3D 1; > + } > +#endif > +} > + > +void > +initializecpucache(void) > +{ > =20 > /* > * CPUID with %eax =3D 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 @@ > } > } > =20 > -void > +static void > enable_K6_wt_alloc(void) > { > quad_t size; > @@ -945,7 +990,7 @@ > intr_restore(saveintr); > } > =20 > -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(); > =20 > /* 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(); > =20 > + /* set up SSE/NX registers */ > + initializecpu(); > + > /* set up FPU state on the AP */ > npxinit(); > =20 > - /* set up SSE registers */ > - enable_sse(); > - > if (cpu_ops.cpu_init) > cpu_ops.cpu_init(); > =20 > -#ifdef PAE > - /* Enable the PTE no-execute bit. */ > - if ((amd_feature & AMDID_NX) !=3D 0) { > - uint64_t msr; > - > - msr =3D rdmsr(MSR_EFER) | EFER_NXE; > - wrmsr(MSR_EFER, msr); > - } > -#endif > - > /* A quick check from sanity claus */ > cpuid =3D PCPU_GET(cpuid); > if (PCPU_GET(apic_id) !=3D lapic_id()) { > @@ -1528,6 +1518,7 @@ > } else { > npxresume(&susppcbs[cpu]->sp_fpususpend); > pmap_init_pat(); > + initializecpu(); > PCPU_SET(switchtime, 0); > PCPU_SET(switchticks, ticks); > =20 > --- //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=20 > 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 > =09 > - /* set up SSE registers */ > - enable_sse(); > -#endif > -#if 0 && defined(PAE) > - /* Enable the PTE no-execute bit. */ > - if ((amd_feature & AMDID_NX) !=3D 0) { > - uint64_t msr; > - > - msr =3D 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) =3D=3D 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) =3D=3D 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) =3D=3D 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 > =20 > #include > -#include > #include > #include > #include > @@ -1331,9 +1330,6 @@ > apic_init(void *dummy __unused) > { > struct apic_enumerator *enumerator; > -#ifndef __amd64__ > - uint64_t apic_base; > -#endif > int retval, best; > =20 > /* 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 =3D=3D CPU_686 && cpu_vendor_id =3D=3D CPU_VENDOR_INTEL && > - (cpu_id & 0xff0) =3D=3D 0x610) { > - apic_base =3D rdmsr(MSR_APICBASE); > - apic_base |=3D APICBASE_ENABLED; > - wrmsr(MSR_APICBASE, apic_base); > - } > + ppro_reenable_apic(); > #endif > =20 > /* Probe the CPU's in the system. */ >=20 > --=20 > John Baldwin --fsTlqAwC4TOVu2P/ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUDsElAAoJEJDCuSvBvK1BLb0P/A+X06NBZdnL8a4uM+0JqPCf heLPh3ThbxEc/MFK+oi5i7weAcRZWIAUt5MfwTOJaQWsIgdpBxWMpds8YCXANsEu oZ4EspJ0d53UUWHTogy2sAXsOudm7OR4dA/nrxdKnLg2nGeeaPNWQNmA8IuaDw4B 5wEJvPhM/zO+DjXUXBbakxd8xXNXmuPaPHpel0+BinJuc8jc+nueyvwYjBgccPq/ 0hmEyKs5x0we+hj2ccNS1yMj5tQm70hRDCHryWejUErFMkNFXKAIXoOtpNaJH8SO nPTSDeYBSSe69fV0ZYX5b6eWgtSYZd9d8mF5vwwXh8b2xHk1XgCl0DQsFF/lzrDH xNyz5U7bNmYsxqKZM5rp2FFWnbiWEOWq0nZSuweIarWUJ8jc40D008gZyS5olQY9 njugD/qBKZqZAVHB31pEa2Olfq1Jj/WcKeh3A+gGjsazemBeq2q8NZaQYCY4qliT vOo6WDclrcQdII1AKwWbGPvZbiXtYQZ/f0OkSa6WedF8NiVJcSIcE/mcW75Sh0eD IU7mAevI+C0ksQvdWdmNS4jBocoXWGJ+vMPacaUcA60JGenADlulMwO6kWr4pbZ2 ULEVqMcdOZa5AuSQuoHXUd5E6CT67jep7wM628GLi7shOIuquVGhtaxtZppzReRG J50GwcBt+NZiwNLtdrle =U/AS -----END PGP SIGNATURE----- --fsTlqAwC4TOVu2P/--