[RFC 00/12] KVM: arm/arm64: Allow multiple GICv3 redistributor regions

[RFC 00/12] KVM: arm/arm64: Allow multiple GICv3 redistributor regions

[Eric Auger]

At the moment the KVM VGICv3 only supports a single redistributor
region (whose base address is set through the GICv3 kvm device
KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST). There,
all the redistributors are laid out contiguously. The size of this
single redistributor region is not set explicitly but instead
induced at a late stage by the number of online vcpus.

The GIC specification does not mandate all redistributors to be
contiguous. Moreover DT and ACPI were specified so that multiple
redistributors regions can be defined.

The current interface brings a limitation on QEMU where ARM
virt machine available GPA holes only allowed to assign a
redistributor region fitting a max of 123 vcpus. Overcoming this
limitation would force either to create a new machine or relocate
the single rdist region or allow the allocation of multiple rdist
regions.

This series enables this last alternative. A new GICv3 KVM device
KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows
to register individual redistributor regions whose size is defined
explicitly. Those rdist regions then are filled by vcpu rdist frames
according to the need. The vgic init and related base address checks
are impacted.

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/v4.16-rc5-rdist-regions-rfc

* Testing:
- With QEMU, splitting the single rdist region into 2 seperate ones.
- Work in progress to allow clean support of more than 123 vcpus.

Eric Auger (12):
  KVM: arm/arm64: Avoid multiple dist->spis kfree
  KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
  KVM: arm/arm64: Record RDIST Last bit at registration
  KVM: arm/arm64: Replace the single rdist region by a list
  KVM: arm/arm64: Helper to locate free rdist space
  KVM: arm/arm64: Helper to detect whether an RDIST is the last one
  KVM: arm/arm64: Helper to register a new redistributor region
  KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions
  KVM: arm/arm64: Check vcpu redist base before registering an iodev
  KVM: arm/arm64: Check all vcpu redistributors are set on map_resources
  KVM: arm/arm64: Add KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
  KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  12 +++
 arch/arm/include/uapi/asm/kvm.h                   |   7 +-
 arch/arm64/include/uapi/asm/kvm.h                 |   7 +-
 include/kvm/arm_vgic.h                            |  14 ++-
 virt/kvm/arm/vgic/vgic-init.c                     |  25 ++++-
 virt/kvm/arm/vgic/vgic-kvm-device.c               |  47 ++++++++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c                  | 111 +++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-v3.c                       |  89 +++++++++++++----
 virt/kvm/arm/vgic/vgic.h                          |  42 +++++++-
 9 files changed, 307 insertions(+), 47 deletions(-)

-- 
2.5.5

[RFC 01/12] KVM: arm/arm64: Avoid multiple dist->spis kfree

[Eric Auger]

in case kvm_vgic_map_resources() fails, typically if the vgic
distributor is not defined, __kvm_vgic_destroy will be called
several times. Indeed kvm_vgic_map_resources() is called on
first vcpu run. As a result dist->spis is freeed twice and on
the second time it causes a "kernel BUG at mm/slub.c:3912!"

This patch avoids freeing dist->spis twice.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-init.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 743ca5c..38fd5f1 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -324,7 +324,10 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	dist->ready = false;
 	dist->initialized = false;
 
-	kfree(dist->spis);
+	if (dist->spis) {
+		kfree(dist->spis);
+		dist->spis = NULL;
+	}
 	dist->nr_spis = 0;
 
 	if (vgic_supports_direct_msis(kvm))
-- 
2.5.5

[RFC 02/12] KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Eric Auger]

We introduce a new KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute in
KVM_DEV_ARM_VGIC_GRP_ADDR group. It allows userspace to provide the
base address and size of a redistributor region

Compared to KVM_VGIC_V3_ADDR_TYPE_REDIST, this new attribute allows
to declare several separate redistributor regions.

So the whole redist space does not need to be contiguous anymore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 Documentation/virtual/kvm/devices/arm-vgic-v3.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
index 9293b45..2c0bedf 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
@@ -27,6 +27,18 @@ Groups:
       VCPU and all of the redistributor pages are contiguous.
       Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
       This address needs to be 64K aligned.
+
+    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (rw, 64-bit)
+      The attr field of kvm_device_attr encodes 3 values:
+      bits:     | 63   ....  52  |  51   ....   12 |11 - 0
+      values:   |     pfns       |       base      | index
+      - index encodes the unique redistibutor region index
+      - base field encodes bits [51:12] the guest physical base address
+        of the first redistributor in the region. There are two 64K pages
+        for each VCPU and all of the redistributor pages are contiguous
+        within the redistributor region.
+      - pfns encodes the size of the region in 64kB pages.
+      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
   Errors:
     -E2BIG:  Address outside of addressable IPA range
     -EINVAL: Incorrectly aligned address
-- 
2.5.5

[RFC 03/12] KVM: arm/arm64: Record RDIST Last bit at registration

[Eric Auger]

To prepare for multiple RDIST regions, let's record the RDIST
Last bit field when registering the IO device.

As a reminder the Last bit indicates whether the redistributor
is the highest one in a series of contiguous redistributor pages.

Since at the moment KVM only supports a single redist region,
the RDIST tagged with the last bit set to true corresponds to the
highest vCPU one.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/kvm/arm_vgic.h           | 1 +
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cdbd142..1c8c0ff 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -312,6 +312,7 @@ struct vgic_cpu {
 	 */
 	struct vgic_io_device	rd_iodev;
 	struct vgic_io_device	sgi_iodev;
+	bool rdist_last; /* Is the RDIST the last one of the RDIST region? */
 
 	/* Contains the attributes and gpa of the LPI pending tables. */
 	u64 pendbaser;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 671fe81..75fe689 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -184,12 +184,13 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
 					      gpa_t addr, unsigned int len)
 {
 	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	int target_vcpu_id = vcpu->vcpu_id;
 	u64 value;
 
 	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
 	value |= ((target_vcpu_id & 0xffff) << 8);
-	if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
+	if (vgic_cpu->rdist_last)
 		value |= GICR_TYPER_LAST;
 	if (vgic_has_its(vcpu->kvm))
 		value |= GICR_TYPER_PLPIS;
@@ -580,6 +581,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
 	struct vgic_dist *vgic = &kvm->arch.vgic;
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
 	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
 	gpa_t rd_base, sgi_base;
@@ -632,6 +634,8 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	}
 
 	vgic->vgic_redist_free_offset += 2 * SZ_64K;
+	vgic_cpu->rdist_last =
+		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
 out:
 	mutex_unlock(&kvm->slots_lock);
 	return ret;
-- 
2.5.5

[RFC 04/12] KVM: arm/arm64: Replace the single rdist region by a list

[Eric Auger]

At the moment KVM supports a single rdist region. We want to
support several separate rdist regions so let's introduce a list
of them. This patch currently only cares about a single
entry in this list as the functionality to register several redist
regions is not yet there. So this only translates the existing code
into something functionally similar using that new data struct.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/kvm/arm_vgic.h              | 13 +++++++++----
 virt/kvm/arm/vgic/vgic-init.c       | 17 +++++++++++++++--
 virt/kvm/arm/vgic/vgic-kvm-device.c | 13 +++++++++++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 35 +++++++++++++++++++++++++++--------
 virt/kvm/arm/vgic/vgic-v3.c         | 20 ++++++++++++--------
 5 files changed, 74 insertions(+), 24 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 1c8c0ff..88f7cb2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -196,6 +196,14 @@ struct vgic_its {
 
 struct vgic_state_iter;
 
+struct vgic_redist_region {
+	uint32_t index;
+	gpa_t base;
+	uint32_t pfns;/* number of 64 kB pages or 0 if single region */
+	uint32_t free_pfn_offset;
+	struct list_head list;
+};
+
 struct vgic_dist {
 	bool			in_kernel;
 	bool			ready;
@@ -219,10 +227,7 @@ struct vgic_dist {
 		/* either a GICv2 CPU interface */
 		gpa_t			vgic_cpu_base;
 		/* or a number of GICv3 redistributor regions */
-		struct {
-			gpa_t		vgic_redist_base;
-			gpa_t		vgic_redist_free_offset;
-		};
+		struct list_head rd_regions;
 	};
 
 	/* distributor enabled */
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 38fd5f1..b6b4db0 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -173,8 +173,11 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 	kvm->arch.vgic.vctrl_base = kvm_vgic_global_state.vctrl_base;
 
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
-	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
-	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
+
+	if (type == KVM_DEV_TYPE_ARM_VGIC_V2)
+		kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
+	else
+		INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions);
 
 out_unlock:
 	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
@@ -320,6 +323,7 @@ int vgic_init(struct kvm *kvm)
 static void kvm_vgic_dist_destroy(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
+	struct vgic_redist_region *rdreg, *next;
 
 	dist->ready = false;
 	dist->initialized = false;
@@ -328,6 +332,15 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 		kfree(dist->spis);
 		dist->spis = NULL;
 	}
+
+	if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list) {
+			list_del(&rdreg->list);
+			kfree(rdreg);
+		}
+		INIT_LIST_HEAD(&dist->rd_regions);
+	}
+
 	dist->nr_spis = 0;
 
 	if (vgic_supports_direct_msis(kvm))
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 10ae6f3..e7b5a86 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -66,6 +66,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 	int r = 0;
 	struct vgic_dist *vgic = &kvm->arch.vgic;
 	phys_addr_t *addr_ptr, alignment;
+	uint64_t undef_value = VGIC_ADDR_UNDEF;
 
 	mutex_lock(&kvm->lock);
 	switch (type) {
@@ -84,7 +85,9 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 		addr_ptr = &vgic->vgic_dist_base;
 		alignment = SZ_64K;
 		break;
-	case KVM_VGIC_V3_ADDR_TYPE_REDIST:
+	case KVM_VGIC_V3_ADDR_TYPE_REDIST: {
+		struct vgic_redist_region *rdreg;
+
 		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
 		if (r)
 			break;
@@ -92,8 +95,14 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 			r = vgic_v3_set_redist_base(kvm, *addr);
 			goto out;
 		}
-		addr_ptr = &vgic->vgic_redist_base;
+		rdreg = list_first_entry(&vgic->rd_regions,
+					 struct vgic_redist_region, list);
+		if (!rdreg)
+			addr_ptr = &undef_value;
+		else
+			addr_ptr = &rdreg->base;
 		break;
+	}
 	default:
 		r = -ENODEV;
 	}
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 75fe689..c9dc08f 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -584,6 +584,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
 	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
+	struct vgic_redist_region *rdreg;
 	gpa_t rd_base, sgi_base;
 	int ret;
 
@@ -593,13 +594,15 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	 * function for all VCPUs when the base address is set.  Just return
 	 * without doing any work for now.
 	 */
-	if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
+	rdreg = list_first_entry(&vgic->rd_regions,
+				 struct vgic_redist_region, list);
+	if (!rdreg)
 		return 0;
 
 	if (!vgic_v3_check_base(kvm))
 		return -EINVAL;
 
-	rd_base = vgic->vgic_redist_base + vgic->vgic_redist_free_offset;
+	rd_base = rdreg->base + rdreg->free_pfn_offset * SZ_64K;
 	sgi_base = rd_base + SZ_64K;
 
 	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
@@ -633,7 +636,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
-	vgic->vgic_redist_free_offset += 2 * SZ_64K;
+	rdreg->free_pfn_offset += 2;
 	vgic_cpu->rdist_last =
 		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
 out:
@@ -677,19 +680,31 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
 int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
 {
 	struct vgic_dist *vgic = &kvm->arch.vgic;
+	struct vgic_redist_region *rdreg;
 	int ret;
 
+	if (!list_empty(&vgic->rd_regions))
+		return -EINVAL;
+
+	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
+	if (!rdreg)
+		return -ENOMEM;
+
+	rdreg->base = VGIC_ADDR_UNDEF;
+
 	/* vgic_check_ioaddr makes sure we don't do this twice */
-	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
+	ret = vgic_check_ioaddr(kvm, &rdreg->base, addr, SZ_64K);
 	if (ret)
-		return ret;
+		goto out;
 
-	vgic->vgic_redist_base = addr;
+	rdreg->base = addr;
 	if (!vgic_v3_check_base(kvm)) {
-		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
+	list_add(&rdreg->list, &vgic->rd_regions);
+
 	/*
 	 * Register iodevs for each existing VCPU.  Adding more VCPUs
 	 * afterwards will register the iodevs when needed.
@@ -699,6 +714,10 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
 		return ret;
 
 	return 0;
+
+out:
+	kfree(rdreg);
+	return ret;
 }
 
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 6b32941..14dabda 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -405,6 +405,9 @@ bool vgic_v3_check_base(struct kvm *kvm)
 {
 	struct vgic_dist *d = &kvm->arch.vgic;
 	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
+	struct vgic_redist_region *rdreg =
+		list_first_entry(&d->rd_regions,
+				 struct vgic_redist_region, list);
 
 	redist_size *= atomic_read(&kvm->online_vcpus);
 
@@ -412,18 +415,17 @@ bool vgic_v3_check_base(struct kvm *kvm)
 	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
 		return false;
 
-	if (!IS_VGIC_ADDR_UNDEF(d->vgic_redist_base) &&
-	    d->vgic_redist_base + redist_size < d->vgic_redist_base)
+	if (rdreg && (rdreg->base + redist_size < rdreg->base))
 		return false;
 
 	/* Both base addresses must be set to check if they overlap */
-	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) ||
-	    IS_VGIC_ADDR_UNDEF(d->vgic_redist_base))
+	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) || !rdreg)
 		return true;
 
-	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
+	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= rdreg->base)
 		return true;
-	if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
+
+	if (rdreg->base + redist_size <= d->vgic_dist_base)
 		return true;
 
 	return false;
@@ -433,12 +435,14 @@ int vgic_v3_map_resources(struct kvm *kvm)
 {
 	int ret = 0;
 	struct vgic_dist *dist = &kvm->arch.vgic;
+	struct vgic_redist_region *rdreg =
+		list_first_entry(&dist->rd_regions,
+				 struct vgic_redist_region, list);
 
 	if (vgic_ready(kvm))
 		goto out;
 
-	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
-	    IS_VGIC_ADDR_UNDEF(dist->vgic_redist_base)) {
+	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) || !rdreg) {
 		kvm_err("Need to set vgic distributor addresses first\n");
 		ret = -ENXIO;
 		goto out;
-- 
2.5.5

[RFC 05/12] KVM: arm/arm64: Helper to locate free rdist space

[Eric Auger]

We introduce vgic_v3_rdist_free_slot to help identifying
where we can place a new 2x64KB redistributor.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c |  4 ++--
 virt/kvm/arm/vgic/vgic-v3.c      | 21 +++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h         | 12 ++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index c9dc08f..227b8b6 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -586,6 +586,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
 	struct vgic_redist_region *rdreg;
 	gpa_t rd_base, sgi_base;
+	uint32_t free_pfn_offset;
 	int ret;
 
 	/*
@@ -594,8 +595,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	 * function for all VCPUs when the base address is set.  Just return
 	 * without doing any work for now.
 	 */
-	rdreg = list_first_entry(&vgic->rd_regions,
-				 struct vgic_redist_region, list);
+	rdreg = vgic_v3_rdist_free_slot(&vgic->rd_regions, &free_pfn_offset);
 	if (!rdreg)
 		return 0;
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 14dabda..68cae0e 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -431,6 +431,27 @@ bool vgic_v3_check_base(struct kvm *kvm)
 	return false;
 }
 
+/**
+ * vgic_v3_rdist_free_slot - Lookup registered rdist regions and identify one
+ * which has free space to put a new rdist regions
+ *
+ * If any, return this redist region handle and the pfn offset to the free slot.
+ * Otherwise, returns NULL.
+ */
+struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions,
+						   uint32_t *free_pfn_offset)
+{
+	struct vgic_redist_region *rdreg;
+
+	list_for_each_entry(rdreg, rd_regions, list) {
+		if (!vgic_v3_redist_region_full(rdreg)) {
+			*free_pfn_offset = rdreg->free_pfn_offset;
+			return rdreg;
+		}
+	}
+	return NULL;
+}
+
 int vgic_v3_map_resources(struct kvm *kvm)
 {
 	int ret = 0;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 12c37b8..38b2406 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -245,6 +245,18 @@ static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
 	}
 }
 
+static inline bool
+vgic_v3_redist_region_full(struct vgic_redist_region *region)
+{
+	if (!region->pfns)
+		return false;
+
+	return (region->free_pfn_offset > region->pfns - 2);
+}
+
+struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions,
+						   uint32_t *free_pfn_offset);
+
 int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
 			 u32 devid, u32 eventid, struct vgic_irq **irq);
 struct vgic_its *vgic_msi_to_its(struct kvm *kvm, struct kvm_msi *msi);
-- 
2.5.5

[RFC 06/12] KVM: arm/arm64: Helper to detect whether an RDIST is the last one

[Eric Auger]

The TYPER of an redistributor needs to reflect whether the rdist is
the last one of the redistributor region. With a single region we compared
the vcpu id against the number of online vcpus. With multiple regions,
we also need to check the region is not full.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 3 +--
 virt/kvm/arm/vgic/vgic.h         | 9 +++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 227b8b6..5768034 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -637,8 +637,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	}
 
 	rdreg->free_pfn_offset += 2;
-	vgic_cpu->rdist_last =
-		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
+	vgic_cpu->rdist_last = vgic_v3_last_rdist(vcpu, rdreg);
 out:
 	mutex_unlock(&kvm->slots_lock);
 	return ret;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 38b2406..04456d3 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -254,6 +254,15 @@ vgic_v3_redist_region_full(struct vgic_redist_region *region)
 	return (region->free_pfn_offset > region->pfns - 2);
 }
 
+static inline bool
+vgic_v3_last_rdist(struct kvm_vcpu *vcpu, struct vgic_redist_region *region)
+{
+	if (vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
+		return true;
+
+	return vgic_v3_redist_region_full(region);
+}
+
 struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions,
 						   uint32_t *free_pfn_offset);
 
-- 
2.5.5

[RFC 07/12] KVM: arm/arm64: Helper to register a new redistributor region

[Eric Auger]

We introduce a new helper that creates and inserts a new redistributor
region into the rdist region list. This helper both handles the case
where the redistributor region size is known at registration time
and the legacy case where it is not (eventually depending on the number
of online vcpus). Depending on pfns, we perform all the possible checks
that we can do:

- end of memory crossing
- incorrect alignment of the base address
- collision with distributor region if already defined
- collision with already registered rdist regions
- check of the new index

Rdist regions must be inserted by increasing order of indices. Indices
must be contiguous.

We also introduce vgic_v3_rdist_region_from_index() which will be used
from the vgic kvm-device, later on.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 90 +++++++++++++++++++++++++++++++++-------
 virt/kvm/arm/vgic/vgic-v3.c      | 29 +++++++++++++
 virt/kvm/arm/vgic/vgic.h         | 14 +++++++
 3 files changed, 118 insertions(+), 15 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 5768034..f139382 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -676,13 +676,62 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
 	return ret;
 }
 
-int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
+/**
+ * vgic_v3_insert_redist_region - Insert a new redistributor region
+ *
+ * Performs various checks before inserting the rdist region in the list.
+ * Those tests depend on whether the size of the rdist region is known
+ * (ie. pfns != 0). The list is sorted by rdist region index.
+ *
+ * @kvm: kvm handle
+ * @index: redist region index
+ * @base: base of the new rdist region
+ * @pfns: number of 64kB pages the region is made of (of 0 in the old style
+ * single region, whose size is induced from the number of vcpus
+ *
+ * Return 0 on success, < 0 otherwise
+ */
+static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
+					gpa_t base, uint32_t pfns)
 {
-	struct vgic_dist *vgic = &kvm->arch.vgic;
+	struct vgic_dist *d = &kvm->arch.vgic;
 	struct vgic_redist_region *rdreg;
+	struct list_head *rd_regions = &d->rd_regions;
+	struct list_head *last = rd_regions->prev;
+
+	gpa_t new_start, new_end;
+	size_t size = pfns * SZ_64K;
 	int ret;
 
-	if (!list_empty(&vgic->rd_regions))
+	/* single rdist region already set ?*/
+	if (!pfns && !list_empty(rd_regions))
+		return -EINVAL;
+
+	/* cross the end of memory ? */
+	if (base + size < base)
+		return -EINVAL;
+
+	if (list_empty(rd_regions)) {
+		if (index != 0)
+			return -EINVAL;
+	} else {
+		rdreg = list_entry(last, struct vgic_redist_region, list);
+		if (index != rdreg->index + 1)
+			return -EINVAL;
+	}
+
+	/*
+	 * collision with already set dist region ?
+	 * this assumes we know the size of the new rdist region (pfns != 0)
+	 * otherwise we can only test this when all vcpus are registered
+	 */
+	if (!pfns && !IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
+		(!(d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= base)) &&
+		(!(base + size <= d->vgic_dist_base)))
+		return -EINVAL;
+
+	/* collision with any other rdist region? */
+	if (vgic_v3_rdist_overlap(kvm, base, size))
 		return -EINVAL;
 
 	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
@@ -692,17 +741,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
 	rdreg->base = VGIC_ADDR_UNDEF;
 
 	/* vgic_check_ioaddr makes sure we don't do this twice */
-	ret = vgic_check_ioaddr(kvm, &rdreg->base, addr, SZ_64K);
+	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
 	if (ret)
-		goto out;
+		goto free;
 
-	rdreg->base = addr;
-	if (!vgic_v3_check_base(kvm)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	rdreg->base = base;
+	rdreg->pfns = pfns;
+	rdreg->free_pfn_offset = 0;
+	rdreg->index = index;
+
+	new_start = base;
+	new_end = base + size - 1;
+
+	list_add_tail(&rdreg->list, rd_regions);
+	return 0;
+free:
+	kfree(rdreg);
+	return ret;
+}
+
+int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
+{
+	int ret;
 
-	list_add(&rdreg->list, &vgic->rd_regions);
+	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Register iodevs for each existing VCPU.  Adding more VCPUs
@@ -713,10 +777,6 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
 		return ret;
 
 	return 0;
-
-out:
-	kfree(rdreg);
-	return ret;
 }
 
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 68cae0e..c6ec636 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -397,6 +397,21 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	return 0;
 }
 
+/* return true if there is an overlap between any rdist */
+bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size)
+{
+	struct vgic_dist *d = &kvm->arch.vgic;
+	struct vgic_redist_region *rdreg;
+
+	list_for_each_entry(rdreg, &d->rd_regions, list) {
+		if ((base + size <= rdreg->base) ||
+			(rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <= base))
+			continue;
+		return true;
+	}
+	return false;
+}
+
 /*
  * Check for overlapping regions and for regions crossing the end of memory
  * for base addresses which have already been set.
@@ -452,6 +467,20 @@ struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions,
 	return NULL;
 }
 
+struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
+							   uint32_t index)
+{
+	struct list_head *rd_regions = &kvm->arch.vgic.rd_regions;
+	struct vgic_redist_region *rdreg;
+
+	list_for_each_entry(rdreg, rd_regions, list) {
+		if (rdreg->index == index)
+			return rdreg;
+	}
+	return NULL;
+}
+
+
 int vgic_v3_map_resources(struct kvm *kvm)
 {
 	int ret = 0;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 04456d3..dcf43c7 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -263,9 +263,23 @@ vgic_v3_last_rdist(struct kvm_vcpu *vcpu, struct vgic_redist_region *region)
 	return vgic_v3_redist_region_full(region);
 }
 
+static inline size_t
+vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
+{
+	if (!rdreg->pfns)
+		return atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE;
+	else
+		return rdreg->pfns * SZ_64K;
+}
+
+struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
+							   uint32_t index);
+
 struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions,
 						   uint32_t *free_pfn_offset);
 
+bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size);
+
 int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
 			 u32 devid, u32 eventid, struct vgic_irq **irq);
 struct vgic_its *vgic_msi_to_its(struct kvm *kvm, struct kvm_msi *msi);
-- 
2.5.5

[RFC 08/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions

[Eric Auger]

We introduce a new helper to check there is no overlap between
dist region (if set) and registered rdist regions. This both
handles the case of legacy single rdist region (implicitly sized
with the number of online vcpus) and the new case of multiple
explicitly sized rdist regions.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index c6ec636..547fab6 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -419,31 +419,23 @@ bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size)
 bool vgic_v3_check_base(struct kvm *kvm)
 {
 	struct vgic_dist *d = &kvm->arch.vgic;
-	gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
-	struct vgic_redist_region *rdreg =
-		list_first_entry(&d->rd_regions,
-				 struct vgic_redist_region, list);
-
-	redist_size *= atomic_read(&kvm->online_vcpus);
+	struct vgic_redist_region *rdreg;
 
 	if (!IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
 	    d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
 		return false;
 
-	if (rdreg && (rdreg->base + redist_size < rdreg->base))
-		return false;
-
-	/* Both base addresses must be set to check if they overlap */
-	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) || !rdreg)
-		return true;
-
-	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= rdreg->base)
-		return true;
+	list_for_each_entry(rdreg, &d->rd_regions, list) {
+		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
+			rdreg->base)
+			return false;
+	}
 
-	if (rdreg->base + redist_size <= d->vgic_dist_base)
+	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
 		return true;
 
-	return false;
+	return !vgic_v3_rdist_overlap(kvm, d->vgic_dist_base,
+				      KVM_VGIC_V3_DIST_SIZE);
 }
 
 /**
-- 
2.5.5

[RFC 09/12] KVM: arm/arm64: Check vcpu redist base before registering an iodev

[Eric Auger]

As we are going to register several redist regions,
vgic_register_all_redist_iodevs() may be called several times. We need
to register a redist_iodev for a given vcpu only once. So let's
check if the base address has already been set. Initialize this latter
in kvm_vgic_vcpu_early_init().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-init.c    | 3 +++
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index b6b4db0..03f97ce 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -82,6 +82,9 @@ void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu)
 	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
 	spin_lock_init(&vgic_cpu->ap_list_lock);
 
+	vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
+	vgic_cpu->sgi_iodev.base_addr = VGIC_ADDR_UNDEF;
+
 	/*
 	 * Enable and configure all SGIs to be edge-triggered and
 	 * configure all PPIs as level-triggered.
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index f139382..ea34412 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -589,6 +589,9 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	uint32_t free_pfn_offset;
 	int ret;
 
+	if (!IS_VGIC_ADDR_UNDEF(vgic_cpu->rd_iodev.base_addr))
+		return 0;
+
 	/*
 	 * We may be creating VCPUs before having set the base address for the
 	 * redistributor region, in which case we will come back to this
-- 
2.5.5

[RFC 10/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources

[Eric Auger]

On vcpu first run, we eventually know the actual number of vcpus.
This is a synchronization point to check all redistributors regions
were assigned. On kvm_vgic_map_resources() we check both dist and
redist were set, eventually check potential base address inconsistencies.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 547fab6..45fb87d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -475,16 +475,25 @@ struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
 
 int vgic_v3_map_resources(struct kvm *kvm)
 {
-	int ret = 0;
 	struct vgic_dist *dist = &kvm->arch.vgic;
-	struct vgic_redist_region *rdreg =
-		list_first_entry(&dist->rd_regions,
-				 struct vgic_redist_region, list);
+	struct kvm_vcpu *vcpu;
+	int ret = 0;
+	int c;
 
 	if (vgic_ready(kvm))
 		goto out;
 
-	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) || !rdreg) {
+	kvm_for_each_vcpu(c, vcpu, kvm) {
+		struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+
+		if (IS_VGIC_ADDR_UNDEF(vgic_cpu->rd_iodev.base_addr)) {
+			kvm_err("vcpu %d redistributor base not set\n", c);
+			ret = -ENXIO;
+			goto out;
+		}
+	}
+
+	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base)) {
 		kvm_err("Need to set vgic distributor addresses first\n");
 		ret = -ENXIO;
 		goto out;
-- 
2.5.5

[RFC 11/12] KVM: arm/arm64: Add KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Eric Auger]

This new attribute allows the userspace to set the base address
of a reditributor region, relaxing the constraint of having all
consecutive redistibutor frames contiguous.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm/include/uapi/asm/kvm.h   | 7 ++++---
 arch/arm64/include/uapi/asm/kvm.h | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 6edd177..ce67f1d 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -88,9 +88,10 @@ struct kvm_regs {
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
 /* Supported VGICv3 address types  */
-#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
-#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
-#define KVM_VGIC_ITS_ADDR_TYPE		4
+#define KVM_VGIC_V3_ADDR_TYPE_DIST		2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST		3
+#define KVM_VGIC_ITS_ADDR_TYPE			4
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION	5
 
 #define KVM_VGIC_V3_DIST_SIZE		SZ_64K
 #define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 9abbf30..ef8ad3b 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -88,9 +88,10 @@ struct kvm_regs {
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
 /* Supported VGICv3 address types  */
-#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
-#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
-#define KVM_VGIC_ITS_ADDR_TYPE		4
+#define KVM_VGIC_V3_ADDR_TYPE_DIST		2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST		3
+#define KVM_VGIC_ITS_ADDR_TYPE			4
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION	5
 
 #define KVM_VGIC_V3_DIST_SIZE		SZ_64K
 #define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
-- 
2.5.5

[RFC 12/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Eric Auger]

Now all the internals are ready to handle multiple redistributor
regions, let's allow the userspace to register them.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 34 +++++++++++++++++++++++++++++++++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    |  4 ++--
 virt/kvm/arm/vgic/vgic.h            |  7 ++++++-
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index e7b5a86..6a7f8ee 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -92,7 +92,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 		if (r)
 			break;
 		if (write) {
-			r = vgic_v3_set_redist_base(kvm, *addr);
+			r = vgic_v3_set_redist_base(kvm, 0, *addr, 0);
 			goto out;
 		}
 		rdreg = list_first_entry(&vgic->rd_regions,
@@ -103,6 +103,38 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 			addr_ptr = &rdreg->base;
 		break;
 	}
+	case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION:
+	{
+		struct vgic_redist_region *rdreg;
+		uint8_t index;
+
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
+		if (r)
+			break;
+
+		index = *addr & KVM_VGIC_V3_RDIST_INDEX_MASK;
+
+		if (write) {
+			gpa_t base = *addr & KVM_VGIC_V3_RDIST_BASE_MASK;
+			uint32_t pfns = (*addr & KVM_VGIC_V3_RDIST_PFNS_MASK)
+					>> KVM_VGIC_V3_RDIST_PFNS_SHIFT;
+
+			if (!pfns)
+				r = -EINVAL;
+			else
+				r = vgic_v3_set_redist_base(kvm, index,
+							    base, pfns);
+			goto out;
+		}
+
+		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
+		if (!rdreg)
+			r = -ENODEV;
+
+		*addr_ptr = rdreg->base & index &
+			(uint64_t)rdreg->pfns << KVM_VGIC_V3_RDIST_PFNS_SHIFT;
+		break;
+	}
 	default:
 		r = -ENODEV;
 	}
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ea34412..8fa6c08 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -763,11 +763,11 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
 	return ret;
 }
 
-int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
+int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 pfns)
 {
 	int ret;
 
-	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
+	ret = vgic_v3_insert_redist_region(kvm, index, addr, pfns);
 	if (ret)
 		return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index dcf43c7..57165fe 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -96,6 +96,11 @@
 /* we only support 64 kB translation table page size */
 #define KVM_ITS_L1E_ADDR_MASK		GENMASK_ULL(51, 16)
 
+#define KVM_VGIC_V3_RDIST_INDEX_MASK	GENMASK_ULL(1, 0)
+#define KVM_VGIC_V3_RDIST_BASE_MASK	GENMASK_ULL(51, 12)
+#define KVM_VGIC_V3_RDIST_PFNS_MASK	GENMASK_ULL(63, 52)
+#define KVM_VGIC_V3_RDIST_PFNS_SHIFT	52
+
 static inline bool irq_is_pending(struct vgic_irq *irq)
 {
 	if (irq->config == VGIC_CONFIG_EDGE)
@@ -195,7 +200,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_v3_save_pending_tables(struct kvm *kvm);
-int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr);
+int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 pfns);
 int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
 bool vgic_v3_check_base(struct kvm *kvm);
 
-- 
2.5.5

Re: [RFC 01/12] KVM: arm/arm64: Avoid multiple dist->spis kfree

[Marc Zyngier]

On 19/03/18 09:20, Eric Auger wrote:
> in case kvm_vgic_map_resources() fails, typically if the vgic
> distributor is not defined, __kvm_vgic_destroy will be called
> several times. Indeed kvm_vgic_map_resources() is called on
> first vcpu run. As a result dist->spis is freeed twice and on
> the second time it causes a "kernel BUG at mm/slub.c:3912!"
> 
> This patch avoids freeing dist->spis twice.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-init.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 743ca5c..38fd5f1 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -324,7 +324,10 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>  	dist->ready = false;
>  	dist->initialized = false;
>  
> -	kfree(dist->spis);
> +	if (dist->spis) {
> +		kfree(dist->spis);
> +		dist->spis = NULL;
> +	}

Given that kfree(NULL) is always a valid thing to do, you could write
the same thing just as

	dist-> spis = NULL;

without any test.

>  	dist->nr_spis = 0;
>  
>  	if (vgic_supports_direct_msis(kvm))
> 

You also may want to add a Fixes tag to it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 02/12] KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Marc Zyngier]

On 19/03/18 09:20, Eric Auger wrote:
> We introduce a new KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute in
> KVM_DEV_ARM_VGIC_GRP_ADDR group. It allows userspace to provide the
> base address and size of a redistributor region
> 
> Compared to KVM_VGIC_V3_ADDR_TYPE_REDIST, this new attribute allows
> to declare several separate redistributor regions.
> 
> So the whole redist space does not need to be contiguous anymore.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 9293b45..2c0bedf 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -27,6 +27,18 @@ Groups:
>        VCPU and all of the redistributor pages are contiguous.
>        Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
>        This address needs to be 64K aligned.
> +
> +    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (rw, 64-bit)
> +      The attr field of kvm_device_attr encodes 3 values:
> +      bits:     | 63   ....  52  |  51   ....   12 |11 - 0
> +      values:   |     pfns       |       base      | index
> +      - index encodes the unique redistibutor region index
> +      - base field encodes bits [51:12] the guest physical base address
> +        of the first redistributor in the region. There are two 64K pages
> +        for each VCPU and all of the redistributor pages are contiguous
> +        within the redistributor region.

Why does base have to include bits [15:12] of the IPA? If it is 64kB
aligned (as it should), these bits are guaranteed to be 0. This also
avoid having to return -EINVAL in case of alignment problems.

> +      - pfns encodes the size of the region in 64kB pages.
> +      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.

"pfns" a bit of an odd name, and is a bit unnatural. Why don't we
instead encode the number of redistributors? You've already imposed that
a redistributor has 2 64kB pages, so let's take advantage of this by
saving yet another bit.

You could dedicate a couple of these bits as a selector for the ITS
version (v3, v4 or whatever comes after that, if ever...). Just make 0
the only valid version for the time being.

>    Errors:
>      -E2BIG:  Address outside of addressable IPA range
>      -EINVAL: Incorrectly aligned address
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 03/12] KVM: arm/arm64: Record RDIST Last bit at registration

[Marc Zyngier]

On 19/03/18 09:20, Eric Auger wrote:
> To prepare for multiple RDIST regions, let's record the RDIST
> Last bit field when registering the IO device.
> 
> As a reminder the Last bit indicates whether the redistributor
> is the highest one in a series of contiguous redistributor pages.
> 
> Since at the moment KVM only supports a single redist region,
> the RDIST tagged with the last bit set to true corresponds to the
> highest vCPU one.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/kvm/arm_vgic.h           | 1 +
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index cdbd142..1c8c0ff 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -312,6 +312,7 @@ struct vgic_cpu {
>  	 */
>  	struct vgic_io_device	rd_iodev;
>  	struct vgic_io_device	sgi_iodev;
> +	bool rdist_last; /* Is the RDIST the last one of the RDIST region? */
>  
>  	/* Contains the attributes and gpa of the LPI pending tables. */
>  	u64 pendbaser;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 671fe81..75fe689 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -184,12 +184,13 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>  					      gpa_t addr, unsigned int len)
>  {
>  	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	int target_vcpu_id = vcpu->vcpu_id;
>  	u64 value;
>  
>  	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
>  	value |= ((target_vcpu_id & 0xffff) << 8);
> -	if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
> +	if (vgic_cpu->rdist_last)
>  		value |= GICR_TYPER_LAST;
>  	if (vgic_has_its(vcpu->kvm))
>  		value |= GICR_TYPER_PLPIS;
> @@ -580,6 +581,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	struct vgic_dist *vgic = &kvm->arch.vgic;
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
>  	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
>  	gpa_t rd_base, sgi_base;
> @@ -632,6 +634,8 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  	}
>  
>  	vgic->vgic_redist_free_offset += 2 * SZ_64K;
> +	vgic_cpu->rdist_last =
> +		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
>  out:
>  	mutex_unlock(&kvm->slots_lock);
>  	return ret;
> 

I don't really like the idea of keeping this "Last" bit around, because
it doesn't have much to do with the state of a redistributor, and has
more to do with its position in the region.

What is wrong with the current approach of dynamically computing the
Last bit? If you have multiple regions, all you need to know is which
region this redistributor belongs to. From that point (and assuming you
know how many redistributors have been instantiated in that region, you
can know whether the Last bit is set or not.

One of the issue is that the current code works in term of "target cpu",
while we really want is to use the addr parameter to work out if the
Last bit is set or not. I'd be a lot more confident if we emulate it
like that (which is how the HW would generate it too).

What do you think?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 01/12] KVM: arm/arm64: Avoid multiple dist->spis kfree

[Auger Eric]

Hi Marc,

On 19/03/18 14:46, Marc Zyngier wrote:
> On 19/03/18 09:20, Eric Auger wrote:
>> in case kvm_vgic_map_resources() fails, typically if the vgic
>> distributor is not defined, __kvm_vgic_destroy will be called
>> several times. Indeed kvm_vgic_map_resources() is called on
>> first vcpu run. As a result dist->spis is freeed twice and on
>> the second time it causes a "kernel BUG at mm/slub.c:3912!"
>>
>> This patch avoids freeing dist->spis twice.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-init.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>> index 743ca5c..38fd5f1 100644
>> --- a/virt/kvm/arm/vgic/vgic-init.c
>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>> @@ -324,7 +324,10 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>>  	dist->ready = false;
>>  	dist->initialized = false;
>>  
>> -	kfree(dist->spis);
>> +	if (dist->spis) {
>> +		kfree(dist->spis);
>> +		dist->spis = NULL;
>> +	}
> 
> Given that kfree(NULL) is always a valid thing to do, you could write
> the same thing just as
> 
> 	dist-> spis = NULL;
> 
> without any test.
sure
> 
>>  	dist->nr_spis = 0;
>>  
>>  	if (vgic_supports_direct_msis(kvm))
>>
> 
> You also may want to add a Fixes tag to it.
OK

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: [RFC 02/12] KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Auger Eric]

Hi Marc,

On 19/03/18 15:15, Marc Zyngier wrote:
> On 19/03/18 09:20, Eric Auger wrote:
>> We introduce a new KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute in
>> KVM_DEV_ARM_VGIC_GRP_ADDR group. It allows userspace to provide the
>> base address and size of a redistributor region
>>
>> Compared to KVM_VGIC_V3_ADDR_TYPE_REDIST, this new attribute allows
>> to declare several separate redistributor regions.
>>
>> So the whole redist space does not need to be contiguous anymore.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> index 9293b45..2c0bedf 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> @@ -27,6 +27,18 @@ Groups:
>>        VCPU and all of the redistributor pages are contiguous.
>>        Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
>>        This address needs to be 64K aligned.
>> +
>> +    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (rw, 64-bit)
>> +      The attr field of kvm_device_attr encodes 3 values:
>> +      bits:     | 63   ....  52  |  51   ....   12 |11 - 0
>> +      values:   |     pfns       |       base      | index
>> +      - index encodes the unique redistibutor region index
>> +      - base field encodes bits [51:12] the guest physical base address
>> +        of the first redistributor in the region. There are two 64K pages
>> +        for each VCPU and all of the redistributor pages are contiguous
>> +        within the redistributor region.
> 
> Why does base have to include bits [15:12] of the IPA? If it is 64kB
> aligned (as it should), these bits are guaranteed to be 0. This also
> avoid having to return -EINVAL in case of alignment problems.
Agreed. I hesitated but saw addresses were generally described  with
bits[51:12] in regs such as PROPBASER/PENDBASER and I did not really
need the spare bits.
> 
>> +      - pfns encodes the size of the region in 64kB pages.
>> +      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
> 
> "pfns" a bit of an odd name, and is a bit unnatural. Why don't we
> instead encode the number of redistributors? You've already imposed that
> a redistributor has 2 64kB pages,
I copied the description in KVM_VGIC_V3_ADDR_TYPE_REDIST ;-) I knew that
for GICv4 the number of 64kB pages per redist is different so I thought
it was more generic. But I don't have a strong opinion.
 so let's take advantage of this by
> saving yet another bit.
> 
> You could dedicate a couple of these bits as a selector for the ITS
> version (v3, v4 or whatever comes after that, if ever...). Just make 0
> the only valid version for the time being.
OK

Thanks

Eric
> 
>>    Errors:
>>      -E2BIG:  Address outside of addressable IPA range
>>      -EINVAL: Incorrectly aligned address
>>
> 
> Thanks,
> 
> 	M.
> 

Re: [RFC 03/12] KVM: arm/arm64: Record RDIST Last bit at registration

[Auger Eric]

Hi Marc,

On 19/03/18 16:57, Marc Zyngier wrote:
> On 19/03/18 09:20, Eric Auger wrote:
>> To prepare for multiple RDIST regions, let's record the RDIST
>> Last bit field when registering the IO device.
>>
>> As a reminder the Last bit indicates whether the redistributor
>> is the highest one in a series of contiguous redistributor pages.
>>
>> Since at the moment KVM only supports a single redist region,
>> the RDIST tagged with the last bit set to true corresponds to the
>> highest vCPU one.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/kvm/arm_vgic.h           | 1 +
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +++++-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index cdbd142..1c8c0ff 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -312,6 +312,7 @@ struct vgic_cpu {
>>  	 */
>>  	struct vgic_io_device	rd_iodev;
>>  	struct vgic_io_device	sgi_iodev;
>> +	bool rdist_last; /* Is the RDIST the last one of the RDIST region? */
>>  
>>  	/* Contains the attributes and gpa of the LPI pending tables. */
>>  	u64 pendbaser;
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 671fe81..75fe689 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -184,12 +184,13 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>>  					      gpa_t addr, unsigned int len)
>>  {
>>  	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>  	int target_vcpu_id = vcpu->vcpu_id;
>>  	u64 value;
>>  
>>  	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
>>  	value |= ((target_vcpu_id & 0xffff) << 8);
>> -	if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
>> +	if (vgic_cpu->rdist_last)
>>  		value |= GICR_TYPER_LAST;
>>  	if (vgic_has_its(vcpu->kvm))
>>  		value |= GICR_TYPER_PLPIS;
>> @@ -580,6 +581,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>  {
>>  	struct kvm *kvm = vcpu->kvm;
>>  	struct vgic_dist *vgic = &kvm->arch.vgic;
>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>  	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
>>  	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
>>  	gpa_t rd_base, sgi_base;
>> @@ -632,6 +634,8 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>  	}
>>  
>>  	vgic->vgic_redist_free_offset += 2 * SZ_64K;
>> +	vgic_cpu->rdist_last =
>> +		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
>>  out:
>>  	mutex_unlock(&kvm->slots_lock);
>>  	return ret;
>>
> 
> I don't really like the idea of keeping this "Last" bit around, because
> it doesn't have much to do with the state of a redistributor, and has
> more to do with its position in the region.
agreed. What about putting it in vgic_io_device then?.
> 
> What is wrong with the current approach of dynamically computing the
> Last bit? If you have multiple regions, all you need to know is which
> region this redistributor belongs to. From that point (and assuming you
> know how many redistributors have been instantiated in that region, you
> can know whether the Last bit is set or not.
Well, looked to me more natural to compute the information once when
registering the rdist into its region instead of each time we read the
RDIST TYPER. Especially because at that moment we map the redist, we
need to know anyway if we have sufficient space to put it.
> 
> One of the issue is that the current code works in term of "target cpu",
> while we really want is to use the addr parameter to work out if the
> Last bit is set or not. I'd be a lot more confident if we emulate it
> like that (which is how the HW would generate it too).
Sorry I don't understand your comment above.

Thanks

Eric
> 
> What do you think?
> 
> 	M.
> 

Re: [RFC 03/12] KVM: arm/arm64: Record RDIST Last bit at registration

[Marc Zyngier]

On 19/03/18 21:06, Auger Eric wrote:
> Hi Marc,
> 
> On 19/03/18 16:57, Marc Zyngier wrote:
>> On 19/03/18 09:20, Eric Auger wrote:
>>> To prepare for multiple RDIST regions, let's record the RDIST
>>> Last bit field when registering the IO device.
>>>
>>> As a reminder the Last bit indicates whether the redistributor
>>> is the highest one in a series of contiguous redistributor pages.
>>>
>>> Since at the moment KVM only supports a single redist region,
>>> the RDIST tagged with the last bit set to true corresponds to the
>>> highest vCPU one.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  include/kvm/arm_vgic.h           | 1 +
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +++++-
>>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index cdbd142..1c8c0ff 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -312,6 +312,7 @@ struct vgic_cpu {
>>>  	 */
>>>  	struct vgic_io_device	rd_iodev;
>>>  	struct vgic_io_device	sgi_iodev;
>>> +	bool rdist_last; /* Is the RDIST the last one of the RDIST region? */
>>>  
>>>  	/* Contains the attributes and gpa of the LPI pending tables. */
>>>  	u64 pendbaser;
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 671fe81..75fe689 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -184,12 +184,13 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>>>  					      gpa_t addr, unsigned int len)
>>>  {
>>>  	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>  	int target_vcpu_id = vcpu->vcpu_id;
>>>  	u64 value;
>>>  
>>>  	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
>>>  	value |= ((target_vcpu_id & 0xffff) << 8);
>>> -	if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
>>> +	if (vgic_cpu->rdist_last)
>>>  		value |= GICR_TYPER_LAST;
>>>  	if (vgic_has_its(vcpu->kvm))
>>>  		value |= GICR_TYPER_PLPIS;
>>> @@ -580,6 +581,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct kvm *kvm = vcpu->kvm;
>>>  	struct vgic_dist *vgic = &kvm->arch.vgic;
>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>  	struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
>>>  	struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
>>>  	gpa_t rd_base, sgi_base;
>>> @@ -632,6 +634,8 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>>  	}
>>>  
>>>  	vgic->vgic_redist_free_offset += 2 * SZ_64K;
>>> +	vgic_cpu->rdist_last =
>>> +		(vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
>>>  out:
>>>  	mutex_unlock(&kvm->slots_lock);
>>>  	return ret;
>>>
>>
>> I don't really like the idea of keeping this "Last" bit around, because
>> it doesn't have much to do with the state of a redistributor, and has
>> more to do with its position in the region.
>
> agreed. What about putting it in vgic_io_device then?.

I'm not sure either. See below.

>>
>> What is wrong with the current approach of dynamically computing the
>> Last bit? If you have multiple regions, all you need to know is which
>> region this redistributor belongs to. From that point (and assuming you
>> know how many redistributors have been instantiated in that region, you
>> can know whether the Last bit is set or not.
>
> Well, looked to me more natural to compute the information once when
> registering the rdist into its region instead of each time we read the
> RDIST TYPER.

That's a trade-off (memory vs time). Reading GICR_TYPER is a rare thing
(as for all MMIO), and comparing two addresses is cheap. On the other
hand, adding immutable state to each vcpu seems a bit strange.

> Especially because at that moment we map the redist, we
> need to know anyway if we have sufficient space to put it.

What do you mean by that? Is that related to the way we currently map
redistributors?

>>
>> One of the issue is that the current code works in term of "target cpu",
>> while we really want is to use the addr parameter to work out if the
>> Last bit is set or not. I'd be a lot more confident if we emulate it
>> like that (which is how the HW would generate it too).
>
> Sorry I don't understand your comment above.

What I mean is that the current code would be better written as

	if (addr = redist_base + 128K * (nr_vcpus -1) + GICR_TYPER)
		value |= GICR_TYPER_LAST;

rather than considering the vcpu_id of the target redistributor
associated vcpu (which I find crazy complicated). This check is the way
a HW implementation would perform it, not by storing some extra state.
You can easily expand this to multiple rdist banks.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 02/12] KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Peter Maydell]

On 19 March 2018 at 09:20, Eric Auger <eric.auger@redhat.com> wrote:
> We introduce a new KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute in
> KVM_DEV_ARM_VGIC_GRP_ADDR group. It allows userspace to provide the
> base address and size of a redistributor region
>
> Compared to KVM_VGIC_V3_ADDR_TYPE_REDIST, this new attribute allows
> to declare several separate redistributor regions.
>
> So the whole redist space does not need to be contiguous anymore.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 9293b45..2c0bedf 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -27,6 +27,18 @@ Groups:
>        VCPU and all of the redistributor pages are contiguous.
>        Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
>        This address needs to be 64K aligned.
> +
> +    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (rw, 64-bit)
> +      The attr field of kvm_device_attr encodes 3 values:
> +      bits:     | 63   ....  52  |  51   ....   12 |11 - 0
> +      values:   |     pfns       |       base      | index
> +      - index encodes the unique redistibutor region index

"redistributor"

> +      - base field encodes bits [51:12] the guest physical base address

"of the guest"

> +        of the first redistributor in the region. There are two 64K pages
> +        for each VCPU and all of the redistributor pages are contiguous
> +        within the redistributor region.
> +      - pfns encodes the size of the region in 64kB pages.
> +      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.

You should say something here about what happens if userspace
tries to use both KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION and
KVM_VGIC_V3_ADDR_TYPE_REDIST. I think this should be an error
(reported by whichever of the two you try to set second).

>    Errors:
>      -E2BIG:  Address outside of addressable IPA range
>      -EINVAL: Incorrectly aligned address

Marc wrote:
> Why does base have to include bits [15:12] of the IPA? If it is 64kB
> aligned (as it should), these bits are guaranteed to be 0. This also
> avoid having to return -EINVAL in case of alignment problems.

If you're not using the bits for anything else you want to
check they're 0 anyway. Otherwise we can't guarantee to safely
use them for something else in future, because userspace might
be handing us garbage in those bits without noticing.

thanks
-- PMM