Re: Interrupt codepath review

[Cherry G.Mathew <cherry%zyx.in@localhost>]

Manuel Bouyer <bouyer%antioche.eu.org@localhost> writes:

> 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.
>> 


Hi Manuel,

Thanks for the very useful comments.

>> 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.
>>

There seem to be two debug related events.

One in do_hypervisor_callback():
#ifdef EARLY_DEBUG_EVENT
        if (xen_atomic_test_bit(&s->evtchn_pending[0], debug_port)) {
               xen_debug_handler(NULL);
               xen_atomic_clear_bit(&s->evtchn_pending[0],
                    debug_port);
        }
#endif

And another with port == PORT_DEBUG.

The first one seems to be a debug "VIRQ" - it is handled in the way you
describe. The second one seems to be a fixed value (port number 4),
which is handled the same way other ports would be.

Are you referring to this second one ?

>> 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).

I'm loading the dom0 - so far so good. I will relook the locking issue
and get back with updates.


-- 
~cherry