Re: [PATCH v4 2/3] KVM: arm64: selftests: Guarantee interrupts are handled

[Oliver Upton <oliver.upton@linux.dev>]

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@google.com>
> ---
>  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