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

[PATCH v3 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-rdist-regions-v3
Previous version:
https://github.com/eauger/linux/tree/4.16-rc7-rdist-regions-rfc-v2

History:

v2 -> v3:
- Add details to the user API documentation
- early exit if vgic_v3_rdist_region_from_index() fails
- return -EINVAL if legacy and new redist region API are mixed

v1 -> v2:
- Rework the uapi. Only bits [51:16] of the redist region are
  exposed. Also a new flags field was introduced
- Do not store the last bit in the vgic_cpu struct anymore
- remove dist->spis check in 1st patch
- add last patch to bump VGIC_V3_MAX_CPUS to 512
- advertise the new attribute


Eric Auger (12):
  KVM: arm/arm64: Set dist->spis to NULL after kfree
  KVM: arm/arm64: Document KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
  KVM: arm/arm64: Replace the single rdist region by a list
  KVM: arm/arm64: Helper to locate free rdist index
  KVM: arm/arm64: Revisit Redistributor TYPER last bit computation
  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
  KVM: arm/arm64: Bump VGIC_V3_MAX_CPUS to 512

 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  25 ++++-
 arch/arm/include/uapi/asm/kvm.h                   |   7 +-
 arch/arm64/include/uapi/asm/kvm.h                 |   7 +-
 include/kvm/arm_vgic.h                            |  16 ++-
 virt/kvm/arm/vgic/vgic-init.c                     |  20 +++-
 virt/kvm/arm/vgic/vgic-kvm-device.c               |  55 +++++++++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c                  | 120 +++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-v3.c                       |  87 ++++++++++++----
 virt/kvm/arm/vgic/vgic.h                          |  34 +++++-
 9 files changed, 320 insertions(+), 51 deletions(-)

-- 
2.5.5

[PATCH v3 01/12] KVM: arm/arm64: Set dist->spis to NULL after 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 more than once
and on the second time it causes a "kernel BUG at mm/slub.c:3912!"

Set dist->spis to NULL to avoid the crash.

Fixes: ad275b8bb1e6 ("KVM: arm/arm64: vgic-new: vgic_init: implement
vgic_init")

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

---

v2 -> v3:
- added Marc's R-b and Fixed commit
---
 virt/kvm/arm/vgic/vgic-init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 68378fe..c52f03d 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -308,6 +308,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	dist->initialized = false;
 
 	kfree(dist->spis);
+	dist->spis = NULL;
 	dist->nr_spis = 0;
 
 	if (vgic_supports_direct_msis(kvm))
-- 
2.5.5

Re: [PATCH v3 01/12] KVM: arm/arm64: Set dist->spis to NULL after kfree

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:47AM +0200, 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 more than once
> and on the second time it causes a "kernel BUG at mm/slub.c:3912!"
> 
> Set dist->spis to NULL to avoid the crash.
> 
> Fixes: ad275b8bb1e6 ("KVM: arm/arm64: vgic-new: vgic_init: implement
> vgic_init")
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

> 
> ---
> 
> v2 -> v3:
> - added Marc's R-b and Fixed commit
> ---
>  virt/kvm/arm/vgic/vgic-init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 68378fe..c52f03d 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -308,6 +308,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>  	dist->initialized = false;
>  
>  	kfree(dist->spis);
> +	dist->spis = NULL;
>  	dist->nr_spis = 0;
>  
>  	if (vgic_supports_direct_msis(kvm))
> -- 
> 2.5.5
> 

[PATCH v3 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 | 25 ++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
index 9293b45..cbc4328 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
@@ -27,9 +27,32 @@ 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   ....   16 | 15 - 12  |11 - 0
+      values:   |     count      |       base      |  flags   | index
+      - index encodes the unique redistributor region index
+      - flags: reserved for future use, currently 0
+      - base field encodes bits [51:16] of the guest physical base address
+        of the first redistributor in the region.
+      - count encodes the number of redistributors in the region. Must be
+        greater than 0.
+      There are two 64K pages for each redistributor in the region and
+      redistributors are laid out contiguously within the region. Regions
+      are filled with redistributors in the index order. The sum of all
+      region count fields must be greater than or equal to the number of
+      VCPUs. Redistributor regions must be registered in the incremental
+      index order, starting from index 0.
+      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
+
+  It is invalid to mix calls with KVM_VGIC_V3_ADDR_TYPE_REDIST and
+  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attributes.
+
   Errors:
     -E2BIG:  Address outside of addressable IPA range
-    -EINVAL: Incorrectly aligned address
+    -EINVAL: Incorrectly aligned address, bad redistributor region
+             count/index, mixed redistributor region attribute usage
     -EEXIST: Address already configured
     -ENXIO:  The group or attribute is unknown/unsupported for this device
              or hardware support is missing.
-- 
2.5.5

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

[Peter Maydell]

On 13 April 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>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:48AM +0200, 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 | 25 ++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 9293b45..cbc4328 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -27,9 +27,32 @@ 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   ....   16 | 15 - 12  |11 - 0
> +      values:   |     count      |       base      |  flags   | index
> +      - index encodes the unique redistributor region index

I'm not entirely sure I understand the purpose of the index field.
Isn't a redistributor region identified uniquely by its base address?

Otherwise this looks good.

Thanks,
-Christoffer


> +      - flags: reserved for future use, currently 0
> +      - base field encodes bits [51:16] of the guest physical base address
> +        of the first redistributor in the region.
> +      - count encodes the number of redistributors in the region. Must be
> +        greater than 0.
> +      There are two 64K pages for each redistributor in the region and
> +      redistributors are laid out contiguously within the region. Regions
> +      are filled with redistributors in the index order. The sum of all
> +      region count fields must be greater than or equal to the number of
> +      VCPUs. Redistributor regions must be registered in the incremental
> +      index order, starting from index 0.
> +      Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
> +
> +  It is invalid to mix calls with KVM_VGIC_V3_ADDR_TYPE_REDIST and
> +  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attributes.
> +
>    Errors:
>      -E2BIG:  Address outside of addressable IPA range
> -    -EINVAL: Incorrectly aligned address
> +    -EINVAL: Incorrectly aligned address, bad redistributor region
> +             count/index, mixed redistributor region attribute usage
>      -EEXIST: Address already configured
>      -ENXIO:  The group or attribute is unknown/unsupported for this device
>               or hardware support is missing.
> -- 
> 2.5.5
> 

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

[Peter Maydell]

On 24 April 2018 at 17:46, Christoffer Dall <christoffer.dall@arm.com> wrote:
> On Fri, Apr 13, 2018 at 10:20:48AM +0200, Eric Auger wrote:
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> @@ -27,9 +27,32 @@ 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   ....   16 | 15 - 12  |11 - 0
>> +      values:   |     count      |       base      |  flags   | index
>> +      - index encodes the unique redistributor region index
>
> I'm not entirely sure I understand the purpose of the index field.
> Isn't a redistributor region identified uniquely by its base address?

You need a way to tell the difference beween:
 (1) redistributors for CPUs 0..63 at 0x40000000, redistributors
     for 64..127 at 0x80000000
 (2) redistributors for CPUs 0..63 at 0x80000000, redistributors
     for 64..127 at 0x40000000

The index field tells you which order the redistributor
regions go in.

thanks
-- PMM

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

[Auger Eric]

Hi Christoffer, Peter,

On 04/24/2018 06:50 PM, Peter Maydell wrote:
> On 24 April 2018 at 17:46, Christoffer Dall <christoffer.dall@arm.com> wrote:
>> On Fri, Apr 13, 2018 at 10:20:48AM +0200, Eric Auger wrote:
>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>>> @@ -27,9 +27,32 @@ 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   ....   16 | 15 - 12  |11 - 0
>>> +      values:   |     count      |       base      |  flags   | index
>>> +      - index encodes the unique redistributor region index
>>
>> I'm not entirely sure I understand the purpose of the index field.
>> Isn't a redistributor region identified uniquely by its base address?
> 
> You need a way to tell the difference beween:
>  (1) redistributors for CPUs 0..63 at 0x40000000, redistributors
>      for 64..127 at 0x80000000
>  (2) redistributors for CPUs 0..63 at 0x80000000, redistributors
>      for 64..127 at 0x40000000
> 
> The index field tells you which order the redistributor
> regions go in.

Yes redistributors are filled in the index order. This matches DT
description
(Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt):

<0x0 0x2d000000 0 0x800000>,      // GICR 1: CPUs 0-31
<0x0 0x2e000000 0 0x800000>;      // GICR 2: CPUs 32-63

Thanks

Eric


> 
> thanks
> -- PMM
> 

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

[Christoffer Dall]

On Tue, Apr 24, 2018 at 05:50:37PM +0100, Peter Maydell wrote:
> On 24 April 2018 at 17:46, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > On Fri, Apr 13, 2018 at 10:20:48AM +0200, Eric Auger wrote:
> >> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> >> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> >> @@ -27,9 +27,32 @@ 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   ....   16 | 15 - 12  |11 - 0
> >> +      values:   |     count      |       base      |  flags   | index
> >> +      - index encodes the unique redistributor region index
> >
> > I'm not entirely sure I understand the purpose of the index field.
> > Isn't a redistributor region identified uniquely by its base address?
> 
> You need a way to tell the difference beween:
>  (1) redistributors for CPUs 0..63 at 0x40000000, redistributors
>      for 64..127 at 0x80000000
>  (2) redistributors for CPUs 0..63 at 0x80000000, redistributors
>      for 64..127 at 0x40000000
> 
> The index field tells you which order the redistributor
> regions go in.

ah, right.  This could be implied by the order creating the regions
though, but ok, in that case it's nicer for userspace to state it
explicitly.

Thanks,
-Christoffer

[PATCH v3 03/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.

The redistributor region handle is stored in the vgic_cpu structure
to allow later computation of the TYPER last bit.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 24f0394..e5c16d1 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -200,6 +200,14 @@ struct vgic_its {
 
 struct vgic_state_iter;
 
+struct vgic_redist_region {
+	uint32_t index;
+	gpa_t base;
+	uint32_t count; /* number of redistributors or 0 if single region */
+	uint32_t free_index; /* index of the next free redistributor */
+	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 */
@@ -310,6 +315,7 @@ struct vgic_cpu {
 	 */
 	struct vgic_io_device	rd_iodev;
 	struct vgic_io_device	sgi_iodev;
+	struct vgic_redist_region *rdreg;
 
 	/* Contains the attributes and gpa of the LPI pending tables. */
 	u64 pendbaser;
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index c52f03d..6456371 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -167,8 +167,11 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 	kvm->arch.vgic.vgic_model = type;
 
 	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--) {
@@ -303,6 +306,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;
@@ -311,6 +315,14 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	dist->spis = NULL;
 	dist->nr_spis = 0;
 
+	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);
+	}
+
 	if (vgic_supports_direct_msis(kvm))
 		vgic_v4_teardown(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 671fe81..d1aab18 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -580,8 +580,10 @@ 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;
+	struct vgic_redist_region *rdreg;
 	gpa_t rd_base, sgi_base;
 	int ret;
 
@@ -591,13 +593,17 @@ 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;
+	vgic_cpu->rdreg = rdreg;
+
+	rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
 	sgi_base = rd_base + SZ_64K;
 
 	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
@@ -631,7 +637,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
-	vgic->vgic_redist_free_offset += 2 * SZ_64K;
+	rdreg->free_index++;
 out:
 	mutex_unlock(&kvm->slots_lock);
 	return ret;
@@ -673,19 +679,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;
 
 	/* vgic_check_ioaddr makes sure we don't do this twice */
-	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
-	if (ret)
-		return ret;
-
-	vgic->vgic_redist_base = addr;
-	if (!vgic_v3_check_base(kvm)) {
-		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
+	if (!list_empty(&vgic->rd_regions))
 		return -EINVAL;
+
+	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
+	if (!rdreg)
+		return -ENOMEM;
+
+	rdreg->base = VGIC_ADDR_UNDEF;
+
+	ret = vgic_check_ioaddr(kvm, &rdreg->base, addr, SZ_64K);
+	if (ret)
+		goto out;
+
+	rdreg->base = addr;
+	if (!vgic_v3_check_base(kvm)) {
+		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.
@@ -695,6 +713,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 8195f52..94de6cd 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -418,6 +418,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);
 
@@ -425,18 +428,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;
@@ -446,12 +448,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

Re: [PATCH v3 03/12] KVM: arm/arm64: Replace the single rdist region by a list

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:49AM +0200, Eric Auger wrote:
> 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.
> 
> The redistributor region handle is stored in the vgic_cpu structure
> to allow later computation of the TYPER last bit.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

> ---
>  include/kvm/arm_vgic.h              | 14 +++++++++----
>  virt/kvm/arm/vgic/vgic-init.c       | 16 ++++++++++++--
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 13 ++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 42 ++++++++++++++++++++++++++++---------
>  virt/kvm/arm/vgic/vgic-v3.c         | 20 +++++++++++-------
>  5 files changed, 79 insertions(+), 26 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 24f0394..e5c16d1 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -200,6 +200,14 @@ struct vgic_its {
>  
>  struct vgic_state_iter;
>  
> +struct vgic_redist_region {
> +	uint32_t index;
> +	gpa_t base;
> +	uint32_t count; /* number of redistributors or 0 if single region */
> +	uint32_t free_index; /* index of the next free redistributor */
> +	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 */
> @@ -310,6 +315,7 @@ struct vgic_cpu {
>  	 */
>  	struct vgic_io_device	rd_iodev;
>  	struct vgic_io_device	sgi_iodev;
> +	struct vgic_redist_region *rdreg;
>  
>  	/* Contains the attributes and gpa of the LPI pending tables. */
>  	u64 pendbaser;
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index c52f03d..6456371 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -167,8 +167,11 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	kvm->arch.vgic.vgic_model = type;
>  
>  	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--) {
> @@ -303,6 +306,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;
> @@ -311,6 +315,14 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>  	dist->spis = NULL;
>  	dist->nr_spis = 0;
>  
> +	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);
> +	}
> +
>  	if (vgic_supports_direct_msis(kvm))
>  		vgic_v4_teardown(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 671fe81..d1aab18 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -580,8 +580,10 @@ 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;
> +	struct vgic_redist_region *rdreg;
>  	gpa_t rd_base, sgi_base;
>  	int ret;
>  
> @@ -591,13 +593,17 @@ 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;
> +	vgic_cpu->rdreg = rdreg;
> +
> +	rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
>  	sgi_base = rd_base + SZ_64K;
>  
>  	kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> @@ -631,7 +637,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  		goto out;
>  	}
>  
> -	vgic->vgic_redist_free_offset += 2 * SZ_64K;
> +	rdreg->free_index++;
>  out:
>  	mutex_unlock(&kvm->slots_lock);
>  	return ret;
> @@ -673,19 +679,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;
>  
>  	/* vgic_check_ioaddr makes sure we don't do this twice */
> -	ret = vgic_check_ioaddr(kvm, &vgic->vgic_redist_base, addr, SZ_64K);
> -	if (ret)
> -		return ret;
> -
> -	vgic->vgic_redist_base = addr;
> -	if (!vgic_v3_check_base(kvm)) {
> -		vgic->vgic_redist_base = VGIC_ADDR_UNDEF;
> +	if (!list_empty(&vgic->rd_regions))
>  		return -EINVAL;
> +
> +	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
> +	if (!rdreg)
> +		return -ENOMEM;
> +
> +	rdreg->base = VGIC_ADDR_UNDEF;
> +
> +	ret = vgic_check_ioaddr(kvm, &rdreg->base, addr, SZ_64K);
> +	if (ret)
> +		goto out;
> +
> +	rdreg->base = addr;
> +	if (!vgic_v3_check_base(kvm)) {
> +		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.
> @@ -695,6 +713,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 8195f52..94de6cd 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -418,6 +418,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);
>  
> @@ -425,18 +428,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;
> @@ -446,12 +448,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
> 

[PATCH v3 04/12] KVM: arm/arm64: Helper to locate free rdist index

[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 |  3 +--
 virt/kvm/arm/vgic/vgic-v3.c      | 17 +++++++++++++++++
 virt/kvm/arm/vgic/vgic.h         | 11 +++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index d1aab18..49ca176 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -593,8 +593,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);
 	if (!rdreg)
 		return 0;
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 94de6cd..820012a 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -444,6 +444,23 @@ bool vgic_v3_check_base(struct kvm *kvm)
 	return false;
 }
 
+/**
+ * vgic_v3_rdist_free_slot - Look up registered rdist regions and identify one
+ * which has free space to put a new rdist regions
+ *
+ * If any, return this redist region handle, otherwise returns NULL.
+ */
+struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions)
+{
+	struct vgic_redist_region *rdreg;
+
+	list_for_each_entry(rdreg, rd_regions, list) {
+		if (!vgic_v3_redist_region_full(rdreg))
+			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 830e815..fea32cb 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -251,6 +251,17 @@ 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->count)
+		return false;
+
+	return (region->free_index >= region->count);
+}
+
+struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
+
 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

Re: [PATCH v3 04/12] KVM: arm/arm64: Helper to locate free rdist index

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:50AM +0200, Eric Auger wrote:
> 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 |  3 +--
>  virt/kvm/arm/vgic/vgic-v3.c      | 17 +++++++++++++++++
>  virt/kvm/arm/vgic/vgic.h         | 11 +++++++++++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index d1aab18..49ca176 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -593,8 +593,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);
>  	if (!rdreg)
>  		return 0;
>  
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 94de6cd..820012a 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -444,6 +444,23 @@ bool vgic_v3_check_base(struct kvm *kvm)
>  	return false;
>  }
>  
> +/**
> + * vgic_v3_rdist_free_slot - Look up registered rdist regions and identify one
> + * which has free space to put a new rdist regions

Can this structure ever be sparse or do we always find the first empty
one, as we fill them consecutively ?

I assume there is some mapping between the regions and the VCPUs'
redistributors, so perhaps the wording in this comment can be more
precise.

> + *
> + * If any, return this redist region handle, otherwise returns NULL.
> + */
> +struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions)
> +{
> +	struct vgic_redist_region *rdreg;
> +
> +	list_for_each_entry(rdreg, rd_regions, list) {
> +		if (!vgic_v3_redist_region_full(rdreg))
> +			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 830e815..fea32cb 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -251,6 +251,17 @@ 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->count)
> +		return false;
> +
> +	return (region->free_index >= region->count);
> +}
> +
> +struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
> +
>  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
> 

Asides from the above:

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

Re: [PATCH v3 04/12] KVM: arm/arm64: Helper to locate free rdist index

[Auger Eric]

Hi Christoffer,
On 04/24/2018 11:07 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:50AM +0200, Eric Auger wrote:
>> 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 |  3 +--
>>  virt/kvm/arm/vgic/vgic-v3.c      | 17 +++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h         | 11 +++++++++++
>>  3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index d1aab18..49ca176 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -593,8 +593,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);
>>  	if (!rdreg)
>>  		return 0;
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 94de6cd..820012a 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -444,6 +444,23 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>  	return false;
>>  }
>>  
>> +/**
>> + * vgic_v3_rdist_free_slot - Look up registered rdist regions and identify one
>> + * which has free space to put a new rdist regions
> 
> Can this structure ever be sparse or do we always find the first empty
> one, as we fill them consecutively ?
We always find the first empty 2x64kB slot within the region (stride
between redistributors = 0). Regions are filled in the index order. The
only hole that can exist is at the end of the region if its size is not
multiple of 2x64kB.
> 
> I assume there is some mapping between the regions and the VCPUs'
> redistributors, so perhaps the wording in this comment can be more
> precise.
changed into:
"
A redistributor regions maps n redistributors, n = region size / (2 x 64kB).
Stride between redistributors is 0 and regions are filled in the index
order.
"

> 
>> + *
>> + * If any, return this redist region handle, otherwise returns NULL.
>> + */
>> +struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions)
>> +{
>> +	struct vgic_redist_region *rdreg;
>> +
>> +	list_for_each_entry(rdreg, rd_regions, list) {
>> +		if (!vgic_v3_redist_region_full(rdreg))
>> +			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 830e815..fea32cb 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -251,6 +251,17 @@ 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->count)
>> +		return false;
>> +
>> +	return (region->free_index >= region->count);
>> +}
>> +
>> +struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
>> +
>>  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
>>
> 
> Asides from the above:
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
Thanks

Eric
> 

[PATCH v3 05/12] KVM: arm/arm64: Revisit Redistributor TYPER last bit computation

[Eric Auger]

The TYPER of an redistributor reflects whether the rdist is
the last one of the redistributor region. Let's compare the TYPER
GPA against the address of the last occupied slot within the
redistributor region.

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

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 49ca176..ce5c927 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -184,12 +184,17 @@ 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;
+	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
 	int target_vcpu_id = vcpu->vcpu_id;
+	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
+			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
 	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 (addr == last_rdist_typer)
 		value |= GICR_TYPER_LAST;
 	if (vgic_has_its(vcpu->kvm))
 		value |= GICR_TYPER_PLPIS;
-- 
2.5.5

Re: [PATCH v3 05/12] KVM: arm/arm64: Revisit Redistributor TYPER last bit computation

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:51AM +0200, Eric Auger wrote:
> The TYPER of an redistributor reflects whether the rdist is
> the last one of the redistributor region. Let's compare the TYPER
> GPA against the address of the last occupied slot within the
> redistributor region.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 49ca176..ce5c927 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -184,12 +184,17 @@ 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;
> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>  	int target_vcpu_id = vcpu->vcpu_id;
> +	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> +			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
>  	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 (addr == last_rdist_typer)
>  		value |= GICR_TYPER_LAST;
>  	if (vgic_has_its(vcpu->kvm))
>  		value |= GICR_TYPER_PLPIS;
> -- 
> 2.5.5
> 

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

[PATCH v3 06/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 | 95 +++++++++++++++++++++++++++++++++-------
 virt/kvm/arm/vgic/vgic-v3.c      | 29 ++++++++++++
 virt/kvm/arm/vgic/vgic.h         | 14 ++++++
 3 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ce5c927..5273fb8 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -680,14 +680,66 @@ 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. count != 0). The list is sorted by rdist region index.
+ *
+ * @kvm: kvm handle
+ * @index: redist region index
+ * @base: base of the new rdist region
+ * @count: number of redistributors 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 count)
 {
-	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 = count * KVM_VGIC_V3_REDIST_SIZE;
 	int ret;
 
-	/* vgic_check_ioaddr makes sure we don't do this twice */
-	if (!list_empty(&vgic->rd_regions))
+	/* single rdist region already set ?*/
+	if (!count && !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;
+
+		/* Cannot add an explicitly sized regions after legacy region */
+		if (!rdreg->count)
+			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 (!count && !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);
@@ -696,17 +748,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
 
 	rdreg->base = VGIC_ADDR_UNDEF;
 
-	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->count = count;
+	rdreg->free_index = 0;
+	rdreg->index = index;
 
-	list_add(&rdreg->list, &vgic->rd_regions);
+	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;
+
+	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Register iodevs for each existing VCPU.  Adding more VCPUs
@@ -717,10 +784,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 820012a..dbcba5f 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -410,6 +410,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.
@@ -461,6 +476,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 fea32cb..95b8345 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -262,6 +262,20 @@ vgic_v3_redist_region_full(struct vgic_redist_region *region)
 
 struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
 
+static inline size_t
+vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
+{
+	if (!rdreg->count)
+		return atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE;
+	else
+		return rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
+}
+
+struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
+							   uint32_t index);
+
+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

Re: [PATCH v3 06/12] KVM: arm/arm64: Helper to register a new redistributor region

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:52AM +0200, Eric Auger wrote:
> 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 | 95 +++++++++++++++++++++++++++++++++-------
>  virt/kvm/arm/vgic/vgic-v3.c      | 29 ++++++++++++
>  virt/kvm/arm/vgic/vgic.h         | 14 ++++++
>  3 files changed, 122 insertions(+), 16 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ce5c927..5273fb8 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -680,14 +680,66 @@ 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. count != 0). The list is sorted by rdist region index.
> + *
> + * @kvm: kvm handle
> + * @index: redist region index
> + * @base: base of the new rdist region
> + * @count: number of redistributors 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 count)
>  {
> -	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;
> +

nit: extra blank line?

> +	gpa_t new_start, new_end;
> +	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
>  	int ret;
>  
> -	/* vgic_check_ioaddr makes sure we don't do this twice */
> -	if (!list_empty(&vgic->rd_regions))
> +	/* single rdist region already set ?*/
> +	if (!count && !list_empty(rd_regions))
> +		return -EINVAL;
> +
> +	/* cross the end of memory ? */
> +	if (base + size < base)
> +		return -EINVAL;

what is the size of memory?  This seems to check for a gpa_t overflow,
but not againt the IPA space of the VM...

> +
> +	if (list_empty(rd_regions)) {
> +		if (index != 0)
> +			return -EINVAL;

note, I think this can be simplified if we can rid of the index.

> +	} else {
> +		rdreg = list_entry(last, struct vgic_redist_region, list);

you can use list_last_entry here and get rid of the 'last' temporary
variable above.

> +		if (index != rdreg->index + 1)
> +			return -EINVAL;
> +
> +		/* Cannot add an explicitly sized regions after legacy region */
> +		if (!rdreg->count)
> +			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
> +	 */

I don't really understand this commentary... :(

> +	if (!count && !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;

Can't you call vgic_v3_check_base() here instead?

> +
> +	/* collision with any other rdist region? */
> +	if (vgic_v3_rdist_overlap(kvm, base, size))
>  		return -EINVAL;
>  
>  	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
> @@ -696,17 +748,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
>  
>  	rdreg->base = VGIC_ADDR_UNDEF;
>  
> -	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->count = count;
> +	rdreg->free_index = 0;
> +	rdreg->index = index;
>  
> -	list_add(&rdreg->list, &vgic->rd_regions);
> +	new_start = base;
> +	new_end = base + size - 1;

What are these variables used for?

> +
> +	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;
> +
> +	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Register iodevs for each existing VCPU.  Adding more VCPUs
> @@ -717,10 +784,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 820012a..dbcba5f 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -410,6 +410,21 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  	return 0;
>  }
>  
> +/* return true if there is an overlap between any rdist */

Checks if base+size overlaps with any existing redistributor.

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

can you invert the check above and return false instead of the continue?

(DeMorgan's law should be handy here.)

> +	}
> +	return false;
> +}
> +
>  /*
>   * Check for overlapping regions and for regions crossing the end of memory
>   * for base addresses which have already been set.
> @@ -461,6 +476,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;
> +	}

if this ends up being a common operation, we could allocate an array of
pointers for constant-time lookup instead.  Let's hope it's not too
common.

> +	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 fea32cb..95b8345 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -262,6 +262,20 @@ vgic_v3_redist_region_full(struct vgic_redist_region *region)
>  
>  struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
>  
> +static inline size_t
> +vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
> +{
> +	if (!rdreg->count)
> +		return atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE;
> +	else
> +		return rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
> +}
> +
> +struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
> +							   uint32_t index);
> +
> +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
> 

Thanks,
-Christoffer

Re: [PATCH v3 06/12] KVM: arm/arm64: Helper to register a new redistributor region

[Auger Eric]

Hi Christoffer,

On 04/24/2018 06:47 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:52AM +0200, Eric Auger wrote:
>> 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>
>> ---
>>  | 95 +++++++++++++++++++++++++++++++++-------
>>  virt/kvm/arm/vgic/vgic-v3.c      | 29 ++++++++++++
>>  virt/kvm/arm/vgic/vgic.h         | 14 ++++++
>>  3 files changed, 122 insertions(+), 16 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index ce5c927..5273fb8 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -680,14 +680,66 @@ 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. count != 0). The list is sorted by rdist region index.
>> + *
>> + * @kvm: kvm handle
>> + * @index: redist region index
>> + * @base: base of the new rdist region
>> + * @count: number of redistributors 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 count)
>>  {
>> -	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;
>> +
> 
> nit: extra blank line?
> 
>> +	gpa_t new_start, new_end;
>> +	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
>>  	int ret;
>>  
>> -	/* vgic_check_ioaddr makes sure we don't do this twice */
>> -	if (!list_empty(&vgic->rd_regions))
>> +	/* single rdist region already set ?*/
>> +	if (!count && !list_empty(rd_regions))
>> +		return -EINVAL;
>> +
>> +	/* cross the end of memory ? */
>> +	if (base + size < base)
>> +		return -EINVAL;
> 
> what is the size of memory?  This seems to check for a gpa_t overflow,
> but not againt the IPA space of the VM...
Yes it checks for a gpa_t overflow. This check is currently done in
vgic_v3_check_base() for dist and redist region and I replicated it.
> 
>> +
>> +	if (list_empty(rd_regions)) {
>> +		if (index != 0)
>> +			return -EINVAL;
> 
> note, I think this can be simplified if we can rid of the index.
So I eventually keep the index.
> 
>> +	} else {
>> +		rdreg = list_entry(last, struct vgic_redist_region, list);
> 
> you can use list_last_entry here and get rid of the 'last' temporary
> variable above.
definitively, thank you for the nit.
> 
>> +		if (index != rdreg->index + 1)
>> +			return -EINVAL;
>> +
>> +		/* Cannot add an explicitly sized regions after legacy region */
>> +		if (!rdreg->count)
>> +			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
>> +	 */
> 
> I don't really understand this commentary... :(
I meant we cannot perform the check below if we are inserting a unique
legacy rdist region (old API), whose size is not explicitly set but
induced from the number of online vcpus.

> 
>> +	if (!count && !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;
> 
> Can't you call vgic_v3_check_base() here instead?
no I can't because vgic_v3_check_base() currently only works with the
unique legacy rdist region. There, redist_size  is
atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE.
> 
>> +
>> +	/* collision with any other rdist region? */
>> +	if (vgic_v3_rdist_overlap(kvm, base, size))
>>  		return -EINVAL;
>>  
>>  	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
>> @@ -696,17 +748,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
>>  
>>  	rdreg->base = VGIC_ADDR_UNDEF;
>>  
>> -	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->count = count;
>> +	rdreg->free_index = 0;
>> +	rdreg->index = index;
>>  
>> -	list_add(&rdreg->list, &vgic->rd_regions);
>> +	new_start = base;
>> +	new_end = base + size - 1;
> 
> What are these variables used for?
Hum reminder from an old version :-(
> 
>> +
>> +	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;
>> +
>> +	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
>> +	if (ret)
>> +		return ret;
>>  
>>  	/*
>>  	 * Register iodevs for each existing VCPU.  Adding more VCPUs
>> @@ -717,10 +784,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 820012a..dbcba5f 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -410,6 +410,21 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>  	return 0;
>>  }
>>  
>> +/* return true if there is an overlap between any rdist */
> 
> Checks if base+size overlaps with any existing redistributor.
> 
>> +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;
> 
> can you invert the check above and return false instead of the continue?
> 
> (DeMorgan's law should be handy here.)
sure
> 
>> +	}
>> +	return false;
>> +}
>> +
>>  /*
>>   * Check for overlapping regions and for regions crossing the end of memory
>>   * for base addresses which have already been set.
>> @@ -461,6 +476,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;
>> +	}
> 
> if this ends up being a common operation, we could allocate an array of
> pointers for constant-time lookup instead.  Let's hope it's not too
> common.
This is only used when reading the characteristics of a redist region
from userspace so I don't think we care.

Thanks

Eric
> 
>> +	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 fea32cb..95b8345 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -262,6 +262,20 @@ vgic_v3_redist_region_full(struct vgic_redist_region *region)
>>  
>>  struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
>>  
>> +static inline size_t
>> +vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
>> +{
>> +	if (!rdreg->count)
>> +		return atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE;
>> +	else
>> +		return rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
>> +}
>> +
>> +struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
>> +							   uint32_t index);
>> +
>> +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
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v3 06/12] KVM: arm/arm64: Helper to register a new redistributor region

[Christoffer Dall]

On Thu, Apr 26, 2018 at 09:32:49AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 04/24/2018 06:47 PM, Christoffer Dall wrote:
> > On Fri, Apr 13, 2018 at 10:20:52AM +0200, Eric Auger wrote:
> >> 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>
> >> ---
> >>  | 95 +++++++++++++++++++++++++++++++++-------
> >>  virt/kvm/arm/vgic/vgic-v3.c      | 29 ++++++++++++
> >>  virt/kvm/arm/vgic/vgic.h         | 14 ++++++
> >>  3 files changed, 122 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> index ce5c927..5273fb8 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> @@ -680,14 +680,66 @@ 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. count != 0). The list is sorted by rdist region index.
> >> + *
> >> + * @kvm: kvm handle
> >> + * @index: redist region index
> >> + * @base: base of the new rdist region
> >> + * @count: number of redistributors 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 count)
> >>  {
> >> -	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;
> >> +
> > 
> > nit: extra blank line?
> > 
> >> +	gpa_t new_start, new_end;
> >> +	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> >>  	int ret;
> >>  
> >> -	/* vgic_check_ioaddr makes sure we don't do this twice */
> >> -	if (!list_empty(&vgic->rd_regions))
> >> +	/* single rdist region already set ?*/
> >> +	if (!count && !list_empty(rd_regions))
> >> +		return -EINVAL;
> >> +
> >> +	/* cross the end of memory ? */
> >> +	if (base + size < base)
> >> +		return -EINVAL;
> > 
> > what is the size of memory?  This seems to check for a gpa_t overflow,
> > but not againt the IPA space of the VM...
> Yes it checks for a gpa_t overflow. This check is currently done in
> vgic_v3_check_base() for dist and redist region and I replicated it.

fair enough, the comment is a bit misleading though.  We could also
consider checking against KVM_PHYS_SHIFT.

> > 
> >> +
> >> +	if (list_empty(rd_regions)) {
> >> +		if (index != 0)
> >> +			return -EINVAL;
> > 
> > note, I think this can be simplified if we can rid of the index.
> So I eventually keep the index.

Yes.

> > 
> >> +	} else {
> >> +		rdreg = list_entry(last, struct vgic_redist_region, list);
> > 
> > you can use list_last_entry here and get rid of the 'last' temporary
> > variable above.
> definitively, thank you for the nit.
> > 
> >> +		if (index != rdreg->index + 1)
> >> +			return -EINVAL;
> >> +
> >> +		/* Cannot add an explicitly sized regions after legacy region */
> >> +		if (!rdreg->count)
> >> +			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
> >> +	 */
> > 
> > I don't really understand this commentary... :(
> I meant we cannot perform the check below if we are inserting a unique
> legacy rdist region (old API), whose size is not explicitly set but
> induced from the number of online vcpus.
> 

ok, given the complexity of the logic below, I think you should just
explain it:
	/*
	 * For legacy single-region redistributor regions (!count),
	 * check that the redistributor region does not overlap with the
	 * distributor's address space.
	 */

> > 
> >> +	if (!count && !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;
> > 
> > Can't you call vgic_v3_check_base() here instead?
> no I can't because vgic_v3_check_base() currently only works with the
> unique legacy rdist region. There, redist_size  is
> atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE.

Hmmm, ok.  I'm not completely clear if that can be reworked to be reused
or not, but perhaps you could just introduce a primitive ?

	static bool redist_overlaps_dist(struct kvm *kvm, gpa_t rd_base, size_t rd_size);

> > 
> >> +
> >> +	/* collision with any other rdist region? */
> >> +	if (vgic_v3_rdist_overlap(kvm, base, size))
> >>  		return -EINVAL;
> >>  
> >>  	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
> >> @@ -696,17 +748,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> >>  
> >>  	rdreg->base = VGIC_ADDR_UNDEF;
> >>  
> >> -	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->count = count;
> >> +	rdreg->free_index = 0;
> >> +	rdreg->index = index;
> >>  
> >> -	list_add(&rdreg->list, &vgic->rd_regions);
> >> +	new_start = base;
> >> +	new_end = base + size - 1;
> > 
> > What are these variables used for?
> Hum reminder from an old version :-(
> > 
> >> +
> >> +	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;
> >> +
> >> +	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
> >> +	if (ret)
> >> +		return ret;
> >>  
> >>  	/*
> >>  	 * Register iodevs for each existing VCPU.  Adding more VCPUs
> >> @@ -717,10 +784,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 820012a..dbcba5f 100644
> >> --- a/virt/kvm/arm/vgic/vgic-v3.c
> >> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> >> @@ -410,6 +410,21 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> >>  	return 0;
> >>  }
> >>  
> >> +/* return true if there is an overlap between any rdist */
> > 
> > Checks if base+size overlaps with any existing redistributor.
> > 
> >> +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;
> > 
> > can you invert the check above and return false instead of the continue?
> > 
> > (DeMorgan's law should be handy here.)
> sure
> > 
> >> +	}
> >> +	return false;
> >> +}
> >> +
> >>  /*
> >>   * Check for overlapping regions and for regions crossing the end of memory
> >>   * for base addresses which have already been set.
> >> @@ -461,6 +476,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;
> >> +	}
> > 
> > if this ends up being a common operation, we could allocate an array of
> > pointers for constant-time lookup instead.  Let's hope it's not too
> > common.
> This is only used when reading the characteristics of a redist region
> from userspace so I don't think we care.
> 

ok, fine.

Thanks,
-Christoffer

[PATCH v3 07/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 dbcba5f..b80f650 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -432,31 +432,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;
+	list_for_each_entry(rdreg, &d->rd_regions, list) {
+		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
+			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)
+	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
 		return true;
 
-	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= rdreg->base)
-		return true;
-
-	if (rdreg->base + redist_size <= 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

Re: [PATCH v3 07/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:53AM +0200, Eric Auger wrote:
> 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.

I don't understand this change, really.  Is this just a cleanup, or
changing some functionality (why?).

I think this could have come with the introduction of
vgic_v3_rdist_overlap() before patch 6, and then patch 6 could have been
simplified (hopefully) to just call this "check that nothing in the
world ever collides withi itself" function.

> 
> 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 dbcba5f..b80f650 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -432,31 +432,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;
> +	list_for_each_entry(rdreg, &d->rd_regions, list) {
> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> +			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)
> +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
>  		return true;
>  
> -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= rdreg->base)
> -		return true;
> -
> -	if (rdreg->base + redist_size <= 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
> 
Otherwise this patch looks correct to me.

Thanks,
-Christoffer

Re: [PATCH v3 07/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions

[Auger Eric]

Hi Christoffer,
On 04/24/2018 11:07 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:53AM +0200, Eric Auger wrote:
>> 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.
> 
> I don't understand this change, really.  Is this just a cleanup, or
> changing some functionality (why?).
> 
> I think this could have come with the introduction of
> vgic_v3_rdist_overlap() before patch 6, and then patch 6 could have been
> simplified (hopefully) to just call this "check that nothing in the
> world ever collides withi itself" function.
I have merged this patch and vgic_v3_rd_region_size +
vgic_v3_rdist_overlap and put it before this patch.

Also I reworked the commit message which was unclear I acknowledge.

With respect to using the adapted  vgic_v3_check_base() in
vgic_v3_insert_redist_region(), it is less obvious to me.

In vgic_v3_insert_redist_region we do the checks *before* inserting the
new rdist region in the list of redist regions. While
vgic_v3_check_base() does the checks on already registered rdist and
dist regions. So I would be tempted to leave
vgic_v3_insert_redist_region() implementation as it is.

Thanks

Eric
> 
>>
>> 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 dbcba5f..b80f650 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -432,31 +432,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;
>> +	list_for_each_entry(rdreg, &d->rd_regions, list) {
>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>> +			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)
>> +	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
>>  		return true;
>>  
>> -	if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= rdreg->base)
>> -		return true;
>> -
>> -	if (rdreg->base + redist_size <= 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
>>
> Otherwise this patch looks correct to me.
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v3 07/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions

[Christoffer Dall]

On Thu, Apr 26, 2018 at 10:29:35AM +0200, Auger Eric wrote:
> Hi Christoffer,
> On 04/24/2018 11:07 PM, Christoffer Dall wrote:
> > On Fri, Apr 13, 2018 at 10:20:53AM +0200, Eric Auger wrote:
> >> 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.
> > 
> > I don't understand this change, really.  Is this just a cleanup, or
> > changing some functionality (why?).
> > 
> > I think this could have come with the introduction of
> > vgic_v3_rdist_overlap() before patch 6, and then patch 6 could have been
> > simplified (hopefully) to just call this "check that nothing in the
> > world ever collides withi itself" function.
> I have merged this patch and vgic_v3_rd_region_size +
> vgic_v3_rdist_overlap and put it before this patch.
> 
> Also I reworked the commit message which was unclear I acknowledge.
> 
> With respect to using the adapted  vgic_v3_check_base() in
> vgic_v3_insert_redist_region(), it is less obvious to me.
> 
> In vgic_v3_insert_redist_region we do the checks *before* inserting the
> new rdist region in the list of redist regions. While
> vgic_v3_check_base() does the checks on already registered rdist and
> dist regions. So I would be tempted to leave
> vgic_v3_insert_redist_region() implementation as it is.
> 

ok, but do see my suggestion there to factor out the check, which should
make that function slightly easier to read.

Then perhaps you can use that function from vgic_v3_check_base to check
that each rdist doesn't overlap with the distributor?

Thanks,
-Christoffer

Re: [PATCH v3 07/12] KVM: arm/arm64: Adapt vgic_v3_check_base to multiple rdist regions

[Auger Eric]

Hi Christoffer,

On 04/26/2018 12:06 PM, Christoffer Dall wrote:
> On Thu, Apr 26, 2018 at 10:29:35AM +0200, Auger Eric wrote:
>> Hi Christoffer,
>> On 04/24/2018 11:07 PM, Christoffer Dall wrote:
>>> On Fri, Apr 13, 2018 at 10:20:53AM +0200, Eric Auger wrote:
>>>> 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.
>>>
>>> I don't understand this change, really.  Is this just a cleanup, or
>>> changing some functionality (why?).
>>>
>>> I think this could have come with the introduction of
>>> vgic_v3_rdist_overlap() before patch 6, and then patch 6 could have been
>>> simplified (hopefully) to just call this "check that nothing in the
>>> world ever collides withi itself" function.
>> I have merged this patch and vgic_v3_rd_region_size +
>> vgic_v3_rdist_overlap and put it before this patch.
>>
>> Also I reworked the commit message which was unclear I acknowledge.
>>
>> With respect to using the adapted  vgic_v3_check_base() in
>> vgic_v3_insert_redist_region(), it is less obvious to me.
>>
>> In vgic_v3_insert_redist_region we do the checks *before* inserting the
>> new rdist region in the list of redist regions. While
>> vgic_v3_check_base() does the checks on already registered rdist and
>> dist regions. So I would be tempted to leave
>> vgic_v3_insert_redist_region() implementation as it is.
>>
> 
> ok, but do see my suggestion there to factor out the check, which should
> make that function slightly easier to read.
> 
> Then perhaps you can use that function from vgic_v3_check_base to check
> that each rdist doesn't overlap with the distributor?

I introduced the suggested additional helper, vgic_dist_overlap, to
check a region does not overlap with the distributor region and used it
in vgic_v3_insert_redist_region.

However in  vgic_v3_check_base I do not need it as I already use
vgic_v3_rdist_overlap() which does the job, ie. check the dist against
all registered redists.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 

[PATCH v3 08/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 6456371..7e040e7 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 5273fb8..df23e66 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -592,6 +592,9 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
 	gpa_t rd_base, sgi_base;
 	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

Re: [PATCH v3 08/12] KVM: arm/arm64: Check vcpu redist base before registering an iodev

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:54AM +0200, Eric Auger wrote:
> 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. 

Wouldn't it be more natural to change that caller to only register the
iodevs for that region?

Thanks,
-Christoffer

> 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 6456371..7e040e7 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 5273fb8..df23e66 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -592,6 +592,9 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>  	gpa_t rd_base, sgi_base;
>  	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
> 

Re: [PATCH v3 08/12] KVM: arm/arm64: Check vcpu redist base before registering an iodev

[Auger Eric]

Hi Christoffer,

On 04/24/2018 11:07 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:54AM +0200, Eric Auger wrote:
>> 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. 
> 
> Wouldn't it be more natural to change that caller to only register the
> iodevs for that region?

vgic_register_redist_iodev() is the place where we decide where we map a
given vcpu redist into a given redist region.

Calling vgic_register_redist_iodev for only the vcpus mapping to the
redist region would force to inverse the logic. I think it would bring
more upheavals in the code than bringing benefit?

This new check somehow corresponds to what we had before:
"
if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
                return 0;
"

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
>> 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 6456371..7e040e7 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 5273fb8..df23e66 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -592,6 +592,9 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>  	gpa_t rd_base, sgi_base;
>>  	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
>>
> 

Re: [PATCH v3 08/12] KVM: arm/arm64: Check vcpu redist base before registering an iodev

[Christoffer Dall]

On Thu, Apr 26, 2018 at 11:25:06AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 04/24/2018 11:07 PM, Christoffer Dall wrote:
> > On Fri, Apr 13, 2018 at 10:20:54AM +0200, Eric Auger wrote:
> >> 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. 
> > 
> > Wouldn't it be more natural to change that caller to only register the
> > iodevs for that region?
> 
> vgic_register_redist_iodev() is the place where we decide where we map a
> given vcpu redist into a given redist region.
> 
> Calling vgic_register_redist_iodev for only the vcpus mapping to the
> redist region would force to inverse the logic. I think it would bring
> more upheavals in the code than bringing benefit?
> 
> This new check somehow corresponds to what we had before:
> "
> if (IS_VGIC_ADDR_UNDEF(vgic->vgic_redist_base))
>                 return 0;
> "

Ah, this is because we don't enforce any ordering between creating the
redistributors and initializing the vcpus, this always confuses me.

Fine, let's leave it as you suggest here.

Thanks,
-Christoffer

[PATCH v3 09/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 b80f650..feabc24 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -484,16 +484,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

Re: [PATCH v3 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:55AM +0200, Eric Auger wrote:
> On vcpu first run, we eventually know the actual number of vcpus.
> This is a synchronization point to check all redistributors regions
> were assigned.

Isn't it the other way around?  We want to check that all redistributors
(one for every VCPU) has its base address set?  (I don't suppose we care
about unused redistributor regions).

> On kvm_vgic_map_resources() we check both dist and
> redist were set, eventually check potential base address inconsistencies.

Do we need to check that again?  Didn't we check that at
creation/register time?

> 
> 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 b80f650..feabc24 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -484,16 +484,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);

Shouldn't this be a kvm_debug instead?  I think the user can just
provoke this by failing to register enough redistributor regions.

I'm also wondering if we could check this on vgic_init time for gicv3,
which should have a defined vgic init ordering requirement.  That would
make debugging it slightly easier to debug than "my machine isn't
starting, and I get -ENXIO, and it can mean anything".

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

Thanks,
-Christoffer

Re: [PATCH v3 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources

[Auger Eric]

On 04/24/2018 11:08 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:55AM +0200, Eric Auger wrote:
>> On vcpu first run, we eventually know the actual number of vcpus.
>> This is a synchronization point to check all redistributors regions
>> were assigned.
> 
> Isn't it the other way around?  We want to check that all redistributors
> (one for every VCPU) has its base address set?  (I don't suppose we care
> about unused redistributor regions).
Yes I meant "to check all redistributors were assigned"
> 
>> On kvm_vgic_map_resources() we check both dist and
>> redist were set, eventually check potential base address inconsistencies.
> 
> Do we need to check that again?  Didn't we check that at
> creation/register time?
Yes we need to, to handle the case where the userspace does not provide
sufficient rdist region space

create 8 vcpus (no iodev registered)
define a redist region for 4 (4 iodevs registered)
start the vcpus
> 
>>
>> 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 b80f650..feabc24 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -484,16 +484,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);
> 
> Shouldn't this be a kvm_debug instead?  I think the user can just
> provoke this by failing to register enough redistributor regions.
yes indeed.
> 
> I'm also wondering if we could check this on vgic_init time for gicv3,
> which should have a defined vgic init ordering requirement.  That would
> make debugging it slightly easier to debug than "my machine isn't
> starting, and I get -ENXIO, and it can mean anything".

Well at vgic_init time, vcpus are frozen but dist and redist regions are
not forced to be set, hence the late check we do in map_resources, on
1st VCPU run. Not sure I get this one?

Thanks

Eric
> 
>> +			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
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v3 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources

[Christoffer Dall]

On Thu, Apr 26, 2018 at 11:56:10AM +0200, Auger Eric wrote:
> 
> 
> On 04/24/2018 11:08 PM, Christoffer Dall wrote:
> > On Fri, Apr 13, 2018 at 10:20:55AM +0200, Eric Auger wrote:
> >> On vcpu first run, we eventually know the actual number of vcpus.
> >> This is a synchronization point to check all redistributors regions
> >> were assigned.
> > 
> > Isn't it the other way around?  We want to check that all redistributors
> > (one for every VCPU) has its base address set?  (I don't suppose we care
> > about unused redistributor regions).
> Yes I meant "to check all redistributors were assigned"
> > 
> >> On kvm_vgic_map_resources() we check both dist and
> >> redist were set, eventually check potential base address inconsistencies.
> > 
> > Do we need to check that again?  Didn't we check that at
> > creation/register time?
> Yes we need to, to handle the case where the userspace does not provide
> sufficient rdist region space
> 
> create 8 vcpus (no iodev registered)
> define a redist region for 4 (4 iodevs registered)
> start the vcpus
> > 
> >>
> >> 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 b80f650..feabc24 100644
> >> --- a/virt/kvm/arm/vgic/vgic-v3.c
> >> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> >> @@ -484,16 +484,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);
> > 
> > Shouldn't this be a kvm_debug instead?  I think the user can just
> > provoke this by failing to register enough redistributor regions.
> yes indeed.
> > 
> > I'm also wondering if we could check this on vgic_init time for gicv3,
> > which should have a defined vgic init ordering requirement.  That would
> > make debugging it slightly easier to debug than "my machine isn't
> > starting, and I get -ENXIO, and it can mean anything".
> 
> Well at vgic_init time, vcpus are frozen but dist and redist regions are
> not forced to be set, hence the late check we do in map_resources, on
> 1st VCPU run. Not sure I get this one?
> 

I thought we required userspace to register the redist regions before
initializing the vgic, but if there is no defined order there, then
fine.

(Although I'm getting the feeling we should have been more strict on our
ordering requirement, and that the whole "you can create anything you
want in any order you want" makes the kernel implementation really
complex.  I may be reversing my own oppinion on this matter here.)

Thanks,
-Christoffer

[PATCH v3 10/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 2ba95d6..11725bb 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

Re: [PATCH v3 10/12] KVM: arm/arm64: Add KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:56AM +0200, Eric Auger wrote:
> 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 2ba95d6..11725bb 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

I think I'd prefer that we just leave these lines above as it's only and
indentation thing, and this is an exported header so will propagate into
userspace updates, but I don't have a strong feeling about it, nor do I
know if there's a general policy.

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

Otherwise:

Acked-by: Christoffer Dall <christoffer.dall@arm.com>

[PATCH v3 11/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>

---

v2 -> v3:
- early exit if vgic_v3_rdist_region_from_index() fails
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 42 +++++++++++++++++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c    |  4 ++--
 virt/kvm/arm/vgic/vgic.h            |  9 +++++++-
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index e7b5a86..00e03d3 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -65,7 +65,8 @@ 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;
+	phys_addr_t *addr_ptr = NULL;
+	phys_addr_t alignment;
 	uint64_t undef_value = VGIC_ADDR_UNDEF;
 
 	mutex_lock(&kvm->lock);
@@ -92,7 +93,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 +104,42 @@ 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 count = (*addr & KVM_VGIC_V3_RDIST_COUNT_MASK)
+					>> KVM_VGIC_V3_RDIST_COUNT_SHIFT;
+			uint8_t flags = (*addr & KVM_VGIC_V3_RDIST_FLAGS_MASK)
+					>> KVM_VGIC_V3_RDIST_FLAGS_SHIFT;
+
+			if (!count || flags)
+				r = -EINVAL;
+			else
+				r = vgic_v3_set_redist_base(kvm, index,
+							    base, count);
+			goto out;
+		}
+
+		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
+		if (!rdreg) {
+			r = -ENODEV;
+			goto out;
+		}
+
+		*addr_ptr = rdreg->base & index &
+			(uint64_t)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;
+		break;
+	}
 	default:
 		r = -ENODEV;
 	}
@@ -674,6 +711,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 		switch (attr->attr) {
 		case KVM_VGIC_V3_ADDR_TYPE_DIST:
 		case KVM_VGIC_V3_ADDR_TYPE_REDIST:
+		case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION:
 			return 0;
 		}
 		break;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index df23e66..f603fdf 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -770,11 +770,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 count)
 {
 	int ret;
 
-	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
+	ret = vgic_v3_insert_redist_region(kvm, index, addr, count);
 	if (ret)
 		return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 95b8345..0a95b43 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -96,6 +96,13 @@
 /* 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(11, 0)
+#define KVM_VGIC_V3_RDIST_FLAGS_MASK	GENMASK_ULL(15, 12)
+#define KVM_VGIC_V3_RDIST_FLAGS_SHIFT	12
+#define KVM_VGIC_V3_RDIST_BASE_MASK	GENMASK_ULL(51, 16)
+#define KVM_VGIC_V3_RDIST_COUNT_MASK	GENMASK_ULL(63, 52)
+#define KVM_VGIC_V3_RDIST_COUNT_SHIFT	52
+
 /* Requires the irq_lock to be held by the caller. */
 static inline bool irq_is_pending(struct vgic_irq *irq)
 {
@@ -201,7 +208,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 count);
 int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
 bool vgic_v3_check_base(struct kvm *kvm);
 
-- 
2.5.5

Re: [PATCH v3 11/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:57AM +0200, Eric Auger wrote:
> 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>
> 
> ---
> 
> v2 -> v3:
> - early exit if vgic_v3_rdist_region_from_index() fails
> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 42 +++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    |  4 ++--
>  virt/kvm/arm/vgic/vgic.h            |  9 +++++++-
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index e7b5a86..00e03d3 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -65,7 +65,8 @@ 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;
> +	phys_addr_t *addr_ptr = NULL;
> +	phys_addr_t alignment;
>  	uint64_t undef_value = VGIC_ADDR_UNDEF;

nit: mussed this one before, type should be u64

>  
>  	mutex_lock(&kvm->lock);
> @@ -92,7 +93,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 +104,42 @@ 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;
> +

we tend to use u8, u32, etc. in the kernel.

> +		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 count = (*addr & KVM_VGIC_V3_RDIST_COUNT_MASK)
> +					>> KVM_VGIC_V3_RDIST_COUNT_SHIFT;
> +			uint8_t flags = (*addr & KVM_VGIC_V3_RDIST_FLAGS_MASK)
> +					>> KVM_VGIC_V3_RDIST_FLAGS_SHIFT;
> +
> +			if (!count || flags)
> +				r = -EINVAL;
> +			else
> +				r = vgic_v3_set_redist_base(kvm, index,
> +							    base, count);
> +			goto out;
> +		}
> +
> +		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
> +		if (!rdreg) {
> +			r = -ENODEV;
> +			goto out;
> +		}
> +
> +		*addr_ptr = rdreg->base & index &
> +			(uint64_t)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;

This looks fairly broken, isn't this a clear null pointer dereference?

(If we're making this ioctl read-only using the parameter as both in/out
for set/get, that should also be documented in the API text, then you
should consider writing a small test along with your userspace
implementation to actually test that functionality - otherwise we should
just make this write-only and omit the index part.  It could be said
that retrieving what the kernel actually has is a reasonable debug
feature.)

I think you want (notice the | instead of & as well):

		*addr = index;
		*addr |= rdreg->base;
		*addr |= (u64)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;
		goto out;

It is then debatable if the addr_ptr construct gets too convoluted when
not used in every case, and if the logic should be embedded into each
case, and the addr_ptr variable dropped.  Meh, I don't mind leaving it
for now.


> +		break;
> +	}
>  	default:
>  		r = -ENODEV;
>  	}
> @@ -674,6 +711,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  		switch (attr->attr) {
>  		case KVM_VGIC_V3_ADDR_TYPE_DIST:
>  		case KVM_VGIC_V3_ADDR_TYPE_REDIST:
> +		case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION:
>  			return 0;
>  		}
>  		break;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index df23e66..f603fdf 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -770,11 +770,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 count)
>  {
>  	int ret;
>  
> -	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
> +	ret = vgic_v3_insert_redist_region(kvm, index, addr, count);
>  	if (ret)
>  		return ret;
>  
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 95b8345..0a95b43 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -96,6 +96,13 @@
>  /* 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(11, 0)
> +#define KVM_VGIC_V3_RDIST_FLAGS_MASK	GENMASK_ULL(15, 12)
> +#define KVM_VGIC_V3_RDIST_FLAGS_SHIFT	12
> +#define KVM_VGIC_V3_RDIST_BASE_MASK	GENMASK_ULL(51, 16)
> +#define KVM_VGIC_V3_RDIST_COUNT_MASK	GENMASK_ULL(63, 52)
> +#define KVM_VGIC_V3_RDIST_COUNT_SHIFT	52
> +
>  /* Requires the irq_lock to be held by the caller. */
>  static inline bool irq_is_pending(struct vgic_irq *irq)
>  {
> @@ -201,7 +208,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 count);
>  int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
>  bool vgic_v3_check_base(struct kvm *kvm);
>  
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

Re: [PATCH v3 11/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Auger Eric]

Hi Christoffer,

On 04/24/2018 11:06 PM, Christoffer Dall wrote:
> On Fri, Apr 13, 2018 at 10:20:57AM +0200, Eric Auger wrote:
>> 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>
>>
>> ---
>>
>> v2 -> v3:
>> - early exit if vgic_v3_rdist_region_from_index() fails
>> ---
>>  virt/kvm/arm/vgic/vgic-kvm-device.c | 42 +++++++++++++++++++++++++++++++++++--
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c    |  4 ++--
>>  virt/kvm/arm/vgic/vgic.h            |  9 +++++++-
>>  3 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> index e7b5a86..00e03d3 100644
>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> @@ -65,7 +65,8 @@ 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;
>> +	phys_addr_t *addr_ptr = NULL;
>> +	phys_addr_t alignment;
>>  	uint64_t undef_value = VGIC_ADDR_UNDEF;
> 
> nit: mussed this one before, type should be u64
> 
>>  
>>  	mutex_lock(&kvm->lock);
>> @@ -92,7 +93,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 +104,42 @@ 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;
>> +
> 
> we tend to use u8, u32, etc. in the kernel.
> 
>> +		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 count = (*addr & KVM_VGIC_V3_RDIST_COUNT_MASK)
>> +					>> KVM_VGIC_V3_RDIST_COUNT_SHIFT;
>> +			uint8_t flags = (*addr & KVM_VGIC_V3_RDIST_FLAGS_MASK)
>> +					>> KVM_VGIC_V3_RDIST_FLAGS_SHIFT;
>> +
>> +			if (!count || flags)
>> +				r = -EINVAL;
>> +			else
>> +				r = vgic_v3_set_redist_base(kvm, index,
>> +							    base, count);
>> +			goto out;
>> +		}
>> +
>> +		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
>> +		if (!rdreg) {
>> +			r = -ENODEV;
>> +			goto out;
>> +		}
>> +
>> +		*addr_ptr = rdreg->base & index &
>> +			(uint64_t)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;
> 
> This looks fairly broken, isn't this a clear null pointer dereference?
> 
> (If we're making this ioctl read-only using the parameter as both in/out
> for set/get, that should also be documented in the API text, then you
> should consider writing a small test along with your userspace
> implementation to actually test that functionality - otherwise we should
> just make this write-only and omit the index part.  It could be said
> that retrieving what the kernel actually has is a reasonable debug
> feature.)
> 
> I think you want (notice the | instead of & as well):
> 
> 		*addr = index;
> 		*addr |= rdreg->base;
> 		*addr |= (u64)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;
> 		goto out;
> 
> It is then debatable if the addr_ptr construct gets too convoluted when
> not used in every case, and if the logic should be embedded into each
> case, and the addr_ptr variable dropped.  Meh, I don't mind leaving it
> for now.

Please apologize, I skipped this email while respinning into v4. Those
are definitively 2 bugs and I fixed them as you suggested above. As for
the documentation, I added:

"
      The characteristics of a specific redistributor region can be read
      by presetting the index field in the attr data.

  Errors:
../..
    -ENOENT: Attempt to read the characteristics of a non existing
             redistributor region
"

Currently testing the read path with a hacked qemu ;-)

Thanks

Eric
> 
> 
>> +		break;
>> +	}
>>  	default:
>>  		r = -ENODEV;
>>  	}
>> @@ -674,6 +711,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>>  		switch (attr->attr) {
>>  		case KVM_VGIC_V3_ADDR_TYPE_DIST:
>>  		case KVM_VGIC_V3_ADDR_TYPE_REDIST:
>> +		case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION:
>>  			return 0;
>>  		}
>>  		break;
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index df23e66..f603fdf 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -770,11 +770,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 count)
>>  {
>>  	int ret;
>>  
>> -	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
>> +	ret = vgic_v3_insert_redist_region(kvm, index, addr, count);
>>  	if (ret)
>>  		return ret;
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index 95b8345..0a95b43 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -96,6 +96,13 @@
>>  /* 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(11, 0)
>> +#define KVM_VGIC_V3_RDIST_FLAGS_MASK	GENMASK_ULL(15, 12)
>> +#define KVM_VGIC_V3_RDIST_FLAGS_SHIFT	12
>> +#define KVM_VGIC_V3_RDIST_BASE_MASK	GENMASK_ULL(51, 16)
>> +#define KVM_VGIC_V3_RDIST_COUNT_MASK	GENMASK_ULL(63, 52)
>> +#define KVM_VGIC_V3_RDIST_COUNT_SHIFT	52
>> +
>>  /* Requires the irq_lock to be held by the caller. */
>>  static inline bool irq_is_pending(struct vgic_irq *irq)
>>  {
>> @@ -201,7 +208,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 count);
>>  int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
>>  bool vgic_v3_check_base(struct kvm *kvm);
>>  
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v3 11/12] KVM: arm/arm64: Implement KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

[Christoffer Dall]

On Fri, Apr 13, 2018 at 10:20:57AM +0200, Eric Auger wrote:
> 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>
> 
> ---
> 
> v2 -> v3:
> - early exit if vgic_v3_rdist_region_from_index() fails
> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 42 +++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    |  4 ++--
>  virt/kvm/arm/vgic/vgic.h            |  9 +++++++-
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index e7b5a86..00e03d3 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -65,7 +65,8 @@ 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;
> +	phys_addr_t *addr_ptr = NULL;
> +	phys_addr_t alignment;
>  	uint64_t undef_value = VGIC_ADDR_UNDEF;
>  
>  	mutex_lock(&kvm->lock);
> @@ -92,7 +93,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 +104,42 @@ 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 count = (*addr & KVM_VGIC_V3_RDIST_COUNT_MASK)
> +					>> KVM_VGIC_V3_RDIST_COUNT_SHIFT;
> +			uint8_t flags = (*addr & KVM_VGIC_V3_RDIST_FLAGS_MASK)
> +					>> KVM_VGIC_V3_RDIST_FLAGS_SHIFT;
> +
> +			if (!count || flags)
> +				r = -EINVAL;
> +			else
> +				r = vgic_v3_set_redist_base(kvm, index,
> +							    base, count);
> +			goto out;
> +		}
> +
> +		rdreg = vgic_v3_rdist_region_from_index(kvm, index);
> +		if (!rdreg) {
> +			r = -ENODEV;
> +			goto out;
> +		}
> +
> +		*addr_ptr = rdreg->base & index &
> +			(uint64_t)rdreg->count << KVM_VGIC_V3_RDIST_COUNT_SHIFT;

I still think this is a clear NULL-pointer dereference.  It's also
wrong, as you use & where you want to use |.

You should also change the types you use above.

Could you please have a look at my last reply to this patch (I'm happy
to re-send if it got lost somehow) where I suggest how you can handle
this?

Thanks,
-Christoffer

> +		break;
> +	}
>  	default:
>  		r = -ENODEV;
>  	}
> @@ -674,6 +711,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  		switch (attr->attr) {
>  		case KVM_VGIC_V3_ADDR_TYPE_DIST:
>  		case KVM_VGIC_V3_ADDR_TYPE_REDIST:
> +		case KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION:
>  			return 0;
>  		}
>  		break;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index df23e66..f603fdf 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -770,11 +770,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 count)
>  {
>  	int ret;
>  
> -	ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
> +	ret = vgic_v3_insert_redist_region(kvm, index, addr, count);
>  	if (ret)
>  		return ret;
>  
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 95b8345..0a95b43 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -96,6 +96,13 @@
>  /* 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(11, 0)
> +#define KVM_VGIC_V3_RDIST_FLAGS_MASK	GENMASK_ULL(15, 12)
> +#define KVM_VGIC_V3_RDIST_FLAGS_SHIFT	12
> +#define KVM_VGIC_V3_RDIST_BASE_MASK	GENMASK_ULL(51, 16)
> +#define KVM_VGIC_V3_RDIST_COUNT_MASK	GENMASK_ULL(63, 52)
> +#define KVM_VGIC_V3_RDIST_COUNT_SHIFT	52
> +
>  /* Requires the irq_lock to be held by the caller. */
>  static inline bool irq_is_pending(struct vgic_irq *irq)
>  {
> @@ -201,7 +208,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 count);
>  int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
>  bool vgic_v3_check_base(struct kvm *kvm);
>  
> -- 
> 2.5.5
> 

[PATCH v3 12/12] KVM: arm/arm64: Bump VGIC_V3_MAX_CPUS to 512

[Eric Auger]

Let's raise the number of supported vcpus along with
vgic v3 now that HW is looming with more physical CPUs.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/kvm/arm_vgic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index e5c16d1..a9a9f4a 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -28,7 +28,7 @@
 
 #include <linux/irqchip/arm-gic-v4.h>
 
-#define VGIC_V3_MAX_CPUS	255
+#define VGIC_V3_MAX_CPUS	512
 #define VGIC_V2_MAX_CPUS	8
 #define VGIC_NR_IRQS_LEGACY     256
 #define VGIC_NR_SGIS		16
-- 
2.5.5