Re: [RFC v2 5/9] KVM: KVM-VFIO: update user API to program forwarded IRQ

[Eric Auger]

On 09/11/2014 05:10 AM, Christoffer Dall wrote:
> On Mon, Sep 01, 2014 at 02:52:44PM +0200, Eric Auger wrote:
>> add new device group commands:
>> - KVM_DEV_VFIO_DEVICE_FORWARD_IRQ and
>>   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ
>>
>> which enable to turn forwarded IRQ mode on/off.
>>
>> the kvm_arch_forwarded_irq struct embodies a forwarded IRQ
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> v1 -> v2:
>> - struct kvm_arch_forwarded_irq moved from arch/arm/include/uapi/asm/kvm.h
>>   to include/uapi/linux/kvm.h
>>   also irq_index renamed into index and guest_irq renamed into gsi
>> - ASSIGN/DEASSIGN renamed into FORWARD/UNFORWARD
>> ---
>>  Documentation/virtual/kvm/devices/vfio.txt | 26 ++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h                   |  9 +++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt b/Documentation/virtual/kvm/devices/vfio.txt
>> index ef51740..048baa0 100644
>> --- a/Documentation/virtual/kvm/devices/vfio.txt
>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
>> @@ -13,6 +13,7 @@ VFIO-group is held by KVM.
>>  
>>  Groups:
>>    KVM_DEV_VFIO_GROUP
>> +  KVM_DEV_VFIO_DEVICE
>>  
>>  KVM_DEV_VFIO_GROUP attributes:
>>    KVM_DEV_VFIO_GROUP_ADD: Add a VFIO group to VFIO-KVM device tracking
>> @@ -20,3 +21,28 @@ KVM_DEV_VFIO_GROUP attributes:
>>  
>>  For each, kvm_device_attr.addr points to an int32_t file descriptor
>>  for the VFIO group.
>> +
>> +KVM_DEV_VFIO_DEVICE attributes:
>> +  KVM_DEV_VFIO_DEVICE_FORWARD_IRQ
>> +  KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ
>> +
>> +For each, kvm_device_attr.addr points to a kvm_arch_forwarded_irq struct.
>> +This user API makes possible to create a special IRQ handling mode,
> 
>   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ enables a special IRQ handling mode on
>   hardware that supports it,
OK
> 
>> +where KVM and a VFIO platform driver collaborate to improve IRQ
>> +handling performance.
>> +
>> +'fd represents the file descriptor of a valid VFIO device whose physical
> 
> fd is described out of context here.  Can you copy the struct definition
> into this document, perhaps right after the "For each, ..." line above.
yes sure
> 
>> +IRQ, referenced by its index, is injected into the VM guest irq (gsi).
>                                              as a virtual IRQ (specified
> 					     by the gsi field) into the
> 					     VM.
> 
>> +
>> +On FORWARD_IRQ, KVM-VFIO device programs:
>    When setting the  KVM_DEV_VFIO_DEVICE_FORWARD_IRQ attribute, the
>    KVM-VFIO device tells the host (or VFIO?) to not complete the
>    physical IRQ, and instead ensures that KVM (or the VM) completes the
>    physical IRQ.
> 
>> +- the host, to not complete the physical IRQ itself.
>> +- the GIC, to automatically complete the physical IRQ when the guest
>> +  completes the virtual IRQ.
> 
> and drop this bullet form.
ok
> 
>> +This avoids trapping the end-of-interrupt for level sensitive IRQ.
> 
> avoid this last line, it's specific to ARM.
ok
> 
>> +
>> +On UNFORWARD_IRQ, one returns to the mode where the host completes the
>    When setting the KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ attribute, the
>    host (VFIO?) will again complete the physical IRQ and KVM will not...
>  
>> +physical IRQ and the guest completes the virtual IRQ.
>> +
>> +It is up to the caller of this API to make sure the IRQ is not
>> +outstanding when the FORWARD/UNFORWARD is called. This could lead to
> 
> outstanding? can you be specific?
active? and I should add *physical* IRQ
> 
> don't refer to FOWARD/UNFORWARD, either refer to these attributes by
> their full name or use a clear reference in proper English.
ok
> 
>> +some inconsistency on who is going to complete the IRQ.
> 
> This sounds like the whole thing is fragile and if userspace doesn't do
> things right, IRQ handling of a piece of hardware is going to be
> inconsistent?  Is this the case?  If so, we need some stronger
> semantics.  If not, this should be rephrased.
Actually the KVM-VFIO device rejects any attempt to change the
forwarding mode if the physical IRQ is active. So I hope this is robust
and will change the explanation.

Thanks

Eric
> 
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index cf3a2ff..8cd7b0e 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -947,6 +947,12 @@ struct kvm_device_attr {
>>  	__u64	addr;		/* userspace address of attr data */
>>  };
>>  
>> +struct kvm_arch_forwarded_irq {
>> +	__u32 fd; /* file desciptor of the VFIO device */
>> +	__u32 index; /* VFIO device IRQ index */
>> +	__u32 gsi; /* gsi, ie. virtual IRQ number */
>> +};
>> +
>>  #define KVM_DEV_TYPE_FSL_MPIC_20	1
>>  #define KVM_DEV_TYPE_FSL_MPIC_42	2
>>  #define KVM_DEV_TYPE_XICS		3
>> @@ -954,6 +960,9 @@ struct kvm_device_attr {
>>  #define  KVM_DEV_VFIO_GROUP			1
>>  #define   KVM_DEV_VFIO_GROUP_ADD			1
>>  #define   KVM_DEV_VFIO_GROUP_DEL			2
>> +#define  KVM_DEV_VFIO_DEVICE			2
>> +#define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
>> +#define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
>>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
>>  #define KVM_DEV_TYPE_FLIC		6
>>  
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/