Date: Tue, 9 Sep 2014 11:58:13 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r270850 - in head/sys: i386/i386 i386/include i386/isa x86/acpica Message-ID: <20140909085813.GC2737@kib.kiev.ua> In-Reply-To: <1432043.pIBGVXe1sj@ralph.baldwin.cx> References: <201408301748.s7UHmc6H059701@svn.freebsd.org> <20140905084305.GN2737@kib.kiev.ua> <201409051044.05853.jhb@freebsd.org> <1432043.pIBGVXe1sj@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <vm/pmap.h>
> =20
> #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;
> =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/--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140909085813.GC2737>