nitpick: Shortlog should read
KVM: selftests: Ensure pending interrupts are handled in arch_timer test
The fact that we're dealing with pending interrupts is critical here;
the ISB has no interaction with the GIC in terms of interrupt timing as
it gets to the PE.
On Thu, Mar 07, 2024 at 06:39:06PM +0000, Colton Lewis wrote:
> Break up the asm instructions poking daifclr and daifset to handle
> interrupts. R_RBZYL specifies pending interrupts will be handle after
> context synchronization events such as an ISB.
>
> Introduce a function wrapper for the WFI instruction.
>
> Signed-off-by: Colton Lewis <coltonlewis@xxxxxxxxxx>
> ---
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 12 ++++++------
> tools/testing/selftests/kvm/include/aarch64/gic.h | 3 +++
> tools/testing/selftests/kvm/lib/aarch64/gic.c | 5 +++++
> 3 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index d3bf584d2cc1..85f182704d79 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -269,13 +269,13 @@ static void guest_inject(struct test_args *args,
> KVM_INJECT_MULTI(cmd, first_intid, num);
>
> while (irq_handled < num) {
> - asm volatile("wfi\n"
> - "msr daifclr, #2\n"
> - /* handle IRQ */
> - "msr daifset, #2\n"
> - : : : "memory");
> + gic_wfi();
> + local_irq_enable();
> + isb();
> + /* handle IRQ */
> + local_irq_disable();
Sorry, this *still* annoys me. Please move the comment above the ISB,
you're documenting a behavior that is implied by the instruction, not
anything else.
> }
> - asm volatile("msr daifclr, #2" : : : "memory");
> + local_irq_enable();
>
> GUEST_ASSERT_EQ(irq_handled, num);
> for (i = first_intid; i < num + first_intid; i++)
> diff --git a/tools/testing/selftests/kvm/include/aarch64/gic.h b/tools/testing/selftests/kvm/include/aarch64/gic.h
> index 9043eaef1076..f474714e4cb2 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/gic.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/gic.h
> @@ -47,4 +47,7 @@ void gic_irq_clear_pending(unsigned int intid);
> bool gic_irq_get_pending(unsigned int intid);
> void gic_irq_set_config(unsigned int intid, bool is_edge);
>
> +/* Execute a Wait For Interrupt instruction. */
> +void gic_wfi(void);
> +
> #endif /* SELFTEST_KVM_GIC_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic.c b/tools/testing/selftests/kvm/lib/aarch64/gic.c
> index 9d15598d4e34..392e3f581ae0 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/gic.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic.c
> @@ -164,3 +164,8 @@ void gic_irq_set_config(unsigned int intid, bool is_edge)
> GUEST_ASSERT(gic_common_ops);
> gic_common_ops->gic_irq_set_config(intid, is_edge);
> }
> +
> +void gic_wfi(void)
> +{
> + asm volatile("wfi");
> +}
Ok, I left a comment about this last time...
WFI instructions are only relevant in the context of a PE, so it would
be natural to add such a helper to aarch64/processor.h. There are
definitely implementations out there that do not use a GIC and still
have WFI instructions.
--
Thanks,
Oliver
