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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Oliver Upton <oliver.upton@xxxxxxxxx> writes:

Hi Colton,

On Fri, Nov 03, 2023 at 07:29:14PM +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>

What's missing from the changelog is that you've spotted an actual bug,
and this is really a bugfix.

I will update the changelog mentioning this is a bugfix.

---
  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 */

nit: this comment should appear above the isb()

I put it there because the manual specifies pending interrupts are
handled before the first instruction *after* the context sync, but I can
see why to put it above.

+		local_irq_disable();
  	}
-	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);
+

WFIs have nothing to do with the GIC, they're an instruction supported
by the CPU. I'd just merge the definition and declaration into the
header while you're at it.

So, could you throw something like this in aarch64/processor.h?

static inline void wfi(void)
{
	asm volatile("wfi");
}

Will do.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux