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

[Jan Kiszka]

On 2012-07-23 17:19, Peter Maydell wrote:
> On 23 July 2012 13:26, Avi Kivity <address@hidden> wrote:
>> On 07/21/2012 11:54 AM, Peter Maydell wrote:
>>> 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 :-))
>>
>> Agree naming should be improved.  In fact the early series I pushed to
>> decompose local apic, ioapic, and pic, but that didn't happen.  If it
>> did we'd probably not have this conversation.
> 
> OK, let's see if we can get some agreement about naming here.
> 
> First, some test-functions I think we definitely need:
> 
>  kvm_interrupts_are_async()
>    -- true if interrupt delivery is asynchronous
>       default false in kvm_init, set true in kvm_irqchip_create,
>       architectures may set it true in kvm_arch_init [ARM will
>       do so; PPC might want to do so]
> 
>  kvm_irqchip_in_kernel()
>    -- the user-settable option, actual behaviour is arch specific
>       on x86, true means (as it does now) LAPIC,IOAPIC,PIT in kernel
>       on ARM, we ignore this setting and just DTRT

You should reject kernel_irqchip=off as long as you only support an
in-kernel GIC model.

>       on PPC, used as a convenience setting for whether to use
>       an in-kernel model of the interrupt controller
>       Shouldn't be used in non-target-specific code
> 
> and two I'm not quite so sure about:
> 
>  kvm_has_msi_routing()
>    -- true if we can do routing of MSIs

GSI, not MSI.

>       set true only if x86 and kvm_irqchip_in_kernel

It means that the target architecture supports routing of various
interrupt sources (userspace, irqfds, in-kernel device models) to
different in-kernel IRQ sinks (CPU cores, irqchip models, whatever).
Interrupt messages via (binary-state) irqfd depend on it.

> 
>  kvm_has_irqfds()
>    -- true if kernel supports IRQFDs
>       currently true only if x86 and kvm_irqchip_in_kernel

Note that this and the above are currently static feature tests, not
mode checks (i.e. they are true even if kernel_irqchip=off). The
"kvm_has" namespace is reserved for such tests.

> 
> 
> Second, current uses of kvm_irqchip_in_kernel():
> 
> hw/kvmvapic.c, hw/pc.c, hw/pc_piix.c, target-i386/kvm.c:
>  -- these are all x86 specific and can continue to use
>     kvm_irqchip_in_kernel()
> 
> cpus.c:cpu_thread_is_idle()
>  -- should use !kvm_interrupts_are_async() [because halt is
> in userspace iff we're using the synchronous interrupt model]
> 
> kvm-all.c:kvm_irqchip_set_irq():
>  -- (just an assert) should be kvm_interrupts_are_async()

The name kvm_irqchip_set_irq implies so far that it injects into an
in-kernel irqchip model. Either different functions for archs that don't
follow this concept need to be provided, or this function requires
renaming (kvm_set_irq_async or so).

> 
> kvm-all.c:kvm_irqchip_add_msi_route():
>  -- should be kvm_have_msi_routing()

Only if you change the semantics of kvm_has_gsi_routing (and rename it).

> 
> kvm-all.c:kvm_irqchip_assign_irqfd():
>  -- should be true if kvm_has_irqfds()

The same issue. Plus there is that naming conflict again if we should
ever see irqfd without some in-kernel irqchip. But even s390 would have
a logical "irqchip" for me at the point it may route interrupt messages
from devices directly to the CPUs.

> 
> kvm-all.c:kvm_allows_irq0_override():
>  -- this still seems to me to be a completely x86 specific concept;
>     it should move to a source file in target-x86 and then it
>     can continue to use kvm_irqchip_in_kernel()
> 
> hw/virtio-pci.c:virtio_pci_set_guest_notifiers()
>  -- not entirely sure about this one but I think it
>     should be testing kvm_has_msi_routing().

It depends on full irqfd support, which includes IRQ routing to allow
MSI via irqfd. Something like kvm_msi_via_irqfd_available.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux