* [PATCH 0/2] kvm: selftests: aarch64: some fixes for vgic_irq @ 2022-01-20 17:39 Ricardo Koller 2022-01-20 17:39 ` [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg Ricardo Koller 2022-01-20 17:39 ` [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments Ricardo Koller 0 siblings, 2 replies; 6+ messages in thread From: Ricardo Koller @ 2022-01-20 17:39 UTC (permalink / raw) To: kvm, kvmarm, drjones; +Cc: maz, Paolo Bonzini, oupton, reijiw, Ricardo Koller Reiji discovered some issues with the vgic_irq series [0]: - there's an assert that needs fixing. - there are lots of comments that use the wrong formatting for comments. I haven't hit the failed assert, but just by chance: my compiler is initializing the respective local variable to 0. The second commit is not critical, but it would be nice to have. This series applies on top of: [PATCH] kvm: selftests: conditionally build vm_xsave_req_perm() The aarch64 selftests fail to build otherwise. [0] https://lore.kernel.org/kvmarm/164072141023.1027791.3183483860602648119.b4-ty@kernel.org/ Ricardo Koller (2): kvm: selftests: aarch64: fix assert in gicv3_access_reg kvm: selftests: aarch64: fix some vgic related comments tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12 ++++++++---- tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 13 ++++++++----- tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++- 3 files changed, 18 insertions(+), 10 deletions(-) -- 2.35.0.rc0.227.g00780c9af4-goog ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg 2022-01-20 17:39 [PATCH 0/2] kvm: selftests: aarch64: some fixes for vgic_irq Ricardo Koller @ 2022-01-20 17:39 ` Ricardo Koller 2022-01-26 15:17 ` Andrew Jones 2022-01-20 17:39 ` [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments Ricardo Koller 1 sibling, 1 reply; 6+ messages in thread From: Ricardo Koller @ 2022-01-20 17:39 UTC (permalink / raw) To: kvm, kvmarm, drjones; +Cc: maz, Paolo Bonzini, oupton, reijiw, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg 2022-01-20 17:39 ` [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg Ricardo Koller @ 2022-01-26 15:17 ` Andrew Jones 0 siblings, 0 replies; 6+ messages in thread From: Andrew Jones @ 2022-01-26 15:17 UTC (permalink / raw) To: Ricardo Koller; +Cc: kvm, kvmarm, maz, Paolo Bonzini, oupton, reijiw On Thu, Jan 20, 2022 at 09:39:04AM -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> > --- > 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 > Reviewed-by: Andrew Jones <drjones@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments 2022-01-20 17:39 [PATCH 0/2] kvm: selftests: aarch64: some fixes for vgic_irq Ricardo Koller 2022-01-20 17:39 ` [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg Ricardo Koller @ 2022-01-20 17:39 ` Ricardo Koller 2022-01-26 15:22 ` Andrew Jones 1 sibling, 1 reply; 6+ messages in thread From: Ricardo Koller @ 2022-01-20 17:39 UTC (permalink / raw) To: kvm, kvmarm, drjones; +Cc: maz, Paolo Bonzini, oupton, reijiw, 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> --- tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12 ++++++++---- tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 11 +++++++---- tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c index e6c7d7f8fbd1..258bb5150a07 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) @@ -837,7 +840,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..93fc35b88410 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,10 @@ 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 as those are + * currently not supported by readl nor writel, so just asserting here + * until then. */ 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments 2022-01-20 17:39 ` [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments Ricardo Koller @ 2022-01-26 15:22 ` Andrew Jones 2022-01-26 18:44 ` Ricardo Koller 0 siblings, 1 reply; 6+ messages in thread From: Andrew Jones @ 2022-01-26 15:22 UTC (permalink / raw) To: Ricardo Koller; +Cc: kvm, kvmarm, maz, Paolo Bonzini, oupton, reijiw On Thu, Jan 20, 2022 at 09:39:05AM -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> > --- > tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12 ++++++++---- > tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 11 +++++++---- > tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++- > 3 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c > index e6c7d7f8fbd1..258bb5150a07 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) > @@ -837,7 +840,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..93fc35b88410 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,10 @@ 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 as those are > + * currently not supported by readl nor writel, so just asserting here > + * until then. Well, readl and writel will never support 64 bit accesses. We'd need to implement readq and writeq for that. If we're going to rewrite this comment please state it that way instead. > */ > 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 > Thanks, drew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments 2022-01-26 15:22 ` Andrew Jones @ 2022-01-26 18:44 ` Ricardo Koller 0 siblings, 0 replies; 6+ messages in thread From: Ricardo Koller @ 2022-01-26 18:44 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvmarm, maz, Paolo Bonzini, oupton, reijiw On Wed, Jan 26, 2022 at 04:22:03PM +0100, Andrew Jones wrote: > On Thu, Jan 20, 2022 at 09:39:05AM -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> > > --- > > tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12 ++++++++---- > > tools/testing/selftests/kvm/lib/aarch64/gic_v3.c | 11 +++++++---- > > tools/testing/selftests/kvm/lib/aarch64/vgic.c | 3 ++- > > 3 files changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c > > index e6c7d7f8fbd1..258bb5150a07 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) > > @@ -837,7 +840,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..93fc35b88410 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,10 @@ 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 as those are > > + * currently not supported by readl nor writel, so just asserting here > > + * until then. > > Well, readl and writel will never support 64 bit accesses. We'd need to > implement readq and writeq for that. If we're going to rewrite this > comment please state it that way instead. Good point, will make it clearer in v2. Thanks! > > > */ > > 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 > > > > Thanks, > drew > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-26 18:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-20 17:39 [PATCH 0/2] kvm: selftests: aarch64: some fixes for vgic_irq Ricardo Koller 2022-01-20 17:39 ` [PATCH 1/2] kvm: selftests: aarch64: fix assert in gicv3_access_reg Ricardo Koller 2022-01-26 15:17 ` Andrew Jones 2022-01-20 17:39 ` [PATCH 2/2] kvm: selftests: aarch64: fix some vgic related comments Ricardo Koller 2022-01-26 15:22 ` Andrew Jones 2022-01-26 18:44 ` Ricardo Koller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).