Re: In kernel PIC support: kernel patch

Gregory Haskins
2007-06-21T14:01:59Z

On Thu, 2007-06-21 at 13:31 +0000, Gregory Haskins wrote:
> On Thu, 2007-06-21 at 21:02 +0800, Dong, Eddie wrote:
> >
> > Achitectually not only that. A premature IRR->ISR will cause guest OS
> > confuse in many place. A guest (say BIOS) may turn from interrupt
> > enabled mode to polling mode which polls IRR to detect if
> > there is pending IRQ to handle. In this case we have trouble.
>
> I completely agree. But what I am saying is that I can eliminate the
> premature IRR->ISR with the change I am proposing.

After thinking about this some more, I think I finally understand your
point. I was thinking that you were objecting to the multiple unacked
vectors in ISR. But really this could be a problem for *any* unacked
vectors? Hmm..if this is the case, you were right and I was wrong. My
bad :)

So assuming this newly enlightened position is true, I think this means
we have a choice:

1) Drop support for mixed "level-1" mode and move the PIC to the kernel
now as Eddie is doing

2) Keep the level-1/2 distinction, and add support for making sure that
once a vector is acked in the PIC, it truely gets put into service
immediately.

I can think of a really simple interface for (2). All we really need to
do is

a) go back to synchronous injection (as previously suggested)
b) promote kvm_cpuirq_extint to be higher than nmis and localints.

This will ensure the vector is immediately placed into service even over
in-kernel sources. Deferred interrupts would still take a higher
priority, but we would never intack the pic if deferred interrupts were
pending since irq.pending would be true.

Thoughts?
-Greg

>
> >
> >
> > > However, as I mentioned we can fix that issue with just a few new
> > > lines of
> > > code and by
> > > reverting the userspace injection model to the one used in prior
> > > releases without having to implement an entire in-kernel PIC to do it.
> >
> > In-kernel PIC gives us a full in kernel irqchip solution and performance
> > gain. Some OSes may use PIC only.
>
> Agreed. I will answer this down below with the level-0/1/2 question
> since they are related.
>
>
> >
> > >
> > > I think moving the PIC into the kernel has the advantage of allowing
> > > us to move the PIT into the kernel too (which is huge, IMHO), but
> >
> > Not very big, I just want to do one by one. we have done the code
> > in Xen already.
> >
> > > that is its sole advantage. Don't get me wrong: I am in favor of
> > > doing it. However, I wanted to point out that this particular
> > > solution to the problem you found essentially is invalidating
> > > "level-1" mode by only supporting level-0 or level-2, not fixing it.
> >
> > Not exactly understand level-0/1/2 meaning? Do u mean we mixed
> > irqchip mode is a feature requirement? I didn't see the necessity,
> > maybe I am wrong. But isn't both PIC and APIC in kernel much
> > nature and simple for us?
>
> At one point there was a debate between moving just the LAPIC, or moving
> everything (LAPIC/PIC/IOAPIC/PIT). Avi suggested that we start with
> just the LAPIC and see where we were at. Because we need to remain
> forward and backwards ABI compatible, I added an ioctl for setting the
> in-kernel-pic level. "0" is backwards compatible mode (everything in
> userspace). "1" is LAPIC only. "2" is presumable everything in the
> kernel, but today only 0 and 1 are the only two defined so there could
> be more than 3 levels someday if we wanted.
>
> >
> > If you really think supporting mixed irqchip mode is a must
>
> I will leave it to Avi to decide since he implicitly suggested it. But
> suffice to say, if we *dont* want to support it we will need to get the
> other in-kernel stuff into the lapic branch in its entirety before it
> can be merged so we dont create an ABI issue.
>
>
> > , we
> > need to introduce an intack I/F to notify user level irqchip if the irq
> > is really ACKed or not. We can do that but I doubt the necessity.
>
> This is where we disagree, as I know you have mentioned this before.
> The solution I am proposing (which reverts the userspace injection
> method) would not require this ACK that you mention. The reason is that
> the synchronous nature of the injection takes care of things naturally.
> Here is how:
>
> With sync-injection, we can only move a vector from the IRR->ISR if
> RFLAGS.IF and !irq.pending. Once we inject, the vector will move to ISR
> and get queued in kernel (thereby setting an irq.pending bit). It may
> not inject to the guest immediately, but thats ok because no new vectors
> will be acked in the PIC until that previous vector is processed
> (because irq.pending will remain set). Therefore, we will never
> prematurely move IRR->ISR until the previous vector really is
> "in-service". Does this make sense?
>
> My suggestion assumes that a single vector can stay in ISR without
> actually being in service without ill effect (Todays v9 code could
> presumably have multiple vectors in ISR without being in service, which
> I admit is bogus). If this assumption is false, then I agree with you
> that we need extra intack I/F. What are you thoughts here?
>
>
> >
> > > Perhaps we are "ok" with that, but I was under the distinct
> > > impression from Avi et. al. that these variable levels of support
> > > were a design goal for debugging purposes.
> > >
> > > I would prefer that we just fix level-1 mode with the changes I
> > > mentioned instead of just disabling it (in addition to adding level-2
> > > mode as Eddie is working on).
> > >
> > >>
> > >> This patch fixes this by introducing new VM level IOCTL
> > >> KVM_SET_ISA_IRQ_LEVEL & KVM_CREATE_PIC to avoid the ABI hole. The
> > >> original KVM_INTERRUPT is still there to be backward compatible.
> > >>
> > >> WIth this patch, old Qemu, new qemu (after this patch), old kvm, new
> > >> kvm can work in any combination. Both user level code and kernel
> > >> code will automatically decide the irq source base on irqchip
> > >> location (user or kernel).
> > >>
> > >> Some known issues (TODO):
> > >> 1: SVM support is on going.
> > >> The code for VMX to inject irq is same with Xen now since the
> > >> situation is same now (irqchip in hyprevisor), SVM code should be
> > >> able to directly reuse from Xen too.
> > >> 2: live migration break.
> > >> 3: Temply APIC support is removed in CPUID to wait for the merge
> > >> of in kernel APIC patch
> > >
> > > Note that you will need more than just the APIC patch. My patch only
> > > moves the LAPIC down. The IOAPIC and 8259s are still in userspace.
> > > Your patch only moves the 8259s down. This means there is a gap where
> > > the IOAPIC used to be that still needs to be addressed.
> >
> > Why do u think I/O APIC must be moved down too? A new IOCTL like
> > KVM_IOAPIC_MESSAGE can solve the interface issue IMO and quit
> > natual.
>
> I suppose, but it somewhat defeats the purpose IMO. Every pin in the
> 8259 that gets tickled implicitly means an IOAPIC pin was tickled also.
> Do we really want to go to userspace for that? Essentially what happens
> then is the PIT ends up having to go through userspace for every tick at
> that point. On the flip side, the IOAPIC model is very simple so I
> think it makes more sense to move it one to one with the PIC.
>
> Regards,
> -Greg