> Date: Sun, 29 Mar 2020 19:59:02 -0400
> From: George Koehler <kern...@gmail.com>
>
> Here is a new diff for macppc's ofw_stack() problem, without using
> __attribute__((noinline)). I use this diff to build and run a macppc
> kernel with clang. It also works with gcc.
>
> The kernel did 3 steps to prepare an Open Firmware call:
> 1. turn off interrupts (EE and RI in msr)
> 2. move the stack pointer %r1 to firmstk
> 3. switch to Open Firmware's pmap?
>
> I don't understand these steps, but I tried to preserve all 3 steps as
> I shuffled the code. The diff doesn't touch step 3.
>
> The problem was at step 2: ofw_stack() copied the caller's stack frame
> to firmstk, and changed the caller's return address to ofw_back (which
> will restore the old %r1 and msr). If clang inlines the caller into
> another function, then ofw_back would run too late. I move step 2
> into openfirmware(), so there is no more copying a stack frame nor
> hijacking a return address. (I claim that firmstk+NBPG-16 is a
> multiple of 16; that mtctr,bctrl is more idiomatic than mtlr,blrl.)
>
> ofw_stack() becomes s = ofw_msr() and only does step 1, turning off EE
> and RI in msr. ppc_mtmsr(s) restores the saved msr. I don't use
> intr_disable() because it turns off only EE, not RI. I changed
> OF_call_method*() to turn off EE (external interrupts) before they
> touch their static args. Some functions, like OF_boot() and
> OF_quiesce(), seem unused, so I can't know if my changes are correct.
>
> To build a kernel with clang, I do
> # make CC=clang COMPILER_VERSION=clang
>
> Is this OK to commit? Would it be better to use intr_disable() in
> OF_*() and turn off RI in ofwreal.S fwentry?
I think we could actually do everything in openfirm(). But lets worry
about that later. This works and gets us moving forward.
ok kettenis@
P.S. Userland seems to be in good shape as well. I built and rebuilt
the world with clang. That was on a kernel built with gcc, so
I'm repeating that now with a kernel built with clang.
> Index: ofw_machdep.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/macppc/macppc/ofw_machdep.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 ofw_machdep.h
> --- ofw_machdep.h 7 Apr 2015 14:36:34 -0000 1.9
> +++ ofw_machdep.h 29 Mar 2020 16:16:27 -0000
> @@ -26,6 +26,9 @@
> *
> */
>
> +#include <machine/cpu.h>
> +#include <machine/psl.h>
> +
> extern int cons_backlight_available;
>
> void ofwconprobe(void);
> @@ -49,3 +52,12 @@ void of_setbrightness(int);
> void of_setcolors(const uint8_t *, unsigned int, unsigned int);
>
> void OF_quiesce(void);
> +
> +static inline uint32_t
> +ofw_msr(void)
> +{
> + uint32_t s = ppc_mfmsr();
> +
> + ppc_mtmsr(s & ~(PSL_EE|PSL_RI)); /* turn off interrupts */
> + return s;
> +}
> Index: ofwreal.S
> ===================================================================
> RCS file: /cvs/src/sys/arch/macppc/macppc/ofwreal.S,v
> retrieving revision 1.5
> diff -u -p -r1.5 ofwreal.S
> --- ofwreal.S 3 Sep 2019 14:37:22 -0000 1.5
> +++ ofwreal.S 29 Mar 2020 16:16:27 -0000
> @@ -355,96 +355,32 @@ _ENTRY(_C_LABEL(fwentry))
> addi %r1,%r1,16
> blr
>
> +.lcomm firmstk,NBPG,16
> +.comm _C_LABEL(OF_buf),NBPG
> +
> /*
> * OpenFirmware entry point
> + *
> + * Note: caller has to set the machine state register (msr)
> + * to be correct for OpenFirmware.
> */
> _ENTRY(_C_LABEL(openfirmware))
> - stwu %r1,-16(%r1)
> - mflr %r0 /* save return address */
> - stw %r0,20(%r1)
> + mflr %r0
> + stw %r0,4(%r1) /* save return address */
> +
> + /* switch to OpenFirmware real mode stack */
> + lis %r7,firmstk+NBPG-16@ha
> + addi %r7,%r7,firmstk+NBPG-16@l
> + stw %r1,0(%r7)
> + mr %r1,%r7
>
> lis %r4,fwcall@ha
> lwz %r4,fwcall@l(%r4)
>
> - mtlr %r4
> - blrl
> -
> - lwz %r0,20(%r1)
> - mtlr %r0
> - lwz %r1,0(%r1)
> - blr
> -
> -/*
> - * Switch to/from OpenFirmware real mode stack
> - *
> - * Note: has to be called as the very first thing in OpenFirmware interface
> routines.
> - * E.g.:
> - * int
> - * OF_xxx(arg1, arg2)
> - * type arg1, arg2;
> - * {
> - * static struct {
> - * char *name;
> - * int nargs;
> - * int nreturns;
> - * char *method;
> - * int arg1;
> - * int arg2;
> - * int ret;
> - * } args = {
> - * "xxx",
> - * 2,
> - * 1,
> - * };
> - *
> - * ofw_stack();
> - * args.arg1 = arg1;
> - * args.arg2 = arg2;
> - * if (openfirmware(&args) < 0)
> - * return -1;
> - * return args.ret;
> - * }
> - */
> -.lcomm firmstk,NBPG,16
> -.comm _C_LABEL(OF_buf),NBPG
> -
> -_ENTRY(_C_LABEL(ofw_stack))
> - mfmsr %r8 /* turn off interrupts */
> - andi. %r0,%r8,~(PSL_EE|PSL_RI)@l
> - mtmsr %r0
> - stw %r8,4(%r1) /* abuse return address slot */
> -
> - lwz %r5,0(%r1) /* get length of stack frame */
> - subf %r5,%r1,%r5
> -
> - lis %r7,firmstk+NBPG-8@ha
> - addi %r7,%r7,firmstk+NBPG-8@l
> - li %r6,0xf
> - andc %r7,%r7,%r6
> - lis %r6,ofw_back@ha
> - addi %r6,%r6,ofw_back@l
> - subf %r4,%r5,%r7 /* make room for stack frame on new
> stack */
> - stwu %r1,-16(%r7)
> - stw %r6,4(%r7) /* setup return pointer */
> + mtctr %r4
> + bctrl
>
> - stw %r7,-16(%r4)
> -
> - addi %r3,%r1,%r8
> - addi %r1,%r4,-16
> - subi %r5,%r5,%r8
> - subi %r4,%r4,%r8
> -
> - b _C_LABEL(ofbcopy) /* and copy it */
> -
> - .type ofw_back,@function
> -ofw_back:
> lwz %r1,0(%r1) /* get callers original stack pointer */
> -
> - lwz %r0,4(%r1) /* get saved msr from abused slot */
> - mtmsr %r0
> -
> - lwz %r1,0(%r1) /* return */
> lwz %r0,4(%r1)
> mtlr %r0
> blr
> -
> Index: opendev.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/macppc/macppc/opendev.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 opendev.c
> --- opendev.c 9 Mar 2006 23:06:19 -0000 1.9
> +++ opendev.c 29 Mar 2020 16:16:27 -0000
> @@ -34,11 +34,12 @@
> #include <sys/param.h>
> #include <sys/systm.h>
> #include <sys/stdarg.h>
> +#include <machine/cpu.h>
> #include <machine/psl.h>
>
> #include <dev/ofw/openfirm.h>
> +#include "ofw_machdep.h"
>
> -extern void ofw_stack(void);
> extern void ofbcopy(const void *, void *, size_t);
>
> int
> @@ -55,12 +56,17 @@ OF_instance_to_package(int ihandle)
> 1,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.ihandle = ihandle;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.phandle;
> + ret = -1;
> + else
> + ret = args.phandle;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -79,18 +85,24 @@ OF_package_to_path(int phandle, char *bu
> 3,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> if (buflen > PAGE_SIZE)
> return -1;
> + s = ofw_msr();
> args.phandle = phandle;
> args.buf = OF_buf;
> args.buflen = buflen;
> if (openfirmware(&args) < 0)
> - return -1;
> - if (args.length > 0)
> - ofbcopy(OF_buf, buf, args.length);
> - return args.length;
> + ret = -1;
> + else {
> + if (args.length > 0)
> + ofbcopy(OF_buf, buf, args.length);
> + ret = args.length;
> + }
> + ppc_mtmsr(s);
> + return ret;
> }
>
>
> @@ -110,10 +122,12 @@ OF_call_method(char *method, int ihandle
> 2,
> 1,
> };
> - int *ip, n;
> + uint32_t s;
> + int *ip, n, ret;
>
> if (nargs > 6)
> return -1;
> + s = ofw_msr();
> args.nargs = nargs + 2;
> args.nreturns = nreturns + 1;
> args.method = method;
> @@ -121,19 +135,19 @@ OF_call_method(char *method, int ihandle
> va_start(ap, nreturns);
> for (ip = args.args_n_results + (n = nargs); --n >= 0;)
> *--ip = va_arg(ap, int);
> - ofw_stack();
> - if (openfirmware(&args) == -1) {
> - va_end(ap);
> - return -1;
> - }
> - if (args.args_n_results[nargs]) {
> - va_end(ap);
> - return args.args_n_results[nargs];
> + if (openfirmware(&args) == -1)
> + ret = -1;
> + else if (args.args_n_results[nargs])
> + ret = args.args_n_results[nargs];
> + else {
> + for (ip = args.args_n_results + nargs + (n = args.nreturns);
> + --n > 0;)
> + *va_arg(ap, int *) = *--ip;
> + ret = 0;
> }
> - for (ip = args.args_n_results + nargs + (n = args.nreturns); --n > 0;)
> - *va_arg(ap, int *) = *--ip;
> va_end(ap);
> - return 0;
> + ppc_mtmsr(s);
> + return ret;
> }
> int
> OF_call_method_1(char *method, int ihandle, int nargs, ...)
> @@ -151,10 +165,12 @@ OF_call_method_1(char *method, int ihand
> 2,
> 2,
> };
> - int *ip, n;
> + uint32_t s;
> + int *ip, n, ret;
>
> if (nargs > 6)
> return -1;
> + s = ofw_msr();
> args.nargs = nargs + 2;
> args.method = method;
> args.ihandle = ihandle;
> @@ -162,12 +178,14 @@ OF_call_method_1(char *method, int ihand
> for (ip = args.args_n_results + (n = nargs); --n >= 0;)
> *--ip = va_arg(ap, int);
> va_end(ap);
> - ofw_stack();
> if (openfirmware(&args) == -1)
> - return -1;
> - if (args.args_n_results[nargs])
> - return -1;
> - return args.args_n_results[nargs + 1];
> + ret = -1;
> + else if (args.args_n_results[nargs])
> + ret = -1;
> + else
> + ret = args.args_n_results[nargs + 1];
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -184,16 +202,20 @@ OF_open(char *dname)
> 1,
> 1,
> };
> - int l;
> + uint32_t s;
> + int l, ret;
>
> - ofw_stack();
> if ((l = strlen(dname)) >= PAGE_SIZE)
> return -1;
> + s = ofw_msr();
> ofbcopy(dname, OF_buf, l + 1);
> args.dname = OF_buf;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.handle;
> + ret = -1;
> + else
> + ret = args.handle;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> void
> @@ -209,10 +231,12 @@ OF_close(int handle)
> 1,
> 0,
> };
> + uint32_t s;
>
> - ofw_stack();
> + s = ofw_msr();
> args.handle = handle;
> openfirmware(&args);
> + ppc_mtmsr(s);
> }
>
> /*
> @@ -234,28 +258,35 @@ OF_read(int handle, void *addr, int len)
> 3,
> 1,
> };
> - int l, act = 0;
> + uint32_t s;
> + int act = 0, l, ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.ihandle = handle;
> args.addr = OF_buf;
> for (; len > 0; len -= l, addr += l) {
> l = min(PAGE_SIZE, len);
> args.len = l;
> - if (openfirmware(&args) == -1)
> - return -1;
> + if (openfirmware(&args) == -1) {
> + ret = -1;
> + goto out;
> + }
> if (args.actual > 0) {
> ofbcopy(OF_buf, addr, args.actual);
> act += args.actual;
> }
> if (args.actual < l) {
> if (act)
> - return act;
> + ret = act;
> else
> - return args.actual;
> + ret = args.actual;
> + goto out;
> }
> }
> - return act;
> + ret = act;
> +out:
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -274,21 +305,27 @@ OF_write(int handle, void *addr, int len
> 3,
> 1,
> };
> - int l, act = 0;
> + uint32_t s;
> + int act = 0, l, ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.ihandle = handle;
> args.addr = OF_buf;
> for (; len > 0; len -= l, addr += l) {
> l = min(PAGE_SIZE, len);
> ofbcopy(addr, OF_buf, l);
> args.len = l;
> - if (openfirmware(&args) == -1)
> - return -1;
> + if (openfirmware(&args) == -1) {
> + ret = -1;
> + goto out;
> + }
> l = args.actual;
> act += l;
> }
> - return act;
> + ret = act;
> +out:
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -307,12 +344,17 @@ OF_seek(int handle, u_quad_t pos)
> 3,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.handle = handle;
> args.poshi = (int)(pos >> 32);
> args.poslo = (int)pos;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.status;
> + ret = -1;
> + else
> + ret = args.status;
> + ppc_mtmsr(s);
> + return ret;
> }
> Index: openfirm.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/macppc/macppc/openfirm.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 openfirm.c
> --- openfirm.c 3 Sep 2019 17:51:52 -0000 1.12
> +++ openfirm.c 29 Mar 2020 16:16:27 -0000
> @@ -34,11 +34,12 @@
> #include <sys/param.h>
> #include <sys/systm.h>
> #include <sys/stdarg.h>
> +#include <machine/cpu.h>
> #include <machine/psl.h>
>
> #include <dev/ofw/openfirm.h>
> +#include "ofw_machdep.h"
>
> -extern void ofw_stack(void);
> extern void ofbcopy(const void *, void *, size_t);
>
> int
> @@ -55,12 +56,17 @@ OF_peer(int phandle)
> 1,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.phandle = phandle;
> if (openfirmware(&args) == -1)
> - return 0;
> - return args.sibling;
> + ret = 0;
> + else
> + ret = args.sibling;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -77,12 +83,17 @@ OF_child(int phandle)
> 1,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.phandle = phandle;
> if (openfirmware(&args) == -1)
> - return 0;
> - return args.child;
> + ret = 0;
> + else
> + ret = args.child;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -99,12 +110,17 @@ OF_parent(int phandle)
> 1,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.phandle = phandle;
> if (openfirmware(&args) == -1)
> - return 0;
> - return args.parent;
> + ret = 0;
> + else
> + ret = args.parent;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -122,13 +138,18 @@ OF_getproplen(int handle, char *prop)
> 2,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.phandle = handle;
> args.prop = prop;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.size;
> + ret = -1;
> + else
> + ret = args.size;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -148,19 +169,25 @@ OF_getprop(int handle, char *prop, void
> 4,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> if (buflen > NBPG)
> return -1;
> + s = ofw_msr();
> args.phandle = handle;
> args.prop = prop;
> args.buf = OF_buf;
> args.buflen = buflen;
> if (openfirmware(&args) == -1)
> - return -1;
> - if (args.size > 0)
> - ofbcopy(OF_buf, buf, args.size);
> - return args.size;
> + ret = -1;
> + else {
> + if (args.size > 0)
> + ofbcopy(OF_buf, buf, args.size);
> + ret = args.size;
> + }
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -180,18 +207,23 @@ OF_setprop(int handle, char *prop, const
> 4,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> if (buflen > NBPG)
> return -1;
> + s = ofw_msr();
> args.phandle = handle;
> args.prop = prop;
> ofbcopy(buf, OF_buf, buflen);
> args.buf = OF_buf;
> args.buflen = buflen;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.size;
> + ret = -1;
> + else
> + ret = args.size;
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> @@ -210,22 +242,27 @@ OF_nextprop(int handle, char *prop, void
> 3,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.phandle = handle;
> args.prop = prop;
> args.buf = OF_buf;
> if (openfirmware(&args) == -1)
> - return -1;
> - strlcpy(nextprop, OF_buf, 32);
> - return args.flag;
> + ret = -1;
> + else {
> + strlcpy(nextprop, OF_buf, 32);
> + ret = args.flag;
> + }
> + ppc_mtmsr(s);
> + return ret;
> }
>
> int
> OF_interpret(char *cmd, int nreturns, ...)
> {
> va_list ap;
> - int i;
> static struct {
> char *name;
> int nargs;
> @@ -238,23 +275,29 @@ OF_interpret(char *cmd, int nreturns, ..
> 1,
> 2,
> };
> + uint32_t s;
> + int i, ret;
>
> - ofw_stack();
> if (nreturns > 8)
> return -1;
> if ((i = strlen(cmd)) >= NBPG)
> return -1;
> + s = ofw_msr();
> ofbcopy(cmd, OF_buf, i + 1);
> args.cmd = OF_buf;
> args.nargs = 1;
> args.nreturns = nreturns + 1;
> if (openfirmware(&args) == -1)
> - return -1;
> - va_start(ap, nreturns);
> - for (i = 0; i < nreturns; i++)
> - *va_arg(ap, int *) = args.results[i];
> - va_end(ap);
> - return args.status;
> + ret = -1;
> + else {
> + va_start(ap, nreturns);
> + for (i = 0; i < nreturns; i++)
> + *va_arg(ap, int *) = args.results[i];
> + va_end(ap);
> + ret = args.status;
> + }
> + ppc_mtmsr(s);
> + return ret;
> }
>
>
> @@ -272,12 +315,17 @@ OF_finddevice(char *name)
> 1,
> 1,
> };
> + uint32_t s;
> + int ret;
>
> - ofw_stack();
> + s = ofw_msr();
> args.device = name;
> if (openfirmware(&args) == -1)
> - return -1;
> - return args.phandle;
> + ret = -1;
> + else
> + ret = args.phandle;
> + ppc_mtmsr(s);
> + return ret;
> }
> static void OF_rboot(char *bootspec);
>
> @@ -293,12 +341,14 @@ OF_rboot(char *bootspec)
> 0,
> 0,
> };
> + uint32_t s;
> int l;
>
> if ((l = strlen(bootspec)) >= NBPG)
> panic("OF_boot");
> - ofw_stack();
> + s = ofw_msr();
> openfirmware(&args);
> + ppc_mtmsr(s);
> /* will attempt exit in OF_boot */
> }
>
> @@ -325,7 +375,7 @@ OF_exit(void)
> 0,
> };
>
> - ofw_stack();
> + ofw_msr();
> openfirmware(&args);
> panic ("OF_exit returned!"); /* just in case */
> while (1);
> @@ -343,9 +393,11 @@ OF_quiesce(void)
> 0,
> 0,
> };
> + uint32_t s;
>
> - ofw_stack();
> + s = ofw_msr();
> openfirmware(&args);
> + ppc_mtmsr(s);
> }
>
> /* XXX What is the reason to have this instead of bcopy/memcpy? */
>
