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

From: Christoffer Dall
Date: Wed Sep 10 2014 - 23:10:19 EST


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,

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

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

> +This avoids trapping the end-of-interrupt for level sensitive IRQ.

avoid this last line, it's specific to ARM.

> +
> +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?

don't refer to FOWARD/UNFORWARD, either refer to these attributes by
their full name or use a clear reference in proper English.

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

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