Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Eric Auger <eric.auger@linaro.org>]

On 06/24/2014 12:40 PM, Alexander Graf wrote:
> 
> On 24.06.14 11:47, Paul Mackerras wrote:
>> On Mon, Jun 23, 2014 at 06:47:51PM +0200, Alexander Graf wrote:
>>> On 17.06.14 13:39, Eric Auger wrote:


>>>> Hello,
>>>>
>>>> I have a question related to KVM_IRQFD and KVM_SET_GSI_ROUTING ioctl
>>>> relationship.
>>>>
>>>> When reading the KVM API documentation I do not understand there is any
>>>> dependency between KVM_IRQFD and KVM_SET_GSI_ROUTING. According to the
>>>> text it seems only the gsi field is used and interpreted as the
>>>> irqchip pin.
>>>>
>>>> However irqchip.c kvm_set_irq code relies on an existing and not dummy
>>>> routing table.
>>>>
>>>> My question is: does anyone agree on the fact the user-side must set a
>>>> consistent routing table using KVM_SET_GSI_ROUTING before using
>>>> KVM_IRQFD? The other alternative would have been to build a default
>>>> identity GSI routing table in the kernel (gsi = irqchip.pin).
>>> I untangled irqfd support from the x86 ioapic emulation a while back.
>>> When I
>>> looked at it, I didn't see any easy way to split it out from the routing
>>> too, so I kept that dependency in.
Hi Alex, Paul,

thanks for your replies. hence don't you think the KVM API doc deserves
to be clarified then?
>>>
>>> If you look at the code, you will see that the irq routing entry is
>>> used as
>>> token for an irqfd. So every irqfd only knows its routing table entry,
>>> nothing else.
>> As far as I can see, the routing table entry is used for only two
>> things: to tell whether the interrupt is an MSI or LSI, and to pass to
>> kvm_set_msi.


Indeed I noticed irq_entry in _irqfd struct only is set for MSI routing.
For other IRQS - I guess what you call LSI - , routine table map[] is
used to retrieve the irchip.pin(s) associated to that gsi and call set().
>>
>> One way to tackle the problem would be to abstract out the routing
>> table lookup into a function in irqchip.c, rather than having it
>> open-coded in irqfd_update.  Then, if you don't want to have the
>> routing table you write an alternative function that creates a
>> routing entry from scratch.  It would need information from the
>> low-level irq chip code as to which interrupts are LSIs and which are
>> MSIs.  It also ties you down to having only one kind of interrupt
controller.
> 
> You could also create it along with the irqfd state struct. So instead of
> 
>   kzalloc(sizeof(*irqfd), GFP_KERNEL);
> 
> we could do
> 
>   kzalloc(sizeof(*irqfd) + sizeof(struct kvm_kernel_irq_routing_entry),
> GFP_KERNEL);


Wouldn't it make sense, in kvm_irqfd_assign, to test whether there is a
routing entry associated to that gsi. In the negative, create a default
one where irqchip.pin = gsi or something architecture specific?.

That way would not need a complete & big routing table to be set by
user-side?

> 
> and point the routing entry to the inline irqfd one. We'd lose the
> ability to change any routings with that construct, but I think that's
> ok for irq controllers that are not configurable.
> 
> Alternatively, we could also have a single routing entry that is a
> wildcard match, ranging from say LSI [x...y]. The irqfd structure would
> then need to carry a local payload to define an offset inside that
> wildcard routing entry.

Also whatever the number of IRQs assigned, we have this big
chip[KVM_NR_IRQCHIPS][KVM_IRQCHIP_NUM_PINS] allocated. Also on ARM we
have some difficulties to define KVM_IRQCHIP_NUM_PINS which becomes
quite dynamic.


Best Regards

Eric
> 
> 
> Alex
>