Re: Interrupt codepath review

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

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.

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