Re: In kernel PIC support: kernel patch

Dong, Eddie
2007-06-21T13:02:34Z

Gregory Haskins wrote:
> On Wed, 2007-06-20 at 15:43 +0800, Dong, Eddie wrote:
>> As we discussed, if we move APIC to kernel while leaving PIC in user
>> level, we have ABI level holes if the interrupt a user level qemu
>> injected is not immediately injected to guest for some reason. \
>
> Hi Eddie,
>
> I know you worked hard on this, but I still do not agree with this
> statement. I agree that v9 introduced an issue related to a)
> premature IRR->ISR assignment, and b) lack of support for clearing
> pending ISR with IMR changes (and thank you for pointing it out!).

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.

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

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

If you really think supporting mixed irqchip mode is a must, 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.

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

Thx,
Eddie