MPC750 (iMac G3) and disabling interrupts: sync after mtsr required according to the manual (at least isync in places?)

[Mark Millard]

On 2019-Apr-22, at 15:27, Mark Millard <marklmi at yahoo.com> wrote:

> The below was prompted by observed iMac G3 behavior
> with modern FreeBSD, so MPC750 behavior.
> 
> First I list the most worrisome code that I noticed,
> before the more general background of which the
> below is an example.
> 
> For the 32-bit aim code:
> 
> CNAME(trapexit):        
> 
> /* Disable interrupts: */
>        mfmsr   %r3
>        andi.   %r3,%r3,~PSL_EE at l
>        mtmsr   %r3
> /* Test AST pending: */
>        lwz     %r5,FRAME_SRR1+8(%r1)
>        mtcr    %r5
>        bf      17,1f                   /* branch if PSL_PR is false */
> 
>        GET_CPUINFO(%r3)                /* get per-CPU pointer */
> 
> Note: the 64-bit code has an isync before the
> "Test AST pending" code. (No sync.)

So far the following seems to have changed the MPC750 based
iMac G3 from getting a mix of Machine Checks vs. Data
Storage Interrupts for the same places in the code,
to getting just Data Storage Interrupts for those places:

Index: /usr/src/sys/powerpc/aim/trap_subr32.S
===================================================================
--- /usr/src/sys/powerpc/aim/trap_subr32.S	(revision 345758)
+++ /usr/src/sys/powerpc/aim/trap_subr32.S	(working copy)
@@ -68,7 +68,7 @@
 	lwzu	sr,PM_SR(pmap); \
 	RESTORE_SRS(pmap,sr) \
 	/* Restore SR 12 */ \
-	lwz	sr,12*4(pmap);	mtsr	12,sr
+	lwz	sr,12*4(pmap);	mtsr	12,sr; isync
 
 /*
  * Kernel SRs are loaded directly from kernel_pmap_
@@ -799,6 +799,7 @@
 	mfmsr	%r3
 	andi.	%r3,%r3,~PSL_EE at l
 	mtmsr	%r3
+	isync
 /* Test AST pending: */
 	lwz	%r5,FRAME_SRR1+8(%r1)
 	mtcr	%r5

The MPC750 requires isync after mtsr (or mtsrin) and
the one for "12,sr" is off by itself instead of being
in the middle of the main sequence of mtsr's (which
have a following isync).

[While the above may be necessary, it is far from
sufficient for making the MPC750 operate with active,
competing processes/threads. (boot -s operates
longer if one keeps things simple.) But I did not
intend for the list-submittal to be for getting
the MPC750 context working well overall.]

> The below gets into why that 32-bit code (and
> possibly more) is a worry: lack of sync (or at
> least isync) after using mtmsr to disable
> interrupts.
> 
> 
> https://www.nxp.com/docs/en/reference-manual/MPC750UM.pdf
> reports that a sync after the mtmsr is required for disabling
> interrupts fully before whatever follows. (The example also
> disables ME, FE0, and FE1, intended to be used before explicitly
> loading the cache in contexts where all 4 forms of exceptions
> are a worry.)
> 
> It also has wording that mtmsr "does not ensure subsequent
> instructions execute in the newly established environment".
> 
> A similar example is made relative to sync after setting MSR[IR]
> and/or MSR[DR]. It mentions isync instead of sync relative to
> MSR[PR] via mtmsr. I'll ignore MSR[PR] below.
> 
> (The MPC750 seems to have just one sync instruction with
> one official encoding, no lwsync or other such variants
> with alternate field values in the encoding.)
> 
> 
> As for the code generally . . .
> 
> powerpc64 and 32-bit powerpc FreeBSD currently have:
> 
> static __inline register_t
> intr_disable(void)
> {
>        register_t msr;
> 
>        msr = mfmsr();
>        mtmsr(msr & ~PSL_EE);
>        return (msr);
> }
> 
> where:
> 
> static __inline void
> mtmsr(register_t value)
> {
> 
>        __asm __volatile ("mtmsr %0; isync" :: "r"(value));
> }
> 
> (So there is an implicit isync in mtmsr(...) use.)
> 
> There are lots of places using PSL_EE to disable interrupts
> without calling intr_disable --but they still use mtmsr(...)
> [other than assembler contexts].
> 
> For things that are involved on the MPC750, I'm mostly
> worried about examples that have neither sync nor isync,
> secondarily about not having sync. (I note some
> other points as well.)
> 
> 
> Relative to PDL_EE:
> 
> powerpc_init has a disable with no sync.
> cpu_est_clockrate seem to use a DELAY(1000) for
> the MPC750 (and some others). But other settings
> are also involved. cpu_dep_bootstrap has no sync.
> (Not that cpu_dep_bootstrap is likely to be used
> with MPC750s, certainly not with iMac G3s.)
> 
> aim's trapexit for 32-bit has no sync or isync
> for its disable but 64-bit has just isync. 32-bit
> has just isync for the enable. FRAME_LEAVE uses
> only isync (32-bit and 64-bit). breakpoint (32-bit
> and 64-bit) uses only isync. (Assembler contexts.)
> 
> write_scom uses isync. read_scom uses isync. pcr_set
> uses isync. (So double isync's(?), one from mtmsr(...)
> use?)
> 
> flush_disable_caches uses a following powerpc_sync();
> isync() sequence. (Again a double isync(?): isync;
> sync; isync; sequence?)
> 
> 
> Relative to PSL_IR and PSL_DR:
> 
> aim_early_init has an example of no use of sync
> when ofwentry==0. aim_cpu_init has an example of no use
> of sync.  moea_bootstrap has 2 examples of no use of
> sync.
> 
> FRAME_SETUP uses just isync. FRAME_LEAVE uses just isync
> (but also has a PSL_ME involved). Both 32-bit and 64-bit
> for those. cpu_wakeup_handler (64-bit) uses just isync.
> (Assembler contexts.)
> 
> flush_disable_caches has sync and isync after a PDL_DR
> use. (Again a double isync(?): isync; sync; isync;
> sequence?) [PSL_DR is mixed with PSL_EE. as I remember.]
> 



===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)