Re: Interrupt codepath review

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Thu, Nov 22, 2018 at 12:34:34PM +0100, Manuel Bouyer wrote:
> On Thu, Nov 22, 2018 at 09:16:41AM +0530, Cherry G.Mathew wrote:
> > Cherry G. Mathew <cherry%zyx.in@localhost> writes:
> > 
> > > Hello,
> > >
> > > I'm wondering if there'd be any objections to the approach I'm taking in
> > > the attached patch. This will take us closer to unifying the interrupt
> > > path with native.
> > >
> > > A rationale is included in this discussion:
> > > http://mail-index.netbsd.org/tech-kern/2018/11/18/msg024198.html
> > 
> > Hello,
> > 
> > Here's an updated patch. Taylor asked me on ICB to come up with a
> > diagram - I'm a little short of time (and energy) for that, but if
> > needed, I will spend some time on it. Until then, here's a brief
> > description:
> > 
> > The motivation here is that on XEN we want to re-use the native
> > interrupt codepath, via vector.S
> > 
> > Since XEN's interrupt/event entry is via a named callback function, we
> > need a way to hook into the vector.S codepath in a way that closely
> > mimics native.
> > 
> > The obvious candidate here was spllower(9) since it sets up the stack
> > magic for us to emulate an architectural interrupt event, before
> > (conditionally) passing control to vector.S interrupts stubs.
> > 
> > Since spllower(9) also manages deferred interrupts , and on XEN the
> > callback essentially does exactly the same thing (with slightly
> > different semantics), I thought it was a good match to abuse spllower(9)
> > for this purpose.
> > 
> > So I decided to try the following:
> > 
> > Define a new super high priority IPL - IPL_HYPERVISOR
> > 
> > No interrupts are to be registered using this IPL. Its purpose is to
> > raise the current IPL, so that spllower() can then be used to crank
> > through all the remaining lower IPLs , starting with IPL_HIGH
> > 
> > So in essense, the Xen callback handler will set the ci_ipending bit
> > corresponding to the pending IPLs of pending XEN events, and set the
> > current cpu IPL to IPL_HYPERVISOR using splraise(IPL_HYPERVISOR).
> > 
> > Then it will iterate through all IPLs to ensure that all ci_ipending
> > bits are processed by the spl.S and vector.S entry code - and that all
> > registered and corresponding OS interrupt handlers are called.
> > 
> > At the end, we reset the spl to the original value.
> > 
> > I tested this overnight on amd64 DOMU. It held up to the usual atf tests
> > and build distribution.
> > 
> > Please let me know if you have any comments. I am planning to use this
> > set of patches to inch towards normalising the rest of the intr.c code
> > towards native. I've posted separate patches for that earlier.
> 
> In your patch the debug event is not handled any more, I'd like to keep
> it (it has been usefull in the past). The debug handler should be called right
> away, without ipl considerations.
> 
> It seems that the evtlock[] locks are not taken any more on interrupt
> processing. This will be a problem, especially on dom0 where handlers are
> added/removed quite often. Xspllower doesn't take it actually, and this is not
> a problem because Xspllower() disable interrpts before processing pending
> IPLs, but if we do this with interrupts enabled (and reentrancy),
> this will be an issue.

Also, testing with a domU is not enough. You need to test dom0 too
(dom0 has xenevt).

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--