On Wed, Jun 29, 2022 at 10:34:53PM -0400, George Koehler wrote:
> Hi. I have a question about splx, below.
>
> On Thu, 23 Jun 2022 21:58:48 -0500
> Scott Cheloha <scottchel...@gmail.com> wrote:
>
> > My machine uses openpic(4). I would appreciate tests on a
> > non-openpic(4) Mac, though all tests are appreciated.
>
> We only run on New World Macs, and the only ones without openpic(4)
> might be the oldest models of iMac G3 from 1998; these would attach
> macintr0 and not openpic0 in dmesg. I don't know anyone who might
> have such an iMac. The iMac model PowerMac2,1 from 1999 (with the
> (slot-loading cd drive) does have openpic(4).
If it's imperative we test it on a non-openpic(4) machine I might be
able to scrounge one on craigslist.
... they can't be completely extinct, right?
> > Index: macppc/dev/openpic.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/macppc/dev/openpic.c,v
> > retrieving revision 1.89
> > diff -u -p -r1.89 openpic.c
> > --- macppc/dev/openpic.c 21 Feb 2022 10:38:50 -0000 1.89
> > +++ macppc/dev/openpic.c 24 Jun 2022 02:49:59 -0000
> > @@ -382,6 +382,10 @@ openpic_splx(int newcpl)
> >
> > intr = ppc_intr_disable();
> > openpic_setipl(newcpl);
> > + if (ci->ci_dec_deferred && newcpl < IPL_CLOCK) {
> > + ppc_mtdec(0);
> > + ppc_mtdec(UINT32_MAX); /* raise DEC exception */
> > + }
> > if (newcpl < IPL_SOFTTTY && (ci->ci_ipending & ppc_smask[newcpl])) {
> > s = splsofttty();
> > dosoftint(newcpl);
>
> The 2nd mtdec tries to raise dec_intr by changing bit 1 << 31 of the
> decrementer register from 0 to 1.
Yes, exactly. My read of PowerPC 2.01 is that the DEC exception
is raised when the DEC's MSB goes from 0 to 1.
> I suspect if the decrementer can
> also decrement itself from 0 to UINT32_MAX, and raise dec_intr early,
> before we reach the 2nd mtdec. This would be bad, because this
> ppc_mtdec(UINT32_MAX) would override the ppc_mtdec(nextevent - tb) in
> dec_intr and lose the next scheduled clock interrupt.
To be perfectly clear, you are concerned about this scenario:
> > + if (ci->ci_dec_deferred && newcpl < IPL_CLOCK) {
> > + ppc_mtdec(0);
/* DEC interrupt fires *here*. */
/* We jump to decrint() and then call decr_intr(). */
> > + ppc_mtdec(UINT32_MAX); /* raise DEC exception */
> > + }
I think it's possible for the DEC exception to occur in that spot.
However, external/DEC *interrupts* are explicitly disabled, so I don't
think that we will jump to decrint() until the next time we do
ppc_intr_enable(1);
That first happens in dosoftint(). If we don't call dosoftint(), it
may happen at the end of splx(), provided that interrupts weren't
already disabled when we called splx().
> Testing might miss this problem. For example, a randomly reordered
> kernel might place the 2 mtdec instructions in different pages, which
> has a small chance of a page fault on a Mac G5.
>
> Would this be better?
>
> ppc_mtdec(1 >> UINT32_MAX);
> ppc_mtdec(UINT32_MAX);
I assume you meant to type
ppc_mtdec(UINT32_MAX >> 1);
I will tweak the code and try this out. My reading of PowerPC 2.01
suggests that this will do the job just fine.
But again, I'm unsure whether we need this. External and DEC
interrupts should be masked when we run this code unless I'm
misunderstanding what ppc_intr_disable() actually does.
