Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Eric Auger]

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

In the positive, shouldn't we clarify the KVM API documentation?

Thank you in advance

Best Regards

Eric

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Eric Auger]

Hi,

If I am not wrong I did not get any reply to this question. Can anyone
share his/her experience/knowledge on this gsi routing/irqfd use case?

Thank you in advance

Best Regards

Eric

On 06/17/2014 01:39 PM, 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).
> 
> In the positive, shouldn't we clarify the KVM API documentation?
> 
> Thank you in advance
> 
> Best Regards
> 
> Eric
> 

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Alexander Graf]

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.

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.

Splitting that dependency out is certainly quite tedious work. However, 
I'm sure the IBM folks will be grateful if you do it :).


Alex

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Paul Mackerras]

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

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.

The other way I see to solve my particular problem, which is that
userspace tends to use hardware interrupt numbers starting at 4096, is
to replace the arrays in struct kvm_irq_routing_table with idr
structures or something similar.

> Splitting that dependency out is certainly quite tedious work. However, I'm
> sure the IBM folks will be grateful if you do it :).

Indeed. :)

Paul.

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Alexander Graf]

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

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);

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.


Alex

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Eric Auger]

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
> 

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Alexander Graf]

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

Re: Coupling between KVM_IRQFD and KVM_SET_GSI_ROUTING?

[Eric Auger]

On 06/24/2014 05:47 PM, Alexander Graf wrote:
> 
> 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.
Yep I agree. But currently the usage of routing mechanism is not
explicit neither in 4.75 KVM_IRQFD and that was causing my doubts;-)
On top of that the doc says:
"kvm_irqfd.gsi specifies the irqchip pin toggled by this event" but if
there is no routing table set before by user-side, the injection will fail.
> 
>>
>> 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?
Yes that's correct. but the effort to integrate such feature in the code
need to be further assessed.
Eric
> 
> 
> Alex
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html