Date: Wed, 21 Sep 2016 21:15:42 +0300 From: "Jukka A. Ukkonen" <jau789@gmail.com> To: freebsd-ppc@freebsd.org, chmeeedalf@gmail.com Subject: Re: svn commit: r306065 - in head/sys vs. PowerMacs: Nathan's trail patch included but inappropriate? Message-ID: <4bb1046a-225d-66b2-7b00-067f0d6f6c60@gmail.com> In-Reply-To: <917EFF5A-D054-4424-9D7D-4E4BEF6072EF@gmail.com> References: <C48933C3-DB22-4D94-B22D-B51822197E4E@dsl-only.net> <917EFF5A-D054-4424-9D7D-4E4BEF6072EF@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 09/21/16 20:28, Justin Hibbits wrote: > On Sep 21, 2016, at 1:46 AM, Mark Millard wrote: > >> The following from >> >> https://lists.freebsd.org/pipermail/svn-src-head/2016-September/091934.html >> >> >> seems in include a patch that Nathan made for testing on >> PowerMac/iMac/Xserve G5's that failed the PowerMac7,3 test that was >> tried by Jukka A. Ukkonen. >> >>> Modified: head/sys/powerpc/ofw/ofw_machdep.c >>> ============================================================================== >>> >>> --- head/sys/powerpc/ofw/ofw_machdep.c Wed Sep 21 02:27:23 2016 >>> (r306064) >>> +++ head/sys/powerpc/ofw/ofw_machdep.c Wed Sep 21 02:28:39 2016 >>> (r306065) >>> @@ -99,6 +99,7 @@ ofw_restore_trap_vec(char *restore_trap_ >>> /* >>> * Saved SPRG0-3 from OpenFirmware. Will be restored prior to the >>> callback. >>> */ >>> +#ifndef __powerpc64__ >>> register_t ofw_sprg0_save; >>> >>> static __inline void >>> @@ -140,6 +141,8 @@ ofw_sprg_restore(void) >>> } >>> #endif >>> >>> +#endif >>> + >>> static int >>> parse_ofw_memory(phandle_t node, const char *prop, struct mem_region >>> *output) >>> { >>> @@ -344,11 +347,12 @@ OF_initial_setup(void *fdt_ptr, void *ju >>> ofmsr[0] = mfmsr(); >>> #ifdef __powerpc64__ >>> ofmsr[0] &= ~PSL_SF; >>> - #endif >>> + #else >>> __asm __volatile("mfsprg0 %0" : "=&r"(ofmsr[1])); >>> __asm __volatile("mfsprg1 %0" : "=&r"(ofmsr[2])); >>> __asm __volatile("mfsprg2 %0" : "=&r"(ofmsr[3])); >>> __asm __volatile("mfsprg3 %0" : "=&r"(ofmsr[4])); >>> + #endif >>> openfirmware_entry = openfirm; >>> >>> if (ofmsr[0] & PSL_DR) >>> @@ -440,7 +444,9 @@ openfirmware_core(void *args) >>> */ >>> oldmsr = intr_disable(); >>> >>> +#ifndef __powerpc64__ >>> ofw_sprg_prepare(); >>> +#endif >>> >>> /* Save trap vectors */ >>> ofw_save_trap_vec(save_trap_of); >>> @@ -463,7 +469,9 @@ openfirmware_core(void *args) >>> /* Restore trap vecotrs */ >>> ofw_restore_trap_vec(save_trap_of); >>> >>> +#ifndef __powerpc64__ >>> ofw_sprg_restore(); >>> +#endif >>> >>> intr_restore(oldmsr); >> >> Part of the ofw_sprg_prepare() and ofw_sprg_restore() activity is >> required for PowerMac G5's to boot: the save of the FreeBSD sprg0 >> value and its later restore back to sprg0. (Established by other >> testing after Nathan's patch was tried.) >> >> My guess is that this code was accidentally included in -r306065, not >> intentionally included. >> >> >> [There is the separate issue that avoiding restoring the Open Firmware >> sprg0 value before calling into Open Firmware (leaving the FreeBSD >> value in place) helps PowerMac G5's boot more reliably by allowing >> correct handling some SLB faults that sometimes occur.] >> >> === >> Mark Millard >> markmi at dsl-only.net >> > > Crap, I got extra stuff in my diff. Going to revert this part tonight, > until everything is known good. > > - Justin > _______________________________________________ > freebsd-ppc@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-ppc > To unsubscribe, send any mail to "freebsd-ppc-unsubscribe@freebsd.org" The least that will have to be changed to make the code boot on PowerMac G5 is to change the following ... __asm __volatile("mfsprg0 %0\n\t" "mtsprg0 %1\n\t" "mtsprg1 %2\n\t" "mtsprg2 %3\n\t" "mtsprg3 %4\n\t" : "=&r"(ofw_sprg0_save) : "r"(ofmsr[1]), "r"(ofmsr[2]), "r"(ofmsr[3]), "r"(ofmsr[4])); in function ofw_sprg_prepare() in sys/powerpc/ofw/ofw_machdep.c to this ... __asm __volatile("mfsprg0 %0\n\t" "mtsprg1 %1\n\t" "mtsprg2 %2\n\t" "mtsprg3 %3\n\t" : "=&r"(ofw_sprg0_save) : "r"(ofmsr[2]), "r"(ofmsr[3]), "r"(ofmsr[4])); This only removes "mtsprg0" and its corresponding parameter. Alternatively one could use the absolutely minimalistic ... __asm __volatile("mfsprg0 %0\n\t" : "=&r"(ofw_sprg0_save) : ); on a G5. I have been running exactly this since my testing sessions with no ill effects at all. The downside is that nobody has tested this minimalistic approach on any other ppc variant but my G5. I have had a pending plan to test the same approach on my 32-bit G4, but the task being somewhat time consuming it is still pending. ;-) Anyhow the old code will not boot on a G5 without a change, neither will it boot with Nathan's patch included. Releasing 11.0-stable in a state that will not actually boot on one of the supported platforms will not look awfully bright either. I guess this is an issue for the re team to decide. --jau
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4bb1046a-225d-66b2-7b00-067f0d6f6c60>