Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Alexander Graf <agraf@suse.de>]

On 24.06.14 17:05, Eric Auger wrote:
> 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?.

"no entry" really should be "no entry". I think if we want some other 
mechanism, we should make it be explicit.

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

Right, if you go with the ranged routing entries you should be able to 
get by with a very small number of NUM_PINS, no?


Alex