[PATCH 0/7] KVM: arm/arm64: gsi routing support

[PATCH 0/7] KVM: arm/arm64: gsi routing support

[Eric Auger]

With the advent of GICv3 ITS in-kernel emulation, KVM GSI routing
appears to be requested. More specifically MSI routing is needed.
irqchip routing does not sound to be really useful on arm but usage of
MSI routing also mandates to integrate irqchip routing. The initial
implementation of irqfd on arm must be upgraded with the integration
of kvm irqchip.c code and the implementation of its standard hooks
in the architecture specific part.

In case KVM_SET_GSI_ROUTING ioctl is not called, a default routing
table with flat irqchip routing entries is built enabling to inject gsi
corresponding to the SPI indexes seen by the guest.

As soon as KVM_SET_GSI_ROUTING is called, user-space overwrites this
default routing table and is responsible for building the whole routing
table.

for arm/arm64 KVM_SET_GSI_ROUTING has a limited support:
- only applies to KVM_IRQFD and not to KVM_IRQ_LINE

- irqchip routing was tested on Calxeda midway (VFIO with irqfd)
- MSI routing without GICv3 ITS was tested using APM Xgene-I
  (qemu VIRTIO-PCI vhost net without gsi_direct_mapping).
- MSI routing with GICv2 ITS is NOT yet tested.

Code can be found at https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-gsi-routing-patch

It applies on Andre's [PATCH 00/13] arm64: KVM: GICv3 ITS emulation
(http://www.spinics.net/lists/kvm/msg117402.html)

History:

RFC -> PATCH:
- clearly state limited support on arm/arm64:
  KVM_IRQ_LINE not impacted by GSI routing
- add default routing table feature (new patch file)
- changed uapi to use padding field area
- reword api.txt


Eric Auger (7):
  KVM: api: add kvm_irq_routing_extended_msi
  KVM: kvm_host: add kvm_extended_msi
  KVM: irqchip: convey devid to kvm_set_msi
  KVM: arm/arm64: enable irqchip routing
  KVM: arm/arm64: build a default routing table
  KVM: arm/arm64: enable MSI routing
  KVM: arm: implement kvm_set_msi by gsi direct mapping

 Documentation/virtual/kvm/api.txt |  30 ++++++++--
 arch/arm/include/asm/kvm_host.h   |   2 +
 arch/arm/kvm/Kconfig              |   3 +
 arch/arm/kvm/Makefile             |   2 +-
 arch/arm64/include/asm/kvm_host.h |   1 +
 arch/arm64/kvm/Kconfig            |   2 +
 arch/arm64/kvm/Makefile           |   2 +-
 include/kvm/arm_vgic.h            |   9 ---
 include/linux/kvm_host.h          |  10 ++++
 include/uapi/linux/kvm.h          |   6 +-
 virt/kvm/arm/vgic.c               | 117 ++++++++++++++++++++++++++++----------
 virt/kvm/irqchip.c                |  20 +++++--
 12 files changed, 154 insertions(+), 50 deletions(-)

-- 
1.9.1

[PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

On ARM, the MSI msg (address and data) comes along with
out-of-band device ID information. The device ID encodes the device
that composes the MSI msg. Let's create a new routing entry type,
dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
to convey the device ID.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

RFC -> PATCH
- remove kvm_irq_routing_extended_msi and use union instead
---
 Documentation/virtual/kvm/api.txt | 9 ++++++++-
 include/uapi/linux/kvm.h          | 6 +++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index d20fd94..6426ae9 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
 	__u32 gsi;
 	__u32 type;
 	__u32 flags;
-	__u32 pad;
+	union {
+		__u32 pad;
+		__u32 devid;
+	};
 	union {
 		struct kvm_irq_routing_irqchip irqchip;
 		struct kvm_irq_routing_msi msi;
@@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
 #define KVM_IRQ_ROUTING_IRQCHIP 1
 #define KVM_IRQ_ROUTING_MSI 2
 #define KVM_IRQ_ROUTING_S390_ADAPTER 3
+#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
+
+In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
+the device ID.
 
 No flags are specified so far, the corresponding field must be set to zero.
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 2a23705..8484681 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
 #define KVM_IRQ_ROUTING_IRQCHIP 1
 #define KVM_IRQ_ROUTING_MSI 2
 #define KVM_IRQ_ROUTING_S390_ADAPTER 3
+#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
 
 struct kvm_irq_routing_entry {
 	__u32 gsi;
 	__u32 type;
 	__u32 flags;
-	__u32 pad;
+	union {
+		__u32 pad;
+		__u32 devid;
+	};
 	union {
 		struct kvm_irq_routing_irqchip irqchip;
 		struct kvm_irq_routing_msi msi;
-- 
1.9.1

[PATCH 2/7] KVM: kvm_host: add kvm_extended_msi

[Eric Auger]

Add a new kvm_extended_msi struct to store the additional device ID
specific to ARM. kvm_kernel_irq_routing_entry union now encompasses
this new struct.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

RFC -> PATCH:
- reword the commit message after change in first patch (uapi)
---
 include/linux/kvm_host.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ad45054..e1c1c0d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -304,6 +304,13 @@ struct kvm_s390_adapter_int {
 	u32 adapter_id;
 };
 
+struct kvm_extended_msi {
+	u32     address_lo;     /* low 32 bits of msi message address */
+	u32     address_hi;     /* high 32 bits of msi message address */
+	u32     data;           /* 16 bits of msi message data */
+	u32	devid;		/* out-of-band device ID */
+};
+
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
 	u32 type;
@@ -317,6 +324,7 @@ struct kvm_kernel_irq_routing_entry {
 		} irqchip;
 		struct msi_msg msi;
 		struct kvm_s390_adapter_int adapter;
+		struct kvm_extended_msi ext_msi;
 	};
 	struct hlist_node link;
 };
-- 
1.9.1

[PATCH 3/7] KVM: irqchip: convey devid to kvm_set_msi

[Eric Auger]

on ARM, a devid field is conveyed in kvm_msi struct. Let's choose the
routing type and struct according to its availability and fill the
corresponding struct. Also remove the flag check now this latter can
be non null.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 virt/kvm/irqchip.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 1d56a90..e76c7d2 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -73,12 +73,22 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 {
 	struct kvm_kernel_irq_routing_entry route;
 
-	if (!irqchip_in_kernel(kvm) || msi->flags != 0)
+	if (!irqchip_in_kernel(kvm))
 		return -EINVAL;
 
-	route.msi.address_lo = msi->address_lo;
-	route.msi.address_hi = msi->address_hi;
-	route.msi.data = msi->data;
+	if (msi->flags & KVM_MSI_VALID_DEVID) {
+		route.type = KVM_IRQ_ROUTING_EXTENDED_MSI;
+		route.ext_msi.address_lo = msi->address_lo;
+		route.ext_msi.address_hi = msi->address_hi;
+		route.ext_msi.data = msi->data;
+		route.ext_msi.devid= msi->devid;
+	}
+	else {
+		route.type = KVM_IRQ_ROUTING_MSI;
+		route.msi.address_lo = msi->address_lo;
+		route.msi.address_hi = msi->address_hi;
+		route.msi.data = msi->data;
+	}
 
 	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, false);
 }
-- 
1.9.1

[PATCH 4/7] KVM: arm/arm64: enable irqchip routing

[Eric Auger]

This patch adds compilation and link against irqchip.

On ARM, irqchip routing is not really useful since there is
a single irqchip. However main motivation behind using irqchip
code is to enable MSI routing code. With the support of in-kernel
GICv3 ITS emulation, it now seems to be a MUST HAVE requirement.

Functions previously implemented in vgic.c and substitute
to more complex irqchip implementation are removed:

- kvm_send_userspace_msi
- kvm_irq_map_chip_pin
- kvm_set_irq
- kvm_irq_map_gsi.

They implemented a kernel default identity GSI routing. This is now
replaced by user-side provided routing.

Routing standard hooks are now implemented in vgic:
- kvm_set_routing_entry
- kvm_set_irq
- kvm_set_msi

Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.

MSI routing is not yet allowed.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

RFC -> PATCH
- reword api.txt:
  x move MSI routing comments in a subsequent patch,
  x clearly state GSI routing does not apply to KVM_IRQ_LINE
---
 Documentation/virtual/kvm/api.txt | 12 ++++--
 arch/arm/include/asm/kvm_host.h   |  2 +
 arch/arm/kvm/Kconfig              |  2 +
 arch/arm/kvm/Makefile             |  2 +-
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/Kconfig            |  2 +
 arch/arm64/kvm/Makefile           |  2 +-
 include/kvm/arm_vgic.h            |  9 -----
 virt/kvm/arm/vgic.c               | 78 ++++++++++++++++++++++++---------------
 virt/kvm/irqchip.c                |  2 +
 10 files changed, 68 insertions(+), 44 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 6426ae9..1e0d5f5 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1395,13 +1395,16 @@ KVM_ASSIGN_DEV_IRQ. Partial deassignment of host or guest IRQ is allowed.
 4.52 KVM_SET_GSI_ROUTING
 
 Capability: KVM_CAP_IRQ_ROUTING
-Architectures: x86 s390
+Architectures: x86 s390 arm arm64
 Type: vm ioctl
 Parameters: struct kvm_irq_routing (in)
 Returns: 0 on success, -1 on error
 
 Sets the GSI routing table entries, overwriting any previously set entries.
 
+On arm/arm64, GSI routing has the following limitation:
+- GSI routing does not apply to KVM_IRQ_LINE but only to KVM_IRQFD.
+
 struct kvm_irq_routing {
 	__u32 nr;
 	__u32 flags;
@@ -2308,9 +2311,10 @@ Note that closing the resamplefd is not sufficient to disable the
 irqfd.  The KVM_IRQFD_FLAG_RESAMPLE is only necessary on assignment
 and need not be specified with KVM_IRQFD_FLAG_DEASSIGN.
 
-On ARM/ARM64, the gsi field in the kvm_irqfd struct specifies the Shared
-Peripheral Interrupt (SPI) index, such that the GIC interrupt ID is
-given by gsi + 32.
+On arm/arm64, gsi routing being supported, the following can happen:
+- in case no routing entry is associated to this gsi, injection fails
+- in case the gsi is associated to an irqchip routing entry,
+  irqchip.pin + 32 corresponds to the injected SPI ID.
 
 4.76 KVM_PPC_ALLOCATE_HTAB
 
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d71607c..452697e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -42,6 +42,8 @@
 
 #define KVM_VCPU_MAX_FEATURES 2
 
+#define KVM_IRQCHIP_NUM_PINS 988 /* 1020 -32 is the number of SPI */
+
 #include <kvm/arm_vgic.h>
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index bfb915d..151e710 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -31,6 +31,8 @@ config KVM
 	select KVM_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
+	select HAVE_KVM_IRQCHIP
+	select HAVE_KVM_IRQ_ROUTING
 	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
 	---help---
 	  Support hosting virtualized guest machines.
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index c5eef02c..1a8f48a 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)
 
 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vfio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vfio.o $(KVM)/irqchip.o
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f0f58c9..751210a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -44,6 +44,7 @@
 #include <kvm/arm_arch_timer.h>
 
 #define KVM_VCPU_MAX_FEATURES 3
+#define KVM_IRQCHIP_NUM_PINS 988 /* 1020 -32 is the number of SPI */
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index ff9722f..1a9900d 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -32,6 +32,8 @@ config KVM
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
 	select HAVE_KVM_MSI
+	select HAVE_KVM_IRQCHIP
+	select HAVE_KVM_IRQ_ROUTING
 	---help---
 	  Support hosting virtualized guest machines.
 
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 9803307..90a08457 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -11,7 +11,7 @@ ARM=../../../arch/arm/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += kvm.o
 
-kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vfio.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vfio.o $(KVM)/irqchip.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 8f1be6a..10dc596 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -366,13 +366,4 @@ static inline int vgic_v3_probe(struct device_node *vgic_node,
 }
 #endif
 
-#ifdef CONFIG_HAVE_KVM_MSI
-int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi);
-#else
-static inline int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
-{
-	return -ENODEV;
-}
-#endif
-
 #endif
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 09b1f46..212a5ff 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2220,47 +2220,67 @@ out_free_irq:
 	return ret;
 }
 
-int kvm_irq_map_gsi(struct kvm *kvm,
-		    struct kvm_kernel_irq_routing_entry *entries,
-		    int gsi)
+int vgic_irqfd_set_irq(struct kvm_kernel_irq_routing_entry *e,
+			struct kvm *kvm, int irq_source_id,
+			int level, bool line_status)
 {
-	return 0;
-}
-
-int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin)
-{
-	return pin;
-}
-
-int kvm_set_irq(struct kvm *kvm, int irq_source_id,
-		u32 irq, int level, bool line_status)
-{
-	unsigned int spi = irq + VGIC_NR_PRIVATE_IRQS;
+	unsigned int spi_id = e->irqchip.pin + VGIC_NR_PRIVATE_IRQS;
 
-	trace_kvm_set_irq(irq, level, irq_source_id);
+	trace_kvm_set_irq(spi_id, level, irq_source_id);
 
 	BUG_ON(!vgic_initialized(kvm));
 
-	if (spi > kvm->arch.vgic.nr_irqs)
+	if (spi_id > kvm->arch.vgic.nr_irqs)
 		return -EINVAL;
-	return kvm_vgic_inject_irq(kvm, 0, spi, level);
+	return kvm_vgic_inject_irq(kvm, 0, spi_id, level);
 
 }
 
-/* MSI not implemented yet */
+/**
+ * Populates a kvm routing entry from a user routing entry
+ * @e: kvm internal formatted entry
+ * @ue: user api formatted entry
+ * return 0 on success, -EINVAL on errors.
+ */
+int kvm_set_routing_entry(struct kvm_kernel_irq_routing_entry *e,
+			  const struct kvm_irq_routing_entry *ue)
+{
+	int r = -EINVAL;
+
+	switch (ue->type) {
+	case KVM_IRQ_ROUTING_IRQCHIP:
+		e->set = vgic_irqfd_set_irq;
+		e->irqchip.irqchip = ue->u.irqchip.irqchip;
+		e->irqchip.pin = ue->u.irqchip.pin;
+		if ((e->irqchip.pin >= KVM_IRQCHIP_NUM_PINS) ||
+		    (e->irqchip.irqchip >= KVM_NR_IRQCHIPS))
+			goto out;
+		break;
+	default:
+		goto out;
+	}
+	r = 0;
+out:
+	return r;
+}
+
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 		struct kvm *kvm, int irq_source_id,
 		int level, bool line_status)
 {
-	return 0;
-}
+	struct kvm_msi msi;
 
-#ifdef CONFIG_HAVE_KVM_MSI
-int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
-{
-	if (kvm->arch.vgic.vm_ops.inject_msi)
-		return kvm->arch.vgic.vm_ops.inject_msi(kvm, msi);
-	else
-		return -ENODEV;
+	switch (e->type) {
+	case KVM_IRQ_ROUTING_EXTENDED_MSI:
+		msi.address_lo = e->ext_msi.address_lo;
+		msi.address_hi = e->ext_msi.address_hi;
+		msi.data = e->ext_msi.data;
+		msi.flags = e->ext_msi.devid;
+		if (kvm->arch.vgic.vm_ops.inject_msi)
+			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
+		else
+			return -ENODEV;
+	default:
+		return -EINVAL;
+	}
 }
-#endif
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index e76c7d2..924a29d 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -29,7 +29,9 @@
 #include <linux/srcu.h>
 #include <linux/export.h>
 #include <trace/events/kvm.h>
+#if !defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
 #include "irq.h"
+#endif
 
 struct kvm_irq_routing_table {
 	int chip[KVM_NR_IRQCHIPS][KVM_IRQCHIP_NUM_PINS];
-- 
1.9.1

[PATCH 5/7] KVM: arm/arm64: build a default routing table

[Eric Auger]

Implement a default routing table made of flat irqchip routing
entries (gsi = irqchip.pin) covering the VGIC SPI indexes.
This routing table is overwritten by the first user-space call
to KVM_SET_GSI_ROUTING ioctl.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

PATCH: creation
---
 virt/kvm/arm/vgic.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 212a5ff..410fee1 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1783,6 +1783,8 @@ int vgic_init(struct kvm *kvm)
 	ret |= vgic_init_bitmap(&dist->irq_cfg, nr_cpus, nr_irqs);
 	ret |= vgic_init_bytemap(&dist->irq_priority, nr_cpus, nr_irqs);
 
+	ret |= kvm_setup_default_irq_routing(kvm);
+
 	if (ret)
 		goto out;
 
@@ -2264,6 +2266,25 @@ out:
 	return r;
 }
 
+int kvm_setup_default_irq_routing(struct kvm *kvm)
+{
+	struct kvm_irq_routing_entry *entries;
+	u32 nr = kvm->arch.vgic.nr_irqs - VGIC_NR_PRIVATE_IRQS;
+	int i, ret;
+
+	entries = kcalloc(nr, sizeof(struct kvm_kernel_irq_routing_entry),
+			  GFP_KERNEL);
+	for (i = 0; i < nr; i++) {
+		entries[i].gsi = i;
+		entries[i].type = KVM_IRQ_ROUTING_IRQCHIP;
+		entries[i].u.irqchip.irqchip = 0;
+		entries[i].u.irqchip.pin = i;
+	}
+	ret = kvm_set_irq_routing(kvm, entries, nr, 0);
+	kfree(entries);
+	return ret;
+}
+
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 		struct kvm *kvm, int irq_source_id,
 		int level, bool line_status)
-- 
1.9.1

[PATCH 6/7] KVM: arm/arm64: enable MSI routing

[Eric Auger]

Up to now, only irqchip routing entries could be set. This patch
adds the capability to insert MSI routing entries, extended or
standard ones. Although standard MSI entries can be set, their
injection still is not supported. For ARM64, let's also increase
KVM_MAX_IRQ_ROUTES to 4096: include SPI irqchip flat routes plus
MSI routes. In the future this might be extended.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

RFC -> PATCH:
- move api MSI routing updates into that patch file
- use new devid field of user api struct
---
 Documentation/virtual/kvm/api.txt |  9 +++++++++
 include/linux/kvm_host.h          |  2 ++
 virt/kvm/arm/vgic.c               | 13 +++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 1e0d5f5..b411232 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1405,6 +1405,10 @@ Sets the GSI routing table entries, overwriting any previously set entries.
 On arm/arm64, GSI routing has the following limitation:
 - GSI routing does not apply to KVM_IRQ_LINE but only to KVM_IRQFD.
 
+On arm/arm64, MSI routing through in-kernel GICv3 ITS must use
+KVM_IRQ_ROUTING_EXTENDED_MSI routing type to specify additionnal device ID.
+Otherwise, KVM_IRQ_ROUTING_MSI must be used.
+
 struct kvm_irq_routing {
 	__u32 nr;
 	__u32 flags;
@@ -2315,6 +2319,11 @@ On arm/arm64, gsi routing being supported, the following can happen:
 - in case no routing entry is associated to this gsi, injection fails
 - in case the gsi is associated to an irqchip routing entry,
   irqchip.pin + 32 corresponds to the injected SPI ID.
+- in case the gsi is associated to an MSI routing entry,
+  * without GICv3 ITS in-kernel emulation, MSI data patches the SPI ID
+    of the injected SPI
+  * with GICv3 ITS in-kernel emulation, the MSI message and device ID
+    are translated into an LPI.
 
 4.76 KVM_PPC_ALLOCATE_HTAB
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e1c1c0d..4ca8f8e 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -927,6 +927,8 @@ static inline int mmu_notifier_retry(struct kvm *kvm, unsigned long mmu_seq)
 
 #ifdef CONFIG_S390
 #define KVM_MAX_IRQ_ROUTES 4096 //FIXME: we can have more than that...
+#elif defined(CONFIG_ARM64)
+#define KVM_MAX_IRQ_ROUTES 4096
 #else
 #define KVM_MAX_IRQ_ROUTES 1024
 #endif
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 410fee1..0b4c48c 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2258,6 +2258,19 @@ int kvm_set_routing_entry(struct kvm_kernel_irq_routing_entry *e,
 		    (e->irqchip.irqchip >= KVM_NR_IRQCHIPS))
 			goto out;
 		break;
+	case KVM_IRQ_ROUTING_MSI:
+		e->set = kvm_set_msi;
+		e->msi.address_lo = ue->u.msi.address_lo;
+		e->msi.address_hi = ue->u.msi.address_hi;
+		e->msi.data = ue->u.msi.data;
+		break;
+	case KVM_IRQ_ROUTING_EXTENDED_MSI:
+		e->set = kvm_set_msi;
+		e->ext_msi.address_lo = ue->u.msi.address_lo;
+		e->ext_msi.address_hi = ue->u.msi.address_hi;
+		e->ext_msi.data = ue->u.msi.data;
+		e->ext_msi.devid = ue->devid;
+		break;
 	default:
 		goto out;
 	}
-- 
1.9.1

[PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Eric Auger]

If the ITS modality is not available, let's simply support MSI
injection by transforming the MSI.data into an SPI ID.

This becomes possible to use KVM_SIGNAL_MSI ioctl for arm too.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 arch/arm/kvm/Kconfig | 1 +
 virt/kvm/arm/vgic.c  | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 151e710..0f58baf 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -31,6 +31,7 @@ config KVM
 	select KVM_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
+	select HAVE_KVM_MSI
 	select HAVE_KVM_IRQCHIP
 	select HAVE_KVM_IRQ_ROUTING
 	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 0b4c48c..b3c10dc 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2314,6 +2314,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
 		else
 			return -ENODEV;
+	case KVM_IRQ_ROUTING_MSI:
+		if (kvm->arch.vgic.vm_ops.inject_msi)
+			return -EINVAL;
+		else
+			return kvm_vgic_inject_irq(kvm, 0, e->msi.data, level);
 	default:
 		return -EINVAL;
 	}
-- 
1.9.1

Re: [PATCH 4/7] KVM: arm/arm64: enable irqchip routing

[Andre Przywara]

Hi Eric,

On 29/06/15 16:37, Eric Auger wrote:
> This patch adds compilation and link against irqchip.
> 
> On ARM, irqchip routing is not really useful since there is
> a single irqchip. However main motivation behind using irqchip
> code is to enable MSI routing code. With the support of in-kernel
> GICv3 ITS emulation, it now seems to be a MUST HAVE requirement.
> 
> Functions previously implemented in vgic.c and substitute
> to more complex irqchip implementation are removed:
> 
> - kvm_send_userspace_msi
> - kvm_irq_map_chip_pin
> - kvm_set_irq
> - kvm_irq_map_gsi.
> 
> They implemented a kernel default identity GSI routing. This is now
> replaced by user-side provided routing.
> 
> Routing standard hooks are now implemented in vgic:
> - kvm_set_routing_entry
> - kvm_set_irq
> - kvm_set_msi
> 
> Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
> KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.
> 
> MSI routing is not yet allowed.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
...

> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 09b1f46..212a5ff 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -2220,47 +2220,67 @@ out_free_irq:
>  	return ret;
>  }
>  
> -int kvm_irq_map_gsi(struct kvm *kvm,
> -		    struct kvm_kernel_irq_routing_entry *entries,
> -		    int gsi)
> +int vgic_irqfd_set_irq(struct kvm_kernel_irq_routing_entry *e,
> +			struct kvm *kvm, int irq_source_id,
> +			int level, bool line_status)
>  {
> -	return 0;
> -}
> -
> -int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin)
> -{
> -	return pin;
> -}
> -
> -int kvm_set_irq(struct kvm *kvm, int irq_source_id,
> -		u32 irq, int level, bool line_status)
> -{
> -	unsigned int spi = irq + VGIC_NR_PRIVATE_IRQS;
> +	unsigned int spi_id = e->irqchip.pin + VGIC_NR_PRIVATE_IRQS;
>  
> -	trace_kvm_set_irq(irq, level, irq_source_id);
> +	trace_kvm_set_irq(spi_id, level, irq_source_id);
>  
>  	BUG_ON(!vgic_initialized(kvm));
>  
> -	if (spi > kvm->arch.vgic.nr_irqs)
> +	if (spi_id > kvm->arch.vgic.nr_irqs)
>  		return -EINVAL;
> -	return kvm_vgic_inject_irq(kvm, 0, spi, level);
> +	return kvm_vgic_inject_irq(kvm, 0, spi_id, level);
>  
>  }
>  
> -/* MSI not implemented yet */
> +/**
> + * Populates a kvm routing entry from a user routing entry
> + * @e: kvm internal formatted entry
> + * @ue: user api formatted entry
> + * return 0 on success, -EINVAL on errors.
> + */
> +int kvm_set_routing_entry(struct kvm_kernel_irq_routing_entry *e,
> +			  const struct kvm_irq_routing_entry *ue)
> +{
> +	int r = -EINVAL;
> +
> +	switch (ue->type) {
> +	case KVM_IRQ_ROUTING_IRQCHIP:
> +		e->set = vgic_irqfd_set_irq;
> +		e->irqchip.irqchip = ue->u.irqchip.irqchip;
> +		e->irqchip.pin = ue->u.irqchip.pin;
> +		if ((e->irqchip.pin >= KVM_IRQCHIP_NUM_PINS) ||
> +		    (e->irqchip.irqchip >= KVM_NR_IRQCHIPS))
> +			goto out;
> +		break;
> +	default:
> +		goto out;
> +	}
> +	r = 0;
> +out:
> +	return r;
> +}
> +
>  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>  		struct kvm *kvm, int irq_source_id,
>  		int level, bool line_status)
>  {
> -	return 0;
> -}
> +	struct kvm_msi msi;
>  
> -#ifdef CONFIG_HAVE_KVM_MSI
> -int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
> -{
> -	if (kvm->arch.vgic.vm_ops.inject_msi)
> -		return kvm->arch.vgic.vm_ops.inject_msi(kvm, msi);
> -	else
> -		return -ENODEV;
> +	switch (e->type) {
> +	case KVM_IRQ_ROUTING_EXTENDED_MSI:
> +		msi.address_lo = e->ext_msi.address_lo;
> +		msi.address_hi = e->ext_msi.address_hi;
> +		msi.data = e->ext_msi.data;
> +		msi.flags = e->ext_msi.devid;

You probably meant to write:
+		msi.flags = KVM_MSI_VALID_DEVID;
+		msi.devid = e->ext_msi.devid;

With this change I could get it (your patches on top of my v1.5) to work
with my hacked kvmtool - at least with vhost=0. On to fixing irqfd now ...

Cheers,
Andre.

> +		if (kvm->arch.vgic.vm_ops.inject_msi)
> +			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
> +		else
> +			return -ENODEV;
> +	default:
> +		return -EINVAL;
> +	}
>  }
> -#endif

Re: [PATCH 4/7] KVM: arm/arm64: enable irqchip routing

[Eric Auger]

On 06/30/2015 03:39 PM, Andre Przywara wrote:
> Hi Eric,
> 
> On 29/06/15 16:37, Eric Auger wrote:
>> This patch adds compilation and link against irqchip.
>>
>> On ARM, irqchip routing is not really useful since there is
>> a single irqchip. However main motivation behind using irqchip
>> code is to enable MSI routing code. With the support of in-kernel
>> GICv3 ITS emulation, it now seems to be a MUST HAVE requirement.
>>
>> Functions previously implemented in vgic.c and substitute
>> to more complex irqchip implementation are removed:
>>
>> - kvm_send_userspace_msi
>> - kvm_irq_map_chip_pin
>> - kvm_set_irq
>> - kvm_irq_map_gsi.
>>
>> They implemented a kernel default identity GSI routing. This is now
>> replaced by user-side provided routing.
>>
>> Routing standard hooks are now implemented in vgic:
>> - kvm_set_routing_entry
>> - kvm_set_irq
>> - kvm_set_msi
>>
>> Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
>> KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.
>>
>> MSI routing is not yet allowed.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
> ...
> 
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 09b1f46..212a5ff 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -2220,47 +2220,67 @@ out_free_irq:
>>  	return ret;
>>  }
>>  
>> -int kvm_irq_map_gsi(struct kvm *kvm,
>> -		    struct kvm_kernel_irq_routing_entry *entries,
>> -		    int gsi)
>> +int vgic_irqfd_set_irq(struct kvm_kernel_irq_routing_entry *e,
>> +			struct kvm *kvm, int irq_source_id,
>> +			int level, bool line_status)
>>  {
>> -	return 0;
>> -}
>> -
>> -int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin)
>> -{
>> -	return pin;
>> -}
>> -
>> -int kvm_set_irq(struct kvm *kvm, int irq_source_id,
>> -		u32 irq, int level, bool line_status)
>> -{
>> -	unsigned int spi = irq + VGIC_NR_PRIVATE_IRQS;
>> +	unsigned int spi_id = e->irqchip.pin + VGIC_NR_PRIVATE_IRQS;
>>  
>> -	trace_kvm_set_irq(irq, level, irq_source_id);
>> +	trace_kvm_set_irq(spi_id, level, irq_source_id);
>>  
>>  	BUG_ON(!vgic_initialized(kvm));
>>  
>> -	if (spi > kvm->arch.vgic.nr_irqs)
>> +	if (spi_id > kvm->arch.vgic.nr_irqs)
>>  		return -EINVAL;
>> -	return kvm_vgic_inject_irq(kvm, 0, spi, level);
>> +	return kvm_vgic_inject_irq(kvm, 0, spi_id, level);
>>  
>>  }
>>  
>> -/* MSI not implemented yet */
>> +/**
>> + * Populates a kvm routing entry from a user routing entry
>> + * @e: kvm internal formatted entry
>> + * @ue: user api formatted entry
>> + * return 0 on success, -EINVAL on errors.
>> + */
>> +int kvm_set_routing_entry(struct kvm_kernel_irq_routing_entry *e,
>> +			  const struct kvm_irq_routing_entry *ue)
>> +{
>> +	int r = -EINVAL;
>> +
>> +	switch (ue->type) {
>> +	case KVM_IRQ_ROUTING_IRQCHIP:
>> +		e->set = vgic_irqfd_set_irq;
>> +		e->irqchip.irqchip = ue->u.irqchip.irqchip;
>> +		e->irqchip.pin = ue->u.irqchip.pin;
>> +		if ((e->irqchip.pin >= KVM_IRQCHIP_NUM_PINS) ||
>> +		    (e->irqchip.irqchip >= KVM_NR_IRQCHIPS))
>> +			goto out;
>> +		break;
>> +	default:
>> +		goto out;
>> +	}
>> +	r = 0;
>> +out:
>> +	return r;
>> +}
>> +
>>  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>>  		struct kvm *kvm, int irq_source_id,
>>  		int level, bool line_status)
>>  {
>> -	return 0;
>> -}
>> +	struct kvm_msi msi;
>>  
>> -#ifdef CONFIG_HAVE_KVM_MSI
>> -int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
>> -{
>> -	if (kvm->arch.vgic.vm_ops.inject_msi)
>> -		return kvm->arch.vgic.vm_ops.inject_msi(kvm, msi);
>> -	else
>> -		return -ENODEV;
>> +	switch (e->type) {
>> +	case KVM_IRQ_ROUTING_EXTENDED_MSI:
>> +		msi.address_lo = e->ext_msi.address_lo;
>> +		msi.address_hi = e->ext_msi.address_hi;
>> +		msi.data = e->ext_msi.data;
>> +		msi.flags = e->ext_msi.devid;
> 
> You probably meant to write:
> +		msi.flags = KVM_MSI_VALID_DEVID;
> +		msi.devid = e->ext_msi.devid;
> 
> With this change I could get it (your patches on top of my v1.5) to work
> with my hacked kvmtool - at least with vhost=0. On to fixing irqfd now ...

Yes sure. Thanks for the catch!

I will correct in next respin

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>> +		if (kvm->arch.vgic.vm_ops.inject_msi)
>> +			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
>> +		else
>> +			return -ENODEV;
>> +	default:
>> +		return -EINVAL;
>> +	}
>>  }
>> -#endif

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
> Sent: Monday, June 29, 2015 6:37 PM
> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
> 
> On ARM, the MSI msg (address and data) comes along with
> out-of-band device ID information. The device ID encodes the device
> that composes the MSI msg. Let's create a new routing entry type,
> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
> to convey the device ID.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> RFC -> PATCH
> - remove kvm_irq_routing_extended_msi and use union instead
> ---
>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>  include/uapi/linux/kvm.h          | 6 +++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index d20fd94..6426ae9 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>  	__u32 gsi;
>  	__u32 type;
>  	__u32 flags;
> -	__u32 pad;
> +	union {
> +		__u32 pad;
> +		__u32 devid;
> +	};
>  	union {
>  		struct kvm_irq_routing_irqchip irqchip;
>  		struct kvm_irq_routing_msi msi;

 devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
It also has reserved pad.

> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>  #define KVM_IRQ_ROUTING_MSI 2
>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
> +
> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
> +the device ID.
> 
>  No flags are specified so far, the corresponding field must be set to zero.

What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
believe this would make an API more consistent and introduce less new definitions.

> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 2a23705..8484681 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>  #define KVM_IRQ_ROUTING_MSI 2
>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
> 
>  struct kvm_irq_routing_entry {
>  	__u32 gsi;
>  	__u32 type;
>  	__u32 flags;
> -	__u32 pad;
> +	union {
> +		__u32 pad;
> +		__u32 devid;
> +	};
>  	union {
>  		struct kvm_irq_routing_irqchip irqchip;
>  		struct kvm_irq_routing_msi msi;
> --
> 1.9.1
> 
> --
> 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

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
> Sent: Monday, June 29, 2015 6:37 PM
> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
> Subject: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping
> 
> If the ITS modality is not available, let's simply support MSI
> injection by transforming the MSI.data into an SPI ID.
> 
> This becomes possible to use KVM_SIGNAL_MSI ioctl for arm too.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  arch/arm/kvm/Kconfig | 1 +
>  virt/kvm/arm/vgic.c  | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
> index 151e710..0f58baf 100644
> --- a/arch/arm/kvm/Kconfig
> +++ b/arch/arm/kvm/Kconfig
> @@ -31,6 +31,7 @@ config KVM
>  	select KVM_VFIO
>  	select HAVE_KVM_EVENTFD
>  	select HAVE_KVM_IRQFD
> +	select HAVE_KVM_MSI
>  	select HAVE_KVM_IRQCHIP
>  	select HAVE_KVM_IRQ_ROUTING
>  	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 0b4c48c..b3c10dc 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -2314,6 +2314,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>  			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
>  		else
>  			return -ENODEV;
> +	case KVM_IRQ_ROUTING_MSI:
> +		if (kvm->arch.vgic.vm_ops.inject_msi)
> +			return -EINVAL;
> +		else
> +			return kvm_vgic_inject_irq(kvm, 0, e->msi.data, level);

 Given API change i suggest (using KVM_MSI_VALID_DEVID flag), we could get rid of all these if()'s
here. Just forward all parameters to vGIC implementation code and let it do its checks.

>  	default:
>  		return -EINVAL;
>  	}
> --
> 1.9.1
> 
> --
> 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

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hello!

> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI
> definition? I
> believe this would make an API more consistent and introduce less new definitions.

 I have just found one more flaw in your implementation. If you take a look at irqfd_wakeup()...
--- cut ---
		/* An event has been signaled, inject an interrupt */
		if (irq.type == KVM_IRQ_ROUTING_MSI)
			kvm_set_msi(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
					false);
		else
			schedule_work(&irqfd->inject);
--- cut ---
 You apparently missed KVM_IRQ_ROUTING_EXTENDED_MSI here, as well as in irqfd_update(). But, if you
accept my API proposal, this becomes irrelevant.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

Hi Pavel,
On 07/02/2015 09:26 AM, Pavel Fedin wrote:
>  Hello!
> 
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>> Sent: Monday, June 29, 2015 6:37 PM
>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
>>
>> On ARM, the MSI msg (address and data) comes along with
>> out-of-band device ID information. The device ID encodes the device
>> that composes the MSI msg. Let's create a new routing entry type,
>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
>> to convey the device ID.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> RFC -> PATCH
>> - remove kvm_irq_routing_extended_msi and use union instead
>> ---
>>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>>  include/uapi/linux/kvm.h          | 6 +++++-
>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index d20fd94..6426ae9 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>>  	__u32 gsi;
>>  	__u32 type;
>>  	__u32 flags;
>> -	__u32 pad;
>> +	union {
>> +		__u32 pad;
>> +		__u32 devid;
>> +	};
>>  	union {
>>  		struct kvm_irq_routing_irqchip irqchip;
>>  		struct kvm_irq_routing_msi msi;
> 
>  devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
> It also has reserved pad.
Well this makes sense to me to associate the devid to the msi and put
devid in the pad field of struct kvm_irq_routing_msi.

André, Christoffer, would you agree on this change? - I would like to
avoid doing/undoing things ;-) -

> 
>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>  #define KVM_IRQ_ROUTING_MSI 2
>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>> +
>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
>> +the device ID.
>>
>>  No flags are specified so far, the corresponding field must be set to zero.
> 
> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
> believe this would make an API more consistent and introduce less new definitions.
do you mean using type == KVM_IRQ_ROUTING_MSI and flag ==
KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the
way for new routing entry types. I add a new one here.

Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and
add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words
as well. But most probably this is even uglier.

Let's see if this thread is heading to a consensus...

Best Regards

Eric
> 
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 2a23705..8484681 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>  #define KVM_IRQ_ROUTING_MSI 2
>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>
>>  struct kvm_irq_routing_entry {
>>  	__u32 gsi;
>>  	__u32 type;
>>  	__u32 flags;
>> -	__u32 pad;
>> +	union {
>> +		__u32 pad;
>> +		__u32 devid;
>> +	};
>>  	union {
>>  		struct kvm_irq_routing_irqchip irqchip;
>>  		struct kvm_irq_routing_msi msi;
>> --
>> 1.9.1
>>
>> --
>> 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
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

On 07/02/2015 10:41 AM, Pavel Fedin wrote:
>  Hello!
> 
>> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI
>> definition? I
>> believe this would make an API more consistent and introduce less new definitions.
> 
>  I have just found one more flaw in your implementation. If you take a look at irqfd_wakeup()...
> --- cut ---
> 		/* An event has been signaled, inject an interrupt */
> 		if (irq.type == KVM_IRQ_ROUTING_MSI)
> 			kvm_set_msi(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
> 					false);
> 		else
> 			schedule_work(&irqfd->inject);
> --- cut ---
>  You apparently missed KVM_IRQ_ROUTING_EXTENDED_MSI here, as well as in irqfd_update(). But, if you
> accept my API proposal, this becomes irrelevant.

Hi Pavel,

thanks for spotting this bug. Whatever the user-api API choice I will
respin shortly fixing this  plus the one reported by André.

Thanks for the review.

Best Regards

Eric


> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Eric Auger]

Hi Pavel,
On 07/02/2015 09:53 AM, Pavel Fedin wrote:
>  Hello!
> 
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>> Sent: Monday, June 29, 2015 6:37 PM
>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>> Subject: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping
>>
>> If the ITS modality is not available, let's simply support MSI
>> injection by transforming the MSI.data into an SPI ID.
>>
>> This becomes possible to use KVM_SIGNAL_MSI ioctl for arm too.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  arch/arm/kvm/Kconfig | 1 +
>>  virt/kvm/arm/vgic.c  | 5 +++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
>> index 151e710..0f58baf 100644
>> --- a/arch/arm/kvm/Kconfig
>> +++ b/arch/arm/kvm/Kconfig
>> @@ -31,6 +31,7 @@ config KVM
>>  	select KVM_VFIO
>>  	select HAVE_KVM_EVENTFD
>>  	select HAVE_KVM_IRQFD
>> +	select HAVE_KVM_MSI
>>  	select HAVE_KVM_IRQCHIP
>>  	select HAVE_KVM_IRQ_ROUTING
>>  	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 0b4c48c..b3c10dc 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -2314,6 +2314,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>>  			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
>>  		else
>>  			return -ENODEV;
>> +	case KVM_IRQ_ROUTING_MSI:
>> +		if (kvm->arch.vgic.vm_ops.inject_msi)
>> +			return -EINVAL;
>> +		else
>> +			return kvm_vgic_inject_irq(kvm, 0, e->msi.data, level);
> 
>  Given API change i suggest (using KVM_MSI_VALID_DEVID flag), we could get rid of all these if()'s
> here. Just forward all parameters to vGIC implementation code and let it do its checks.
I don't understand this comment. Here this is the kernel struct that is
used (struct kvm_kernel_irq_routing_entry) and not the user one
(kvm_irq_routing_entry). The kernel struct does not have the flag field.
Another reason I think to keep using the type for homogeneity. To be
noted that in the kernel struct, the devid is passed in
kvm_extended_msi, as you suggested for the user-space struct.

Thanks

Eric
> 
>>  	default:
>>  		return -EINVAL;
>>  	}
>> --
>> 1.9.1
>>
>> --
>> 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
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Eric,

On 02/07/15 15:49, Eric Auger wrote:
> Hi Pavel,
> On 07/02/2015 09:26 AM, Pavel Fedin wrote:
>>  Hello!
>>
>>> -----Original Message-----
>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>>> Sent: Monday, June 29, 2015 6:37 PM
>>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
>>>
>>> On ARM, the MSI msg (address and data) comes along with
>>> out-of-band device ID information. The device ID encodes the device
>>> that composes the MSI msg. Let's create a new routing entry type,
>>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
>>> to convey the device ID.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>>
>>> RFC -> PATCH
>>> - remove kvm_irq_routing_extended_msi and use union instead
>>> ---
>>>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>>>  include/uapi/linux/kvm.h          | 6 +++++-
>>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index d20fd94..6426ae9 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>>>  	__u32 gsi;
>>>  	__u32 type;
>>>  	__u32 flags;
>>> -	__u32 pad;
>>> +	union {
>>> +		__u32 pad;
>>> +		__u32 devid;
>>> +	};
>>>  	union {
>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>  		struct kvm_irq_routing_msi msi;
>>
>>  devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
>> It also has reserved pad.
> Well this makes sense to me to associate the devid to the msi and put
> devid in the pad field of struct kvm_irq_routing_msi.
> 
> André, Christoffer, would you agree on this change? - I would like to
> avoid doing/undoing things ;-) -

Yes, that makes sense to me. TBH I haven't had a closer look at the
patches yet, but clearly devid belongs into struct kvm_irq_routing_msi.

>>
>>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>> +
>>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
>>> +the device ID.
>>>
>>>  No flags are specified so far, the corresponding field must be set to zero.
>>
>> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
>> believe this would make an API more consistent and introduce less new definitions.
> do you mean using type == KVM_IRQ_ROUTING_MSI and flag ==
> KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the
> way for new routing entry types. I add a new one here.

I tend to agree with Pavel's solution. When hacking IRQ routing support
into kvmtool I saw that it's nasty being forced to differentiate between
the two MSI routing types. Actually userland should be able to query the
kernel about what kind of routing it requires. Also there is the issue
that we must _not_ set the flag on x86, since that breaks older kernels
(due to that check that Eric removes in 3/7).
So from my point of view the cleanest solution would be to always use
KVM_IRQ_ROUTING_MSI, and add the device ID if the kernel needs it (true
for ITS guests, false for GICv2M, x86, ...)
I am looking for a clever solution for this now.

Cheers,
Andre.

> 
> Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and
> add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words
> as well. But most probably this is even uglier.

> 
> Let's see if this thread is heading to a consensus...
> 
> Best Regards
> 
> Eric
>>
>>>
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index 2a23705..8484681 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>>
>>>  struct kvm_irq_routing_entry {
>>>  	__u32 gsi;
>>>  	__u32 type;
>>>  	__u32 flags;
>>> -	__u32 pad;
>>> +	union {
>>> +		__u32 pad;
>>> +		__u32 devid;
>>> +	};
>>>  	union {
>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>  		struct kvm_irq_routing_msi msi;
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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
>>
>> Kind regards,
>> Pavel Fedin
>> Expert Engineer
>> Samsung Electronics Research center Russia
>>
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

Hi Andre,
On 07/02/2015 05:14 PM, Andre Przywara wrote:
> Hi Eric,
> 
> On 02/07/15 15:49, Eric Auger wrote:
>> Hi Pavel,
>> On 07/02/2015 09:26 AM, Pavel Fedin wrote:
>>>  Hello!
>>>
>>>> -----Original Message-----
>>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>>>> Sent: Monday, June 29, 2015 6:37 PM
>>>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>>>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>>>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>>>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
>>>>
>>>> On ARM, the MSI msg (address and data) comes along with
>>>> out-of-band device ID information. The device ID encodes the device
>>>> that composes the MSI msg. Let's create a new routing entry type,
>>>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
>>>> to convey the device ID.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> RFC -> PATCH
>>>> - remove kvm_irq_routing_extended_msi and use union instead
>>>> ---
>>>>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>>>>  include/uapi/linux/kvm.h          | 6 +++++-
>>>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>> index d20fd94..6426ae9 100644
>>>> --- a/Documentation/virtual/kvm/api.txt
>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>>>>  	__u32 gsi;
>>>>  	__u32 type;
>>>>  	__u32 flags;
>>>> -	__u32 pad;
>>>> +	union {
>>>> +		__u32 pad;
>>>> +		__u32 devid;
>>>> +	};
>>>>  	union {
>>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>>  		struct kvm_irq_routing_msi msi;
>>>
>>>  devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
>>> It also has reserved pad.
>> Well this makes sense to me to associate the devid to the msi and put
>> devid in the pad field of struct kvm_irq_routing_msi.
>>
>> André, Christoffer, would you agree on this change? - I would like to
>> avoid doing/undoing things ;-) -
> 
> Yes, that makes sense to me. TBH I haven't had a closer look at the
> patches yet, but clearly devid belongs into struct kvm_irq_routing_msi.
thanks for your quick reply.
OK so let's go with that change.
> 
>>>
>>>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>>> +
>>>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
>>>> +the device ID.
>>>>
>>>>  No flags are specified so far, the corresponding field must be set to zero.
>>>
>>> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
>>> believe this would make an API more consistent and introduce less new definitions.
>> do you mean using type == KVM_IRQ_ROUTING_MSI and flag ==
>> KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the
>> way for new routing entry types. I add a new one here.
> 
> I tend to agree with Pavel's solution. When hacking IRQ routing support
> into kvmtool I saw that it's nasty being forced to differentiate between
> the two MSI routing types. Actually userland should be able to query the
> kernel about what kind of routing it requires. Also there is the issue
> that we must _not_ set the flag on x86, since that breaks older kernels
> (due to that check that Eric removes in 3/7).
> So from my point of view the cleanest solution would be to always use
> KVM_IRQ_ROUTING_MSI, and add the device ID if the kernel needs it (true
> for ITS guests, false for GICv2M, x86, ...)
> I am looking for a clever solution for this now.
OK thanks for sharing. I need some more time to study qemu code too.

- Eric

> 
> Cheers,
> Andre.
> 
>>
>> Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and
>> add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words
>> as well. But most probably this is even uglier.
> 
>>
>> Let's see if this thread is heading to a consensus...
>>
>> Best Regards
>>
>> Eric
>>>
>>>>
>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>> index 2a23705..8484681 100644
>>>> --- a/include/uapi/linux/kvm.h
>>>> +++ b/include/uapi/linux/kvm.h
>>>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>>>
>>>>  struct kvm_irq_routing_entry {
>>>>  	__u32 gsi;
>>>>  	__u32 type;
>>>>  	__u32 flags;
>>>> -	__u32 pad;
>>>> +	union {
>>>> +		__u32 pad;
>>>> +		__u32 devid;
>>>> +	};
>>>>  	union {
>>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>>  		struct kvm_irq_routing_msi msi;
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> 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
>>>
>>> Kind regards,
>>> Pavel Fedin
>>> Expert Engineer
>>> Samsung Electronics Research center Russia
>>>
>>

RE: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Pavel Fedin]

 Hello!

> >  Given API change i suggest (using KVM_MSI_VALID_DEVID flag), we could get rid of all these
if()'s
> > here. Just forward all parameters to vGIC implementation code and let it do its checks.
> I don't understand this comment. Here this is the kernel struct that is
> used (struct kvm_kernel_irq_routing_entry) and not the user one
> (kvm_irq_routing_entry). The kernel struct does not have the flag field.

  Easy. ARM code can always use struct kvm_extended_msi, and flags can go to this structure.

> Another reason I think to keep using the type for homogeneity.

 Homogeneity is perfect IMHO.
 If that would be simpler for you, i could post a patch for this which i made on top of your series.
Sorry, i don't have time to respin the whole thing, busy with qemu GICv3 fight :)

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hello!

> OK thanks for sharing. I need some more time to study qemu code too.

 I am currently working on supporting this in qemu. Not ready yet, need some time. But, with API i
suggest, things are really much-much simpler.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

On 07/02/2015 05:39 PM, Pavel Fedin wrote:
>  Hello!
> 
>> OK thanks for sharing. I need some more time to study qemu code too.
> 
>  I am currently working on supporting this in qemu. Not ready yet, need some time. But, with API i
> suggest, things are really much-much simpler.

OK so both of you say the same thing. Will respin accordingly

Eric
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [PATCH 2/7] KVM: kvm_host: add kvm_extended_msi

[Andre Przywara]

Hi Eric,

just played a bit with the code and I could make things easier by the
following change:

On 29/06/15 16:37, Eric Auger wrote:
> Add a new kvm_extended_msi struct to store the additional device ID
> specific to ARM. kvm_kernel_irq_routing_entry union now encompasses
> this new struct.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> RFC -> PATCH:
> - reword the commit message after change in first patch (uapi)
> ---
>  include/linux/kvm_host.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ad45054..e1c1c0d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -304,6 +304,13 @@ struct kvm_s390_adapter_int {
>  	u32 adapter_id;
>  };
>  
> +struct kvm_extended_msi {
> +	u32     address_lo;     /* low 32 bits of msi message address */
> +	u32     address_hi;     /* high 32 bits of msi message address */
> +	u32     data;           /* 16 bits of msi message data */
> +	u32	devid;		/* out-of-band device ID */
> +};
> +

I got rid of this structure at all, instead using:

 @@ -317,6 +324,7 @@ struct kvm_kernel_irq_routing_entry {
  		} irqchip;
-		struct msi_msg msi;
+		struct {
+			struct msi_msg msi;
+			u32 devid;
+		};
  		struct kvm_s390_adapter_int adapter;
  	};
  	struct hlist_node link;
  };

This re-uses the existing MSI fields in struct msi_msg, so all the extra
code you added in the next patches to set address and data could be
skipped. If needed we can add a flags field here as well to avoid that
extra type.
That simplified a lot for me.

Cheers,
André

Re: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Andre Przywara]

Hi Eric,

On 29/06/15 16:37, Eric Auger wrote:
> If the ITS modality is not available, let's simply support MSI
> injection by transforming the MSI.data into an SPI ID.
> 
> This becomes possible to use KVM_SIGNAL_MSI ioctl for arm too.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  arch/arm/kvm/Kconfig | 1 +
>  virt/kvm/arm/vgic.c  | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
> index 151e710..0f58baf 100644
> --- a/arch/arm/kvm/Kconfig
> +++ b/arch/arm/kvm/Kconfig
> @@ -31,6 +31,7 @@ config KVM
>  	select KVM_VFIO
>  	select HAVE_KVM_EVENTFD
>  	select HAVE_KVM_IRQFD
> +	select HAVE_KVM_MSI
>  	select HAVE_KVM_IRQCHIP
>  	select HAVE_KVM_IRQ_ROUTING
>  	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 0b4c48c..b3c10dc 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -2314,6 +2314,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>  			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
>  		else
>  			return -ENODEV;
> +	case KVM_IRQ_ROUTING_MSI:
> +		if (kvm->arch.vgic.vm_ops.inject_msi)
> +			return -EINVAL;
> +		else
> +			return kvm_vgic_inject_irq(kvm, 0, e->msi.data, level);

If you add:

static int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
{
	return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
}

to vgic-v2-emul.c and wire it up accordingly, you can simplify the above
kvm_set_msi, getting rid of all those extra case handling.
This also helps merging KVM_IRQ_ROUTING_MSI and the extended case.

I have hacked this up and it seems to work for me.

Cheers,
Andre.

Re: [PATCH 7/7] KVM: arm: implement kvm_set_msi by gsi direct mapping

[Eric Auger]

Hi Andre,
On 07/02/2015 07:10 PM, Andre Przywara wrote:
> Hi Eric,
> 
> On 29/06/15 16:37, Eric Auger wrote:
>> If the ITS modality is not available, let's simply support MSI
>> injection by transforming the MSI.data into an SPI ID.
>>
>> This becomes possible to use KVM_SIGNAL_MSI ioctl for arm too.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  arch/arm/kvm/Kconfig | 1 +
>>  virt/kvm/arm/vgic.c  | 5 +++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
>> index 151e710..0f58baf 100644
>> --- a/arch/arm/kvm/Kconfig
>> +++ b/arch/arm/kvm/Kconfig
>> @@ -31,6 +31,7 @@ config KVM
>>  	select KVM_VFIO
>>  	select HAVE_KVM_EVENTFD
>>  	select HAVE_KVM_IRQFD
>> +	select HAVE_KVM_MSI
>>  	select HAVE_KVM_IRQCHIP
>>  	select HAVE_KVM_IRQ_ROUTING
>>  	depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 0b4c48c..b3c10dc 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -2314,6 +2314,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>>  			return kvm->arch.vgic.vm_ops.inject_msi(kvm, &msi);
>>  		else
>>  			return -ENODEV;
>> +	case KVM_IRQ_ROUTING_MSI:
>> +		if (kvm->arch.vgic.vm_ops.inject_msi)
>> +			return -EINVAL;
>> +		else
>> +			return kvm_vgic_inject_irq(kvm, 0, e->msi.data, level);
> 
> If you add:
> 
> static int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
> {
> 	return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
> }
> 
> to vgic-v2-emul.c and wire it up accordingly, you can simplify the above
> kvm_set_msi, getting rid of all those extra case handling.
> This also helps merging KVM_IRQ_ROUTING_MSI and the extended case.
> 
> I have hacked this up and it seems to work for me.

OK thanks I will respin either today or on monday.

Best Regards

Eric
> 
> Cheers,
> Andre.
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Pavel,

On 02/07/15 08:26, Pavel Fedin wrote:
>  Hello!
> 
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>> Sent: Monday, June 29, 2015 6:37 PM
>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
>>
>> On ARM, the MSI msg (address and data) comes along with
>> out-of-band device ID information. The device ID encodes the device
>> that composes the MSI msg. Let's create a new routing entry type,
>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
>> to convey the device ID.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> RFC -> PATCH
>> - remove kvm_irq_routing_extended_msi and use union instead
>> ---
>>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>>  include/uapi/linux/kvm.h          | 6 +++++-
>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index d20fd94..6426ae9 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>>  	__u32 gsi;
>>  	__u32 type;
>>  	__u32 flags;
>> -	__u32 pad;
>> +	union {
>> +		__u32 pad;
>> +		__u32 devid;
>> +	};
>>  	union {
>>  		struct kvm_irq_routing_irqchip irqchip;
>>  		struct kvm_irq_routing_msi msi;
> 
>  devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
> It also has reserved pad.
> 
>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>  #define KVM_IRQ_ROUTING_MSI 2
>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>> +
>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
>> +the device ID.
>>
>>  No flags are specified so far, the corresponding field must be set to zero.
> 
> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
> believe this would make an API more consistent and introduce less new definitions.

I like this approach, but it runs into problems:
As you read above the current documentation says that the flags field
must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
isn't. So userland would need to know whether it's safe to set that
field. Introducing a new KVM_CAP_... value seems overkill if we could
just have a new routing entry type. So we could still reuse the existing
struct kvm_irq_routing_msi (and extend that with the devid field), but
we would have to add a new routing type number.
Maybe we could collapse this into the existing MSI type + flag when
handing it further down the kernel?

Cheers,
Andre.

> 
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 2a23705..8484681 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>  #define KVM_IRQ_ROUTING_MSI 2
>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>
>>  struct kvm_irq_routing_entry {
>>  	__u32 gsi;
>>  	__u32 type;
>>  	__u32 flags;
>> -	__u32 pad;
>> +	union {
>> +		__u32 pad;
>> +		__u32 devid;
>> +	};
>>  	union {
>>  		struct kvm_irq_routing_irqchip irqchip;
>>  		struct kvm_irq_routing_msi msi;
>> --
>> 1.9.1
>>
>> --
>> 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
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hi!

> OK so both of you say the same thing. Will respin accordingly

 You may also want to add this:
Tested-by: Pavel Fedin <p.fedin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

On 07/03/2015 05:29 PM, Pavel Fedin wrote:
>  Hi!
> 
>> OK so both of you say the same thing. Will respin accordingly
> 
>  You may also want to add this:
> Tested-by: Pavel Fedin <p.fedin@samsung.com>

Thanks Pavel for the intent. However since I am going to change the uapi
and correct the bug you spotted out, this will need to be tested again.
T-b is applied when the code is stable and bug-free I think ;-)

Best Regards

Eric
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi,

On 03/07/15 10:05, Andre Przywara wrote:
> Hi Pavel,
> 
> On 02/07/15 08:26, Pavel Fedin wrote:
>>  Hello!
>>
>>> -----Original Message-----
>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger
>>> Sent: Monday, June 29, 2015 6:37 PM
>>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org;
>>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com;
>>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com
>>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi
>>>
>>> On ARM, the MSI msg (address and data) comes along with
>>> out-of-band device ID information. The device ID encodes the device
>>> that composes the MSI msg. Let's create a new routing entry type,
>>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space
>>> to convey the device ID.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>>
>>> RFC -> PATCH
>>> - remove kvm_irq_routing_extended_msi and use union instead
>>> ---
>>>  Documentation/virtual/kvm/api.txt | 9 ++++++++-
>>>  include/uapi/linux/kvm.h          | 6 +++++-
>>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index d20fd94..6426ae9 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry {
>>>  	__u32 gsi;
>>>  	__u32 type;
>>>  	__u32 flags;
>>> -	__u32 pad;
>>> +	union {
>>> +		__u32 pad;
>>> +		__u32 devid;
>>> +	};
>>>  	union {
>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>  		struct kvm_irq_routing_msi msi;
>>
>>  devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then?
>> It also has reserved pad.
>>
>>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry {
>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>> +
>>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey
>>> +the device ID.
>>>
>>>  No flags are specified so far, the corresponding field must be set to zero.
>>
>> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I
>> believe this would make an API more consistent and introduce less new definitions.
> 
> I like this approach, but it runs into problems:
> As you read above the current documentation says that the flags field
> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
> isn't. So userland would need to know whether it's safe to set that
> field. Introducing a new KVM_CAP_... value seems overkill if we could
> just have a new routing entry type. So we could still reuse the existing
> struct kvm_irq_routing_msi (and extend that with the devid field), but
> we would have to add a new routing type number.

> Maybe we could collapse this into the existing MSI type + flag when
> handing it further down the kernel?

FWIW, I gave this a try, this doesn't look to bad. I carried the new
type down till virt/kvm/arm/vgic.c:kvm_set_routing_entry(), where the
EXTENDED type got turned back into the normal MSI type while setting the
flag in the internal struct kvm_kernel_irq_routing_entry. This keeps the
new type only to the userland facing side, with the kernel code staying
mostly the same.
Together with a new KVM_CAP_MSIS_REQUIRE_DEVID capability I can now
drive both GICv2M and ITS emulation from the same userland base in a
sane manner.
If someone wants to have a look now, tell me, otherwise I will wait for
Eric's upcoming code drop and comment on that then.

Cheers,
Andre.

> 
> Cheers,
> Andre.
> 
>>
>>>
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index 2a23705..8484681 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter {
>>>  #define KVM_IRQ_ROUTING_IRQCHIP 1
>>>  #define KVM_IRQ_ROUTING_MSI 2
>>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4
>>>
>>>  struct kvm_irq_routing_entry {
>>>  	__u32 gsi;
>>>  	__u32 type;
>>>  	__u32 flags;
>>> -	__u32 pad;
>>> +	union {
>>> +		__u32 pad;
>>> +		__u32 devid;
>>> +	};
>>>  	union {
>>>  		struct kvm_irq_routing_irqchip irqchip;
>>>  		struct kvm_irq_routing_msi msi;
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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
>>
>> Kind regards,
>> Pavel Fedin
>> Expert Engineer
>> Samsung Electronics Research center Russia
>>

Re: [PATCH 0/7] KVM: arm/arm64: gsi routing support

[Christoffer Dall]

On Mon, Jun 29, 2015 at 05:37:10PM +0200, Eric Auger wrote:
> With the advent of GICv3 ITS in-kernel emulation, KVM GSI routing
> appears to be requested. More specifically MSI routing is needed.
> irqchip routing does not sound to be really useful on arm but usage of
> MSI routing also mandates to integrate irqchip routing. The initial
> implementation of irqfd on arm must be upgraded with the integration
> of kvm irqchip.c code and the implementation of its standard hooks
> in the architecture specific part.
> 
> In case KVM_SET_GSI_ROUTING ioctl is not called, a default routing
> table with flat irqchip routing entries is built enabling to inject gsi
> corresponding to the SPI indexes seen by the guest.
> 
> As soon as KVM_SET_GSI_ROUTING is called, user-space overwrites this
> default routing table and is responsible for building the whole routing
> table.
> 
> for arm/arm64 KVM_SET_GSI_ROUTING has a limited support:
> - only applies to KVM_IRQFD and not to KVM_IRQ_LINE
> 
> - irqchip routing was tested on Calxeda midway (VFIO with irqfd)
> - MSI routing without GICv3 ITS was tested using APM Xgene-I
>   (qemu VIRTIO-PCI vhost net without gsi_direct_mapping).
> - MSI routing with GICv2 ITS is NOT yet tested.

Do you mean GICv3 ITS here?

> 
> Code can be found at https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-gsi-routing-patch
> 
> It applies on Andre's [PATCH 00/13] arm64: KVM: GICv3 ITS emulation
> (http://www.spinics.net/lists/kvm/msg117402.html)
> 
> History:
> 
> RFC -> PATCH:
> - clearly state limited support on arm/arm64:
>   KVM_IRQ_LINE not impacted by GSI routing
> - add default routing table feature (new patch file)
> - changed uapi to use padding field area
> - reword api.txt
> 
> 
> Eric Auger (7):
>   KVM: api: add kvm_irq_routing_extended_msi
>   KVM: kvm_host: add kvm_extended_msi
>   KVM: irqchip: convey devid to kvm_set_msi
>   KVM: arm/arm64: enable irqchip routing
>   KVM: arm/arm64: build a default routing table
>   KVM: arm/arm64: enable MSI routing
>   KVM: arm: implement kvm_set_msi by gsi direct mapping
> 
>  Documentation/virtual/kvm/api.txt |  30 ++++++++--
>  arch/arm/include/asm/kvm_host.h   |   2 +
>  arch/arm/kvm/Kconfig              |   3 +
>  arch/arm/kvm/Makefile             |   2 +-
>  arch/arm64/include/asm/kvm_host.h |   1 +
>  arch/arm64/kvm/Kconfig            |   2 +
>  arch/arm64/kvm/Makefile           |   2 +-
>  include/kvm/arm_vgic.h            |   9 ---
>  include/linux/kvm_host.h          |  10 ++++
>  include/uapi/linux/kvm.h          |   6 +-
>  virt/kvm/arm/vgic.c               | 117 ++++++++++++++++++++++++++++----------
>  virt/kvm/irqchip.c                |  20 +++++--
>  12 files changed, 154 insertions(+), 50 deletions(-)
> 
> -- 
> 1.9.1
> 

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hello!

> I like this approach, but it runs into problems:
> As you read above the current documentation says that the flags field
> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
> isn't. So userland would need to know whether it's safe to set that
> field.

 This problem does not exist because:
a) Older platforms do not need this flag, so they expect to get zero.
b) ARM64 + GICv3 platform cannot work without this flag.

 This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or
not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS
code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to
the related calls.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Pavel,

On 06/07/15 07:42, Pavel Fedin wrote:
>  Hello!
> 
>> I like this approach, but it runs into problems:
>> As you read above the current documentation says that the flags field
>> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
>> isn't. So userland would need to know whether it's safe to set that
>> field.
> 
>  This problem does not exist because:
> a) Older platforms do not need this flag, so they expect to get zero.
> b) ARM64 + GICv3 platform cannot work without this flag.
> 
>  This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or
> not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS
> code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to
> the related calls.

Well, I had this solution before in kvmtool: If ARM && ITS then set the
flag. But I wasn't really happy with this, as the IRQ routing, setup and
injection code is rather architecture agnostic (implementing the generic
KVM interface), so spraying in some architecture hacks sounded not very
elegant.
Also as the flag describes a rather generic feature (provide an unique
device ID), I'd rather avoid to make this an ARM hack.

That being said this is not a show stopper for me, so if the others are
happy with this, I will go down your road.

Cheers,
Andre.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Christoffer Dall]

On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote:
> Hi Pavel,
> 
> On 06/07/15 07:42, Pavel Fedin wrote:
> >  Hello!
> > 
> >> I like this approach, but it runs into problems:
> >> As you read above the current documentation says that the flags field
> >> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
> >> isn't. So userland would need to know whether it's safe to set that
> >> field.
> > 
> >  This problem does not exist because:
> > a) Older platforms do not need this flag, so they expect to get zero.
> > b) ARM64 + GICv3 platform cannot work without this flag.
> > 
> >  This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or
> > not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS
> > code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to
> > the related calls.
> 
> Well, I had this solution before in kvmtool: If ARM && ITS then set the
> flag. But I wasn't really happy with this, as the IRQ routing, setup and
> injection code is rather architecture agnostic (implementing the generic
> KVM interface), so spraying in some architecture hacks sounded not very
> elegant.
> Also as the flag describes a rather generic feature (provide an unique
> device ID), I'd rather avoid to make this an ARM hack.
> 
> That being said this is not a show stopper for me, so if the others are
> happy with this, I will go down your road.
> 
There must be some way for userspace to discover if it's valid to set
the flag or not; either through a well-defined error-code probing
mechanism for KVM_SET_GSI_ROUTING or through advertising a capability.

-Christoffer

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Christoffer,

On 06/07/15 10:30, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote:
>> Hi Pavel,
>>
>> On 06/07/15 07:42, Pavel Fedin wrote:
>>>  Hello!
>>>
>>>> I like this approach, but it runs into problems:
>>>> As you read above the current documentation says that the flags field
>>>> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
>>>> isn't. So userland would need to know whether it's safe to set that
>>>> field.
>>>
>>>  This problem does not exist because:
>>> a) Older platforms do not need this flag, so they expect to get zero.
>>> b) ARM64 + GICv3 platform cannot work without this flag.
>>>
>>>  This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or
>>> not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS
>>> code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to
>>> the related calls.
>>
>> Well, I had this solution before in kvmtool: If ARM && ITS then set the
>> flag. But I wasn't really happy with this, as the IRQ routing, setup and
>> injection code is rather architecture agnostic (implementing the generic
>> KVM interface), so spraying in some architecture hacks sounded not very
>> elegant.
>> Also as the flag describes a rather generic feature (provide an unique
>> device ID), I'd rather avoid to make this an ARM hack.
>>
>> That being said this is not a show stopper for me, so if the others are
>> happy with this, I will go down your road.
>>
> There must be some way for userspace to discover if it's valid to set
> the flag or not; either through a well-defined error-code probing
> mechanism for KVM_SET_GSI_ROUTING or through advertising a capability.

Right, makes sense, I was wondering about this requirement earlier, but
couldn't find really good prior art in the code (KVM_GET_VCPU_EVENTS
seems to be bad example).
So I think we get along with a new VM specific capability like
KVM_CAP_MSIS_REQUIRE_DEVID. This isn't strictly a "capability" (as it's
more a requirement), but I guess it fits here anyway. It has to be
per-VM, as a GICv2M guest does not need it, but an ITS guest does.
We can use this very flag for both the KVM_SIGNAL_MSI and the
KVM_SET_GSI_ROUTING ioctl, so interface churn is kept minimal.

Does that make sense?

Actually I have implemented this already last week, I will send it out
along with a v2 of the ITS emulation later this week.

Cheers,
Andre.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Christoffer Dall]

On Mon, Jul 06, 2015 at 11:05:26AM +0100, Andre Przywara wrote:
> Hi Christoffer,
> 
> On 06/07/15 10:30, Christoffer Dall wrote:
> > On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote:
> >> Hi Pavel,
> >>
> >> On 06/07/15 07:42, Pavel Fedin wrote:
> >>>  Hello!
> >>>
> >>>> I like this approach, but it runs into problems:
> >>>> As you read above the current documentation says that the flags field
> >>>> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it
> >>>> isn't. So userland would need to know whether it's safe to set that
> >>>> field.
> >>>
> >>>  This problem does not exist because:
> >>> a) Older platforms do not need this flag, so they expect to get zero.
> >>> b) ARM64 + GICv3 platform cannot work without this flag.
> >>>
> >>>  This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or
> >>> not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS
> >>> code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to
> >>> the related calls.
> >>
> >> Well, I had this solution before in kvmtool: If ARM && ITS then set the
> >> flag. But I wasn't really happy with this, as the IRQ routing, setup and
> >> injection code is rather architecture agnostic (implementing the generic
> >> KVM interface), so spraying in some architecture hacks sounded not very
> >> elegant.
> >> Also as the flag describes a rather generic feature (provide an unique
> >> device ID), I'd rather avoid to make this an ARM hack.
> >>
> >> That being said this is not a show stopper for me, so if the others are
> >> happy with this, I will go down your road.
> >>
> > There must be some way for userspace to discover if it's valid to set
> > the flag or not; either through a well-defined error-code probing
> > mechanism for KVM_SET_GSI_ROUTING or through advertising a capability.
> 
> Right, makes sense, I was wondering about this requirement earlier, but
> couldn't find really good prior art in the code (KVM_GET_VCPU_EVENTS
> seems to be bad example).
> So I think we get along with a new VM specific capability like
> KVM_CAP_MSIS_REQUIRE_DEVID. This isn't strictly a "capability" (as it's
> more a requirement), but I guess it fits here anyway. It has to be
> per-VM, as a GICv2M guest does not need it, but an ITS guest does.
> We can use this very flag for both the KVM_SIGNAL_MSI and the
> KVM_SET_GSI_ROUTING ioctl, so interface churn is kept minimal.
> 
> Does that make sense?
> 
> Actually I have implemented this already last week, I will send it out
> along with a v2 of the ITS emulation later this week.
> 
I don't view it as 'the kernel requires this' but as 'the kernel will
not complain with arbitrary error code if you set the devid flag'
capability, and it's up to userspace (as usual) to provide the correct
arguments for things to work, and up to the kernel to ensure we don't
crash the system etc.

Thus, if you want to advertise it as a capability, I would rather call
it KVM_CAP_MSI_DEVID.

The question is if userspace code that sets the devid flag will anyway
depend on some discovery mechanism of whether or not the kernel supports
arm64 irqfd etc. and if so, can we be sure to add the required support
at once in the kernel so that EINVAL never means 'you set the flags
field on the ioctl on an old kernel'?

This smells an awful lot like a capability to me.

-Christoffer

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Paolo Bonzini]

On 06/07/2015 12:37, Christoffer Dall wrote:
> I don't view it as 'the kernel requires this' but as 'the kernel will
> not complain with arbitrary error code if you set the devid flag'
> capability, and it's up to userspace (as usual) to provide the correct
> arguments for things to work, and up to the kernel to ensure we don't
> crash the system etc.
> 
> Thus, if you want to advertise it as a capability, I would rather call
> it KVM_CAP_MSI_DEVID.

I agree.  Does userspace know that ITS guests always require devid?  I
guess it's okay to return -EINVAL if the userspace doesn't set the flag
but the virtual hardware requires it.

Paolo

> The question is if userspace code that sets the devid flag will anyway
> depend on some discovery mechanism of whether or not the kernel supports
> arm64 irqfd etc. and if so, can we be sure to add the required support
> at once in the kernel so that EINVAL never means 'you set the flags
> field on the ioctl on an old kernel'?
> 
> This smells an awful lot like a capability to me.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Paolo,

thanks for looking at this!

On 06/07/15 12:07, Paolo Bonzini wrote:
> 
> 
> On 06/07/2015 12:37, Christoffer Dall wrote:
>> I don't view it as 'the kernel requires this' but as 'the kernel will
>> not complain with arbitrary error code if you set the devid flag'
>> capability, and it's up to userspace (as usual) to provide the correct
>> arguments for things to work, and up to the kernel to ensure we don't
>> crash the system etc.
>>
>> Thus, if you want to advertise it as a capability, I would rather call
>> it KVM_CAP_MSI_DEVID.
> 
> I agree.  Does userspace know that ITS guests always require devid?

Well, as we are about to implement this: yes. But the issue is that MSI
injection and GSI routing code is generic PCI code in userland (at least
in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
ARM specific code in there. The idea is to always provide the device ID
from the PCI code (for PCI devices it's just the B/D/F triplet), but
only send it to the kernel if needed. Querying a KVM capability is
perfectly fine for this IMO.

> I
> guess it's okay to return -EINVAL if the userspace doesn't set the flag
> but the virtual hardware requires it.

Yes, that is what I do in the kernel implementation. And that is
perfectly fine: the ITS emulation does not work without a device ID, the
ITS driver in the guest assigns the very same payload (and address) to
different devices, so there is no way to tell the MSIs apart without a
unique device ID.

Thanks,
Andre.

> 
> Paolo
> 
>> The question is if userspace code that sets the devid flag will anyway
>> depend on some discovery mechanism of whether or not the kernel supports
>> arm64 irqfd etc. and if so, can we be sure to add the required support
>> at once in the kernel so that EINVAL never means 'you set the flags
>> field on the ioctl on an old kernel'?
>>
>> This smells an awful lot like a capability to me.
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Paolo Bonzini]

On 06/07/2015 13:23, Andre Przywara wrote:
> Hi Paolo,
> 
> thanks for looking at this!
> 
> On 06/07/15 12:07, Paolo Bonzini wrote:
>>
>>
>> On 06/07/2015 12:37, Christoffer Dall wrote:
>>> I don't view it as 'the kernel requires this' but as 'the kernel will
>>> not complain with arbitrary error code if you set the devid flag'
>>> capability, and it's up to userspace (as usual) to provide the correct
>>> arguments for things to work, and up to the kernel to ensure we don't
>>> crash the system etc.
>>>
>>> Thus, if you want to advertise it as a capability, I would rather call
>>> it KVM_CAP_MSI_DEVID.
>>
>> I agree.  Does userspace know that ITS guests always require devid?
> 
> Well, as we are about to implement this: yes. But the issue is that MSI
> injection and GSI routing code is generic PCI code in userland (at least
> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
> ARM specific code in there. The idea is to always provide the device ID
> from the PCI code (for PCI devices it's just the B/D/F triplet), but
> only send it to the kernel if needed. Querying a KVM capability is
> perfectly fine for this IMO.

Yes, I agree.

Paolo

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Christoffer Dall]

On Mon, Jul 06, 2015 at 12:23:19PM +0100, Andre Przywara wrote:
> Hi Paolo,
> 
> thanks for looking at this!
> 
> On 06/07/15 12:07, Paolo Bonzini wrote:
> > 
> > 
> > On 06/07/2015 12:37, Christoffer Dall wrote:
> >> I don't view it as 'the kernel requires this' but as 'the kernel will
> >> not complain with arbitrary error code if you set the devid flag'
> >> capability, and it's up to userspace (as usual) to provide the correct
> >> arguments for things to work, and up to the kernel to ensure we don't
> >> crash the system etc.
> >>
> >> Thus, if you want to advertise it as a capability, I would rather call
> >> it KVM_CAP_MSI_DEVID.
> > 
> > I agree.  Does userspace know that ITS guests always require devid?
> 
> Well, as we are about to implement this: yes. But the issue is that MSI
> injection and GSI routing code is generic PCI code in userland (at least
> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
> ARM specific code in there. The idea is to always provide the device ID
> from the PCI code (for PCI devices it's just the B/D/F triplet), but
> only send it to the kernel if needed. Querying a KVM capability is
> perfectly fine for this IMO.
> 
> > I
> > guess it's okay to return -EINVAL if the userspace doesn't set the flag
> > but the virtual hardware requires it.
> 
> Yes, that is what I do in the kernel implementation. And that is
> perfectly fine: the ITS emulation does not work without a device ID, the
> ITS driver in the guest assigns the very same payload (and address) to
> different devices, so there is no way to tell the MSIs apart without a
> unique device ID.
> 
Just so I'm sure I understand: The way the kernel differentiates between
no-devid and devid==0, is whether or not the devid flag is set, correct?

-Christoffer

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hi!

> > Well, as we are about to implement this: yes. But the issue is that MSI
> > injection and GSI routing code is generic PCI code in userland (at least
> > in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
> > ARM specific code in there. The idea is to always provide the device ID
> > from the PCI code (for PCI devices it's just the B/D/F triplet), but
> > only send it to the kernel if needed. Querying a KVM capability is
> > perfectly fine for this IMO.
> 
> Yes, I agree.

 Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability,
and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID. And there is no other way to
use irqfds with GICv3.
 Just for example, this is what i have done in qemu:
--- cut ---
int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev)
{
    struct kvm_irq_routing_entry kroute = {};
    int virq;

    if (kvm_gsi_direct_mapping()) {
        return kvm_arch_msi_data_to_gsi(msg.data);
    }

    if (!kvm_gsi_routing_enabled()) {
        return -ENOSYS;
    }

    virq = kvm_irqchip_get_virq(s);
    if (virq < 0) {
        return virq;
    }

    kroute.gsi = virq;
    kroute.type = KVM_IRQ_ROUTING_MSI;
    kroute.u.msi.address_lo = (uint32_t)msg.address;
    kroute.u.msi.address_hi = msg.address >> 32;
    kroute.u.msi.data = le32_to_cpu(msg.data);
    kroute.flags = kvm_msi_flags;
    if (kroute.flags & KVM_MSI_VALID_DEVID) {
        kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
    }

    if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) {
        kvm_irqchip_release_virq(s, virq);
        return -EINVAL;
    }

    kvm_add_routing_entry(s, &kroute);
    kvm_irqchip_commit_routes(s);

    return virq;
}
--- cut ---

 ITS code in qemu just does:

---cut ---
    msi_supported = true;
    kvm_msi_flags = KVM_MSI_VALID_DEVID;
    kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing();
    kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed;
--- cut ---

 I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if
kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps
be:
--- cut ---
if (kvm_has_gsi_routing()) {
    kvm_msi_flags = KVM_MSI_VALID_DEVID;
    kvm_gsi_routing_allowed = true;
    kvm_msi_via_irqfd_allowed = true;
}
--- cut ---

 I can post my sets as RFCs to qemu mailing list, if you want to take a look at the whole change
set.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

> Just so I'm sure I understand: The way the kernel differentiates between
> no-devid and devid==0, is whether or not the devid flag is set, correct?

 Yes, exactly.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

Hi all,
On 07/06/2015 03:32 PM, Pavel Fedin wrote:
>  Hi!
> 
>>> Well, as we are about to implement this: yes. But the issue is that MSI
>>> injection and GSI routing code is generic PCI code in userland (at least
>>> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
>>> ARM specific code in there. The idea is to always provide the device ID
>>> from the PCI code (for PCI devices it's just the B/D/F triplet), but
>>> only send it to the kernel if needed. Querying a KVM capability is
>>> perfectly fine for this IMO.
>>
>> Yes, I agree.
> 
>  Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability,
> and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID. And there is no other way to
> use irqfds with GICv3.
>  Just for example, this is what i have done in qemu:
> --- cut ---
> int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev)
> {
>     struct kvm_irq_routing_entry kroute = {};
>     int virq;
> 
>     if (kvm_gsi_direct_mapping()) {
>         return kvm_arch_msi_data_to_gsi(msg.data);
>     }
> 
>     if (!kvm_gsi_routing_enabled()) {
>         return -ENOSYS;
>     }
> 
>     virq = kvm_irqchip_get_virq(s);
>     if (virq < 0) {
>         return virq;
>     }
> 
>     kroute.gsi = virq;
>     kroute.type = KVM_IRQ_ROUTING_MSI;
>     kroute.u.msi.address_lo = (uint32_t)msg.address;
>     kroute.u.msi.address_hi = msg.address >> 32;
>     kroute.u.msi.data = le32_to_cpu(msg.data);
>     kroute.flags = kvm_msi_flags;
>     if (kroute.flags & KVM_MSI_VALID_DEVID) {
>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>     }
> 
>     if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) {
>         kvm_irqchip_release_virq(s, virq);
>         return -EINVAL;
>     }
> 
>     kvm_add_routing_entry(s, &kroute);
>     kvm_irqchip_commit_routes(s);
> 
>     return virq;
> }
> --- cut ---
> 
>  ITS code in qemu just does:
> 
> ---cut ---
>     msi_supported = true;
>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
>     kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing();
>     kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed;
> --- cut ---
> 
>  I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if
> kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps
> be:
> --- cut ---
> if (kvm_has_gsi_routing()) {
>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
Personally I prefer a capability rather than hardcoding a global
variable value in the qemu interrupt controller code. All the more so
typically there is KVM GSI routing cap that could/should? be queried
instead of hardcoding the value I think.

So not sure whether we eventually concluded;-)
- introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not
convinced?
- userspaces puts the devid in struct kvm_irq_routing_msi pad field:
consensus (we do not intrduce a new kvm_irq_routing_ext_msi)
- userspace tells it conveyed a devid by setting
A) the kvm_irq_routing_entry's field?
B) the kvm_irq_routing_entry's type
no consensus. If there is a cap, does it really matter?

Best Regards

Eric
>     kvm_gsi_routing_allowed = true;
>     kvm_msi_via_irqfd_allowed = true;
> }
> --- cut ---
> 
>  I can post my sets as RFCs to qemu mailing list, if you want to take a look at the whole change
> set.
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

On 06/07/15 13:08, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 12:23:19PM +0100, Andre Przywara wrote:
>> Hi Paolo,
>>
>> thanks for looking at this!
>>
>> On 06/07/15 12:07, Paolo Bonzini wrote:
>>>
>>>
>>> On 06/07/2015 12:37, Christoffer Dall wrote:
>>>> I don't view it as 'the kernel requires this' but as 'the kernel will
>>>> not complain with arbitrary error code if you set the devid flag'
>>>> capability, and it's up to userspace (as usual) to provide the correct
>>>> arguments for things to work, and up to the kernel to ensure we don't
>>>> crash the system etc.
>>>>
>>>> Thus, if you want to advertise it as a capability, I would rather call
>>>> it KVM_CAP_MSI_DEVID.
>>>
>>> I agree.  Does userspace know that ITS guests always require devid?
>>
>> Well, as we are about to implement this: yes. But the issue is that MSI
>> injection and GSI routing code is generic PCI code in userland (at least
>> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
>> ARM specific code in there. The idea is to always provide the device ID
>> from the PCI code (for PCI devices it's just the B/D/F triplet), but
>> only send it to the kernel if needed. Querying a KVM capability is
>> perfectly fine for this IMO.
>>
>>> I
>>> guess it's okay to return -EINVAL if the userspace doesn't set the flag
>>> but the virtual hardware requires it.
>>
>> Yes, that is what I do in the kernel implementation. And that is
>> perfectly fine: the ITS emulation does not work without a device ID, the
>> ITS driver in the guest assigns the very same payload (and address) to
>> different devices, so there is no way to tell the MSIs apart without a
>> unique device ID.
>>
> Just so I'm sure I understand: The way the kernel differentiates between
> no-devid and devid==0, is whether or not the devid flag is set, correct?

Yes, that is the idea. The plan for the implementation is like this:
1) If the kernel does not need the device ID (x86, GICv2M), it does not
care about the flag or the value at all.
2) In case for ITS on ARM64, the kernel returns an error is the flag is
not set.

Cheers,
Andre.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Hi Pavel,

On 06/07/15 14:32, Pavel Fedin wrote:
>  Hi!
> 
>>> Well, as we are about to implement this: yes. But the issue is that MSI
>>> injection and GSI routing code is generic PCI code in userland (at least
>>> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of
>>> ARM specific code in there. The idea is to always provide the device ID
>>> from the PCI code (for PCI devices it's just the B/D/F triplet), but
>>> only send it to the kernel if needed. Querying a KVM capability is
>>> perfectly fine for this IMO.
>>
>> Yes, I agree.
> 
>  Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability,
> and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID.

This is the connection that I don't like: We make the decision to
support a flag on a generic KVM interface dependent on some particular
device emulation (for some very specific architecture, also).

> And there is no other way to
> use irqfds with GICv3.

For now: yes, but I fail to see why the GICv3 is so special that is
justifies an extra handling in the KVM interrupt routing code. If it is
special, lets name it explicitly why: we need a device ID.

>  Just for example, this is what i have done in qemu:
> --- cut ---
> int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev)
> {
>     struct kvm_irq_routing_entry kroute = {};
>     int virq;
> 
>     if (kvm_gsi_direct_mapping()) {
>         return kvm_arch_msi_data_to_gsi(msg.data);
>     }
> 
>     if (!kvm_gsi_routing_enabled()) {
>         return -ENOSYS;
>     }
> 
>     virq = kvm_irqchip_get_virq(s);
>     if (virq < 0) {
>         return virq;
>     }
> 
>     kroute.gsi = virq;
>     kroute.type = KVM_IRQ_ROUTING_MSI;
>     kroute.u.msi.address_lo = (uint32_t)msg.address;
>     kroute.u.msi.address_hi = msg.address >> 32;
>     kroute.u.msi.data = le32_to_cpu(msg.data);
>     kroute.flags = kvm_msi_flags;
>     if (kroute.flags & KVM_MSI_VALID_DEVID) {
>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>     }

Wouldn't:
    if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) {
        kroute.flags = KVM_MSI_VALID_DEVID;
        kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
    }

be saner (without a global variable)?
That would make the interface more consistent, with a new flag being
protected by a new capability.

Cheers,
Andre.

>     if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) {
>         kvm_irqchip_release_virq(s, virq);
>         return -EINVAL;
>     }
> 
>     kvm_add_routing_entry(s, &kroute);
>     kvm_irqchip_commit_routes(s);
> 
>     return virq;
> }

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Salut Eric,

....

>>  ITS code in qemu just does:
>>
>> ---cut ---
>>     msi_supported = true;
>>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
>>     kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing();
>>     kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed;
>> --- cut ---
>>
>>  I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if
>> kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps
>> be:
>> --- cut ---
>> if (kvm_has_gsi_routing()) {
>>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
> Personally I prefer a capability rather than hardcoding a global
> variable value in the qemu interrupt controller code. All the more so
> typically there is KVM GSI routing cap that could/should? be queried
> instead of hardcoding the value I think.
> 
> So not sure whether we eventually concluded;-)
> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not
> convinced?

OK for me.

> - userspaces puts the devid in struct kvm_irq_routing_msi pad field:
> consensus (we do not intrduce a new kvm_irq_routing_ext_msi)

OK for me.

> - userspace tells it conveyed a devid by setting
> A) the kvm_irq_routing_entry's field?

You mean kvm_irq_routing_entry's "flags" here?

> B) the kvm_irq_routing_entry's type

So personally I don't like it so much to use the generic flags field to
specify the meaning within one particular type only. Using a new type
instead seems to be more consistent, reusing an existing struct for that
sounds even better.
As written before (and coded in my branch) we can collapse that into the
existing MSI type while translating that into the kernel internal
routing structure to make the kernel code changes minimal.

> no consensus. If there is a cap, does it really matter?

I guess not. But I prefer the new type anyway, as it also has a known
error path for older kernels.

Cheers,
Andre.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Paolo Bonzini]

On 06/07/2015 17:37, Andre Przywara wrote:
> Wouldn't:
>     if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) {
>         kroute.flags = KVM_MSI_VALID_DEVID;
>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>     }
> 
> be saner (without a global variable)?
> That would make the interface more consistent, with a new flag being
> protected by a new capability.

I agree that your version is niceer, but you still need to cache the
kvm_vm_check_extension result... in a global variable. :)

Paolo

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

On 06/07/15 16:54, Paolo Bonzini wrote:
> 
> 
> On 06/07/2015 17:37, Andre Przywara wrote:
>> Wouldn't:
>>     if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) {
>>         kroute.flags = KVM_MSI_VALID_DEVID;
>>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>>     }
>>
>> be saner (without a global variable)?
>> That would make the interface more consistent, with a new flag being
>> protected by a new capability.
> 
> I agree that your version is niceer, but you still need to cache the
> kvm_vm_check_extension result... in a global variable. :)

I used a static variable in a wrapper function in kvmtool ;-)
TBH my argument wasn't so much about global variables (just saw that
QEMU seems to use them already), but more about a consistent and
architecture agnostic interface.

Ciao!
Andre.

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

On 07/06/2015 05:52 PM, Andre Przywara wrote:
> Salut Eric,
> 
> ....
> 
>>>  ITS code in qemu just does:
>>>
>>> ---cut ---
>>>     msi_supported = true;
>>>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
>>>     kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing();
>>>     kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed;
>>> --- cut ---
>>>
>>>  I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if
>>> kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps
>>> be:
>>> --- cut ---
>>> if (kvm_has_gsi_routing()) {
>>>     kvm_msi_flags = KVM_MSI_VALID_DEVID;
>> Personally I prefer a capability rather than hardcoding a global
>> variable value in the qemu interrupt controller code. All the more so
>> typically there is KVM GSI routing cap that could/should? be queried
>> instead of hardcoding the value I think.
>>
>> So not sure whether we eventually concluded;-)
>> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not
>> convinced?
> 
> OK for me.
> 
>> - userspaces puts the devid in struct kvm_irq_routing_msi pad field:
>> consensus (we do not intrduce a new kvm_irq_routing_ext_msi)
> 
> OK for me.
> 
>> - userspace tells it conveyed a devid by setting
>> A) the kvm_irq_routing_entry's field?
> 
> You mean kvm_irq_routing_entry's "flags" here?
yes!!
> 
>> B) the kvm_irq_routing_entry's type
> 
> So personally I don't like it so much to use the generic flags field to
> specify the meaning within one particular type only. Using a new type
> instead seems to be more consistent, reusing an existing struct for that
> sounds even better.
> As written before (and coded in my branch) we can collapse that into the
> existing MSI type while translating that into the kernel internal
> routing structure to make the kernel code changes minimal.
> 
>> no consensus. If there is a cap, does it really matter?
> 
> I guess not. But I prefer the new type anyway, as it also has a known
> error path for older kernels.
I am fine with the new type too.

Eric
> 
> Cheers,
> Andre.
> 

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hello!

> Wouldn't:
>     if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) {
>         kroute.flags = KVM_MSI_VALID_DEVID;
>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>     }
> 
> be saner (without a global variable)?

 No it would not, because:
a) kvm_vm_check_extension() calls ioctl every time, therefore it's slow. But, well, doesn't really
matter because it's possible to check for the capability once in generic code, and cache it.
b) Capability is a global thing as far as i understand. The kernel either has it, or doesn't have.
However, whether we want this flag or not, depends also on what GIC model we use. GICv2(m) doesn't
want it, GICv3 does. qemu actually has two sets of flags: one set actually specifies capabilities,
another set enables use of these capabilities.
 But, well, you can make GICv2 kernel code simply ignoring it instead of bailing out if flags != 0.
And add the capability for ARM64 architecture (ARM32 can't use GICv3, can it?). And this will work
and it'll be OK. So, i'm not against it, and if you want it, you can do it. I just want to point
that it is not strictly necessary to add new APIs, because existing ones are pretty much enough.
But, you are the architects here, so you of course can do it if you want. It's just me being not a
big fan of adding APIs without which it's completely possible to live.

 Below i'm answering to Eric's comment, because my reply is tightly coupled with this one.

> So not sure whether we eventually concluded;-)
> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not
>  convinced?

 See above. I'm not against it, i just don't think it's necessary. You can do it if you want, it
actually won't change things much.

> - userspaces puts the devid in struct kvm_irq_routing_msi pad field:
> consensus (we do not intrduce a new kvm_irq_routing_ext_msi)

 Yes.

> - userspace tells it conveyed a devid by setting
> A) the kvm_irq_routing_entry's field?
> B) the kvm_irq_routing_entry's type
> no consensus. If there is a cap, does it really matter?

 It has absolutely nothing to do with the cap. My argument here is the same as above again - why
adding new API's / definitions? We already have KVM_MSI_VALID_DEVID and we already have 'flags'
field. Using them would just make the API more consistent because KVM_SIGNAL_MSI already uses them
in absolutely the same manner. That's my point and nothing more.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

 Hi!

> I guess not. But I prefer the new type anyway, as it also has a known
> error path for older kernels.

 flags != 0 has known error path too, and it's absolutely the same.
 Sorry, read this after writing my previous reply, so this is a short addendum.

 I see lots of people agreed on a new type. If my argument about reusing existing definitions is not
enough, you can ignore it. Three people beat one definitely. :)
 And yes, since we are talking about it, actually KVM_MSI_VALID_DEVID flag is not yet a part of
mainline, so it's not set in stone. Then, perhaps you could throw it away completely and invent
KVM_SIGNAL_EXT_MSI ioctl for sending MSIs with device ID. This would also be consistent IMO.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Eric Auger]

Hi Pavel,
On 07/07/2015 09:23 AM, Pavel Fedin wrote:
>  Hi!
> 
>> I guess not. But I prefer the new type anyway, as it also has a known
>> error path for older kernels.
> 
>  flags != 0 has known error path too, and it's absolutely the same.
>  Sorry, read this after writing my previous reply, so this is a short addendum.
> 
>  I see lots of people agreed on a new type. If my argument about reusing existing definitions is not
> enough, you can ignore it. Three people beat one definitely. :)
OK. let's move forward and use this new type. I will repost soon so
everyone can re-check the fit at kvmtool/qemu.

Thanks

Eric
>  And yes, since we are talking about it, actually KVM_MSI_VALID_DEVID flag is not yet a part of
> mainline, so it's not set in stone. Then, perhaps you could throw it away completely and invent
> KVM_SIGNAL_EXT_MSI ioctl for sending MSIs with device ID. This would also be consistent IMO.
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Andre Przywara]

Good morning Pavel,

On 07/07/15 08:16, Pavel Fedin wrote:
>  Hello!
> 
>> Wouldn't:
>>     if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) {
>>         kroute.flags = KVM_MSI_VALID_DEVID;
>>         kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn;
>>     }
>>
>> be saner (without a global variable)?
> 
>  No it would not, because:
> a) kvm_vm_check_extension() calls ioctl every time, therefore it's slow. But, well, doesn't really
> matter because it's possible to check for the capability once in generic code, and cache it.

Indeed, as mentioned before I have it in a wrapper function with a
static variable.

> b) Capability is a global thing as far as i understand. The kernel either has it, or doesn't have.

There are two flavours of capabilities: global and per-VM ones,
depending on which fd you are issuing the ioctl. The per-VM ones are
just not very widely used yet (I only found PowerPC doing so).

> However, whether we want this flag or not, depends also on what GIC model we use. GICv2(m) doesn't
> want it, GICv3 does. qemu actually has two sets of flags: one set actually specifies capabilities,
> another set enables use of these capabilities.

That's why I do a per-VM capability check and I do it as late as
possible to let the GIC initialize first (hence the wrapper function).

>  But, well, you can make GICv2 kernel code simply ignoring it instead of bailing out if flags != 0.
> And add the capability for ARM64 architecture (ARM32 can't use GICv3, can it?). And this will work
> and it'll be OK. So, i'm not against it, and if you want it, you can do it. I just want to point
> that it is not strictly necessary to add new APIs, because existing ones are pretty much enough.

As said before I don't like the idea of inferring the validity of a flag
by some hard-coded dependencies like "GICv3 on ARM64". I guess ARM(32)
will get GICv3 support sooner or later (I think I saw patches to do so
already). So as soon as a kernel supports it, we automatically get the
support from userland without changing a single line there. Also what
tells you that no other architecture or IRQ controller will ever need a
device ID? I just don't want to end up with something like:
(GICV3 && ARM64) || (GICV3 && ARM && KERNEL>4.4) || (SuperIRQC && i986)
or
(ARM || ARM64) && HAS_IRQ_ROUTING

Instead: If the kernel needs it, it tells you. Full stop.

> But, you are the architects here, so you of course can do it if you want.
> It's just me being not a
> big fan of adding APIs without which it's completely possible to live.
> 
>  Below i'm answering to Eric's comment, because my reply is tightly coupled with this one.
> 
>> So not sure whether we eventually concluded;-)
>> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not
>>  convinced?
> 
>  See above. I'm not against it, i just don't think it's necessary. You can do it if you want, it
> actually won't change things much.
> 
>> - userspaces puts the devid in struct kvm_irq_routing_msi pad field:
>> consensus (we do not intrduce a new kvm_irq_routing_ext_msi)
> 
>  Yes.
> 
>> - userspace tells it conveyed a devid by setting
>> A) the kvm_irq_routing_entry's field?
>> B) the kvm_irq_routing_entry's type
>> no consensus. If there is a cap, does it really matter?
> 
>  It has absolutely nothing to do with the cap. My argument here is the same as above again - why
> adding new API's / definitions? We already have KVM_MSI_VALID_DEVID and we already have 'flags'
> field. Using them would just make the API more consistent because KVM_SIGNAL_MSI already uses them
> in absolutely the same manner. That's my point and nothing more.

To be honest it's me to blame here to not having introduced the
capability earlier. At the moment ARM has a different code path for
KVM_SIGNAL_MSI, which does not bail out if the flag field is set. With
Eric's patches this changes and we use the irqchip.c generic code, which
returns -EINVAL atm. So I plan to introduce this capability already with
the ITS emulation series, so we can just pick it up in the IRQ routing
series.
So we now have already two users of this, if that makes more sense.

Cheers,
Andre.

RE: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi

[Pavel Fedin]

> I just don't want to end up with something like:
> (GICV3 && ARM64) || (GICV3 && ARM && KERNEL>4.4) || (SuperIRQC && i986)
> or
> (ARM || ARM64) && HAS_IRQ_ROUTING
> 
> Instead: If the kernel needs it, it tells you. Full stop.

 Agree.

> To be honest it's me to blame here to not having introduced the
> capability earlier. At the moment ARM has a different code path for
> KVM_SIGNAL_MSI, which does not bail out if the flag field is set. With
> Eric's patches this changes and we use the irqchip.c generic code, which
> returns -EINVAL atm. So I plan to introduce this capability already with
> the ITS emulation series, so we can just pick it up in the IRQ routing
> series.

 Then may be you follow https://lkml.org/lkml/2015/7/7/115 and replace flag with something like
KVM_SIGNAL_EXT_MSI ioctl ? After all you were one of people who voted against using flags.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia