[PATCH v2 0/5] kvm: selftests: aarch64: some fixes for vgic_irq

[PATCH v2 0/5] kvm: selftests: aarch64: some fixes for vgic_irq

[Ricardo Koller]

Reiji discovered multiple issues with the vgic_irq series [0]:
1. there's an assert that needs fixing.
2. some guest arguments are not set correctly.
3. the failure test in kvm_set_gsi_routing_irqchip_check is wrong.
4. there are lots of comments that use the wrong formatting.
5. vgic_poke_irq() could use a tighter assert check.

The first 3 issues above are critical, the last 2 would be nice to have.  I
haven't hit the failed assert (1.), but just by chance: my compiler is
initializing the respective local variable to 0. The second issue (2.) leads to
not testing one of the injection methods (irqfd). The third issue could be hit
if we tested more intids.

v1 -> v2:
- adding 3 more fixes: 2, 3, 5 above. (Reiji)
- corrected the comments in 4 above. (Andrew)
- dded drjones@ reviewed-by tag.

[0] https://lore.kernel.org/kvmarm/164072141023.1027791.3183483860602648119.b4-ty@kernel.org/

Ricardo Koller (5):
  kvm: selftests: aarch64: fix assert in gicv3_access_reg
  kvm: selftests: aarch64: pass vgic_irq guest args as a pointer
  kvm: selftests: aarch64: fix the failure check in
    kvm_set_gsi_routing_irqchip_check
  kvm: selftests: aarch64: fix some vgic related comments
  kvm: selftests: aarch64: use a tighter assert in vgic_poke_irq()

 .../testing/selftests/kvm/aarch64/vgic_irq.c  | 45 +++++++++++--------
 .../selftests/kvm/lib/aarch64/gic_v3.c        | 12 ++---
 .../testing/selftests/kvm/lib/aarch64/vgic.c  |  9 ++--
 3 files changed, 38 insertions(+), 28 deletions(-)

-- 
2.35.0.rc0.227.g00780c9af4-goog

[PATCH v2 1/5] kvm: selftests: aarch64: fix assert in gicv3_access_reg

[Ricardo Koller]

The val argument in gicv3_access_reg can have any value when used for a
read, not necessarily 0.  Fix the assert by checking val only for
writes.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reported-by: Reiji Watanabe <reijiw@google.com>
Cc: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
index 00f613c0583c..e4945fe66620 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
@@ -159,7 +159,7 @@ static void gicv3_access_reg(uint32_t intid, uint64_t offset,
 	uint32_t cpu_or_dist;
 
 	GUEST_ASSERT(bits_per_field <= reg_bits);
-	GUEST_ASSERT(*val < (1U << bits_per_field));
+	GUEST_ASSERT(!write || *val < (1U << bits_per_field));
 	/* Some registers like IROUTER are 64 bit long. Those are currently not
 	 * supported by readl nor writel, so just asserting here until then.
 	 */
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v2 1/5] kvm: selftests: aarch64: fix assert in gicv3_access_reg

[Andrew Jones]

On Wed, Jan 26, 2022 at 07:08:54PM -0800, Ricardo Koller wrote:
> The val argument in gicv3_access_reg can have any value when used for a
> read, not necessarily 0.  Fix the assert by checking val only for
> writes.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Cc: Andrew Jones <drjones@redhat.com>

Commit message said my r-b would be here, but it doesn't appear to be.
Here it is again

Reviewed-by: Andrew Jones <drjones@redhat.com>

> ---
>  tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> index 00f613c0583c..e4945fe66620 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> @@ -159,7 +159,7 @@ static void gicv3_access_reg(uint32_t intid, uint64_t offset,
>  	uint32_t cpu_or_dist;
>  
>  	GUEST_ASSERT(bits_per_field <= reg_bits);
> -	GUEST_ASSERT(*val < (1U << bits_per_field));
> +	GUEST_ASSERT(!write || *val < (1U << bits_per_field));
>  	/* Some registers like IROUTER are 64 bit long. Those are currently not
>  	 * supported by readl nor writel, so just asserting here until then.
>  	 */
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
> 

[PATCH v2 2/5] kvm: selftests: aarch64: pass vgic_irq guest args as a pointer

[Ricardo Koller]

The guest in vgic_irq gets its arguments in a struct. This struct used
to fit nicely in a single register so vcpu_args_set() was able to pass
it by value by setting x0 with it. Unfortunately, this args struct grew
after some commits and some guest args became random (specically
kvm_supports_irqfd).

Fix this by passing the guest args as a pointer (after allocating some
guest memory for it).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reported-by: Reiji Watanabe <reijiw@google.com>
Cc: Andrew Jones <drjones@redhat.com>
---
 .../testing/selftests/kvm/aarch64/vgic_irq.c  | 29 ++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index e6c7d7f8fbd1..b701eb80128d 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -472,10 +472,10 @@ static void test_restore_active(struct test_args *args, struct kvm_inject_desc *
 		guest_restore_active(args, MIN_SPI, 4, f->cmd);
 }
 
-static void guest_code(struct test_args args)
+static void guest_code(struct test_args *args)
 {
-	uint32_t i, nr_irqs = args.nr_irqs;
-	bool level_sensitive = args.level_sensitive;
+	uint32_t i, nr_irqs = args->nr_irqs;
+	bool level_sensitive = args->level_sensitive;
 	struct kvm_inject_desc *f, *inject_fns;
 
 	gic_init(GIC_V3, 1, dist, redist);
@@ -484,11 +484,11 @@ static void guest_code(struct test_args args)
 		gic_irq_enable(i);
 
 	for (i = MIN_SPI; i < nr_irqs; i++)
-		gic_irq_set_config(i, !args.level_sensitive);
+		gic_irq_set_config(i, !level_sensitive);
 
-	gic_set_eoi_split(args.eoi_split);
+	gic_set_eoi_split(args->eoi_split);
 
-	reset_priorities(&args);
+	reset_priorities(args);
 	gic_set_priority_mask(CPU_PRIO_MASK);
 
 	inject_fns  = level_sensitive ? inject_level_fns
@@ -497,17 +497,17 @@ static void guest_code(struct test_args args)
 	local_irq_enable();
 
 	/* Start the tests. */
-	for_each_supported_inject_fn(&args, inject_fns, f) {
-		test_injection(&args, f);
-		test_preemption(&args, f);
-		test_injection_failure(&args, f);
+	for_each_supported_inject_fn(args, inject_fns, f) {
+		test_injection(args, f);
+		test_preemption(args, f);
+		test_injection_failure(args, f);
 	}
 
 	/* Restore the active state of IRQs. This would happen when live
 	 * migrating IRQs in the middle of being handled.
 	 */
-	for_each_supported_activate_fn(&args, set_active_fns, f)
-		test_restore_active(&args, f);
+	for_each_supported_activate_fn(args, set_active_fns, f)
+		test_restore_active(args, f);
 
 	GUEST_DONE();
 }
@@ -739,6 +739,7 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split)
 	int gic_fd;
 	struct kvm_vm *vm;
 	struct kvm_inject_args inject_args;
+	vm_vaddr_t args_gva;
 
 	struct test_args args = {
 		.nr_irqs = nr_irqs,
@@ -757,7 +758,9 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split)
 	vcpu_init_descriptor_tables(vm, VCPU_ID);
 
 	/* Setup the guest args page (so it gets the args). */
-	vcpu_args_set(vm, 0, 1, args);
+	args_gva = vm_vaddr_alloc_page(vm);
+	memcpy(addr_gva2hva(vm, args_gva), &args, sizeof(args));
+	vcpu_args_set(vm, 0, 1, args_gva);
 
 	gic_fd = vgic_v3_setup(vm, 1, nr_irqs,
 			GICD_BASE_GPA, GICR_BASE_GPA);
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v2 2/5] kvm: selftests: aarch64: pass vgic_irq guest args as a pointer

[Andrew Jones]

On Wed, Jan 26, 2022 at 07:08:55PM -0800, Ricardo Koller wrote:
> The guest in vgic_irq gets its arguments in a struct. This struct used
> to fit nicely in a single register so vcpu_args_set() was able to pass
> it by value by setting x0 with it.

Ouch.

> Unfortunately, this args struct grew
> after some commits and some guest args became random (specically
> kvm_supports_irqfd).
> 
> Fix this by passing the guest args as a pointer (after allocating some
> guest memory for it).
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Cc: Andrew Jones <drjones@redhat.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_irq.c  | 29 ++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index e6c7d7f8fbd1..b701eb80128d 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -472,10 +472,10 @@ static void test_restore_active(struct test_args *args, struct kvm_inject_desc *
>  		guest_restore_active(args, MIN_SPI, 4, f->cmd);
>  }
>  
> -static void guest_code(struct test_args args)
> +static void guest_code(struct test_args *args)
>  {
> -	uint32_t i, nr_irqs = args.nr_irqs;
> -	bool level_sensitive = args.level_sensitive;
> +	uint32_t i, nr_irqs = args->nr_irqs;
> +	bool level_sensitive = args->level_sensitive;
>  	struct kvm_inject_desc *f, *inject_fns;
>  
>  	gic_init(GIC_V3, 1, dist, redist);
> @@ -484,11 +484,11 @@ static void guest_code(struct test_args args)
>  		gic_irq_enable(i);
>  
>  	for (i = MIN_SPI; i < nr_irqs; i++)
> -		gic_irq_set_config(i, !args.level_sensitive);
> +		gic_irq_set_config(i, !level_sensitive);
>  
> -	gic_set_eoi_split(args.eoi_split);
> +	gic_set_eoi_split(args->eoi_split);
>  
> -	reset_priorities(&args);
> +	reset_priorities(args);
>  	gic_set_priority_mask(CPU_PRIO_MASK);
>  
>  	inject_fns  = level_sensitive ? inject_level_fns
> @@ -497,17 +497,17 @@ static void guest_code(struct test_args args)
>  	local_irq_enable();
>  
>  	/* Start the tests. */
> -	for_each_supported_inject_fn(&args, inject_fns, f) {
> -		test_injection(&args, f);
> -		test_preemption(&args, f);
> -		test_injection_failure(&args, f);
> +	for_each_supported_inject_fn(args, inject_fns, f) {
> +		test_injection(args, f);
> +		test_preemption(args, f);
> +		test_injection_failure(args, f);
>  	}
>  
>  	/* Restore the active state of IRQs. This would happen when live
>  	 * migrating IRQs in the middle of being handled.
>  	 */
> -	for_each_supported_activate_fn(&args, set_active_fns, f)
> -		test_restore_active(&args, f);
> +	for_each_supported_activate_fn(args, set_active_fns, f)
> +		test_restore_active(args, f);
>  
>  	GUEST_DONE();
>  }
> @@ -739,6 +739,7 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split)
>  	int gic_fd;
>  	struct kvm_vm *vm;
>  	struct kvm_inject_args inject_args;
> +	vm_vaddr_t args_gva;
>  
>  	struct test_args args = {
>  		.nr_irqs = nr_irqs,
> @@ -757,7 +758,9 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split)
>  	vcpu_init_descriptor_tables(vm, VCPU_ID);
>  
>  	/* Setup the guest args page (so it gets the args). */
> -	vcpu_args_set(vm, 0, 1, args);
> +	args_gva = vm_vaddr_alloc_page(vm);
> +	memcpy(addr_gva2hva(vm, args_gva), &args, sizeof(args));
> +	vcpu_args_set(vm, 0, 1, args_gva);
>  
>  	gic_fd = vgic_v3_setup(vm, 1, nr_irqs,
>  			GICD_BASE_GPA, GICR_BASE_GPA);
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v2 3/5] kvm: selftests: aarch64: fix the failure check in kvm_set_gsi_routing_irqchip_check

[Ricardo Koller]

kvm_set_gsi_routing_irqchip_check(expect_failure=true) is used to check
the error code returned by the kernel when trying to setup an invalid
gsi routing table. The ioctl fails if "pin >= KVM_IRQCHIP_NUM_PINS", so
kvm_set_gsi_routing_irqchip_check() should test the error only when
"intid >= KVM_IRQCHIP_NUM_PINS+32". The issue is that the test check is
"intid >= KVM_IRQCHIP_NUM_PINS", so for a case like "intid =
KVM_IRQCHIP_NUM_PINS" the test wrongly assumes that the kernel will
return an error.  Fix this by using the right check.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reported-by: Reiji Watanabe <reijiw@google.com>
Cc: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index b701eb80128d..0106fc464afe 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -573,8 +573,8 @@ static void kvm_set_gsi_routing_irqchip_check(struct kvm_vm *vm,
 		kvm_gsi_routing_write(vm, routing);
 	} else {
 		ret = _kvm_gsi_routing_write(vm, routing);
-		/* The kernel only checks for KVM_IRQCHIP_NUM_PINS. */
-		if (intid >= KVM_IRQCHIP_NUM_PINS)
+		/* The kernel only checks e->irqchip.pin >= KVM_IRQCHIP_NUM_PINS */
+		if (((uint64_t)intid + num - 1 - MIN_SPI) >= KVM_IRQCHIP_NUM_PINS)
 			TEST_ASSERT(ret != 0 && errno == EINVAL,
 				"Bad intid %u did not cause KVM_SET_GSI_ROUTING "
 				"error: rc: %i errno: %i", intid, ret, errno);
-- 
2.35.0.rc0.227.g00780c9af4-goog

[PATCH v2 4/5] kvm: selftests: aarch64: fix some vgic related comments

[Ricardo Koller]

Fix the formatting of some comments and the wording of one of them (in
gicv3_access_reg).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reported-by: Reiji Watanabe <reijiw@google.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/aarch64/vgic_irq.c   | 12 ++++++++----
 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 10 ++++++----
 tools/testing/selftests/kvm/lib/aarch64/vgic.c   |  3 ++-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index 0106fc464afe..f0230711fbe9 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -306,7 +306,8 @@ static void guest_restore_active(struct test_args *args,
 	uint32_t prio, intid, ap1r;
 	int i;
 
-	/* Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
+	/*
+	 * Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
 	 * in descending order, so intid+1 can preempt intid.
 	 */
 	for (i = 0, prio = (num - 1) * 8; i < num; i++, prio -= 8) {
@@ -315,7 +316,8 @@ static void guest_restore_active(struct test_args *args,
 		gic_set_priority(intid, prio);
 	}
 
-	/* In a real migration, KVM would restore all GIC state before running
+	/*
+	 * In a real migration, KVM would restore all GIC state before running
 	 * guest code.
 	 */
 	for (i = 0; i < num; i++) {
@@ -503,7 +505,8 @@ static void guest_code(struct test_args *args)
 		test_injection_failure(args, f);
 	}
 
-	/* Restore the active state of IRQs. This would happen when live
+	/*
+	 * Restore the active state of IRQs. This would happen when live
 	 * migrating IRQs in the middle of being handled.
 	 */
 	for_each_supported_activate_fn(args, set_active_fns, f)
@@ -840,7 +843,8 @@ int main(int argc, char **argv)
 		}
 	}
 
-	/* If the user just specified nr_irqs and/or gic_version, then run all
+	/*
+	 * If the user just specified nr_irqs and/or gic_version, then run all
 	 * combinations.
 	 */
 	if (default_args) {
diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
index e4945fe66620..263bf3ed8fd5 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
@@ -19,7 +19,7 @@ struct gicv3_data {
 	unsigned int nr_spis;
 };
 
-#define sgi_base_from_redist(redist_base) 	(redist_base + SZ_64K)
+#define sgi_base_from_redist(redist_base)	(redist_base + SZ_64K)
 #define DIST_BIT				(1U << 31)
 
 enum gicv3_intid_range {
@@ -105,7 +105,8 @@ static void gicv3_set_eoi_split(bool split)
 {
 	uint32_t val;
 
-	/* All other fields are read-only, so no need to read CTLR first. In
+	/*
+	 * All other fields are read-only, so no need to read CTLR first. In
 	 * fact, the kernel does the same.
 	 */
 	val = split ? (1U << 1) : 0;
@@ -160,8 +161,9 @@ static void gicv3_access_reg(uint32_t intid, uint64_t offset,
 
 	GUEST_ASSERT(bits_per_field <= reg_bits);
 	GUEST_ASSERT(!write || *val < (1U << bits_per_field));
-	/* Some registers like IROUTER are 64 bit long. Those are currently not
-	 * supported by readl nor writel, so just asserting here until then.
+	/*
+	 * This function does not support 64 bit accesses. Just asserting here
+	 * until we implement readq/writeq.
 	 */
 	GUEST_ASSERT(reg_bits == 32);
 
diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
index b3a0fca0d780..79864b941617 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
@@ -150,7 +150,8 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
 		attr += SZ_64K;
 	}
 
-	/* All calls will succeed, even with invalid intid's, as long as the
+	/*
+	 * All calls will succeed, even with invalid intid's, as long as the
 	 * addr part of the attr is within 32 bits (checked above). An invalid
 	 * intid will just make the read/writes point to above the intended
 	 * register space (i.e., ICPENDR after ISPENDR).
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v2 4/5] kvm: selftests: aarch64: fix some vgic related comments

[Andrew Jones]

On Wed, Jan 26, 2022 at 07:08:57PM -0800, Ricardo Koller wrote:
> Fix the formatting of some comments and the wording of one of them (in
> gicv3_access_reg).
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>

I didn't give my r-b to this patch before, but you can keep it, because
here's another one

Reviewed-by: Andrew Jones <drjones@redhat.com>

> ---
>  tools/testing/selftests/kvm/aarch64/vgic_irq.c   | 12 ++++++++----
>  tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 10 ++++++----
>  tools/testing/selftests/kvm/lib/aarch64/vgic.c   |  3 ++-
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index 0106fc464afe..f0230711fbe9 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -306,7 +306,8 @@ static void guest_restore_active(struct test_args *args,
>  	uint32_t prio, intid, ap1r;
>  	int i;
>  
> -	/* Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
> +	/*
> +	 * Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
>  	 * in descending order, so intid+1 can preempt intid.
>  	 */
>  	for (i = 0, prio = (num - 1) * 8; i < num; i++, prio -= 8) {
> @@ -315,7 +316,8 @@ static void guest_restore_active(struct test_args *args,
>  		gic_set_priority(intid, prio);
>  	}
>  
> -	/* In a real migration, KVM would restore all GIC state before running
> +	/*
> +	 * In a real migration, KVM would restore all GIC state before running
>  	 * guest code.
>  	 */
>  	for (i = 0; i < num; i++) {
> @@ -503,7 +505,8 @@ static void guest_code(struct test_args *args)
>  		test_injection_failure(args, f);
>  	}
>  
> -	/* Restore the active state of IRQs. This would happen when live
> +	/*
> +	 * Restore the active state of IRQs. This would happen when live
>  	 * migrating IRQs in the middle of being handled.
>  	 */
>  	for_each_supported_activate_fn(args, set_active_fns, f)
> @@ -840,7 +843,8 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	/* If the user just specified nr_irqs and/or gic_version, then run all
> +	/*
> +	 * If the user just specified nr_irqs and/or gic_version, then run all
>  	 * combinations.
>  	 */
>  	if (default_args) {
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> index e4945fe66620..263bf3ed8fd5 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> @@ -19,7 +19,7 @@ struct gicv3_data {
>  	unsigned int nr_spis;
>  };
>  
> -#define sgi_base_from_redist(redist_base) 	(redist_base + SZ_64K)
> +#define sgi_base_from_redist(redist_base)	(redist_base + SZ_64K)
>  #define DIST_BIT				(1U << 31)
>  
>  enum gicv3_intid_range {
> @@ -105,7 +105,8 @@ static void gicv3_set_eoi_split(bool split)
>  {
>  	uint32_t val;
>  
> -	/* All other fields are read-only, so no need to read CTLR first. In
> +	/*
> +	 * All other fields are read-only, so no need to read CTLR first. In
>  	 * fact, the kernel does the same.
>  	 */
>  	val = split ? (1U << 1) : 0;
> @@ -160,8 +161,9 @@ static void gicv3_access_reg(uint32_t intid, uint64_t offset,
>  
>  	GUEST_ASSERT(bits_per_field <= reg_bits);
>  	GUEST_ASSERT(!write || *val < (1U << bits_per_field));
> -	/* Some registers like IROUTER are 64 bit long. Those are currently not
> -	 * supported by readl nor writel, so just asserting here until then.
> +	/*
> +	 * This function does not support 64 bit accesses. Just asserting here
> +	 * until we implement readq/writeq.
>  	 */
>  	GUEST_ASSERT(reg_bits == 32);
>  
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> index b3a0fca0d780..79864b941617 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -150,7 +150,8 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
>  		attr += SZ_64K;
>  	}
>  
> -	/* All calls will succeed, even with invalid intid's, as long as the
> +	/*
> +	 * All calls will succeed, even with invalid intid's, as long as the
>  	 * addr part of the attr is within 32 bits (checked above). An invalid
>  	 * intid will just make the read/writes point to above the intended
>  	 * register space (i.e., ICPENDR after ISPENDR).
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
> 

Re: [PATCH v2 4/5] kvm: selftests: aarch64: fix some vgic related comments

[Ricardo Koller]

On Wed, Jan 26, 2022 at 11:49 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Jan 26, 2022 at 07:08:57PM -0800, Ricardo Koller wrote:
> > Fix the formatting of some comments and the wording of one of them (in
> > gicv3_access_reg).
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > Reported-by: Reiji Watanabe <reijiw@google.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
>
> I didn't give my r-b to this patch before, but you can keep it, because
> here's another one
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>

Thanks Andrew. Sorry, it was supposed to go to the assert one.

> > ---
> >  tools/testing/selftests/kvm/aarch64/vgic_irq.c   | 12 ++++++++----
> >  tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 10 ++++++----
> >  tools/testing/selftests/kvm/lib/aarch64/vgic.c   |  3 ++-
> >  3 files changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> > index 0106fc464afe..f0230711fbe9 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> > @@ -306,7 +306,8 @@ static void guest_restore_active(struct test_args *args,
> >       uint32_t prio, intid, ap1r;
> >       int i;
> >
> > -     /* Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
> > +     /*
> > +      * Set the priorities of the first (KVM_NUM_PRIOS - 1) IRQs
> >        * in descending order, so intid+1 can preempt intid.
> >        */
> >       for (i = 0, prio = (num - 1) * 8; i < num; i++, prio -= 8) {
> > @@ -315,7 +316,8 @@ static void guest_restore_active(struct test_args *args,
> >               gic_set_priority(intid, prio);
> >       }
> >
> > -     /* In a real migration, KVM would restore all GIC state before running
> > +     /*
> > +      * In a real migration, KVM would restore all GIC state before running
> >        * guest code.
> >        */
> >       for (i = 0; i < num; i++) {
> > @@ -503,7 +505,8 @@ static void guest_code(struct test_args *args)
> >               test_injection_failure(args, f);
> >       }
> >
> > -     /* Restore the active state of IRQs. This would happen when live
> > +     /*
> > +      * Restore the active state of IRQs. This would happen when live
> >        * migrating IRQs in the middle of being handled.
> >        */
> >       for_each_supported_activate_fn(args, set_active_fns, f)
> > @@ -840,7 +843,8 @@ int main(int argc, char **argv)
> >               }
> >       }
> >
> > -     /* If the user just specified nr_irqs and/or gic_version, then run all
> > +     /*
> > +      * If the user just specified nr_irqs and/or gic_version, then run all
> >        * combinations.
> >        */
> >       if (default_args) {
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > index e4945fe66620..263bf3ed8fd5 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> > @@ -19,7 +19,7 @@ struct gicv3_data {
> >       unsigned int nr_spis;
> >  };
> >
> > -#define sgi_base_from_redist(redist_base)    (redist_base + SZ_64K)
> > +#define sgi_base_from_redist(redist_base)    (redist_base + SZ_64K)
> >  #define DIST_BIT                             (1U << 31)
> >
> >  enum gicv3_intid_range {
> > @@ -105,7 +105,8 @@ static void gicv3_set_eoi_split(bool split)
> >  {
> >       uint32_t val;
> >
> > -     /* All other fields are read-only, so no need to read CTLR first. In
> > +     /*
> > +      * All other fields are read-only, so no need to read CTLR first. In
> >        * fact, the kernel does the same.
> >        */
> >       val = split ? (1U << 1) : 0;
> > @@ -160,8 +161,9 @@ static void gicv3_access_reg(uint32_t intid, uint64_t offset,
> >
> >       GUEST_ASSERT(bits_per_field <= reg_bits);
> >       GUEST_ASSERT(!write || *val < (1U << bits_per_field));
> > -     /* Some registers like IROUTER are 64 bit long. Those are currently not
> > -      * supported by readl nor writel, so just asserting here until then.
> > +     /*
> > +      * This function does not support 64 bit accesses. Just asserting here
> > +      * until we implement readq/writeq.
> >        */
> >       GUEST_ASSERT(reg_bits == 32);
> >
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > index b3a0fca0d780..79864b941617 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > @@ -150,7 +150,8 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
> >               attr += SZ_64K;
> >       }
> >
> > -     /* All calls will succeed, even with invalid intid's, as long as the
> > +     /*
> > +      * All calls will succeed, even with invalid intid's, as long as the
> >        * addr part of the attr is within 32 bits (checked above). An invalid
> >        * intid will just make the read/writes point to above the intended
> >        * register space (i.e., ICPENDR after ISPENDR).
> > --
> > 2.35.0.rc0.227.g00780c9af4-goog
> >
>

[PATCH v2 5/5] kvm: selftests: aarch64: use a tighter assert in vgic_poke_irq()

[Ricardo Koller]

vgic_poke_irq() checks that the attr argument passed to the vgic device
ioctl is sane. Make this check tighter by moving it to after the last
attr update.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reported-by: Reiji Watanabe <reijiw@google.com>
Cc: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/lib/aarch64/vgic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
index 79864b941617..f365c32a7296 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
@@ -138,9 +138,6 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
 	uint64_t val;
 	bool intid_is_private = INTID_IS_SGI(intid) || INTID_IS_PPI(intid);
 
-	/* Check that the addr part of the attr is within 32 bits. */
-	assert(attr <= KVM_DEV_ARM_VGIC_OFFSET_MASK);
-
 	uint32_t group = intid_is_private ? KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
 					  : KVM_DEV_ARM_VGIC_GRP_DIST_REGS;
 
@@ -150,6 +147,9 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
 		attr += SZ_64K;
 	}
 
+	/* Check that the addr part of the attr is within 32 bits. */
+	assert((attr & ~KVM_DEV_ARM_VGIC_OFFSET_MASK) == 0);
+
 	/*
 	 * All calls will succeed, even with invalid intid's, as long as the
 	 * addr part of the attr is within 32 bits (checked above). An invalid
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH v2 5/5] kvm: selftests: aarch64: use a tighter assert in vgic_poke_irq()

[Andrew Jones]

On Wed, Jan 26, 2022 at 07:08:58PM -0800, Ricardo Koller wrote:
> vgic_poke_irq() checks that the attr argument passed to the vgic device
> ioctl is sane. Make this check tighter by moving it to after the last
> attr update.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Cc: Andrew Jones <drjones@redhat.com>
> ---
>  tools/testing/selftests/kvm/lib/aarch64/vgic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> index 79864b941617..f365c32a7296 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -138,9 +138,6 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
>  	uint64_t val;
>  	bool intid_is_private = INTID_IS_SGI(intid) || INTID_IS_PPI(intid);
>  
> -	/* Check that the addr part of the attr is within 32 bits. */
> -	assert(attr <= KVM_DEV_ARM_VGIC_OFFSET_MASK);
> -
>  	uint32_t group = intid_is_private ? KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
>  					  : KVM_DEV_ARM_VGIC_GRP_DIST_REGS;
>  
> @@ -150,6 +147,9 @@ static void vgic_poke_irq(int gic_fd, uint32_t intid,
>  		attr += SZ_64K;
>  	}
>  
> +	/* Check that the addr part of the attr is within 32 bits. */
> +	assert((attr & ~KVM_DEV_ARM_VGIC_OFFSET_MASK) == 0);
> +
>  	/*
>  	 * All calls will succeed, even with invalid intid's, as long as the
>  	 * addr part of the attr is within 32 bits (checked above). An invalid
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
>

 
Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 0/5] kvm: selftests: aarch64: some fixes for vgic_irq

[Marc Zyngier]

On Wed, 26 Jan 2022 19:08:53 -0800, Ricardo Koller wrote:
> Reiji discovered multiple issues with the vgic_irq series [0]:
> 1. there's an assert that needs fixing.
> 2. some guest arguments are not set correctly.
> 3. the failure test in kvm_set_gsi_routing_irqchip_check is wrong.
> 4. there are lots of comments that use the wrong formatting.
> 5. vgic_poke_irq() could use a tighter assert check.
> 
> [...]

Applied to next, thanks!

[1/5] kvm: selftests: aarch64: fix assert in gicv3_access_reg
      commit: cc94d47ce16d4147d546e47c8248e8bd12ba5fe5
[2/5] kvm: selftests: aarch64: pass vgic_irq guest args as a pointer
      commit: 11024a7a0ac26dd31ddfa0f6590e158bdf9ab858
[3/5] kvm: selftests: aarch64: fix the failure check in kvm_set_gsi_routing_irqchip_check
      commit: 5b7898648f02083012900e48d063e51ccbdad165
[4/5] kvm: selftests: aarch64: fix some vgic related comments
      commit: a5cd38fd9c47b23abc6df08d6ee6a71b39038185
[5/5] kvm: selftests: aarch64: use a tighter assert in vgic_poke_irq()
      commit: b53de63a89244c196d8a2ea76b6754e3fdb4b626

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.