Re: [Qemu-devel] [PATCH] kvm: Move kvm_allows_irq0_override() to target-i386

[Peter Maydell]

On 21 July 2012 10:14, Jan Kiszka <address@hidden> wrote:
> On 2012-07-21 10:54, Peter Maydell wrote:
>> On 21 July 2012 07:57, Jan Kiszka <address@hidden> wrote:
>>> On 2012-07-20 21:14, Peter Maydell wrote:
>>>> I'm sure this isn't the only x86ism in the KVM generic source
>>>> files. However the thing I'm specifically trying to do is
>>>> nuke all the uses of kvm_irqchip_in_kernel() in common code,
>>>
>>> No, "irqchip in kernel" is supposed to be a generic concept. We will
>>> also have it on Power. Not sure what your plans are for ARM, maybe it
>>> will always be true there.
>>
>> I agree that "irqchip in kernel?" is generic (though as you'll see
>> below there's disagreement about what that ought to mean or imply).
>> "irq0_override" though seems to me to be absolutely x86 specific.
>
> Naming is x86 specific, semantic not. It means that KVM doesn't prevent
> remapping of IRQs. Granted, I really hope you don't make such mistakes
> in your arch.

What does "remapping of IRQs" mean here? This is still sounding
not very generic to me, in that I really don't know what it would
mean in an ARM context. The fact that the only caller of this is
in hw/pc.c is also a big red flag that this isn't exactly generic.

>>> That said, maybe there is room for discussion about what it means for
>>> the general KVM code and its users if the irqchip is in the kernel. Two
>>> things that should be common for every arch:
>>>  - VCPU idle management is done inside the kernel
>>
>> The trouble is that at the moment QEMU assumes that "is the
>> irqchip in kernel?" == "is VCPU idle management in kernel", for
>> instance. For ARM, VCPU idle management is in kernel whether
>> we're using the kernel's model of the VGIC or not. Alex tells
>> me PPC is the same way. It's only x86 that has tied these two
>> concepts together.
>
> Hmm, and why does Power work despite this mismatch?

I think because hw/ppc.c:ppc_set_irq() both calls cpu_interrupt()
and also kvmppc_set_interrupt(), so we end up with a sort of odd
mix of both models... Alex?

> If cpu_thread_is_idle doesn't work for you, define something like
> kvm_idle_in_kernel() to replace kvm_irqchip_in_kernel here.

Yes, this is kind of where I'm headed. I thought I'd start with this
patch as the easiest one first, though...

>> The reason I want to get rid of common-code uses of kvm_irqchip_in_kernel()
>> is because I think they're all similar to this -- the common code is
>> using the check as a proxy for something else, and it should be directly
>> asking about that something else. The only bits of code that should
>> care about "is the irqchip in kernel?" are:
>>  * target-specific device/machine setup code which needs to know
>>    which apic/etc to instantiate
>>  * target-specific x86 code which has this weird synchronous IRQ
>>    delivery model for irqchip-not-in-kernel
>> (Obviously I might have missed something, I'm flailing around
>> trying to understand this code :-))
>>
>>>  - in-kernel KVM helpers like vhost or VFIO can inject IRQs directly
>>>
>>> The latter point implies that irqfd is available and that interrupt
>>> routes from virtual IRQs (*) (like the one associated with an irqfd) to
>>> the in-kernel IRQ controller have to be established. That's pretty generic.
>>
>> But you can perfectly well have an in-kernel-irqchip that doesn't
>> support irqfd
>
> You could, thought this doesn't make much sense.

Why doesn't it make sense? On ARM, in-kernel-irqchip means you can take
advantage of the hardware support for a virtual GIC, and you can use
the virtual timer support too. These are both big performance advantages
even if QEMU never does anything with irqfds. (In fact the current
ARM KVM VGIC code doesn't support irqfds as far as I can see from
a quick scan of the kernel code.)

>> -- it just means that interrupts from devices have
>> to come in via the ioctls same as anything else. Some in-kernel
>> helpers obviously would depend on the existence and use of a full
>> featured in-kernel irqchip (on ARM you don't get the in kernel timer
>> unless you have in kernel VGIC), but I don't see why the virtio code
>> should be asking "is there an in kernel irqchip?" rather than "can
>> I do irqfd routing?" or whatever the question is it actually wants
>> to ask.  (In fact the virtio code probably needs to do something
>> more complex anyway: you could perfectly well have a system where
>> there is a full-featured irqchip in the kernel but the virtio
>> device is on the "wrong" side of a second interrupt controller
>> which is not in-kernel. So the actual question it needs to ask
>> is "does the interrupt wiring in this specific machine model mean
>> I can get and use an irqfd from where I am to the main CPU
>> interrupt controller?" or something similar.)
>
> Well, same here: then define more precise generic test functions.

First I have to understand what the current code is trying to do;
your explanations have been very helpful in that respect.

-- PMM