linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/13] A bunch of vgic-its fixes
@ 2016-07-18 15:54 Marc Zyngier
  2016-07-18 15:54 ` [PATCH 01/13] KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS Marc Zyngier
                   ` (13 more replies)
  0 siblings, 14 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

As the ITS series has been posted 10 times now, and we're still
finding issues, I've decided to directly fix things rather than
commenting on them and waiting for a respin, as we are getting way too
close to the merge window. So I sat over the weekend and started doing
"stuff".

There is a bit of everything in there: some cosmetic things, but also
some more severe bugs (indirect tables were completely foobar) and
regressions (the cpuif stuff that Eric fixed).

Unless someone shouts at me, I intend to put that on top of Andre's
series, and put the whole thing into -next. I've tested it on a LS2085
with kvmtool, using both flat and indirect device tables.

Thanks,

	M.

Eric Auger (1):
  KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS

Marc Zyngier (12):
  irqchip/gicv3-its: Restore all cacheability attributes
  KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref
  KVM: arm64: vgic-its: Fix handling of indirect tables
  KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling
  KVM: arm64: vgic-its: Fix misleading nr_entries in
    vgic_its_check_device_id
  KVM: arm64: vgic-its: Validate the device table L1 entry
  KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables
  KVM: arm64: vgic-its: Add collection allocator/destructor
  KVM: arm64: vgic-its: Add pointer to corresponding kvm_device
  KVM: arm64: vgic-its: Turn device_id validation into generic ID
    validation
  KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other
    handlers
  KVM: arm64: vgic-its: Simplify MAPI error handling

 include/kvm/arm_vgic.h             |   1 +
 include/linux/irqchip/arm-gic-v3.h |  48 ++++++--
 virt/kvm/arm/vgic/vgic-its.c       | 224 ++++++++++++++++++++-----------------
 virt/kvm/arm/vgic/vgic-mmio-v2.c   |   2 +
 virt/kvm/arm/vgic/vgic-mmio.c      |   4 +-
 virt/kvm/arm/vgic/vgic.c           |  10 +-
 virt/kvm/arm/vgic/vgic.h           |   8 ++
 7 files changed, 175 insertions(+), 122 deletions(-)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 01/13] KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 02/13] irqchip/gicv3-its: Restore all cacheability attributes Marc Zyngier
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

From: Eric Auger <eric.auger@redhat.com>

For VGICv2 save and restore the CPU interface registers
are accessed. Restore the modality which has been altered.
Also explicitly set the iodev_type for both the DIST and CPU
interface.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 2 ++
 virt/kvm/arm/vgic/vgic-mmio.c    | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 4152348..b44b359 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -437,6 +437,7 @@ int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 	struct vgic_io_device dev = {
 		.regions = vgic_v2_cpu_registers,
 		.nr_regions = ARRAY_SIZE(vgic_v2_cpu_registers),
+		.iodev_type = IODEV_CPUIF,
 	};
 
 	return vgic_uaccess(vcpu, &dev, is_write, offset, val);
@@ -448,6 +449,7 @@ int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 	struct vgic_io_device dev = {
 		.regions = vgic_v2_dist_registers,
 		.nr_regions = ARRAY_SIZE(vgic_v2_dist_registers),
+		.iodev_type = IODEV_DIST,
 	};
 
 	return vgic_uaccess(vcpu, &dev, is_write, offset, val);
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 26be827..3bad3c5 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -484,7 +484,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 
 	switch (iodev->iodev_type) {
 	case IODEV_CPUIF:
-		return 1;
+		data = region->read(vcpu, addr, len);
+		break;
 	case IODEV_DIST:
 		data = region->read(vcpu, addr, len);
 		break;
@@ -517,6 +518,7 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 
 	switch (iodev->iodev_type) {
 	case IODEV_CPUIF:
+		region->write(vcpu, addr, len, data);
 		break;
 	case IODEV_DIST:
 		region->write(vcpu, addr, len, data);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 02/13] irqchip/gicv3-its: Restore all cacheability attributes
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
  2016-07-18 15:54 ` [PATCH 01/13] KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 03/13] KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref Marc Zyngier
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Let's restore some of the #defines that have been savagely dropped
by the introduction of the KVM ITS code, as pointlessly break
other users (including series that are already in -next).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irqchip/arm-gic-v3.h | 48 +++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 195fe59..f83e91b 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -146,8 +146,16 @@
 
 #define GICR_PROPBASER_InnerShareable					\
 	GIC_BASER_SHAREABILITY(GICR_PROPBASER, InnerShareable)
-#define GICR_PROPBASER_nC GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, nC)
-#define GICR_PROPBASER_WaWb GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, WaWb)
+
+#define GICR_PROPBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, nCnB)
+#define GICR_PROPBASER_nC 	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, nC)
+#define GICR_PROPBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, RaWt)
+#define GICR_PROPBASER_RaWb	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, RaWt)
+#define GICR_PROPBASER_WaWt	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, WaWt)
+#define GICR_PROPBASER_WaWb	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, WaWb)
+#define GICR_PROPBASER_RaWaWt	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, RaWaWt)
+#define GICR_PROPBASER_RaWaWb	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, RaWaWb)
+
 #define GICR_PROPBASER_IDBITS_MASK			(0x1f)
 
 #define GICR_PENDBASER_SHAREABILITY_SHIFT		(10)
@@ -163,8 +171,16 @@
 
 #define GICR_PENDBASER_InnerShareable					\
 	GIC_BASER_SHAREABILITY(GICR_PENDBASER, InnerShareable)
-#define GICR_PENDBASER_nC GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, nC)
-#define GICR_PENDBASER_WaWb GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, WaWb)
+
+#define GICR_PENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, nCnB)
+#define GICR_PENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, nC)
+#define GICR_PENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, RaWt)
+#define GICR_PENDBASER_RaWb	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, RaWt)
+#define GICR_PENDBASER_WaWt	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, WaWt)
+#define GICR_PENDBASER_WaWb	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, WaWb)
+#define GICR_PENDBASER_RaWaWt	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, RaWaWt)
+#define GICR_PENDBASER_RaWaWb	GIC_BASER_CACHEABILITY(GICR_PENDBASER, INNER, RaWaWb)
+
 #define GICR_PENDBASER_PTZ				BIT_ULL(62)
 
 /*
@@ -237,24 +253,40 @@
 
 #define GITS_CBASER_InnerShareable					\
 	GIC_BASER_SHAREABILITY(GITS_CBASER, InnerShareable)
-#define GITS_CBASER_nC GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, nC)
-#define GITS_CBASER_WaWb GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, WaWb)
+
+#define GITS_CBASER_nCnB	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, nCnB)
+#define GITS_CBASER_nC		GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, nC)
+#define GITS_CBASER_RaWt	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, RaWt)
+#define GITS_CBASER_RaWb	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, RaWt)
+#define GITS_CBASER_WaWt	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, WaWt)
+#define GITS_CBASER_WaWb	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, WaWb)
+#define GITS_CBASER_RaWaWt	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, RaWaWt)
+#define GITS_CBASER_RaWaWb	GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, RaWaWb)
 
 #define GITS_BASER_NR_REGS		8
 
 #define GITS_BASER_VALID			(1UL << 63)
 #define GITS_BASER_INDIRECT			(1ULL << 62)
+
 #define GITS_BASER_INNER_CACHEABILITY_SHIFT	(59)
 #define GITS_BASER_OUTER_CACHEABILITY_SHIFT	(53)
 #define GITS_BASER_INNER_CACHEABILITY_MASK				\
 	GIC_BASER_CACHEABILITY(GITS_BASER, INNER, MASK)
+#define GITS_BASER_CACHEABILITY_MASK		GITS_BASER_INNER_CACHEABILITY_MASK
 #define GITS_BASER_OUTER_CACHEABILITY_MASK				\
 	GIC_BASER_CACHEABILITY(GITS_BASER, OUTER, MASK)
 #define GITS_BASER_SHAREABILITY_MASK					\
 	GIC_BASER_SHAREABILITY(GITS_BASER, SHAREABILITY_MASK)
 
-#define GITS_BASER_nC GIC_BASER_CACHEABILITY(GITS_BASER, INNER, nC)
-#define GITS_BASER_WaWb GIC_BASER_CACHEABILITY(GITS_BASER, INNER, WaWb)
+#define GITS_BASER_nCnB		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, nCnB)
+#define GITS_BASER_nC		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, nC)
+#define GITS_BASER_RaWt		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWt)
+#define GITS_BASER_RaWb		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWt)
+#define GITS_BASER_WaWt		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, WaWt)
+#define GITS_BASER_WaWb		GIC_BASER_CACHEABILITY(GITS_BASER, INNER, WaWb)
+#define GITS_BASER_RaWaWt	GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWaWt)
+#define GITS_BASER_RaWaWb	GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWaWb)
+
 #define GITS_BASER_TYPE_SHIFT			(56)
 #define GITS_BASER_TYPE(r)		(((r) >> GITS_BASER_TYPE_SHIFT) & 7)
 #define GITS_BASER_ENTRY_SIZE_SHIFT		(48)
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 03/13] KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
  2016-07-18 15:54 ` [PATCH 01/13] KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS Marc Zyngier
  2016-07-18 15:54 ` [PATCH 02/13] irqchip/gicv3-its: Restore all cacheability attributes Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 04/13] KVM: arm64: vgic-its: Fix handling of indirect tables Marc Zyngier
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Instead of sprinkling raw kref_get() calls everytime we cannot
do a normal vgic_get_irq(), use the existing vgic_get_irq_kref(),
which does the same thing and is paired with a vgic_put_irq().

vgic_get_irq_kref is moved to vgic.h in order to be easily shared.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c |  2 +-
 virt/kvm/arm/vgic/vgic.c     | 10 +---------
 virt/kvm/arm/vgic/vgic.h     |  8 ++++++++
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d8e8f14..f427fa2 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -80,7 +80,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
 		 * call vgic_put_irq() on the returned pointer once it's
 		 * finished with the IRQ.
 		 */
-		kref_get(&irq->refcount);
+		vgic_get_irq_kref(irq);
 
 		goto out_unlock;
 	}
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 424cb9ce..39f3358 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -71,7 +71,7 @@ static struct vgic_irq *vgic_get_lpi(struct kvm *kvm, u32 intid)
 		 * This increases the refcount, the caller is expected to
 		 * call vgic_put_irq() later once it's finished with the IRQ.
 		 */
-		kref_get(&irq->refcount);
+		vgic_get_irq_kref(irq);
 		goto out_unlock;
 	}
 	irq = NULL;
@@ -106,14 +106,6 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
 	return NULL;
 }
 
-static void vgic_get_irq_kref(struct vgic_irq *irq)
-{
-	if (irq->intid < VGIC_MIN_LPI)
-		return;
-
-	kref_get(&irq->refcount);
-}
-
 /*
  * We can't do anything in here, because we lack the kvm pointer to
  * lock and remove the item from the lpi_list. So we keep this function
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 9d40d7b..1d8e21d 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -64,6 +64,14 @@ int vgic_v2_map_resources(struct kvm *kvm);
 int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
 			     enum vgic_type);
 
+static inline void vgic_get_irq_kref(struct vgic_irq *irq)
+{
+	if (irq->intid < VGIC_MIN_LPI)
+		return;
+
+	kref_get(&irq->refcount);
+}
+
 #ifdef CONFIG_KVM_ARM_VGIC_V3
 void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
 void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 04/13] KVM: arm64: vgic-its: Fix handling of indirect tables
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (2 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 03/13] KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 05/13] KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling Marc Zyngier
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

The current code will fail on valid indirect tables, and happily
use the ones that are pointing out of the guest RAM. Funny what a
small "!" can do for you...

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f427fa2..d6697c4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -702,9 +702,9 @@ static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
 		return false;
 
 	/* Each 1st level entry is represented by a 64-bit value. */
-	if (!kvm_read_guest(kvm,
-			    BASER_ADDRESS(r) + index * sizeof(indirect_ptr),
-			    &indirect_ptr, sizeof(indirect_ptr)))
+	if (kvm_read_guest(kvm,
+			   BASER_ADDRESS(r) + index * sizeof(indirect_ptr),
+			   &indirect_ptr, sizeof(indirect_ptr)))
 		return false;
 
 	/* check the valid bit of the first level entry */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 05/13] KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (3 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 04/13] KVM: arm64: vgic-its: Fix handling of indirect tables Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 06/13] KVM: arm64: vgic-its: Fix misleading nr_entries in vgic_its_check_device_id Marc Zyngier
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

The ITS tables are stored in LE format. If the host is reading
a L1 table entry to check its validity, it must convert it to
the CPU endianness.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d6697c4..2ac5927 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -707,6 +707,8 @@ static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
 			   &indirect_ptr, sizeof(indirect_ptr)))
 		return false;
 
+	indirect_ptr = le64_to_cpu(indirect_ptr);
+
 	/* check the valid bit of the first level entry */
 	if (!(indirect_ptr & BIT_ULL(63)))
 		return false;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 06/13] KVM: arm64: vgic-its: Fix misleading nr_entries in vgic_its_check_device_id
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (4 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 05/13] KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 07/13] KVM: arm64: vgic-its: Validate the device table L1 entry Marc Zyngier
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

The nr_entries variable in vgic_its_check_device_id actually
describe the size of the L1 table, and not the number of
entries in this table.

Rename it to l1_tbl_size, so that we can now change the code
with a better understanding of what is what.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 2ac5927..268a0c7 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -687,18 +687,18 @@ static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
 				     int device_id)
 {
 	u64 r = its->baser_device_table;
-	int nr_entries = GITS_BASER_NR_PAGES(r) * SZ_64K;
+	int l1_tbl_size = GITS_BASER_NR_PAGES(r) * SZ_64K;
 	int index;
 	u64 indirect_ptr;
 	gfn_t gfn;
 
 
 	if (!(r & GITS_BASER_INDIRECT))
-		return device_id < (nr_entries / GITS_BASER_ENTRY_SIZE(r));
+		return device_id < (l1_tbl_size / GITS_BASER_ENTRY_SIZE(r));
 
 	/* calculate and check the index into the 1st level */
 	index = device_id / (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
-	if (index >= (nr_entries / sizeof(u64)))
+	if (index >= (l1_tbl_size / sizeof(u64)))
 		return false;
 
 	/* Each 1st level entry is represented by a 64-bit value. */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 07/13] KVM: arm64: vgic-its: Validate the device table L1 entry
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (5 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 06/13] KVM: arm64: vgic-its: Fix misleading nr_entries in vgic_its_check_device_id Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 08/13] KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables Marc Zyngier
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Checking that the device_id fits if the table, and we must make
sure that the associated memory is also accessible.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 268a0c7..4943d6a 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -693,8 +693,17 @@ static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
 	gfn_t gfn;
 
 
-	if (!(r & GITS_BASER_INDIRECT))
-		return device_id < (l1_tbl_size / GITS_BASER_ENTRY_SIZE(r));
+	if (!(r & GITS_BASER_INDIRECT)) {
+		phys_addr_t addr;
+
+		if (device_id >= (l1_tbl_size / GITS_BASER_ENTRY_SIZE(r)))
+			return false;
+
+		addr = BASER_ADDRESS(r) + device_id * GITS_BASER_ENTRY_SIZE(r);
+		gfn = addr >> PAGE_SHIFT;
+
+		return kvm_is_visible_gfn(kvm, gfn);
+	}
 
 	/* calculate and check the index into the 1st level */
 	index = device_id / (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 08/13] KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (6 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 07/13] KVM: arm64: vgic-its: Validate the device table L1 entry Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 09/13] KVM: arm64: vgic-its: Add collection allocator/destructor Marc Zyngier
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

When checking that the storage address of a device entry is valid,
it is critical to compute the actual address of the entry, rather
than relying on the beginning of the page to match a CPU page of
the same size: for example, if the guest places the table at the
last 64kB boundary of RAM, but RAM size isn't a multiple of 64kB...

Fix this by computing the actual offset of the device ID in the
L2 page, and check the corresponding GFN.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 4943d6a..2faf1f4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -727,7 +727,12 @@ static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
 	 * Any address beyond our supported 48 bits of PA will be caught
 	 * by the actual check in the final step.
 	 */
-	gfn = (indirect_ptr & GENMASK_ULL(51, 16)) >> PAGE_SHIFT;
+	indirect_ptr &= GENMASK_ULL(51, 16);
+
+	/* Find the address of the actual entry */
+	index = device_id % (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
+	indirect_ptr += index * GITS_BASER_ENTRY_SIZE(r);
+	gfn = indirect_ptr >> PAGE_SHIFT;
 
 	return kvm_is_visible_gfn(kvm, gfn);
 }
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 09/13] KVM: arm64: vgic-its: Add collection allocator/destructor
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (7 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 08/13] KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 10/13] KVM: arm64: vgic-its: Add pointer to corresponding kvm_device Marc Zyngier
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Instead of spreading random allocations all over the place,
consolidate allocation/init/freeing of collections in a pair
of constructor/destructor.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 92 ++++++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 38 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 2faf1f4..d6f68e9 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -581,14 +581,45 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
-static void vgic_its_init_collection(struct vgic_its *its,
-				     struct its_collection *collection,
+static int vgic_its_alloc_collection(struct vgic_its *its,
+				     struct its_collection **colp,
 				     u32 coll_id)
 {
+	struct its_collection *collection;
+
+	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
+
 	collection->collection_id = coll_id;
 	collection->target_addr = COLLECTION_NOT_MAPPED;
 
 	list_add_tail(&collection->coll_list, &its->collection_list);
+	*colp = collection;
+
+	return 0;
+}
+
+static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
+{
+	struct its_collection *collection;
+	struct its_device *device;
+	struct its_itte *itte;
+
+	/*
+	 * Clearing the mapping for that collection ID removes the
+	 * entry from the list. If there wasn't any before, we can
+	 * go home early.
+	 */
+	collection = find_collection(its, coll_id);
+	if (!collection)
+		return;
+
+	for_each_lpi_its(device, itte, its)
+		if (itte->collection &&
+		    itte->collection->collection_id == coll_id)
+			itte->collection = NULL;
+
+	list_del(&collection->coll_list);
+	kfree(collection);
 }
 
 /*
@@ -605,6 +636,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;
+	int ret;
 
 	device = find_its_device(its, device_id);
 	if (!device)
@@ -612,9 +644,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 	collection = find_collection(its, coll_id);
 	if (!collection) {
-		new_coll = kzalloc(sizeof(struct its_collection), GFP_KERNEL);
-		if (!new_coll)
-			return -ENOMEM;
+		ret = vgic_its_alloc_collection(its, &collection, coll_id);
+		if (ret)
+			return ret;
+		new_coll = collection;
 	}
 
 	if (subcmd == GITS_CMD_MAPTI)
@@ -623,27 +656,22 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		lpi_nr = event_id;
 	if (lpi_nr < GIC_LPI_OFFSET ||
 	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser)) {
-		kfree(new_coll);
-		return E_ITS_MAPTI_PHYSICALID_OOR;
+		ret = E_ITS_MAPTI_PHYSICALID_OOR;
+		goto err;
 	}
 
 	itte = find_itte(its, device_id, event_id);
 	if (!itte) {
 		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
 		if (!itte) {
-			kfree(new_coll);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err;
 		}
 
 		itte->event_id	= event_id;
 		list_add_tail(&itte->itte_list, &device->itt_head);
 	}
 
-	if (!collection) {
-		collection = new_coll;
-		vgic_its_init_collection(its, collection, coll_id);
-	}
-
 	itte->collection = collection;
 	itte->lpi = lpi_nr;
 	itte->irq = vgic_add_lpi(kvm, lpi_nr);
@@ -657,6 +685,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	update_lpi_config(kvm, itte->irq, NULL);
 
 	return 0;
+err:
+	if (new_coll)
+		vgic_its_free_collection(its, coll_id);
+	return ret;
 }
 
 /* Requires the its_lock to be held. */
@@ -809,34 +841,18 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
 	if (coll_id >= vgic_its_nr_collection_ids(its))
 		return E_ITS_MAPC_COLLECTION_OOR;
 
-	collection = find_collection(its, coll_id);
-
 	if (!valid) {
-		struct its_device *device;
-		struct its_itte *itte;
-		/*
-		 * Clearing the mapping for that collection ID removes the
-		 * entry from the list. If there wasn't any before, we can
-		 * go home early.
-		 */
-		if (!collection)
-			return 0;
-
-		for_each_lpi_its(device, itte, its)
-			if (itte->collection &&
-			    itte->collection->collection_id == coll_id)
-				itte->collection = NULL;
-
-		list_del(&collection->coll_list);
-		kfree(collection);
+		vgic_its_free_collection(its, coll_id);
 	} else {
+		collection = find_collection(its, coll_id);
+
 		if (!collection) {
-			collection = kzalloc(sizeof(struct its_collection),
-					     GFP_KERNEL);
-			if (!collection)
-				return -ENOMEM;
+			int ret;
 
-			vgic_its_init_collection(its, collection, coll_id);
+			ret = vgic_its_alloc_collection(its, &collection,
+							coll_id);
+			if (ret)
+				return ret;
 			collection->target_addr = target_addr;
 		} else {
 			collection->target_addr = target_addr;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 10/13] KVM: arm64: vgic-its: Add pointer to corresponding kvm_device
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (8 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 09/13] KVM: arm64: vgic-its: Add collection allocator/destructor Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 11/13] KVM: arm64: vgic-its: Turn device_id validation into generic ID validation Marc Zyngier
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Going from the ITS structure to the corresponding KVM structure
would be quite handy at times. The kvm_device pointer that is
passed at create time is quite convenient for this, so let's
keep a copy of it in the vgic_its structure.

This will be put to a good use in subsequent patches.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h       | 1 +
 virt/kvm/arm/vgic/vgic-its.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4e63a07..540da51 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -138,6 +138,7 @@ struct vgic_its {
 	bool			enabled;
 	bool			initialized;
 	struct vgic_io_device	iodev;
+	struct kvm_device	*dev;
 
 	/* These registers correspond to GITS_BASER{0,1} */
 	u64			baser_device_table;
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d6f68e9..dcae567 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1368,6 +1368,7 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	dev->kvm->arch.vgic.has_its = true;
 	its->initialized = false;
 	its->enabled = false;
+	its->dev = dev;
 
 	its->baser_device_table = INITIAL_BASER_VALUE			|
 		((u64)GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 11/13] KVM: arm64: vgic-its: Turn device_id validation into generic ID validation
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (9 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 10/13] KVM: arm64: vgic-its: Add pointer to corresponding kvm_device Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 12/13] KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other handlers Marc Zyngier
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

There is no need to have separate functions to validate devices
and collections, as the architecture doesn't really distinguish the
two, and they are supposed to be managed the same way.

Let's turn the DevID checker into a generic one.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 134 ++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 72 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index dcae567..996e3e1 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -581,12 +581,73 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
+/*
+ * Check whether an ID can be stored into the corresponding guest table.
+ * For a direct table this is pretty easy, but gets a bit nasty for
+ * indirect tables. We check whether the resulting guest physical address
+ * is actually valid (covered by a memslot and guest accessbible).
+ * For this we have to read the respective first level entry.
+ */
+static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
+{
+	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
+	int index;
+	u64 indirect_ptr;
+	gfn_t gfn;
+
+	if (!(baser & GITS_BASER_INDIRECT)) {
+		phys_addr_t addr;
+
+		if (id >= (l1_tbl_size / GITS_BASER_ENTRY_SIZE(baser)))
+			return false;
+
+		addr = BASER_ADDRESS(baser) + id * GITS_BASER_ENTRY_SIZE(baser);
+		gfn = addr >> PAGE_SHIFT;
+
+		return kvm_is_visible_gfn(its->dev->kvm, gfn);
+	}
+
+	/* calculate and check the index into the 1st level */
+	index = id / (SZ_64K / GITS_BASER_ENTRY_SIZE(baser));
+	if (index >= (l1_tbl_size / sizeof(u64)))
+		return false;
+
+	/* Each 1st level entry is represented by a 64-bit value. */
+	if (kvm_read_guest(its->dev->kvm,
+			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
+			   &indirect_ptr, sizeof(indirect_ptr)))
+		return false;
+
+	indirect_ptr = le64_to_cpu(indirect_ptr);
+
+	/* check the valid bit of the first level entry */
+	if (!(indirect_ptr & BIT_ULL(63)))
+		return false;
+
+	/*
+	 * Mask the guest physical address and calculate the frame number.
+	 * Any address beyond our supported 48 bits of PA will be caught
+	 * by the actual check in the final step.
+	 */
+	indirect_ptr &= GENMASK_ULL(51, 16);
+
+	/* Find the address of the actual entry */
+	index = id % (SZ_64K / GITS_BASER_ENTRY_SIZE(baser));
+	indirect_ptr += index * GITS_BASER_ENTRY_SIZE(baser);
+	gfn = indirect_ptr >> PAGE_SHIFT;
+
+	return kvm_is_visible_gfn(its->dev->kvm, gfn);
+}
+
 static int vgic_its_alloc_collection(struct vgic_its *its,
 				     struct its_collection **colp,
 				     u32 coll_id)
 {
 	struct its_collection *collection;
 
+	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id))
+		return E_ITS_MAPC_COLLECTION_OOR;
+
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
 
 	collection->collection_id = coll_id;
@@ -709,67 +770,6 @@ static void vgic_its_unmap_device(struct kvm *kvm, struct its_device *device)
 }
 
 /*
- * Check whether a device ID can be stored into the guest device tables.
- * For a direct table this is pretty easy, but gets a bit nasty for
- * indirect tables. We check whether the resulting guest physical address
- * is actually valid (covered by a memslot and guest accessbible).
- * For this we have to read the respective first level entry.
- */
-static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
-				     int device_id)
-{
-	u64 r = its->baser_device_table;
-	int l1_tbl_size = GITS_BASER_NR_PAGES(r) * SZ_64K;
-	int index;
-	u64 indirect_ptr;
-	gfn_t gfn;
-
-
-	if (!(r & GITS_BASER_INDIRECT)) {
-		phys_addr_t addr;
-
-		if (device_id >= (l1_tbl_size / GITS_BASER_ENTRY_SIZE(r)))
-			return false;
-
-		addr = BASER_ADDRESS(r) + device_id * GITS_BASER_ENTRY_SIZE(r);
-		gfn = addr >> PAGE_SHIFT;
-
-		return kvm_is_visible_gfn(kvm, gfn);
-	}
-
-	/* calculate and check the index into the 1st level */
-	index = device_id / (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
-	if (index >= (l1_tbl_size / sizeof(u64)))
-		return false;
-
-	/* Each 1st level entry is represented by a 64-bit value. */
-	if (kvm_read_guest(kvm,
-			   BASER_ADDRESS(r) + index * sizeof(indirect_ptr),
-			   &indirect_ptr, sizeof(indirect_ptr)))
-		return false;
-
-	indirect_ptr = le64_to_cpu(indirect_ptr);
-
-	/* check the valid bit of the first level entry */
-	if (!(indirect_ptr & BIT_ULL(63)))
-		return false;
-
-	/*
-	 * Mask the guest physical address and calculate the frame number.
-	 * Any address beyond our supported 48 bits of PA will be caught
-	 * by the actual check in the final step.
-	 */
-	indirect_ptr &= GENMASK_ULL(51, 16);
-
-	/* Find the address of the actual entry */
-	index = device_id % (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
-	indirect_ptr += index * GITS_BASER_ENTRY_SIZE(r);
-	gfn = indirect_ptr >> PAGE_SHIFT;
-
-	return kvm_is_visible_gfn(kvm, gfn);
-}
-
-/*
  * MAPD maps or unmaps a device ID to Interrupt Translation Tables (ITTs).
  * Must be called with the its_lock mutex held.
  */
@@ -780,7 +780,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	bool valid = its_cmd_get_validbit(its_cmd);
 	struct its_device *device;
 
-	if (!vgic_its_check_device_id(kvm, its, device_id))
+	if (!vgic_its_check_id(its, its->baser_device_table, device_id))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	device = find_its_device(its, device_id);
@@ -812,13 +812,6 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
-static int vgic_its_nr_collection_ids(struct vgic_its *its)
-{
-	u64 r = its->baser_coll_table;
-
-	return (GITS_BASER_NR_PAGES(r) * SZ_64K) / GITS_BASER_ENTRY_SIZE(r);
-}
-
 /*
  * The MAPC command maps collection IDs to redistributors.
  * Must be called with the its_lock mutex held.
@@ -838,9 +831,6 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
 	if (target_addr >= atomic_read(&kvm->online_vcpus))
 		return E_ITS_MAPC_PROCNUM_OOR;
 
-	if (coll_id >= vgic_its_nr_collection_ids(its))
-		return E_ITS_MAPC_COLLECTION_OOR;
-
 	if (!valid) {
 		vgic_its_free_collection(its, coll_id);
 	} else {
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 12/13] KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other handlers
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (10 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 11/13] KVM: arm64: vgic-its: Turn device_id validation into generic ID validation Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 15:54 ` [PATCH 13/13] KVM: arm64: vgic-its: Simplify MAPI error handling Marc Zyngier
  2016-07-18 17:47 ` [PATCH 00/13] A bunch of vgic-its fixes Andre Przywara
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

vgic_its_cmd_handle_mapi has an extra "subcmd" argument, which is
already contained in the command buffer that all command handlers
obtain from the command queue. Let's drop it, as it is not that
useful.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 996e3e1..ec7e07b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -688,7 +688,7 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
  * Must be called with its_lock mutex held.
  */
 static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
-				    u64 *its_cmd, u8 subcmd)
+				    u64 *its_cmd)
 {
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
@@ -711,7 +711,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		new_coll = collection;
 	}
 
-	if (subcmd == GITS_CMD_MAPTI)
+	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
 		lpi_nr = its_cmd_get_physical_id(its_cmd);
 	else
 		lpi_nr = event_id;
@@ -999,11 +999,10 @@ static int vgic_its_cmd_handle_int(struct kvm *kvm, struct vgic_its *its,
 static int vgic_its_handle_command(struct kvm *kvm, struct vgic_its *its,
 				   u64 *its_cmd)
 {
-	u8 cmd = its_cmd_get_command(its_cmd);
 	int ret = -ENODEV;
 
 	mutex_lock(&its->its_lock);
-	switch (cmd) {
+	switch (its_cmd_get_command(its_cmd)) {
 	case GITS_CMD_MAPD:
 		ret = vgic_its_cmd_handle_mapd(kvm, its, its_cmd);
 		break;
@@ -1011,10 +1010,10 @@ static int vgic_its_handle_command(struct kvm *kvm, struct vgic_its *its,
 		ret = vgic_its_cmd_handle_mapc(kvm, its, its_cmd);
 		break;
 	case GITS_CMD_MAPI:
-		ret = vgic_its_cmd_handle_mapi(kvm, its, its_cmd, cmd);
+		ret = vgic_its_cmd_handle_mapi(kvm, its, its_cmd);
 		break;
 	case GITS_CMD_MAPTI:
-		ret = vgic_its_cmd_handle_mapi(kvm, its, its_cmd, cmd);
+		ret = vgic_its_cmd_handle_mapi(kvm, its, its_cmd);
 		break;
 	case GITS_CMD_MOVI:
 		ret = vgic_its_cmd_handle_movi(kvm, its, its_cmd);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 13/13] KVM: arm64: vgic-its: Simplify MAPI error handling
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (11 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 12/13] KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other handlers Marc Zyngier
@ 2016-07-18 15:54 ` Marc Zyngier
  2016-07-18 17:47 ` [PATCH 00/13] A bunch of vgic-its fixes Andre Przywara
  13 siblings, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2016-07-18 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

If we care to move all the checks that do not involve any memory
allocation, we can simplify the MAPI error handling. Let's do that,
it cannot hurt.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index ec7e07b..07411cf 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -697,36 +697,34 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;
-	int ret;
 
 	device = find_its_device(its, device_id);
 	if (!device)
 		return E_ITS_MAPTI_UNMAPPED_DEVICE;
 
-	collection = find_collection(its, coll_id);
-	if (!collection) {
-		ret = vgic_its_alloc_collection(its, &collection, coll_id);
-		if (ret)
-			return ret;
-		new_coll = collection;
-	}
-
 	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
 		lpi_nr = its_cmd_get_physical_id(its_cmd);
 	else
 		lpi_nr = event_id;
 	if (lpi_nr < GIC_LPI_OFFSET ||
-	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser)) {
-		ret = E_ITS_MAPTI_PHYSICALID_OOR;
-		goto err;
+	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
+		return E_ITS_MAPTI_PHYSICALID_OOR;
+
+	collection = find_collection(its, coll_id);
+	if (!collection) {
+		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
+		if (ret)
+			return ret;
+		new_coll = collection;
 	}
 
 	itte = find_itte(its, device_id, event_id);
 	if (!itte) {
 		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
 		if (!itte) {
-			ret = -ENOMEM;
-			goto err;
+			if (new_coll)
+				vgic_its_free_collection(its, coll_id);
+			return -ENOMEM;
 		}
 
 		itte->event_id	= event_id;
@@ -746,10 +744,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	update_lpi_config(kvm, itte->irq, NULL);
 
 	return 0;
-err:
-	if (new_coll)
-		vgic_its_free_collection(its, coll_id);
-	return ret;
 }
 
 /* Requires the its_lock to be held. */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 00/13] A bunch of vgic-its fixes
  2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
                   ` (12 preceding siblings ...)
  2016-07-18 15:54 ` [PATCH 13/13] KVM: arm64: vgic-its: Simplify MAPI error handling Marc Zyngier
@ 2016-07-18 17:47 ` Andre Przywara
  13 siblings, 0 replies; 15+ messages in thread
From: Andre Przywara @ 2016-07-18 17:47 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Marc,

On 18/07/16 16:54, Marc Zyngier wrote:
> As the ITS series has been posted 10 times now, and we're still
> finding issues, I've decided to directly fix things rather than
> commenting on them and waiting for a respin, as we are getting way too
> close to the merge window. So I sat over the weekend and started doing
> "stuff".
> 
> There is a bit of everything in there: some cosmetic things, but also
> some more severe bugs (indirect tables were completely foobar) and
> regressions (the cpuif stuff that Eric fixed).

I wonder if it's worth to squash:
- 01/13 into 09/17
- 02/13 into 07/17
- 04/13 into 15/17
with appropriate comments in the commit message to credit the bug fixes
and their authors?

That worked for me without further ado and would leave the git tree
bisectable - both in terms of compilability (every single commit
compiled for me with arm64/defconfig without warnings) and functionality
(tested new commit 17/17 with direct and indirect tables on the
model(ITS) and qemu and kvmtool on Juno).

It would be a pity if other people bisecting the kernel get punished for
my silly mistakes.

> Unless someone shouts at me, I intend to put that on top of Andre's
> series, and put the whole thing into -next. I've tested it on a LS2085
> with kvmtool, using both flat and indirect device tables.

I am fine with all those.

Thanks!
Andre.

> Eric Auger (1):
>   KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS
> 
> Marc Zyngier (12):
>   irqchip/gicv3-its: Restore all cacheability attributes
>   KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref
>   KVM: arm64: vgic-its: Fix handling of indirect tables
>   KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling
>   KVM: arm64: vgic-its: Fix misleading nr_entries in
>     vgic_its_check_device_id
>   KVM: arm64: vgic-its: Validate the device table L1 entry
>   KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables
>   KVM: arm64: vgic-its: Add collection allocator/destructor
>   KVM: arm64: vgic-its: Add pointer to corresponding kvm_device
>   KVM: arm64: vgic-its: Turn device_id validation into generic ID
>     validation
>   KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other
>     handlers
>   KVM: arm64: vgic-its: Simplify MAPI error handling
> 
>  include/kvm/arm_vgic.h             |   1 +
>  include/linux/irqchip/arm-gic-v3.h |  48 ++++++--
>  virt/kvm/arm/vgic/vgic-its.c       | 224 ++++++++++++++++++++-----------------
>  virt/kvm/arm/vgic/vgic-mmio-v2.c   |   2 +
>  virt/kvm/arm/vgic/vgic-mmio.c      |   4 +-
>  virt/kvm/arm/vgic/vgic.c           |  10 +-
>  virt/kvm/arm/vgic/vgic.h           |   8 ++
>  7 files changed, 175 insertions(+), 122 deletions(-)
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-07-18 17:47 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-18 15:54 [PATCH 00/13] A bunch of vgic-its fixes Marc Zyngier
2016-07-18 15:54 ` [PATCH 01/13] KVM: arm/arm64: Fix vGICv2 KVM_DEV_ARM_VGIC_GRP_CPU/DIST_REGS Marc Zyngier
2016-07-18 15:54 ` [PATCH 02/13] irqchip/gicv3-its: Restore all cacheability attributes Marc Zyngier
2016-07-18 15:54 ` [PATCH 03/13] KVM: arm64: vgic-its: Generalize use of vgic_get_irq_kref Marc Zyngier
2016-07-18 15:54 ` [PATCH 04/13] KVM: arm64: vgic-its: Fix handling of indirect tables Marc Zyngier
2016-07-18 15:54 ` [PATCH 05/13] KVM: arm64: vgic-its: Fix vgic_its_check_device_id BE handling Marc Zyngier
2016-07-18 15:54 ` [PATCH 06/13] KVM: arm64: vgic-its: Fix misleading nr_entries in vgic_its_check_device_id Marc Zyngier
2016-07-18 15:54 ` [PATCH 07/13] KVM: arm64: vgic-its: Validate the device table L1 entry Marc Zyngier
2016-07-18 15:54 ` [PATCH 08/13] KVM: arm64: vgic-its: Fix L2 entry validation for indirect tables Marc Zyngier
2016-07-18 15:54 ` [PATCH 09/13] KVM: arm64: vgic-its: Add collection allocator/destructor Marc Zyngier
2016-07-18 15:54 ` [PATCH 10/13] KVM: arm64: vgic-its: Add pointer to corresponding kvm_device Marc Zyngier
2016-07-18 15:54 ` [PATCH 11/13] KVM: arm64: vgic-its: Turn device_id validation into generic ID validation Marc Zyngier
2016-07-18 15:54 ` [PATCH 12/13] KVM: arm64: vgic-its: Make vgic_its_cmd_handle_mapi similar to other handlers Marc Zyngier
2016-07-18 15:54 ` [PATCH 13/13] KVM: arm64: vgic-its: Simplify MAPI error handling Marc Zyngier
2016-07-18 17:47 ` [PATCH 00/13] A bunch of vgic-its fixes Andre Przywara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).