[PATCH v3 00/12] KVM: arm64: selftests: Introduce arch_timer selftest

[PATCH v3 00/12] KVM: arm64: selftests: Introduce arch_timer selftest

[Raghavendra Rao Ananta]

Hello,

The patch series adds a KVM selftest to validate the behavior of
ARM's generic timer (patch-11). The test programs the timer IRQs
periodically, and for each interrupt, it validates the behaviour
against the architecture specifications. The test further provides
a command-line interface to configure the number of vCPUs, the
period of the timer, and the number of iterations that the test
has to run for.

Patch-12 adds an option to randomly migrate the vCPUs to different
physical CPUs across the system. The bug for the fix provided by
Marc with commit 3134cc8beb69d0d ("KVM: arm64: vgic: Resample HW
pending state on deactivation") was discovered using arch_timer
test with vCPU migrations.

Since the test heavily depends on interrupts, patch-10 adds a host
library to setup ARM Generic Interrupt Controller v3 (GICv3). This
includes creating a vGIC device, setting up distributor and
redistributor attributes, and mapping the guest physical addresses.
Symmetrical to this, patch-9 adds a guest library to talk to the vGIC,
which includes initializing the controller, enabling/disabling the
interrupts, and so on.

Furthermore, additional processor utilities such as accessing the MMIO
(via readl/writel), read/write to assembler unsupported registers,
basic delay generation, enable/disable local IRQs, and so on, are also
introduced that the test/GICv3 takes advantage of (patches 1 through 8).

The patch series, specifically the library support, is derived from the
kvm-unit-tests and the kernel itself.

Regards,
Raghavendra

v2 -> v3:

- Addressed the comments from Ricardo regarding moving the vGIC host
  support for selftests to its own library.
- Added an option (-m) to migrate the guest vCPUs to physical CPUs
  in the system.

v1 -> v2:

Addressed comments from Zenghui in include/aarch64/arch_timer.h:
- Correct the header description
- Remove unnecessary inclusion of linux/sizes.h
- Re-arrange CTL_ defines in ascending order
- Remove inappropriate 'return' from timer_set_* functions, which
  returns 'void'.

Raghavendra Rao Ananta (12):
  KVM: arm64: selftests: Add MMIO readl/writel support
  KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s
  KVM: arm64: selftests: Add support for cpu_relax
  KVM: arm64: selftests: Add basic support for arch_timers
  KVM: arm64: selftests: Add basic support to generate delays
  KVM: arm64: selftests: Add support to disable and enable local IRQs
  KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1
  KVM: arm64: selftests: Add light-weight spinlock support
  KVM: arm64: selftests: Add basic GICv3 support
  KVM: arm64: selftests: Add host support for vGIC
  KVM: arm64: selftests: Add arch_timer test
  KVM: arm64: selftests: arch_timer: Support vCPU migration

 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   3 +-
 .../selftests/kvm/aarch64/arch_timer.c        | 457 ++++++++++++++++++
 .../kvm/include/aarch64/arch_timer.h          | 142 ++++++
 .../selftests/kvm/include/aarch64/delay.h     |  25 +
 .../selftests/kvm/include/aarch64/gic.h       |  21 +
 .../selftests/kvm/include/aarch64/processor.h | 140 +++++-
 .../selftests/kvm/include/aarch64/spinlock.h  |  13 +
 .../selftests/kvm/include/aarch64/vgic.h      |  14 +
 tools/testing/selftests/kvm/lib/aarch64/gic.c |  93 ++++
 .../selftests/kvm/lib/aarch64/gic_private.h   |  21 +
 .../selftests/kvm/lib/aarch64/gic_v3.c        | 240 +++++++++
 .../selftests/kvm/lib/aarch64/gic_v3.h        |  70 +++
 .../selftests/kvm/lib/aarch64/spinlock.c      |  27 ++
 .../testing/selftests/kvm/lib/aarch64/vgic.c  |  67 +++
 15 files changed, 1332 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer.c
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/arch_timer.h
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/delay.h
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/gic.h
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/spinlock.h
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_private.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/spinlock.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c

-- 
2.33.0.153.gba50c8fa24-goog

[PATCH v3 01/12] KVM: arm64: selftests: Add MMIO readl/writel support

[Raghavendra Rao Ananta]

Define the readl() and writel() functions for the guests to
access (4-byte) the MMIO region.

The routines, and their dependents, are inspired from the kernel's
arch/arm64/include/asm/io.h and arch/arm64/include/asm/barrier.h.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/include/aarch64/processor.h | 45 ++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index c0273aefa63d..3cbaf5c1e26b 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -130,6 +130,49 @@ void vm_install_sync_handler(struct kvm_vm *vm,
 	val;								  \
 })
 
-#define isb()	asm volatile("isb" : : : "memory")
+#define isb()		asm volatile("isb" : : : "memory")
+#define dsb(opt)	asm volatile("dsb " #opt : : : "memory")
+#define dmb(opt)	asm volatile("dmb " #opt : : : "memory")
+
+#define dma_wmb()	dmb(oshst)
+#define __iowmb()	dma_wmb()
+
+#define dma_rmb()	dmb(oshld)
+
+#define __iormb(v)							\
+({									\
+	unsigned long tmp;						\
+									\
+	dma_rmb();							\
+									\
+	/*								\
+	 * Courtesy of arch/arm64/include/asm/io.h:			\
+	 * Create a dummy control dependency from the IO read to any	\
+	 * later instructions. This ensures that a subsequent call	\
+	 * to udelay() will be ordered due to the ISB in __delay().	\
+	 */								\
+	asm volatile("eor	%0, %1, %1\n"				\
+		     "cbnz	%0, ."					\
+		     : "=r" (tmp) : "r" ((unsigned long)(v))		\
+		     : "memory");					\
+})
+
+static __always_inline void __raw_writel(u32 val, volatile void *addr)
+{
+	asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr));
+}
+
+static __always_inline u32 __raw_readl(const volatile void *addr)
+{
+	u32 val;
+	asm volatile("ldr %w0, [%1]" : "=r" (val) : "r" (addr));
+	return val;
+}
+
+#define writel_relaxed(v,c)	((void)__raw_writel((__force u32)cpu_to_le32(v),(c)))
+#define readl_relaxed(c)	({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
+
+#define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c));})
+#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(__v); __v; })
 
 #endif /* SELFTEST_KVM_PROCESSOR_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 01/12] KVM: arm64: selftests: Add MMIO readl/writel support

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:01PM +0000, Raghavendra Rao Ananta wrote:
> Define the readl() and writel() functions for the guests to
> access (4-byte) the MMIO region.
> 
> The routines, and their dependents, are inspired from the kernel's
> arch/arm64/include/asm/io.h and arch/arm64/include/asm/barrier.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h | 45 ++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index c0273aefa63d..3cbaf5c1e26b 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -130,6 +130,49 @@ void vm_install_sync_handler(struct kvm_vm *vm,
>  	val;								  \
>  })
>  
> -#define isb()	asm volatile("isb" : : : "memory")
> +#define isb()		asm volatile("isb" : : : "memory")

Is this a stray diff?

Otherwise:

Reviewed-by: Oliver Upton <oupton@google.com>

Re: [PATCH v3 01/12] KVM: arm64: selftests: Add MMIO readl/writel support

[Raghavendra Rao Ananta]

On Wed, Sep 1, 2021 at 2:23 PM Oliver Upton <oupton@google.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:01PM +0000, Raghavendra Rao Ananta wrote:
> > Define the readl() and writel() functions for the guests to
> > access (4-byte) the MMIO region.
> >
> > The routines, and their dependents, are inspired from the kernel's
> > arch/arm64/include/asm/io.h and arch/arm64/include/asm/barrier.h.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> >  .../selftests/kvm/include/aarch64/processor.h | 45 ++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > index c0273aefa63d..3cbaf5c1e26b 100644
> > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > @@ -130,6 +130,49 @@ void vm_install_sync_handler(struct kvm_vm *vm,
> >       val;                                                              \
> >  })
> >
> > -#define isb()        asm volatile("isb" : : : "memory")
> > +#define isb()                asm volatile("isb" : : : "memory")
>
> Is this a stray diff?
>
Oh no, that's intentional. Just trying to align with others below.

Regards,
Raghavendra
> Otherwise:
>
> Reviewed-by: Oliver Upton <oupton@google.com>

Re: [PATCH v3 01/12] KVM: arm64: selftests: Add MMIO readl/writel support

[Oliver Upton]

On Wed, Sep 1, 2021 at 3:43 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> On Wed, Sep 1, 2021 at 2:23 PM Oliver Upton <oupton@google.com> wrote:
> >
> > On Wed, Sep 01, 2021 at 09:14:01PM +0000, Raghavendra Rao Ananta wrote:
> > > Define the readl() and writel() functions for the guests to
> > > access (4-byte) the MMIO region.
> > >
> > > The routines, and their dependents, are inspired from the kernel's
> > > arch/arm64/include/asm/io.h and arch/arm64/include/asm/barrier.h.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > ---
> > >  .../selftests/kvm/include/aarch64/processor.h | 45 ++++++++++++++++++-
> > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > index c0273aefa63d..3cbaf5c1e26b 100644
> > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > @@ -130,6 +130,49 @@ void vm_install_sync_handler(struct kvm_vm *vm,
> > >       val;                                                              \
> > >  })
> > >
> > > -#define isb()        asm volatile("isb" : : : "memory")
> > > +#define isb()                asm volatile("isb" : : : "memory")
> >
> > Is this a stray diff?
> >
> Oh no, that's intentional. Just trying to align with others below.

You are of course right, I read the diff wrong and didn't think it was
correctly aligned.

Thanks,
Oliver

Re: [PATCH v3 01/12] KVM: arm64: selftests: Add MMIO readl/writel support

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:01PM +0000, Raghavendra Rao Ananta wrote:
> Define the readl() and writel() functions for the guests to
> access (4-byte) the MMIO region.
> 
> The routines, and their dependents, are inspired from the kernel's
> arch/arm64/include/asm/io.h and arch/arm64/include/asm/barrier.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h | 45 ++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index c0273aefa63d..3cbaf5c1e26b 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -130,6 +130,49 @@ void vm_install_sync_handler(struct kvm_vm *vm,
>  	val;								  \
>  })
>  
> -#define isb()	asm volatile("isb" : : : "memory")
> +#define isb()		asm volatile("isb" : : : "memory")
> +#define dsb(opt)	asm volatile("dsb " #opt : : : "memory")
> +#define dmb(opt)	asm volatile("dmb " #opt : : : "memory")
> +
> +#define dma_wmb()	dmb(oshst)
> +#define __iowmb()	dma_wmb()
> +
> +#define dma_rmb()	dmb(oshld)
> +
> +#define __iormb(v)							\
> +({									\
> +	unsigned long tmp;						\
> +									\
> +	dma_rmb();							\
> +									\
> +	/*								\
> +	 * Courtesy of arch/arm64/include/asm/io.h:			\
> +	 * Create a dummy control dependency from the IO read to any	\
> +	 * later instructions. This ensures that a subsequent call	\
> +	 * to udelay() will be ordered due to the ISB in __delay().	\

We don't have udelay or __delay yet, but I assume they're coming soon.

> +	 */								\
> +	asm volatile("eor	%0, %1, %1\n"				\
> +		     "cbnz	%0, ."					\
> +		     : "=r" (tmp) : "r" ((unsigned long)(v))		\
> +		     : "memory");					\
> +})
> +
> +static __always_inline void __raw_writel(u32 val, volatile void *addr)
> +{
> +	asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr));
> +}
> +
> +static __always_inline u32 __raw_readl(const volatile void *addr)
> +{
> +	u32 val;
> +	asm volatile("ldr %w0, [%1]" : "=r" (val) : "r" (addr));
> +	return val;
> +}
> +
> +#define writel_relaxed(v,c)	((void)__raw_writel((__force u32)cpu_to_le32(v),(c)))
> +#define readl_relaxed(c)	({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })

Might want to explicitly include linux/types.h for these __force symbols.

> +
> +#define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c));})
> +#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(__v); __v; })
>  
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> -- 
> 2.33.0.153.gba50c8fa24-goog


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

Thanks,
drew

> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

[PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Raghavendra Rao Ananta]

For register names that are unsupported by the assembler or the ones
without architectural names, add the macros write_sysreg_s and
read_sysreg_s to support them.

The functionality is derived from kvm-unit-tests and kernel's
arch/arm64/include/asm/sysreg.h.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 3cbaf5c1e26b..082cc97ad8d3 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -118,6 +118,67 @@ void vm_install_exception_handler(struct kvm_vm *vm,
 void vm_install_sync_handler(struct kvm_vm *vm,
 		int vector, int ec, handler_fn handler);
 
+/*
+ * ARMv8 ARM reserves the following encoding for system registers:
+ * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
+ *  C5.2, version:ARM DDI 0487A.f)
+ *	[20-19] : Op0
+ *	[18-16] : Op1
+ *	[15-12] : CRn
+ *	[11-8]  : CRm
+ *	[7-5]   : Op2
+ */
+#define Op0_shift	19
+#define Op0_mask	0x3
+#define Op1_shift	16
+#define Op1_mask	0x7
+#define CRn_shift	12
+#define CRn_mask	0xf
+#define CRm_shift	8
+#define CRm_mask	0xf
+#define Op2_shift	5
+#define Op2_mask	0x7
+
+/*
+ * When accessed from guests, the ARM64_SYS_REG() doesn't work since it
+ * generates a different encoding for additional KVM processing, and is
+ * only suitable for userspace to access the register via ioctls.
+ * Hence, define a 'pure' sys_reg() here to generate the encodings as per spec.
+ */
+#define sys_reg(op0, op1, crn, crm, op2) \
+	(((op0) << Op0_shift) | ((op1) << Op1_shift) | \
+	 ((crn) << CRn_shift) | ((crm) << CRm_shift) | \
+	 ((op2) << Op2_shift))
+
+asm(
+"	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
+"	.equ	.L__reg_num_x\\num, \\num\n"
+"	.endr\n"
+"	.equ	.L__reg_num_xzr, 31\n"
+"\n"
+"	.macro	mrs_s, rt, sreg\n"
+"	.inst	0xd5200000|(\\sreg)|(.L__reg_num_\\rt)\n"
+"	.endm\n"
+"\n"
+"	.macro	msr_s, sreg, rt\n"
+"	.inst	0xd5000000|(\\sreg)|(.L__reg_num_\\rt)\n"
+"	.endm\n"
+);
+
+/*
+ * read_sysreg_s() and write_sysreg_s()'s 'reg' has to be encoded via sys_reg()
+ */
+#define read_sysreg_s(reg) ({						\
+	u64 __val;							\
+	asm volatile("mrs_s %0, "__stringify(reg) : "=r" (__val));	\
+	__val;								\
+})
+
+#define write_sysreg_s(reg, val) do {					\
+	u64 __val = (u64)val;						\
+	asm volatile("msr_s "__stringify(reg) ", %x0" : : "rZ" (__val));\
+} while (0)
+
 #define write_sysreg(reg, val)						  \
 ({									  \
 	u64 __val = (u64)(val);						  \
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> For register names that are unsupported by the assembler or the ones
> without architectural names, add the macros write_sysreg_s and
> read_sysreg_s to support them.
> 
> The functionality is derived from kvm-unit-tests and kernel's
> arch/arm64/include/asm/sysreg.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

Would it be possible to just include <asm/sysreg.h>? See
tools/arch/arm64/include/asm/sysreg.h

> ---
>  .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 3cbaf5c1e26b..082cc97ad8d3 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -118,6 +118,67 @@ void vm_install_exception_handler(struct kvm_vm *vm,
>  void vm_install_sync_handler(struct kvm_vm *vm,
>  		int vector, int ec, handler_fn handler);
>  
> +/*
> + * ARMv8 ARM reserves the following encoding for system registers:
> + * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> + *  C5.2, version:ARM DDI 0487A.f)
> + *	[20-19] : Op0
> + *	[18-16] : Op1
> + *	[15-12] : CRn
> + *	[11-8]  : CRm
> + *	[7-5]   : Op2
> + */
> +#define Op0_shift	19
> +#define Op0_mask	0x3
> +#define Op1_shift	16
> +#define Op1_mask	0x7
> +#define CRn_shift	12
> +#define CRn_mask	0xf
> +#define CRm_shift	8
> +#define CRm_mask	0xf
> +#define Op2_shift	5
> +#define Op2_mask	0x7
> +
> +/*
> + * When accessed from guests, the ARM64_SYS_REG() doesn't work since it
> + * generates a different encoding for additional KVM processing, and is
> + * only suitable for userspace to access the register via ioctls.
> + * Hence, define a 'pure' sys_reg() here to generate the encodings as per spec.
> + */
> +#define sys_reg(op0, op1, crn, crm, op2) \
> +	(((op0) << Op0_shift) | ((op1) << Op1_shift) | \
> +	 ((crn) << CRn_shift) | ((crm) << CRm_shift) | \
> +	 ((op2) << Op2_shift))
> +
> +asm(
> +"	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> +"	.equ	.L__reg_num_x\\num, \\num\n"
> +"	.endr\n"
> +"	.equ	.L__reg_num_xzr, 31\n"
> +"\n"
> +"	.macro	mrs_s, rt, sreg\n"
> +"	.inst	0xd5200000|(\\sreg)|(.L__reg_num_\\rt)\n"
> +"	.endm\n"
> +"\n"
> +"	.macro	msr_s, sreg, rt\n"
> +"	.inst	0xd5000000|(\\sreg)|(.L__reg_num_\\rt)\n"
> +"	.endm\n"
> +);
> +
> +/*
> + * read_sysreg_s() and write_sysreg_s()'s 'reg' has to be encoded via sys_reg()
> + */
> +#define read_sysreg_s(reg) ({						\
> +	u64 __val;							\
> +	asm volatile("mrs_s %0, "__stringify(reg) : "=r" (__val));	\
> +	__val;								\
> +})
> +
> +#define write_sysreg_s(reg, val) do {					\
> +	u64 __val = (u64)val;						\
> +	asm volatile("msr_s "__stringify(reg) ", %x0" : : "rZ" (__val));\
> +} while (0)
> +
>  #define write_sysreg(reg, val)						  \
>  ({									  \
>  	u64 __val = (u64)(val);						  \
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:28:28PM +0000, Oliver Upton wrote:
> On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> > For register names that are unsupported by the assembler or the ones
> > without architectural names, add the macros write_sysreg_s and
> > read_sysreg_s to support them.
> > 
> > The functionality is derived from kvm-unit-tests and kernel's
> > arch/arm64/include/asm/sysreg.h.
> > 
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> 
> Would it be possible to just include <asm/sysreg.h>? See
> tools/arch/arm64/include/asm/sysreg.h

Geez, sorry for the noise. I mistakenly searched from the root of my
repository, not the tools/ directory.

In any case, you could perhaps just drop the kernel header there just to
use the exact same source for kernel and selftest.

Thanks,
Oliver

> > ---
> >  .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > index 3cbaf5c1e26b..082cc97ad8d3 100644
> > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > @@ -118,6 +118,67 @@ void vm_install_exception_handler(struct kvm_vm *vm,
> >  void vm_install_sync_handler(struct kvm_vm *vm,
> >  		int vector, int ec, handler_fn handler);
> >  
> > +/*
> > + * ARMv8 ARM reserves the following encoding for system registers:
> > + * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> > + *  C5.2, version:ARM DDI 0487A.f)
> > + *	[20-19] : Op0
> > + *	[18-16] : Op1
> > + *	[15-12] : CRn
> > + *	[11-8]  : CRm
> > + *	[7-5]   : Op2
> > + */
> > +#define Op0_shift	19
> > +#define Op0_mask	0x3
> > +#define Op1_shift	16
> > +#define Op1_mask	0x7
> > +#define CRn_shift	12
> > +#define CRn_mask	0xf
> > +#define CRm_shift	8
> > +#define CRm_mask	0xf
> > +#define Op2_shift	5
> > +#define Op2_mask	0x7
> > +
> > +/*
> > + * When accessed from guests, the ARM64_SYS_REG() doesn't work since it
> > + * generates a different encoding for additional KVM processing, and is
> > + * only suitable for userspace to access the register via ioctls.
> > + * Hence, define a 'pure' sys_reg() here to generate the encodings as per spec.
> > + */
> > +#define sys_reg(op0, op1, crn, crm, op2) \
> > +	(((op0) << Op0_shift) | ((op1) << Op1_shift) | \
> > +	 ((crn) << CRn_shift) | ((crm) << CRm_shift) | \
> > +	 ((op2) << Op2_shift))
> > +
> > +asm(
> > +"	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> > +"	.equ	.L__reg_num_x\\num, \\num\n"
> > +"	.endr\n"
> > +"	.equ	.L__reg_num_xzr, 31\n"
> > +"\n"
> > +"	.macro	mrs_s, rt, sreg\n"
> > +"	.inst	0xd5200000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > +"	.endm\n"
> > +"\n"
> > +"	.macro	msr_s, sreg, rt\n"
> > +"	.inst	0xd5000000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > +"	.endm\n"
> > +);
> > +
> > +/*
> > + * read_sysreg_s() and write_sysreg_s()'s 'reg' has to be encoded via sys_reg()
> > + */
> > +#define read_sysreg_s(reg) ({						\
> > +	u64 __val;							\
> > +	asm volatile("mrs_s %0, "__stringify(reg) : "=r" (__val));	\
> > +	__val;								\
> > +})
> > +
> > +#define write_sysreg_s(reg, val) do {					\
> > +	u64 __val = (u64)val;						\
> > +	asm volatile("msr_s "__stringify(reg) ", %x0" : : "rZ" (__val));\
> > +} while (0)
> > +
> >  #define write_sysreg(reg, val)						  \
> >  ({									  \
> >  	u64 __val = (u64)(val);						  \
> > -- 
> > 2.33.0.153.gba50c8fa24-goog
> > 

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Raghavendra Rao Ananta]

On Wed, Sep 1, 2021 at 3:08 PM Oliver Upton <oupton@google.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:28:28PM +0000, Oliver Upton wrote:
> > On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> > > For register names that are unsupported by the assembler or the ones
> > > without architectural names, add the macros write_sysreg_s and
> > > read_sysreg_s to support them.
> > >
> > > The functionality is derived from kvm-unit-tests and kernel's
> > > arch/arm64/include/asm/sysreg.h.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> >
> > Would it be possible to just include <asm/sysreg.h>? See
> > tools/arch/arm64/include/asm/sysreg.h
>
> Geez, sorry for the noise. I mistakenly searched from the root of my
> repository, not the tools/ directory.
>
No worries :)

> In any case, you could perhaps just drop the kernel header there just to
> use the exact same source for kernel and selftest.
>
You mean just copy/paste the entire header? There's a lot of stuff in
there which we
don't need it (yet).

Regards,
Raghavendra
> Thanks,
> Oliver
>
> > > ---
> > >  .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > index 3cbaf5c1e26b..082cc97ad8d3 100644
> > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > @@ -118,6 +118,67 @@ void vm_install_exception_handler(struct kvm_vm *vm,
> > >  void vm_install_sync_handler(struct kvm_vm *vm,
> > >             int vector, int ec, handler_fn handler);
> > >
> > > +/*
> > > + * ARMv8 ARM reserves the following encoding for system registers:
> > > + * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> > > + *  C5.2, version:ARM DDI 0487A.f)
> > > + * [20-19] : Op0
> > > + * [18-16] : Op1
> > > + * [15-12] : CRn
> > > + * [11-8]  : CRm
> > > + * [7-5]   : Op2
> > > + */
> > > +#define Op0_shift  19
> > > +#define Op0_mask   0x3
> > > +#define Op1_shift  16
> > > +#define Op1_mask   0x7
> > > +#define CRn_shift  12
> > > +#define CRn_mask   0xf
> > > +#define CRm_shift  8
> > > +#define CRm_mask   0xf
> > > +#define Op2_shift  5
> > > +#define Op2_mask   0x7
> > > +
> > > +/*
> > > + * When accessed from guests, the ARM64_SYS_REG() doesn't work since it
> > > + * generates a different encoding for additional KVM processing, and is
> > > + * only suitable for userspace to access the register via ioctls.
> > > + * Hence, define a 'pure' sys_reg() here to generate the encodings as per spec.
> > > + */
> > > +#define sys_reg(op0, op1, crn, crm, op2) \
> > > +   (((op0) << Op0_shift) | ((op1) << Op1_shift) | \
> > > +    ((crn) << CRn_shift) | ((crm) << CRm_shift) | \
> > > +    ((op2) << Op2_shift))
> > > +
> > > +asm(
> > > +"  .irp    num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> > > +"  .equ    .L__reg_num_x\\num, \\num\n"
> > > +"  .endr\n"
> > > +"  .equ    .L__reg_num_xzr, 31\n"
> > > +"\n"
> > > +"  .macro  mrs_s, rt, sreg\n"
> > > +"  .inst   0xd5200000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > > +"  .endm\n"
> > > +"\n"
> > > +"  .macro  msr_s, sreg, rt\n"
> > > +"  .inst   0xd5000000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > > +"  .endm\n"
> > > +);
> > > +
> > > +/*
> > > + * read_sysreg_s() and write_sysreg_s()'s 'reg' has to be encoded via sys_reg()
> > > + */
> > > +#define read_sysreg_s(reg) ({                                              \
> > > +   u64 __val;                                                      \
> > > +   asm volatile("mrs_s %0, "__stringify(reg) : "=r" (__val));      \
> > > +   __val;                                                          \
> > > +})
> > > +
> > > +#define write_sysreg_s(reg, val) do {                                      \
> > > +   u64 __val = (u64)val;                                           \
> > > +   asm volatile("msr_s "__stringify(reg) ", %x0" : : "rZ" (__val));\
> > > +} while (0)
> > > +
> > >  #define write_sysreg(reg, val)                                               \
> > >  ({                                                                   \
> > >     u64 __val = (u64)(val);                                           \
> > > --
> > > 2.33.0.153.gba50c8fa24-goog
> > >

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Oliver Upton]

On Wed, Sep 01, 2021 at 03:48:40PM -0700, Raghavendra Rao Ananta wrote:
> On Wed, Sep 1, 2021 at 3:08 PM Oliver Upton <oupton@google.com> wrote:
> >
> > On Wed, Sep 01, 2021 at 09:28:28PM +0000, Oliver Upton wrote:
> > > On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> > > > For register names that are unsupported by the assembler or the ones
> > > > without architectural names, add the macros write_sysreg_s and
> > > > read_sysreg_s to support them.
> > > >
> > > > The functionality is derived from kvm-unit-tests and kernel's
> > > > arch/arm64/include/asm/sysreg.h.
> > > >
> > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > >
> > > Would it be possible to just include <asm/sysreg.h>? See
> > > tools/arch/arm64/include/asm/sysreg.h
> >
> > Geez, sorry for the noise. I mistakenly searched from the root of my
> > repository, not the tools/ directory.
> >
> No worries :)
> 
> > In any case, you could perhaps just drop the kernel header there just to
> > use the exact same source for kernel and selftest.
> >
> You mean just copy/paste the entire header? There's a lot of stuff in
> there which we
> don't need it (yet).

Right. It's mostly register definitions, which I don't think is too high
of an overhead. Don't know where others stand, but I would prefer a
header that is equivalent between kernel & selftests over a concise
header.

--
Thanks,
Oliver

> > Thanks,
> > Oliver
> >
> > > > ---
> > > >  .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
> > > >  1 file changed, 61 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > index 3cbaf5c1e26b..082cc97ad8d3 100644
> > > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > @@ -118,6 +118,67 @@ void vm_install_exception_handler(struct kvm_vm *vm,
> > > >  void vm_install_sync_handler(struct kvm_vm *vm,
> > > >             int vector, int ec, handler_fn handler);
> > > >
> > > > +/*
> > > > + * ARMv8 ARM reserves the following encoding for system registers:
> > > > + * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> > > > + *  C5.2, version:ARM DDI 0487A.f)
> > > > + * [20-19] : Op0
> > > > + * [18-16] : Op1
> > > > + * [15-12] : CRn
> > > > + * [11-8]  : CRm
> > > > + * [7-5]   : Op2
> > > > + */
> > > > +#define Op0_shift  19
> > > > +#define Op0_mask   0x3
> > > > +#define Op1_shift  16
> > > > +#define Op1_mask   0x7
> > > > +#define CRn_shift  12
> > > > +#define CRn_mask   0xf
> > > > +#define CRm_shift  8
> > > > +#define CRm_mask   0xf
> > > > +#define Op2_shift  5
> > > > +#define Op2_mask   0x7
> > > > +
> > > > +/*
> > > > + * When accessed from guests, the ARM64_SYS_REG() doesn't work since it
> > > > + * generates a different encoding for additional KVM processing, and is
> > > > + * only suitable for userspace to access the register via ioctls.
> > > > + * Hence, define a 'pure' sys_reg() here to generate the encodings as per spec.
> > > > + */
> > > > +#define sys_reg(op0, op1, crn, crm, op2) \
> > > > +   (((op0) << Op0_shift) | ((op1) << Op1_shift) | \
> > > > +    ((crn) << CRn_shift) | ((crm) << CRm_shift) | \
> > > > +    ((op2) << Op2_shift))
> > > > +
> > > > +asm(
> > > > +"  .irp    num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> > > > +"  .equ    .L__reg_num_x\\num, \\num\n"
> > > > +"  .endr\n"
> > > > +"  .equ    .L__reg_num_xzr, 31\n"
> > > > +"\n"
> > > > +"  .macro  mrs_s, rt, sreg\n"
> > > > +"  .inst   0xd5200000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > > > +"  .endm\n"
> > > > +"\n"
> > > > +"  .macro  msr_s, sreg, rt\n"
> > > > +"  .inst   0xd5000000|(\\sreg)|(.L__reg_num_\\rt)\n"
> > > > +"  .endm\n"
> > > > +);
> > > > +
> > > > +/*
> > > > + * read_sysreg_s() and write_sysreg_s()'s 'reg' has to be encoded via sys_reg()
> > > > + */
> > > > +#define read_sysreg_s(reg) ({                                              \
> > > > +   u64 __val;                                                      \
> > > > +   asm volatile("mrs_s %0, "__stringify(reg) : "=r" (__val));      \
> > > > +   __val;                                                          \
> > > > +})
> > > > +
> > > > +#define write_sysreg_s(reg, val) do {                                      \
> > > > +   u64 __val = (u64)val;                                           \
> > > > +   asm volatile("msr_s "__stringify(reg) ", %x0" : : "rZ" (__val));\
> > > > +} while (0)
> > > > +
> > > >  #define write_sysreg(reg, val)                                               \
> > > >  ({                                                                   \
> > > >     u64 __val = (u64)(val);                                           \
> > > > --
> > > > 2.33.0.153.gba50c8fa24-goog
> > > >

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Andrew Jones]

On Wed, Sep 01, 2021 at 11:06:10PM +0000, Oliver Upton wrote:
> On Wed, Sep 01, 2021 at 03:48:40PM -0700, Raghavendra Rao Ananta wrote:
> > On Wed, Sep 1, 2021 at 3:08 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > On Wed, Sep 01, 2021 at 09:28:28PM +0000, Oliver Upton wrote:
> > > > On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> > > > > For register names that are unsupported by the assembler or the ones
> > > > > without architectural names, add the macros write_sysreg_s and
> > > > > read_sysreg_s to support them.
> > > > >
> > > > > The functionality is derived from kvm-unit-tests and kernel's
> > > > > arch/arm64/include/asm/sysreg.h.
> > > > >
> > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > >
> > > > Would it be possible to just include <asm/sysreg.h>? See
> > > > tools/arch/arm64/include/asm/sysreg.h
> > >
> > > Geez, sorry for the noise. I mistakenly searched from the root of my
> > > repository, not the tools/ directory.
> > >
> > No worries :)
> > 
> > > In any case, you could perhaps just drop the kernel header there just to
> > > use the exact same source for kernel and selftest.
> > >
> > You mean just copy/paste the entire header? There's a lot of stuff in
> > there which we
> > don't need it (yet).
> 
> Right. It's mostly register definitions, which I don't think is too high
> of an overhead. Don't know where others stand, but I would prefer a
> header that is equivalent between kernel & selftests over a concise
> header.
>

Until now we haven't needed the sys_reg(...) type of definitions for
sysregs in selftests. In case we did, we defined the registers we
needed for get/set_one_reg by their parts, e.g.

 #define ID_AA64DFR0_EL1 3, 0,  0, 5, 0

allowing us to choose how we use them, ARM64_SYS_REG(...) vs.
sys_reg(...).

Bringing over sysreg.h is probably a good idea though. If we do, then
I'd suggest we define a new macro that allows us to convert a SYS_*
register definition from sysreg.h into an ARM64_SYS_REG definition
for get/set_one_reg in order to avoid redundant definitions.

Thanks,
drew

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Raghavendra Rao Ananta]

On Thu, Sep 2, 2021 at 5:31 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 11:06:10PM +0000, Oliver Upton wrote:
> > On Wed, Sep 01, 2021 at 03:48:40PM -0700, Raghavendra Rao Ananta wrote:
> > > On Wed, Sep 1, 2021 at 3:08 PM Oliver Upton <oupton@google.com> wrote:
> > > >
> > > > On Wed, Sep 01, 2021 at 09:28:28PM +0000, Oliver Upton wrote:
> > > > > On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> > > > > > For register names that are unsupported by the assembler or the ones
> > > > > > without architectural names, add the macros write_sysreg_s and
> > > > > > read_sysreg_s to support them.
> > > > > >
> > > > > > The functionality is derived from kvm-unit-tests and kernel's
> > > > > > arch/arm64/include/asm/sysreg.h.
> > > > > >
> > > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > > >
> > > > > Would it be possible to just include <asm/sysreg.h>? See
> > > > > tools/arch/arm64/include/asm/sysreg.h
> > > >
> > > > Geez, sorry for the noise. I mistakenly searched from the root of my
> > > > repository, not the tools/ directory.
> > > >
> > > No worries :)
> > >
> > > > In any case, you could perhaps just drop the kernel header there just to
> > > > use the exact same source for kernel and selftest.
> > > >
> > > You mean just copy/paste the entire header? There's a lot of stuff in
> > > there which we
> > > don't need it (yet).
> >
> > Right. It's mostly register definitions, which I don't think is too high
> > of an overhead. Don't know where others stand, but I would prefer a
> > header that is equivalent between kernel & selftests over a concise
> > header.
> >
>
> Until now we haven't needed the sys_reg(...) type of definitions for
> sysregs in selftests. In case we did, we defined the registers we
> needed for get/set_one_reg by their parts, e.g.
>
>  #define ID_AA64DFR0_EL1 3, 0,  0, 5, 0
>
> allowing us to choose how we use them, ARM64_SYS_REG(...) vs.
> sys_reg(...).
>
> Bringing over sysreg.h is probably a good idea though. If we do, then
> I'd suggest we define a new macro that allows us to convert a SYS_*
> register definition from sysreg.h into an ARM64_SYS_REG definition
> for get/set_one_reg in order to avoid redundant definitions.
>

I agree. Will look into it, and plan to pull the original sysreg.h
into selftests.

Regards,
Raghavendra

> Thanks,
> drew
>

Re: [PATCH v3 02/12] KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:02PM +0000, Raghavendra Rao Ananta wrote:
> For register names that are unsupported by the assembler or the ones
> without architectural names, add the macros write_sysreg_s and
> read_sysreg_s to support them.
> 
> The functionality is derived from kvm-unit-tests and kernel's
> arch/arm64/include/asm/sysreg.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)

If we don't replace with an import of arch/arm64/include/asm/sysreg.h

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

[PATCH v3 03/12] KVM: arm64: selftests: Add support for cpu_relax

[Raghavendra Rao Ananta]

Implement the guest helper routine, cpu_relax(), to yield
the processor to other tasks.

The function was derived from
arch/arm64/include/asm/vdso/processor.h.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/include/aarch64/processor.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 082cc97ad8d3..78df059dc974 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -191,6 +191,11 @@ asm(
 	val;								  \
 })
 
+static inline void cpu_relax(void)
+{
+	asm volatile("yield" ::: "memory");
+}
+
 #define isb()		asm volatile("isb" : : : "memory")
 #define dsb(opt)	asm volatile("dsb " #opt : : : "memory")
 #define dmb(opt)	asm volatile("dmb " #opt : : : "memory")
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 03/12] KVM: arm64: selftests: Add support for cpu_relax

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:03PM +0000, Raghavendra Rao Ananta wrote:
> Implement the guest helper routine, cpu_relax(), to yield
> the processor to other tasks.
> 
> The function was derived from
> arch/arm64/include/asm/vdso/processor.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

Reviewed-by: Oliver Upton <oupton@google.com>

> ---
>  tools/testing/selftests/kvm/include/aarch64/processor.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 082cc97ad8d3..78df059dc974 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -191,6 +191,11 @@ asm(
>  	val;								  \
>  })
>  
> +static inline void cpu_relax(void)
> +{
> +	asm volatile("yield" ::: "memory");
> +}
> +
>  #define isb()		asm volatile("isb" : : : "memory")
>  #define dsb(opt)	asm volatile("dsb " #opt : : : "memory")
>  #define dmb(opt)	asm volatile("dmb " #opt : : : "memory")
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 03/12] KVM: arm64: selftests: Add support for cpu_relax

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:03PM +0000, Raghavendra Rao Ananta wrote:
> Implement the guest helper routine, cpu_relax(), to yield
> the processor to other tasks.
> 
> The function was derived from
> arch/arm64/include/asm/vdso/processor.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

Reviewed-by: Oliver Upton <oupton@google.com>

> ---
>  tools/testing/selftests/kvm/include/aarch64/processor.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 082cc97ad8d3..78df059dc974 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -191,6 +191,11 @@ asm(
>  	val;								  \
>  })
>  
> +static inline void cpu_relax(void)
> +{
> +	asm volatile("yield" ::: "memory");
> +}
> +
>  #define isb()		asm volatile("isb" : : : "memory")
>  #define dsb(opt)	asm volatile("dsb " #opt : : : "memory")
>  #define dmb(opt)	asm volatile("dmb " #opt : : : "memory")
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 03/12] KVM: arm64: selftests: Add support for cpu_relax

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:03PM +0000, Raghavendra Rao Ananta wrote:
> Implement the guest helper routine, cpu_relax(), to yield
> the processor to other tasks.
> 
> The function was derived from
> arch/arm64/include/asm/vdso/processor.h.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/include/aarch64/processor.h | 5 +++++
>  1 file changed, 5 insertions(+)
>

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

[PATCH v3 04/12] KVM: arm64: selftests: Add basic support for arch_timers

[Raghavendra Rao Ananta]

Add a minimalistic library support to access the virtual timers,
that can be used for simple timing functionalities, such as
introducing delays in the guest.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../kvm/include/aarch64/arch_timer.h          | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/arch_timer.h

diff --git a/tools/testing/selftests/kvm/include/aarch64/arch_timer.h b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
new file mode 100644
index 000000000000..9df5b63abc47
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
@@ -0,0 +1,142 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Generic Timer specific interface
+ */
+
+#ifndef SELFTEST_KVM_ARCH_TIMER_H
+#define SELFTEST_KVM_ARCH_TIMER_H
+
+#include "processor.h"
+
+enum arch_timer {
+	VIRTUAL,
+	PHYSICAL,
+};
+
+#define CTL_ENABLE	(1 << 0)
+#define CTL_IMASK	(1 << 1)
+#define CTL_ISTATUS	(1 << 2)
+
+#define msec_to_cycles(msec)	\
+	(timer_get_cntfrq() * (uint64_t)(msec) / 1000)
+
+#define usec_to_cycles(usec)	\
+	(timer_get_cntfrq() * (uint64_t)(usec) / 1000000)
+
+#define cycles_to_usec(cycles) \
+	((uint64_t)(cycles) * 1000000 / timer_get_cntfrq())
+
+static inline uint32_t timer_get_cntfrq(void)
+{
+	return read_sysreg(cntfrq_el0);
+}
+
+static inline uint64_t timer_get_cntct(enum arch_timer timer)
+{
+	isb();
+
+	switch (timer) {
+	case VIRTUAL:
+		return read_sysreg(cntvct_el0);
+	case PHYSICAL:
+		return read_sysreg(cntpct_el0);
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	/* We should not reach here */
+	return 0;
+}
+
+static inline void timer_set_cval(enum arch_timer timer, uint64_t cval)
+{
+	switch (timer) {
+	case VIRTUAL:
+		write_sysreg(cntv_cval_el0, cval);
+		break;
+	case PHYSICAL:
+		write_sysreg(cntp_cval_el0, cval);
+		break;
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	isb();
+}
+
+static inline uint64_t timer_get_cval(enum arch_timer timer)
+{
+	switch (timer) {
+	case VIRTUAL:
+		return read_sysreg(cntv_cval_el0);
+	case PHYSICAL:
+		return read_sysreg(cntp_cval_el0);
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	/* We should not reach here */
+	return 0;
+}
+
+static inline void timer_set_tval(enum arch_timer timer, uint32_t tval)
+{
+	switch (timer) {
+	case VIRTUAL:
+		write_sysreg(cntv_tval_el0, tval);
+		break;
+	case PHYSICAL:
+		write_sysreg(cntp_tval_el0, tval);
+		break;
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	isb();
+}
+
+static inline void timer_set_ctl(enum arch_timer timer, uint32_t ctl)
+{
+	switch (timer) {
+	case VIRTUAL:
+		write_sysreg(cntv_ctl_el0, ctl);
+		break;
+	case PHYSICAL:
+		write_sysreg(cntp_ctl_el0, ctl);
+		break;
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	isb();
+}
+
+static inline uint32_t timer_get_ctl(enum arch_timer timer)
+{
+	switch (timer) {
+	case VIRTUAL:
+		return read_sysreg(cntv_ctl_el0);
+	case PHYSICAL:
+		return read_sysreg(cntp_ctl_el0);
+	default:
+		GUEST_ASSERT_1(0, timer);
+	}
+
+	/* We should not reach here */
+	return 0;
+}
+
+static inline void timer_set_next_cval_ms(enum arch_timer timer, uint32_t msec)
+{
+	uint64_t now_ct = timer_get_cntct(timer);
+	uint64_t next_ct = now_ct + msec_to_cycles(msec);
+
+	timer_set_cval(timer, next_ct);
+}
+
+static inline void timer_set_next_tval_ms(enum arch_timer timer, uint32_t msec)
+{
+	timer_set_tval(timer, msec_to_cycles(msec));
+}
+
+#endif /* SELFTEST_KVM_ARCH_TIMER_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 04/12] KVM: arm64: selftests: Add basic support for arch_timers

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:04PM +0000, Raghavendra Rao Ananta wrote:
> Add a minimalistic library support to access the virtual timers,
> that can be used for simple timing functionalities, such as
> introducing delays in the guest.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../kvm/include/aarch64/arch_timer.h          | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/arch_timer.h b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> new file mode 100644
> index 000000000000..9df5b63abc47
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Timer specific interface
> + */
> +
> +#ifndef SELFTEST_KVM_ARCH_TIMER_H
> +#define SELFTEST_KVM_ARCH_TIMER_H
> +
> +#include "processor.h"
> +
> +enum arch_timer {
> +	VIRTUAL,
> +	PHYSICAL,
> +};
> +
> +#define CTL_ENABLE	(1 << 0)
> +#define CTL_IMASK	(1 << 1)
> +#define CTL_ISTATUS	(1 << 2)
> +
> +#define msec_to_cycles(msec)	\
> +	(timer_get_cntfrq() * (uint64_t)(msec) / 1000)
> +
> +#define usec_to_cycles(usec)	\
> +	(timer_get_cntfrq() * (uint64_t)(usec) / 1000000)
> +
> +#define cycles_to_usec(cycles) \
> +	((uint64_t)(cycles) * 1000000 / timer_get_cntfrq())
> +
> +static inline uint32_t timer_get_cntfrq(void)
> +{
> +	return read_sysreg(cntfrq_el0);
> +}
> +
> +static inline uint64_t timer_get_cntct(enum arch_timer timer)
> +{
> +	isb();
> +
> +	switch (timer) {
> +	case VIRTUAL:
> +		return read_sysreg(cntvct_el0);
> +	case PHYSICAL:
> +		return read_sysreg(cntpct_el0);
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	/* We should not reach here */
> +	return 0;
> +}
> +
> +static inline void timer_set_cval(enum arch_timer timer, uint64_t cval)
> +{
> +	switch (timer) {
> +	case VIRTUAL:
> +		write_sysreg(cntv_cval_el0, cval);
> +		break;
> +	case PHYSICAL:
> +		write_sysreg(cntp_cval_el0, cval);

Huh, looks like we managed to merge a backwards write_sysreg into kvm
selftests. write_sysreg in the kernel and kvm-unit-tests is (value, reg).
We should post a patch fixing that before adding more calls to it.

> +		break;
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	isb();
> +}
> +
> +static inline uint64_t timer_get_cval(enum arch_timer timer)
> +{
> +	switch (timer) {
> +	case VIRTUAL:
> +		return read_sysreg(cntv_cval_el0);
> +	case PHYSICAL:
> +		return read_sysreg(cntp_cval_el0);
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	/* We should not reach here */
> +	return 0;
> +}
> +
> +static inline void timer_set_tval(enum arch_timer timer, uint32_t tval)
> +{
> +	switch (timer) {
> +	case VIRTUAL:
> +		write_sysreg(cntv_tval_el0, tval);
> +		break;
> +	case PHYSICAL:
> +		write_sysreg(cntp_tval_el0, tval);
> +		break;
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	isb();
> +}
> +
> +static inline void timer_set_ctl(enum arch_timer timer, uint32_t ctl)
> +{
> +	switch (timer) {
> +	case VIRTUAL:
> +		write_sysreg(cntv_ctl_el0, ctl);
> +		break;
> +	case PHYSICAL:
> +		write_sysreg(cntp_ctl_el0, ctl);
> +		break;
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	isb();
> +}
> +
> +static inline uint32_t timer_get_ctl(enum arch_timer timer)
> +{
> +	switch (timer) {
> +	case VIRTUAL:
> +		return read_sysreg(cntv_ctl_el0);
> +	case PHYSICAL:
> +		return read_sysreg(cntp_ctl_el0);
> +	default:
> +		GUEST_ASSERT_1(0, timer);
> +	}
> +
> +	/* We should not reach here */
> +	return 0;
> +}

I'll have to look at the test code that uses the above functions, but I
wonder if it wouldn't be better to define two test functions, one for
vtimer and one for ptimer where the sysreg accesses are direct, rather
than all these switched wrappers.

> +
> +static inline void timer_set_next_cval_ms(enum arch_timer timer, uint32_t msec)
> +{
> +	uint64_t now_ct = timer_get_cntct(timer);
> +	uint64_t next_ct = now_ct + msec_to_cycles(msec);
> +
> +	timer_set_cval(timer, next_ct);
> +}
> +
> +static inline void timer_set_next_tval_ms(enum arch_timer timer, uint32_t msec)
> +{
> +	timer_set_tval(timer, msec_to_cycles(msec));
> +}

I'll also look at how these wrappers are used, since open coding them may
be OK.

Thanks,
drew

> +
> +#endif /* SELFTEST_KVM_ARCH_TIMER_H */
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

[PATCH v3 05/12] KVM: arm64: selftests: Add basic support to generate delays

[Raghavendra Rao Ananta]

Add udelay() support to generate a delay in the guest.

The routines are derived and simplified from kernel's
arch/arm64/lib/delay.c.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/include/aarch64/delay.h     | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/delay.h

diff --git a/tools/testing/selftests/kvm/include/aarch64/delay.h b/tools/testing/selftests/kvm/include/aarch64/delay.h
new file mode 100644
index 000000000000..329e4f5079ea
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/delay.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM simple delay routines
+ */
+
+#ifndef SELFTEST_KVM_ARM_DELAY_H
+#define SELFTEST_KVM_ARM_DELAY_H
+
+#include "arch_timer.h"
+
+static inline void __delay(uint64_t cycles)
+{
+	enum arch_timer timer = VIRTUAL;
+	uint64_t start = timer_get_cntct(timer);
+
+	while ((timer_get_cntct(timer) - start) < cycles)
+		cpu_relax();
+}
+
+static inline void udelay(unsigned long usec)
+{
+	__delay(usec_to_cycles(usec));
+}
+
+#endif /* SELFTEST_KVM_ARM_DELAY_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 05/12] KVM: arm64: selftests: Add basic support to generate delays

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:05PM +0000, Raghavendra Rao Ananta wrote:
> Add udelay() support to generate a delay in the guest.
> 
> The routines are derived and simplified from kernel's
> arch/arm64/lib/delay.c.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/delay.h     | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/delay.h
>

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

Re: [PATCH v3 05/12] KVM: arm64: selftests: Add basic support to generate delays

[Oliver Upton]

On Wed, Sep 1, 2021 at 2:14 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> Add udelay() support to generate a delay in the guest.
>
> The routines are derived and simplified from kernel's
> arch/arm64/lib/delay.c.
>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/delay.h     | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/delay.h
>

Reviewed-by: Oliver Upton <oupton@google.com>

[PATCH v3 06/12] KVM: arm64: selftests: Add support to disable and enable local IRQs

[Raghavendra Rao Ananta]

Add functions local_irq_enable() and local_irq_disable() to
enable and disable the IRQs from the guest, respectively.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../testing/selftests/kvm/include/aarch64/processor.h  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 78df059dc974..c35bb7b8e870 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -241,4 +241,14 @@ static __always_inline u32 __raw_readl(const volatile void *addr)
 #define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c));})
 #define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(__v); __v; })
 
+static inline void local_irq_enable(void)
+{
+	asm volatile("msr daifclr, #3" : : : "memory");
+}
+
+static inline void local_irq_disable(void)
+{
+	asm volatile("msr daifset, #3" : : : "memory");
+}
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 06/12] KVM: arm64: selftests: Add support to disable and enable local IRQs

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:06PM +0000, Raghavendra Rao Ananta wrote:
> Add functions local_irq_enable() and local_irq_disable() to
> enable and disable the IRQs from the guest, respectively.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

Reviewed-by: Oliver Upton <oupton@google.com>

> ---
>  .../testing/selftests/kvm/include/aarch64/processor.h  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 78df059dc974..c35bb7b8e870 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -241,4 +241,14 @@ static __always_inline u32 __raw_readl(const volatile void *addr)
>  #define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c));})
>  #define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(__v); __v; })
>  
> +static inline void local_irq_enable(void)
> +{
> +	asm volatile("msr daifclr, #3" : : : "memory");
> +}
> +
> +static inline void local_irq_disable(void)
> +{
> +	asm volatile("msr daifset, #3" : : : "memory");
> +}
> +
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 06/12] KVM: arm64: selftests: Add support to disable and enable local IRQs

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:06PM +0000, Raghavendra Rao Ananta wrote:
> Add functions local_irq_enable() and local_irq_disable() to
> enable and disable the IRQs from the guest, respectively.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../testing/selftests/kvm/include/aarch64/processor.h  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

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

[PATCH v3 07/12] KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1

[Raghavendra Rao Ananta]

At times, such as when in the interrupt handler, the guest wants to
get the vCPU-id that it's running on. As a result, introduce
get_vcpuid() that parses the MPIDR_EL1 and returns the vcpuid to the
requested caller.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/include/aarch64/processor.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index c35bb7b8e870..8b372cd427da 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -251,4 +251,23 @@ static inline void local_irq_disable(void)
 	asm volatile("msr daifset, #3" : : : "memory");
 }
 
+#define MPIDR_LEVEL_BITS 8
+#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
+#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
+#define MPIDR_AFFINITY_LEVEL(mpidr, level) \
+	((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
+
+static inline uint32_t get_vcpuid(void)
+{
+	uint32_t vcpuid = 0;
+	uint64_t mpidr = read_sysreg(mpidr_el1);
+
+	/* KVM limits only 16 vCPUs at level 0 */
+	vcpuid = mpidr & 0x0f;
+	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 4;
+	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 12;
+
+	return vcpuid;
+}
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 07/12] KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:07PM +0000, Raghavendra Rao Ananta wrote:
> At times, such as when in the interrupt handler, the guest wants to
> get the vCPU-id that it's running on. As a result, introduce
> get_vcpuid() that parses the MPIDR_EL1 and returns the vcpuid to the
> requested caller.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index c35bb7b8e870..8b372cd427da 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -251,4 +251,23 @@ static inline void local_irq_disable(void)
>  	asm volatile("msr daifset, #3" : : : "memory");
>  }
>  
> +#define MPIDR_LEVEL_BITS 8
> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
> +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \
> +	((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
> +
> +static inline uint32_t get_vcpuid(void)
> +{
> +	uint32_t vcpuid = 0;
> +	uint64_t mpidr = read_sysreg(mpidr_el1);
> +
> +	/* KVM limits only 16 vCPUs at level 0 */
> +	vcpuid = mpidr & 0x0f;
> +	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 4;
> +	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 12;
> +
> +	return vcpuid;
> +}

Are we guaranteed that KVM will always compose vCPU IDs the same way? I
do not believe this is guaranteed ABI.

For the base case, you could pass the vCPU ID as an arg to the guest
function.

I do agree that finding the vCPU ID is a bit more challenging in an
interrupt context. Maybe use a ucall to ask userspace? But of course,
every test implements its own run loop, so its yet another case that
tests need to handle.

Or, you could allocate an array at runtime of length KVM_CAP_MAX_VCPUS
(use the KVM_CHECK_EXTENSION ioctl to get the value). Once all vCPUs are
instantiated, iterate over them from userspace to populate the {MPIDR,
VCPU_ID} map. You'd need to guarantee that callers initialize the vGIC
*after* adding vCPUs to the guest.

--
Thanks,
Oliver

>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 07/12] KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1

[Andrew Jones]

On Wed, Sep 01, 2021 at 11:48:12PM +0000, Oliver Upton wrote:
> On Wed, Sep 01, 2021 at 09:14:07PM +0000, Raghavendra Rao Ananta wrote:
> > At times, such as when in the interrupt handler, the guest wants to
> > get the vCPU-id that it's running on. As a result, introduce
> > get_vcpuid() that parses the MPIDR_EL1 and returns the vcpuid to the
> > requested caller.
> > 
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> >  .../selftests/kvm/include/aarch64/processor.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > index c35bb7b8e870..8b372cd427da 100644
> > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > @@ -251,4 +251,23 @@ static inline void local_irq_disable(void)
> >  	asm volatile("msr daifset, #3" : : : "memory");
> >  }
> >  
> > +#define MPIDR_LEVEL_BITS 8
> > +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
> > +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> > +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \
> > +	((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
> > +
> > +static inline uint32_t get_vcpuid(void)
> > +{
> > +	uint32_t vcpuid = 0;
> > +	uint64_t mpidr = read_sysreg(mpidr_el1);
> > +
> > +	/* KVM limits only 16 vCPUs at level 0 */
> > +	vcpuid = mpidr & 0x0f;
> > +	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 4;
> > +	vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 12;
> > +
> > +	return vcpuid;
> > +}
> 
> Are we guaranteed that KVM will always compose vCPU IDs the same way? I
> do not believe this is guaranteed ABI.

I don't believe we are. At least in QEMU we take pains to avoid that
assumption.

> 
> For the base case, you could pass the vCPU ID as an arg to the guest
> function.
> 
> I do agree that finding the vCPU ID is a bit more challenging in an
> interrupt context. Maybe use a ucall to ask userspace? But of course,
> every test implements its own run loop, so its yet another case that
> tests need to handle.
> 
> Or, you could allocate an array at runtime of length KVM_CAP_MAX_VCPUS
> (use the KVM_CHECK_EXTENSION ioctl to get the value). Once all vCPUs are
> instantiated, iterate over them from userspace to populate the {MPIDR,
> VCPU_ID} map. You'd need to guarantee that callers initialize the vGIC
> *after* adding vCPUs to the guest.

I agree with this approach. It may even make sense to create a common
function that returns a {cpu_id,vcpu_index} map for other tests to use.

Thanks,
drew

> 
> --
> Thanks,
> Oliver
> 
> >  #endif /* SELFTEST_KVM_PROCESSOR_H */
> > -- 
> > 2.33.0.153.gba50c8fa24-goog
> > 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Re: [PATCH v3 07/12] KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1

[Raghavendra Rao Ananta]

On Thu, Sep 2, 2021 at 5:37 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 11:48:12PM +0000, Oliver Upton wrote:
> > On Wed, Sep 01, 2021 at 09:14:07PM +0000, Raghavendra Rao Ananta wrote:
> > > At times, such as when in the interrupt handler, the guest wants to
> > > get the vCPU-id that it's running on. As a result, introduce
> > > get_vcpuid() that parses the MPIDR_EL1 and returns the vcpuid to the
> > > requested caller.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > ---
> > >  .../selftests/kvm/include/aarch64/processor.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > index c35bb7b8e870..8b372cd427da 100644
> > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > @@ -251,4 +251,23 @@ static inline void local_irq_disable(void)
> > >     asm volatile("msr daifset, #3" : : : "memory");
> > >  }
> > >
> > > +#define MPIDR_LEVEL_BITS 8
> > > +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
> > > +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> > > +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \
> > > +   ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
> > > +
> > > +static inline uint32_t get_vcpuid(void)
> > > +{
> > > +   uint32_t vcpuid = 0;
> > > +   uint64_t mpidr = read_sysreg(mpidr_el1);
> > > +
> > > +   /* KVM limits only 16 vCPUs at level 0 */
> > > +   vcpuid = mpidr & 0x0f;
> > > +   vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 4;
> > > +   vcpuid |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 12;
> > > +
> > > +   return vcpuid;
> > > +}
> >
> > Are we guaranteed that KVM will always compose vCPU IDs the same way? I
> > do not believe this is guaranteed ABI.
>
> I don't believe we are. At least in QEMU we take pains to avoid that
> assumption.
>
> >
> > For the base case, you could pass the vCPU ID as an arg to the guest
> > function.
> >
> > I do agree that finding the vCPU ID is a bit more challenging in an
> > interrupt context. Maybe use a ucall to ask userspace? But of course,
> > every test implements its own run loop, so its yet another case that
> > tests need to handle.
> >
> > Or, you could allocate an array at runtime of length KVM_CAP_MAX_VCPUS
> > (use the KVM_CHECK_EXTENSION ioctl to get the value). Once all vCPUs are
> > instantiated, iterate over them from userspace to populate the {MPIDR,
> > VCPU_ID} map. You'd need to guarantee that callers initialize the vGIC
> > *after* adding vCPUs to the guest.
>
> I agree with this approach. It may even make sense to create a common
> function that returns a {cpu_id,vcpu_index} map for other tests to use.
>
Interesting idea. I'll look into this.

Regards,
Raghavendra

> Thanks,
> drew
>
> >
> > --
> > Thanks,
> > Oliver
> >
> > >  #endif /* SELFTEST_KVM_PROCESSOR_H */
> > > --
> > > 2.33.0.153.gba50c8fa24-goog
> > >
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> >
>

[PATCH v3 08/12] KVM: arm64: selftests: Add light-weight spinlock support

[Raghavendra Rao Ananta]

Add a simpler version of spinlock support for ARM64 for
the guests to use.

The implementation is loosely based on the spinlock
implementation in kvm-unit-tests.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../selftests/kvm/include/aarch64/spinlock.h  | 13 +++++++++
 .../selftests/kvm/lib/aarch64/spinlock.c      | 27 +++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/spinlock.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/spinlock.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 5d05801ab816..61f0d376af99 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -35,7 +35,7 @@ endif
 
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
 LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
-LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S
+LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
diff --git a/tools/testing/selftests/kvm/include/aarch64/spinlock.h b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
new file mode 100644
index 000000000000..cf0984106d14
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef SELFTEST_KVM_ARM64_SPINLOCK_H
+#define SELFTEST_KVM_ARM64_SPINLOCK_H
+
+struct spinlock {
+	int v;
+};
+
+extern void spin_lock(struct spinlock *lock);
+extern void spin_unlock(struct spinlock *lock);
+
+#endif /* SELFTEST_KVM_ARM64_SPINLOCK_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/spinlock.c b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
new file mode 100644
index 000000000000..6d66a3dac237
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM64 Spinlock support
+ */
+#include <stdint.h>
+
+#include "spinlock.h"
+
+void spin_lock(struct spinlock *lock)
+{
+	uint32_t val, res;
+
+	asm volatile(
+	"1:	ldaxr	%w0, [%2]\n"
+	"	cbnz	%w0, 1b\n"
+	"	mov	%w0, #1\n"
+	"	stxr	%w1, %w0, [%2]\n"
+	"	cbnz	%w1, 1b\n"
+	: "=&r" (val), "=&r" (res)
+	: "r" (&lock->v)
+	: "memory");
+}
+
+void spin_unlock(struct spinlock *lock)
+{
+	asm volatile("stlr wzr, [%0]\n"	: : "r" (&lock->v) : "memory");
+}
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 08/12] KVM: arm64: selftests: Add light-weight spinlock support

[Oliver Upton]

On Wed, Sep 1, 2021 at 2:14 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> Add a simpler version of spinlock support for ARM64 for
> the guests to use.
>
> The implementation is loosely based on the spinlock
> implementation in kvm-unit-tests.
>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../selftests/kvm/include/aarch64/spinlock.h  | 13 +++++++++
>  .../selftests/kvm/lib/aarch64/spinlock.c      | 27 +++++++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/spinlock.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/spinlock.c
>
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5d05801ab816..61f0d376af99 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/spinlock.h b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
> new file mode 100644
> index 000000000000..cf0984106d14
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef SELFTEST_KVM_ARM64_SPINLOCK_H
> +#define SELFTEST_KVM_ARM64_SPINLOCK_H
> +
> +struct spinlock {
> +       int v;
> +};
> +
> +extern void spin_lock(struct spinlock *lock);
> +extern void spin_unlock(struct spinlock *lock);
> +
> +#endif /* SELFTEST_KVM_ARM64_SPINLOCK_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/spinlock.c b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
> new file mode 100644
> index 000000000000..6d66a3dac237
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM64 Spinlock support
> + */
> +#include <stdint.h>
> +
> +#include "spinlock.h"
> +
> +void spin_lock(struct spinlock *lock)
> +{
> +       uint32_t val, res;

nit: use 'int' to match the lock value type.

> +
> +       asm volatile(
> +       "1:     ldaxr   %w0, [%2]\n"
> +       "       cbnz    %w0, 1b\n"
> +       "       mov     %w0, #1\n"
> +       "       stxr    %w1, %w0, [%2]\n"
> +       "       cbnz    %w1, 1b\n"
> +       : "=&r" (val), "=&r" (res)
> +       : "r" (&lock->v)
> +       : "memory");
> +}
> +
> +void spin_unlock(struct spinlock *lock)
> +{
> +       asm volatile("stlr wzr, [%0]\n" : : "r" (&lock->v) : "memory");
> +}
> --
> 2.33.0.153.gba50c8fa24-goog
>

Otherwise, LGTM.

Reviewed-by: Oliver Upton <oupton@google.com>

Re: [PATCH v3 08/12] KVM: arm64: selftests: Add light-weight spinlock support

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:08PM +0000, Raghavendra Rao Ananta wrote:
> Add a simpler version of spinlock support for ARM64 for
> the guests to use.
> 
> The implementation is loosely based on the spinlock
> implementation in kvm-unit-tests.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../selftests/kvm/include/aarch64/spinlock.h  | 13 +++++++++
>  .../selftests/kvm/lib/aarch64/spinlock.c      | 27 +++++++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/spinlock.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/spinlock.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5d05801ab816..61f0d376af99 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/spinlock.h b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
> new file mode 100644
> index 000000000000..cf0984106d14
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/spinlock.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef SELFTEST_KVM_ARM64_SPINLOCK_H
> +#define SELFTEST_KVM_ARM64_SPINLOCK_H
> +
> +struct spinlock {
> +	int v;
> +};
> +
> +extern void spin_lock(struct spinlock *lock);
> +extern void spin_unlock(struct spinlock *lock);
> +
> +#endif /* SELFTEST_KVM_ARM64_SPINLOCK_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/spinlock.c b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
> new file mode 100644
> index 000000000000..6d66a3dac237
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/spinlock.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM64 Spinlock support
> + */
> +#include <stdint.h>
> +
> +#include "spinlock.h"
> +
> +void spin_lock(struct spinlock *lock)
> +{
> +	uint32_t val, res;
> +
> +	asm volatile(
> +	"1:	ldaxr	%w0, [%2]\n"
> +	"	cbnz	%w0, 1b\n"
> +	"	mov	%w0, #1\n"
> +	"	stxr	%w1, %w0, [%2]\n"
> +	"	cbnz	%w1, 1b\n"
> +	: "=&r" (val), "=&r" (res)
> +	: "r" (&lock->v)
> +	: "memory");
> +}
> +
> +void spin_unlock(struct spinlock *lock)
> +{
> +	asm volatile("stlr wzr, [%0]\n"	: : "r" (&lock->v) : "memory");
> +}
> -- 

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

It makes sense that the explicit barriers in kvm-unit-tests weren't also
inherited, because we already have the implicit barriers with these ld/st
instruction variants. (I suppose we could improve the kvm-unit-tests
implementation at some point.)

Thanks,
drew

[PATCH v3 09/12] KVM: arm64: selftests: Add basic GICv3 support

[Raghavendra Rao Ananta]

Add basic support for ARM Generic Interrupt Controller v3.
The support provides guests to setup interrupts.

The work is inspired from kvm-unit-tests and the kernel's
GIC driver (drivers/irqchip/irq-gic-v3.c).

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   2 +-
 .../selftests/kvm/include/aarch64/gic.h       |  21 ++
 tools/testing/selftests/kvm/lib/aarch64/gic.c |  93 +++++++
 .../selftests/kvm/lib/aarch64/gic_private.h   |  21 ++
 .../selftests/kvm/lib/aarch64/gic_v3.c        | 240 ++++++++++++++++++
 .../selftests/kvm/lib/aarch64/gic_v3.h        |  70 +++++
 6 files changed, 446 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/gic.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_private.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.h

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 61f0d376af99..5476a8ddef60 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -35,7 +35,7 @@ endif
 
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
 LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
-LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c
+LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
diff --git a/tools/testing/selftests/kvm/include/aarch64/gic.h b/tools/testing/selftests/kvm/include/aarch64/gic.h
new file mode 100644
index 000000000000..85dd1e53048e
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/gic.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Generic Interrupt Controller (GIC) specific defines
+ */
+
+#ifndef SELFTEST_KVM_GIC_H
+#define SELFTEST_KVM_GIC_H
+
+enum gic_type {
+	GIC_V3,
+	GIC_TYPE_MAX,
+};
+
+void gic_init(enum gic_type type, unsigned int nr_cpus,
+		void *dist_base, void *redist_base);
+void gic_irq_enable(unsigned int intid);
+void gic_irq_disable(unsigned int intid);
+unsigned int gic_get_and_ack_irq(void);
+void gic_set_eoi(unsigned int intid);
+
+#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
new file mode 100644
index 000000000000..b0b67f5aeaa6
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Generic Interrupt Controller (GIC) support
+ */
+
+#include <errno.h>
+#include <linux/bits.h>
+#include <linux/sizes.h>
+
+#include "kvm_util.h"
+
+#include <gic.h>
+#include "gic_private.h"
+#include "processor.h"
+#include "spinlock.h"
+
+static const struct gic_common_ops *gic_common_ops;
+static struct spinlock gic_lock;
+
+static void gic_cpu_init(unsigned int cpu, void *redist_base)
+{
+	gic_common_ops->gic_cpu_init(cpu, redist_base);
+}
+
+static void
+gic_dist_init(enum gic_type type, unsigned int nr_cpus, void *dist_base)
+{
+	const struct gic_common_ops *gic_ops;
+
+	spin_lock(&gic_lock);
+
+	/* Distributor initialization is needed only once per VM */
+	if (gic_common_ops) {
+		spin_unlock(&gic_lock);
+		return;
+	}
+
+	if (type == GIC_V3)
+		gic_ops = &gicv3_ops;
+
+	gic_ops->gic_init(nr_cpus, dist_base);
+	gic_common_ops = gic_ops;
+
+	/* Make sure that the initialized data is visible to all the vCPUs */
+	dsb(sy);
+
+	spin_unlock(&gic_lock);
+}
+
+void gic_init(enum gic_type type, unsigned int nr_cpus,
+		void *dist_base, void *redist_base)
+{
+	uint32_t cpu = get_vcpuid();
+
+	GUEST_ASSERT(type < GIC_TYPE_MAX);
+	GUEST_ASSERT(dist_base);
+	GUEST_ASSERT(redist_base);
+	GUEST_ASSERT(nr_cpus);
+
+	gic_dist_init(type, nr_cpus, dist_base);
+	gic_cpu_init(cpu, redist_base);
+}
+
+void gic_irq_enable(unsigned int intid)
+{
+	GUEST_ASSERT(gic_common_ops);
+	gic_common_ops->gic_irq_enable(intid);
+}
+
+void gic_irq_disable(unsigned int intid)
+{
+	GUEST_ASSERT(gic_common_ops);
+	gic_common_ops->gic_irq_disable(intid);
+}
+
+unsigned int gic_get_and_ack_irq(void)
+{
+	uint64_t irqstat;
+	unsigned int intid;
+
+	GUEST_ASSERT(gic_common_ops);
+
+	irqstat = gic_common_ops->gic_read_iar();
+	intid = irqstat & GENMASK(23, 0);
+
+	return intid;
+}
+
+void gic_set_eoi(unsigned int intid)
+{
+	GUEST_ASSERT(gic_common_ops);
+	gic_common_ops->gic_write_eoir(intid);
+}
diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_private.h b/tools/testing/selftests/kvm/lib/aarch64/gic_private.h
new file mode 100644
index 000000000000..d81d739433dc
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic_private.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Generic Interrupt Controller (GIC) private defines that's only
+ * shared among the GIC library code.
+ */
+
+#ifndef SELFTEST_KVM_GIC_PRIVATE_H
+#define SELFTEST_KVM_GIC_PRIVATE_H
+
+struct gic_common_ops {
+	void (*gic_init)(unsigned int nr_cpus, void *dist_base);
+	void (*gic_cpu_init)(unsigned int cpu, void *redist_base);
+	void (*gic_irq_enable)(unsigned int intid);
+	void (*gic_irq_disable)(unsigned int intid);
+	uint64_t (*gic_read_iar)(void);
+	void (*gic_write_eoir)(uint32_t irq);
+};
+
+extern const struct gic_common_ops gicv3_ops;
+
+#endif /* SELFTEST_KVM_GIC_PRIVATE_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
new file mode 100644
index 000000000000..4b635ca6a8cb
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Generic Interrupt Controller (GIC) v3 support
+ */
+
+#include <linux/sizes.h>
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "delay.h"
+
+#include "gic_v3.h"
+#include "gic_private.h"
+
+struct gicv3_data {
+	void *dist_base;
+	void *redist_base[GICV3_MAX_CPUS];
+	unsigned int nr_cpus;
+	unsigned int nr_spis;
+};
+
+#define sgi_base_from_redist(redist_base) (redist_base + SZ_64K)
+
+enum gicv3_intid_range {
+	SGI_RANGE,
+	PPI_RANGE,
+	SPI_RANGE,
+	INVALID_RANGE,
+};
+
+static struct gicv3_data gicv3_data;
+
+static void gicv3_gicd_wait_for_rwp(void)
+{
+	unsigned int count = 100000; /* 1s */
+
+	while (readl(gicv3_data.dist_base + GICD_CTLR) & GICD_CTLR_RWP) {
+		GUEST_ASSERT(count--);
+		udelay(10);
+	}
+}
+
+static void gicv3_gicr_wait_for_rwp(void *redist_base)
+{
+	unsigned int count = 100000; /* 1s */
+
+	while (readl(redist_base + GICR_CTLR) & GICR_CTLR_RWP) {
+		GUEST_ASSERT(count--);
+		udelay(10);
+	}
+}
+
+static enum gicv3_intid_range get_intid_range(unsigned int intid)
+{
+	switch (intid) {
+	case 0 ... 15:
+		return SGI_RANGE;
+	case 16 ... 31:
+		return PPI_RANGE;
+	case 32 ... 1019:
+		return SPI_RANGE;
+	}
+
+	/* We should not be reaching here */
+	GUEST_ASSERT(0);
+
+	return INVALID_RANGE;
+}
+
+static uint64_t gicv3_read_iar(void)
+{
+	uint64_t irqstat = read_sysreg_s(SYS_ICC_IAR1_EL1);
+
+	dsb(sy);
+	return irqstat;
+}
+
+static void gicv3_write_eoir(uint32_t irq)
+{
+	write_sysreg_s(SYS_ICC_EOIR1_EL1, irq);
+	isb();
+}
+
+static void
+gicv3_config_irq(unsigned int intid, unsigned int offset)
+{
+	uint32_t cpu = get_vcpuid();
+	uint32_t mask = 1 << (intid % 32);
+	enum gicv3_intid_range intid_range = get_intid_range(intid);
+	void *reg;
+
+	/* We care about 'cpu' only for SGIs or PPIs */
+	if (intid_range == SGI_RANGE || intid_range == PPI_RANGE) {
+		GUEST_ASSERT(cpu < gicv3_data.nr_cpus);
+
+		reg = sgi_base_from_redist(gicv3_data.redist_base[cpu]) +
+			offset;
+		writel(mask, reg);
+		gicv3_gicr_wait_for_rwp(gicv3_data.redist_base[cpu]);
+	} else if (intid_range == SPI_RANGE) {
+		reg = gicv3_data.dist_base + offset + (intid / 32) * 4;
+		writel(mask, reg);
+		gicv3_gicd_wait_for_rwp();
+	} else {
+		GUEST_ASSERT(0);
+	}
+}
+
+static void gicv3_irq_enable(unsigned int intid)
+{
+	gicv3_config_irq(intid, GICD_ISENABLER);
+}
+
+static void gicv3_irq_disable(unsigned int intid)
+{
+	gicv3_config_irq(intid, GICD_ICENABLER);
+}
+
+static void gicv3_enable_redist(void *redist_base)
+{
+	uint32_t val = readl(redist_base + GICR_WAKER);
+	unsigned int count = 100000; /* 1s */
+
+	val &= ~GICR_WAKER_ProcessorSleep;
+	writel(val, redist_base + GICR_WAKER);
+
+	/* Wait until the processor is 'active' */
+	while (readl(redist_base + GICR_WAKER) & GICR_WAKER_ChildrenAsleep) {
+		GUEST_ASSERT(count--);
+		udelay(10);
+	}
+}
+
+static inline void *gicr_base_gpa_cpu(void *redist_base, uint32_t cpu)
+{
+	/* Align all the redistributors sequentially */
+	return redist_base + cpu * SZ_64K * 2;
+}
+
+static void gicv3_cpu_init(unsigned int cpu, void *redist_base)
+{
+	void *sgi_base;
+	unsigned int i;
+	void *redist_base_cpu;
+
+	GUEST_ASSERT(cpu < gicv3_data.nr_cpus);
+
+	redist_base_cpu = gicr_base_gpa_cpu(redist_base, cpu);
+	sgi_base = sgi_base_from_redist(redist_base_cpu);
+
+	gicv3_enable_redist(redist_base_cpu);
+
+	/*
+	 * Mark all the SGI and PPI interrupts as non-secure Group-1.
+	 * Also, deactivate and disable them.
+	 */
+	writel(~0, sgi_base + GICR_IGROUPR0);
+	writel(~0, sgi_base + GICR_ICACTIVER0);
+	writel(~0, sgi_base + GICR_ICENABLER0);
+
+	/* Set a default priority for all the SGIs and PPIs */
+	for (i = 0; i < 32; i += 4)
+		writel(GICD_INT_DEF_PRI_X4,
+				sgi_base + GICR_IPRIORITYR0 + i);
+
+	gicv3_gicr_wait_for_rwp(redist_base_cpu);
+
+	/* Enable the GIC system register (ICC_*) access */
+	write_sysreg_s(SYS_ICC_SRE_EL1,
+			read_sysreg_s(SYS_ICC_SRE_EL1) | ICC_SRE_EL1_SRE);
+
+	/* Set a default priority threshold */
+	write_sysreg_s(SYS_ICC_PMR_EL1, ICC_PMR_DEF_PRIO);
+
+	/* Enable non-secure Group-1 interrupts */
+	write_sysreg_s(SYS_ICC_GRPEN1_EL1, ICC_IGRPEN1_EL1_ENABLE);
+
+	gicv3_data.redist_base[cpu] = redist_base_cpu;
+}
+
+static void gicv3_dist_init(void)
+{
+	void *dist_base = gicv3_data.dist_base;
+	unsigned int i;
+
+	/* Disable the distributor until we set things up */
+	writel(0, dist_base + GICD_CTLR);
+	gicv3_gicd_wait_for_rwp();
+
+	/*
+	 * Mark all the SPI interrupts as non-secure Group-1.
+	 * Also, deactivate and disable them.
+	 */
+	for (i = 32; i < gicv3_data.nr_spis; i += 32) {
+		writel(~0, dist_base + GICD_IGROUPR + i / 8);
+		writel(~0, dist_base + GICD_ICACTIVER + i / 8);
+		writel(~0, dist_base + GICD_ICENABLER + i / 8);
+	}
+
+	/* Set a default priority for all the SPIs */
+	for (i = 32; i < gicv3_data.nr_spis; i += 4)
+		writel(GICD_INT_DEF_PRI_X4,
+				dist_base + GICD_IPRIORITYR + i);
+
+	/* Wait for the settings to sync-in */
+	gicv3_gicd_wait_for_rwp();
+
+	/* Finally, enable the distributor globally with ARE */
+	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A |
+			GICD_CTLR_ENABLE_G1, dist_base + GICD_CTLR);
+	gicv3_gicd_wait_for_rwp();
+}
+
+static void gicv3_init(unsigned int nr_cpus, void *dist_base)
+{
+	GUEST_ASSERT(nr_cpus <= GICV3_MAX_CPUS);
+
+	gicv3_data.nr_cpus = nr_cpus;
+	gicv3_data.dist_base = dist_base;
+	gicv3_data.nr_spis = GICD_TYPER_SPIS(
+				readl(gicv3_data.dist_base + GICD_TYPER));
+	if (gicv3_data.nr_spis > 1020)
+		gicv3_data.nr_spis = 1020;
+
+	/*
+	 * Initialize only the distributor for now.
+	 * The redistributor and CPU interfaces are initialized
+	 * later for every PE.
+	 */
+	gicv3_dist_init();
+}
+
+const struct gic_common_ops gicv3_ops = {
+	.gic_init = gicv3_init,
+	.gic_cpu_init = gicv3_cpu_init,
+	.gic_irq_enable = gicv3_irq_enable,
+	.gic_irq_disable = gicv3_irq_disable,
+	.gic_read_iar = gicv3_read_iar,
+	.gic_write_eoir = gicv3_write_eoir,
+};
diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
new file mode 100644
index 000000000000..d41195e347b3
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Generic Interrupt Controller (GIC) v3 specific defines
+ */
+
+#ifndef SELFTEST_KVM_GICV3_H
+#define SELFTEST_KVM_GICV3_H
+
+#include "processor.h"
+
+/*
+ * Distributor registers
+ */
+#define GICD_CTLR			0x0000
+#define GICD_TYPER			0x0004
+#define GICD_IGROUPR			0x0080
+#define GICD_ISENABLER			0x0100
+#define GICD_ICENABLER			0x0180
+#define GICD_ICACTIVER			0x0380
+#define GICD_IPRIORITYR			0x0400
+
+/*
+ * The assumption is that the guest runs in a non-secure mode.
+ * The following bits of GICD_CTLR are defined accordingly.
+ */
+#define GICD_CTLR_RWP			(1U << 31)
+#define GICD_CTLR_nASSGIreq		(1U << 8)
+#define GICD_CTLR_ARE_NS		(1U << 4)
+#define GICD_CTLR_ENABLE_G1A		(1U << 1)
+#define GICD_CTLR_ENABLE_G1		(1U << 0)
+
+#define GICD_TYPER_SPIS(typer)		((((typer) & 0x1f) + 1) * 32)
+#define GICD_INT_DEF_PRI_X4		0xa0a0a0a0
+
+/*
+ * Redistributor registers
+ */
+#define GICR_CTLR			0x000
+#define GICR_WAKER			0x014
+
+#define GICR_CTLR_RWP			(1U << 3)
+
+#define GICR_WAKER_ProcessorSleep	(1U << 1)
+#define GICR_WAKER_ChildrenAsleep	(1U << 2)
+
+/*
+ * Redistributor registers, offsets from SGI base
+ */
+#define GICR_IGROUPR0			GICD_IGROUPR
+#define GICR_ISENABLER0			GICD_ISENABLER
+#define GICR_ICENABLER0			GICD_ICENABLER
+#define GICR_ICACTIVER0			GICD_ICACTIVER
+#define GICR_IPRIORITYR0		GICD_IPRIORITYR
+
+/* CPU interface registers */
+#define SYS_ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
+#define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
+#define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
+#define SYS_ICC_SRE_EL1			sys_reg(3, 0, 12, 12, 5)
+#define SYS_ICC_GRPEN1_EL1		sys_reg(3, 0, 12, 12, 7)
+
+#define ICC_PMR_DEF_PRIO		0xf0
+
+#define ICC_SRE_EL1_SRE			(1U << 0)
+
+#define ICC_IGRPEN1_EL1_ENABLE		(1U << 0)
+
+#define GICV3_MAX_CPUS			512
+
+#endif /* SELFTEST_KVM_GICV3_H */
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 09/12] KVM: arm64: selftests: Add basic GICv3 support

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:09PM +0000, Raghavendra Rao Ananta wrote:
> Add basic support for ARM Generic Interrupt Controller v3.
> The support provides guests to setup interrupts.
> 
> The work is inspired from kvm-unit-tests and the kernel's
> GIC driver (drivers/irqchip/irq-gic-v3.c).
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |   2 +-
>  .../selftests/kvm/include/aarch64/gic.h       |  21 ++
>  tools/testing/selftests/kvm/lib/aarch64/gic.c |  93 +++++++
>  .../selftests/kvm/lib/aarch64/gic_private.h   |  21 ++
>  .../selftests/kvm/lib/aarch64/gic_v3.c        | 240 ++++++++++++++++++
>  .../selftests/kvm/lib/aarch64/gic_v3.h        |  70 +++++
>  6 files changed, 446 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/gic.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic.c
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_private.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 61f0d376af99..5476a8ddef60 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/gic.h b/tools/testing/selftests/kvm/include/aarch64/gic.h
> new file mode 100644
> index 000000000000..85dd1e53048e
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/gic.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) specific defines
> + */
> +
> +#ifndef SELFTEST_KVM_GIC_H
> +#define SELFTEST_KVM_GIC_H
> +
> +enum gic_type {
> +	GIC_V3,
> +	GIC_TYPE_MAX,
> +};
> +
> +void gic_init(enum gic_type type, unsigned int nr_cpus,
> +		void *dist_base, void *redist_base);
> +void gic_irq_enable(unsigned int intid);
> +void gic_irq_disable(unsigned int intid);
> +unsigned int gic_get_and_ack_irq(void);
> +void gic_set_eoi(unsigned int intid);
> +
> +#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
> new file mode 100644
> index 000000000000..b0b67f5aeaa6
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM Generic Interrupt Controller (GIC) support
> + */
> +
> +#include <errno.h>
> +#include <linux/bits.h>
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +
> +#include <gic.h>

"gic.h"

> +#include "gic_private.h"
> +#include "processor.h"
> +#include "spinlock.h"
> +
> +static const struct gic_common_ops *gic_common_ops;
> +static struct spinlock gic_lock;
> +
> +static void gic_cpu_init(unsigned int cpu, void *redist_base)
> +{
> +	gic_common_ops->gic_cpu_init(cpu, redist_base);
> +}
> +
> +static void
> +gic_dist_init(enum gic_type type, unsigned int nr_cpus, void *dist_base)
> +{
> +	const struct gic_common_ops *gic_ops;
> +
> +	spin_lock(&gic_lock);
> +
> +	/* Distributor initialization is needed only once per VM */
> +	if (gic_common_ops) {
> +		spin_unlock(&gic_lock);
> +		return;
> +	}
> +
> +	if (type == GIC_V3)
> +		gic_ops = &gicv3_ops;
> +
> +	gic_ops->gic_init(nr_cpus, dist_base);
> +	gic_common_ops = gic_ops;
> +
> +	/* Make sure that the initialized data is visible to all the vCPUs */
> +	dsb(sy);
> +
> +	spin_unlock(&gic_lock);
> +}
> +
> +void gic_init(enum gic_type type, unsigned int nr_cpus,
> +		void *dist_base, void *redist_base)
> +{
> +	uint32_t cpu = get_vcpuid();
> +
> +	GUEST_ASSERT(type < GIC_TYPE_MAX);
> +	GUEST_ASSERT(dist_base);
> +	GUEST_ASSERT(redist_base);
> +	GUEST_ASSERT(nr_cpus);
> +
> +	gic_dist_init(type, nr_cpus, dist_base);
> +	gic_cpu_init(cpu, redist_base);
> +}
> +
> +void gic_irq_enable(unsigned int intid)
> +{
> +	GUEST_ASSERT(gic_common_ops);
> +	gic_common_ops->gic_irq_enable(intid);
> +}
> +
> +void gic_irq_disable(unsigned int intid)
> +{
> +	GUEST_ASSERT(gic_common_ops);
> +	gic_common_ops->gic_irq_disable(intid);
> +}
> +
> +unsigned int gic_get_and_ack_irq(void)
> +{
> +	uint64_t irqstat;
> +	unsigned int intid;
> +
> +	GUEST_ASSERT(gic_common_ops);
> +
> +	irqstat = gic_common_ops->gic_read_iar();
> +	intid = irqstat & GENMASK(23, 0);
> +
> +	return intid;
> +}
> +
> +void gic_set_eoi(unsigned int intid)
> +{
> +	GUEST_ASSERT(gic_common_ops);
> +	gic_common_ops->gic_write_eoir(intid);
> +}
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_private.h b/tools/testing/selftests/kvm/lib/aarch64/gic_private.h
> new file mode 100644
> index 000000000000..d81d739433dc
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_private.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) private defines that's only
> + * shared among the GIC library code.
> + */
> +
> +#ifndef SELFTEST_KVM_GIC_PRIVATE_H
> +#define SELFTEST_KVM_GIC_PRIVATE_H
> +
> +struct gic_common_ops {
> +	void (*gic_init)(unsigned int nr_cpus, void *dist_base);
> +	void (*gic_cpu_init)(unsigned int cpu, void *redist_base);
> +	void (*gic_irq_enable)(unsigned int intid);
> +	void (*gic_irq_disable)(unsigned int intid);
> +	uint64_t (*gic_read_iar)(void);
> +	void (*gic_write_eoir)(uint32_t irq);
> +};
> +
> +extern const struct gic_common_ops gicv3_ops;
> +
> +#endif /* SELFTEST_KVM_GIC_PRIVATE_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> new file mode 100644
> index 000000000000..4b635ca6a8cb
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM Generic Interrupt Controller (GIC) v3 support
> + */
> +
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "delay.h"
> +
> +#include "gic_v3.h"
> +#include "gic_private.h"
> +
> +struct gicv3_data {
> +	void *dist_base;
> +	void *redist_base[GICV3_MAX_CPUS];
> +	unsigned int nr_cpus;
> +	unsigned int nr_spis;
> +};
> +
> +#define sgi_base_from_redist(redist_base) (redist_base + SZ_64K)
> +
> +enum gicv3_intid_range {
> +	SGI_RANGE,
> +	PPI_RANGE,
> +	SPI_RANGE,
> +	INVALID_RANGE,
> +};
> +
> +static struct gicv3_data gicv3_data;
> +
> +static void gicv3_gicd_wait_for_rwp(void)
> +{
> +	unsigned int count = 100000; /* 1s */
> +
> +	while (readl(gicv3_data.dist_base + GICD_CTLR) & GICD_CTLR_RWP) {
> +		GUEST_ASSERT(count--);
> +		udelay(10);
> +	}
> +}
> +
> +static void gicv3_gicr_wait_for_rwp(void *redist_base)
> +{
> +	unsigned int count = 100000; /* 1s */
> +
> +	while (readl(redist_base + GICR_CTLR) & GICR_CTLR_RWP) {
> +		GUEST_ASSERT(count--);
> +		udelay(10);
> +	}
> +}
> +
> +static enum gicv3_intid_range get_intid_range(unsigned int intid)
> +{
> +	switch (intid) {
> +	case 0 ... 15:
> +		return SGI_RANGE;
> +	case 16 ... 31:
> +		return PPI_RANGE;
> +	case 32 ... 1019:
> +		return SPI_RANGE;
> +	}
> +
> +	/* We should not be reaching here */
> +	GUEST_ASSERT(0);
> +
> +	return INVALID_RANGE;
> +}
> +
> +static uint64_t gicv3_read_iar(void)
> +{
> +	uint64_t irqstat = read_sysreg_s(SYS_ICC_IAR1_EL1);
> +
> +	dsb(sy);
> +	return irqstat;
> +}
> +
> +static void gicv3_write_eoir(uint32_t irq)
> +{
> +	write_sysreg_s(SYS_ICC_EOIR1_EL1, irq);
> +	isb();
> +}
> +
> +static void
> +gicv3_config_irq(unsigned int intid, unsigned int offset)
> +{
> +	uint32_t cpu = get_vcpuid();
> +	uint32_t mask = 1 << (intid % 32);
> +	enum gicv3_intid_range intid_range = get_intid_range(intid);
> +	void *reg;
> +
> +	/* We care about 'cpu' only for SGIs or PPIs */
> +	if (intid_range == SGI_RANGE || intid_range == PPI_RANGE) {
> +		GUEST_ASSERT(cpu < gicv3_data.nr_cpus);
> +
> +		reg = sgi_base_from_redist(gicv3_data.redist_base[cpu]) +
> +			offset;
> +		writel(mask, reg);
> +		gicv3_gicr_wait_for_rwp(gicv3_data.redist_base[cpu]);
> +	} else if (intid_range == SPI_RANGE) {
> +		reg = gicv3_data.dist_base + offset + (intid / 32) * 4;
> +		writel(mask, reg);
> +		gicv3_gicd_wait_for_rwp();
> +	} else {
> +		GUEST_ASSERT(0);
> +	}
> +}
> +
> +static void gicv3_irq_enable(unsigned int intid)
> +{
> +	gicv3_config_irq(intid, GICD_ISENABLER);
> +}
> +
> +static void gicv3_irq_disable(unsigned int intid)
> +{
> +	gicv3_config_irq(intid, GICD_ICENABLER);
> +}
> +
> +static void gicv3_enable_redist(void *redist_base)
> +{
> +	uint32_t val = readl(redist_base + GICR_WAKER);
> +	unsigned int count = 100000; /* 1s */
> +
> +	val &= ~GICR_WAKER_ProcessorSleep;
> +	writel(val, redist_base + GICR_WAKER);
> +
> +	/* Wait until the processor is 'active' */
> +	while (readl(redist_base + GICR_WAKER) & GICR_WAKER_ChildrenAsleep) {
> +		GUEST_ASSERT(count--);
> +		udelay(10);
> +	}
> +}
> +
> +static inline void *gicr_base_gpa_cpu(void *redist_base, uint32_t cpu)
> +{
> +	/* Align all the redistributors sequentially */
> +	return redist_base + cpu * SZ_64K * 2;
> +}
> +
> +static void gicv3_cpu_init(unsigned int cpu, void *redist_base)
> +{
> +	void *sgi_base;
> +	unsigned int i;
> +	void *redist_base_cpu;
> +
> +	GUEST_ASSERT(cpu < gicv3_data.nr_cpus);
> +
> +	redist_base_cpu = gicr_base_gpa_cpu(redist_base, cpu);
> +	sgi_base = sgi_base_from_redist(redist_base_cpu);
> +
> +	gicv3_enable_redist(redist_base_cpu);
> +
> +	/*
> +	 * Mark all the SGI and PPI interrupts as non-secure Group-1.
> +	 * Also, deactivate and disable them.
> +	 */
> +	writel(~0, sgi_base + GICR_IGROUPR0);
> +	writel(~0, sgi_base + GICR_ICACTIVER0);
> +	writel(~0, sgi_base + GICR_ICENABLER0);
> +
> +	/* Set a default priority for all the SGIs and PPIs */
> +	for (i = 0; i < 32; i += 4)
> +		writel(GICD_INT_DEF_PRI_X4,
> +				sgi_base + GICR_IPRIORITYR0 + i);
> +
> +	gicv3_gicr_wait_for_rwp(redist_base_cpu);
> +
> +	/* Enable the GIC system register (ICC_*) access */
> +	write_sysreg_s(SYS_ICC_SRE_EL1,
> +			read_sysreg_s(SYS_ICC_SRE_EL1) | ICC_SRE_EL1_SRE);
> +
> +	/* Set a default priority threshold */
> +	write_sysreg_s(SYS_ICC_PMR_EL1, ICC_PMR_DEF_PRIO);
> +
> +	/* Enable non-secure Group-1 interrupts */
> +	write_sysreg_s(SYS_ICC_GRPEN1_EL1, ICC_IGRPEN1_EL1_ENABLE);
> +
> +	gicv3_data.redist_base[cpu] = redist_base_cpu;
> +}
> +
> +static void gicv3_dist_init(void)
> +{
> +	void *dist_base = gicv3_data.dist_base;
> +	unsigned int i;
> +
> +	/* Disable the distributor until we set things up */
> +	writel(0, dist_base + GICD_CTLR);
> +	gicv3_gicd_wait_for_rwp();
> +
> +	/*
> +	 * Mark all the SPI interrupts as non-secure Group-1.
> +	 * Also, deactivate and disable them.
> +	 */
> +	for (i = 32; i < gicv3_data.nr_spis; i += 32) {
> +		writel(~0, dist_base + GICD_IGROUPR + i / 8);
> +		writel(~0, dist_base + GICD_ICACTIVER + i / 8);
> +		writel(~0, dist_base + GICD_ICENABLER + i / 8);
> +	}
> +
> +	/* Set a default priority for all the SPIs */
> +	for (i = 32; i < gicv3_data.nr_spis; i += 4)
> +		writel(GICD_INT_DEF_PRI_X4,
> +				dist_base + GICD_IPRIORITYR + i);
> +
> +	/* Wait for the settings to sync-in */
> +	gicv3_gicd_wait_for_rwp();
> +
> +	/* Finally, enable the distributor globally with ARE */
> +	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A |
> +			GICD_CTLR_ENABLE_G1, dist_base + GICD_CTLR);
> +	gicv3_gicd_wait_for_rwp();
> +}
> +
> +static void gicv3_init(unsigned int nr_cpus, void *dist_base)
> +{
> +	GUEST_ASSERT(nr_cpus <= GICV3_MAX_CPUS);
> +
> +	gicv3_data.nr_cpus = nr_cpus;
> +	gicv3_data.dist_base = dist_base;
> +	gicv3_data.nr_spis = GICD_TYPER_SPIS(
> +				readl(gicv3_data.dist_base + GICD_TYPER));
> +	if (gicv3_data.nr_spis > 1020)
> +		gicv3_data.nr_spis = 1020;
> +
> +	/*
> +	 * Initialize only the distributor for now.
> +	 * The redistributor and CPU interfaces are initialized
> +	 * later for every PE.
> +	 */
> +	gicv3_dist_init();
> +}
> +
> +const struct gic_common_ops gicv3_ops = {
> +	.gic_init = gicv3_init,
> +	.gic_cpu_init = gicv3_cpu_init,
> +	.gic_irq_enable = gicv3_irq_enable,
> +	.gic_irq_disable = gicv3_irq_disable,
> +	.gic_read_iar = gicv3_read_iar,
> +	.gic_write_eoir = gicv3_write_eoir,
> +};
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
> new file mode 100644
> index 000000000000..d41195e347b3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) v3 specific defines
> + */
> +
> +#ifndef SELFTEST_KVM_GICV3_H
> +#define SELFTEST_KVM_GICV3_H
> +
> +#include "processor.h"
> +
> +/*
> + * Distributor registers
> + */
> +#define GICD_CTLR			0x0000
> +#define GICD_TYPER			0x0004
> +#define GICD_IGROUPR			0x0080
> +#define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
> +#define GICD_ICACTIVER			0x0380
> +#define GICD_IPRIORITYR			0x0400
> +
> +/*
> + * The assumption is that the guest runs in a non-secure mode.
> + * The following bits of GICD_CTLR are defined accordingly.
> + */
> +#define GICD_CTLR_RWP			(1U << 31)
> +#define GICD_CTLR_nASSGIreq		(1U << 8)
> +#define GICD_CTLR_ARE_NS		(1U << 4)
> +#define GICD_CTLR_ENABLE_G1A		(1U << 1)
> +#define GICD_CTLR_ENABLE_G1		(1U << 0)
> +
> +#define GICD_TYPER_SPIS(typer)		((((typer) & 0x1f) + 1) * 32)
> +#define GICD_INT_DEF_PRI_X4		0xa0a0a0a0
> +
> +/*
> + * Redistributor registers
> + */
> +#define GICR_CTLR			0x000
> +#define GICR_WAKER			0x014
> +
> +#define GICR_CTLR_RWP			(1U << 3)
> +
> +#define GICR_WAKER_ProcessorSleep	(1U << 1)
> +#define GICR_WAKER_ChildrenAsleep	(1U << 2)
> +
> +/*
> + * Redistributor registers, offsets from SGI base
> + */
> +#define GICR_IGROUPR0			GICD_IGROUPR
> +#define GICR_ISENABLER0			GICD_ISENABLER
> +#define GICR_ICENABLER0			GICD_ICENABLER
> +#define GICR_ICACTIVER0			GICD_ICACTIVER
> +#define GICR_IPRIORITYR0		GICD_IPRIORITYR
> +
> +/* CPU interface registers */
> +#define SYS_ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
> +#define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
> +#define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
> +#define SYS_ICC_SRE_EL1			sys_reg(3, 0, 12, 12, 5)
> +#define SYS_ICC_GRPEN1_EL1		sys_reg(3, 0, 12, 12, 7)
> +
> +#define ICC_PMR_DEF_PRIO		0xf0
> +
> +#define ICC_SRE_EL1_SRE			(1U << 0)
> +
> +#define ICC_IGRPEN1_EL1_ENABLE		(1U << 0)
> +
> +#define GICV3_MAX_CPUS			512
> +
> +#endif /* SELFTEST_KVM_GICV3_H */
> -- 
> 2.33.0.153.gba50c8fa24-goog
>

Looks good to me. I also see some nice stuff to bring over to
kvm-unit-tests in order to make some improvements there.

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

Thanks,
drew

[PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Raghavendra Rao Ananta]

Implement a simple library to do perform vGIC-v3
setup from a host of view. This includes creating
a vGIC device, setting up distributor and redistributor
attributes, and mapping the guest physical addresses.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

---
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
 .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 5476a8ddef60..8342f65c1d96 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -35,7 +35,7 @@ endif
 
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
 LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
-LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
+LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
new file mode 100644
index 000000000000..45bbf238147a
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Generic Interrupt Controller (GIC) host specific defines
+ */
+
+#ifndef SELFTEST_KVM_VGIC_H
+#define SELFTEST_KVM_VGIC_H
+
+#include <linux/kvm.h>
+
+int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
+		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
+
+#endif /* SELFTEST_KVM_VGIC_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
new file mode 100644
index 000000000000..a0e4b986d335
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Generic Interrupt Controller (GIC) v3 host support
+ */
+
+#include <linux/kvm.h>
+#include <linux/sizes.h>
+
+#include "kvm_util.h"
+
+#define VGIC_V3_GICD_SZ		(SZ_64K)
+#define VGIC_V3_GICR_SZ		(2 * SZ_64K)
+
+#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
+	(((uint64_t)(count) << 52) | \
+	((uint64_t)((base) >> 16) << 16) | \
+	((uint64_t)(flags) << 12) | \
+	index)
+
+static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
+{
+	unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
+
+	virt_map(vm, addr, addr, n_pages);
+}
+
+/*
+ * vGIC-v3 default host setup
+ *
+ * Input args:
+ *	vm - KVM VM
+ *	nr_vcpus - Number of vCPUs for this VM
+ *	gicd_base_gpa - Guest Physical Address of the Distributor region
+ *	gicr_base_gpa - Guest Physical Address of the Redistributor region
+ *
+ * Output args: None
+ *
+ * Return: GIC file-descriptor or negative error code upon failure
+ *
+ * The function creates a vGIC-v3 device and maps the distributor and
+ * redistributor regions of the guest.
+ */
+int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
+		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
+{
+	uint64_t redist_attr;
+	int gic_fd;
+
+	TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
+			"Invalid number of CPUs: %u\n", nr_vcpus);
+
+	gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+
+	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
+	vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
+
+	redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
+	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
+	vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
+
+	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+				KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+
+	return gic_fd;
+}
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Ricardo Koller]

On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> Implement a simple library to do perform vGIC-v3
> setup from a host of view. This includes creating
> a vGIC device, setting up distributor and redistributor
> attributes, and mapping the guest physical addresses.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> 
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
>  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5476a8ddef60..8342f65c1d96 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> new file mode 100644
> index 000000000000..45bbf238147a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) host specific defines
> + */
> +
> +#ifndef SELFTEST_KVM_VGIC_H
> +#define SELFTEST_KVM_VGIC_H
> +
> +#include <linux/kvm.h>
> +
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
> +
> +#endif /* SELFTEST_KVM_VGIC_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> new file mode 100644
> index 000000000000..a0e4b986d335
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM Generic Interrupt Controller (GIC) v3 host support
> + */
> +
> +#include <linux/kvm.h>
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +
> +#define VGIC_V3_GICD_SZ		(SZ_64K)
> +#define VGIC_V3_GICR_SZ		(2 * SZ_64K)
> +
> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> +	(((uint64_t)(count) << 52) | \
> +	((uint64_t)((base) >> 16) << 16) | \
> +	((uint64_t)(flags) << 12) | \
> +	index)
> +
> +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> +{
> +	unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> +
> +	virt_map(vm, addr, addr, n_pages);
> +}
> +
> +/*
> + * vGIC-v3 default host setup
> + *
> + * Input args:
> + *	vm - KVM VM
> + *	nr_vcpus - Number of vCPUs for this VM
> + *	gicd_base_gpa - Guest Physical Address of the Distributor region
> + *	gicr_base_gpa - Guest Physical Address of the Redistributor region
> + *
> + * Output args: None
> + *
> + * Return: GIC file-descriptor or negative error code upon failure
> + *
> + * The function creates a vGIC-v3 device and maps the distributor and
> + * redistributor regions of the guest.
> + */
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
> +{
> +	uint64_t redist_attr;
> +	int gic_fd;
> +
> +	TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> +			"Invalid number of CPUs: %u\n", nr_vcpus);
> +
> +	gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);

Nit: you can return early if gic_fd is bad.

> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> +	vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);

vgic_v3_map() implies that it's doing something vgic specific, when it's
just converting bytes to pages. What about something like the following?

	virt_map(vm, addr, addr, VM_BYTES_TO_PAGES(vm, VGIC_V3_GICD_SZ));

and you add a VM_BYTES_TO_PAGES macro to include/kvm_util.h? I think
this macro can be useful to others.

> +
> +	redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> +	vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +
> +	return gic_fd;
> +}
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Raghavendra Rao Ananta]

On Thu, Sep 2, 2021 at 10:28 AM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> > Implement a simple library to do perform vGIC-v3
> > setup from a host of view. This includes creating
> > a vGIC device, setting up distributor and redistributor
> > attributes, and mapping the guest physical addresses.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> >
> > ---
> >  tools/testing/selftests/kvm/Makefile          |  2 +-
> >  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
> >  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
> >  3 files changed, 82 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 5476a8ddef60..8342f65c1d96 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -35,7 +35,7 @@ endif
> >
> >  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
> >  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> > -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> > +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
> >  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
> >
> >  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > new file mode 100644
> > index 000000000000..45bbf238147a
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) host specific defines
> > + */
> > +
> > +#ifndef SELFTEST_KVM_VGIC_H
> > +#define SELFTEST_KVM_VGIC_H
> > +
> > +#include <linux/kvm.h>
> > +
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
> > +
> > +#endif /* SELFTEST_KVM_VGIC_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > new file mode 100644
> > index 000000000000..a0e4b986d335
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) v3 host support
> > + */
> > +
> > +#include <linux/kvm.h>
> > +#include <linux/sizes.h>
> > +
> > +#include "kvm_util.h"
> > +
> > +#define VGIC_V3_GICD_SZ              (SZ_64K)
> > +#define VGIC_V3_GICR_SZ              (2 * SZ_64K)
> > +
> > +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> > +     (((uint64_t)(count) << 52) | \
> > +     ((uint64_t)((base) >> 16) << 16) | \
> > +     ((uint64_t)(flags) << 12) | \
> > +     index)
> > +
> > +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> > +{
> > +     unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> > +
> > +     virt_map(vm, addr, addr, n_pages);
> > +}
> > +
> > +/*
> > + * vGIC-v3 default host setup
> > + *
> > + * Input args:
> > + *   vm - KVM VM
> > + *   nr_vcpus - Number of vCPUs for this VM
> > + *   gicd_base_gpa - Guest Physical Address of the Distributor region
> > + *   gicr_base_gpa - Guest Physical Address of the Redistributor region
> > + *
> > + * Output args: None
> > + *
> > + * Return: GIC file-descriptor or negative error code upon failure
> > + *
> > + * The function creates a vGIC-v3 device and maps the distributor and
> > + * redistributor regions of the guest.
> > + */
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
> > +{
> > +     uint64_t redist_attr;
> > +     int gic_fd;
> > +
> > +     TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> > +                     "Invalid number of CPUs: %u\n", nr_vcpus);
> > +
> > +     gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>
> Nit: you can return early if gic_fd is bad.
>
kvm_create_device() already takes care of this and the test would
simply fail if gic_fd was bad.
So, I think we can positively move on :)
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> > +     vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
>
> vgic_v3_map() implies that it's doing something vgic specific, when it's
> just converting bytes to pages. What about something like the following?
>
>         virt_map(vm, addr, addr, VM_BYTES_TO_PAGES(vm, VGIC_V3_GICD_SZ));
>
> and you add a VM_BYTES_TO_PAGES macro to include/kvm_util.h? I think
> this macro can be useful to others.
>
Yeah, good idea. Will do.

Regards,
Raghavendra
> > +
> > +     redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> > +     vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> > +                             KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> > +
> > +     return gic_fd;
> > +}
> > --
> > 2.33.0.153.gba50c8fa24-goog
> >

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> Implement a simple library to do perform vGIC-v3

s/do//

> setup from a host of view.
                   ^ point

> This includes creating
> a vGIC device, setting up distributor and redistributor
> attributes, and mapping the guest physical addresses.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> 
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
>  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5476a8ddef60..8342f65c1d96 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> new file mode 100644
> index 000000000000..45bbf238147a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) host specific defines
> + */
> +
> +#ifndef SELFTEST_KVM_VGIC_H
> +#define SELFTEST_KVM_VGIC_H
> +
> +#include <linux/kvm.h>
> +
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
> +
> +#endif /* SELFTEST_KVM_VGIC_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> new file mode 100644
> index 000000000000..a0e4b986d335
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM Generic Interrupt Controller (GIC) v3 host support
> + */
> +
> +#include <linux/kvm.h>
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +
> +#define VGIC_V3_GICD_SZ		(SZ_64K)
> +#define VGIC_V3_GICR_SZ		(2 * SZ_64K)
> +
> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> +	(((uint64_t)(count) << 52) | \
> +	((uint64_t)((base) >> 16) << 16) | \
> +	((uint64_t)(flags) << 12) | \
> +	index)

This could go in vgic.h allowing us to share it in aarch64/vgic_init.c,
where we already have the same definition.

> +
> +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> +{
> +	unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> +
> +	virt_map(vm, addr, addr, n_pages);
> +}
> +
> +/*
> + * vGIC-v3 default host setup
> + *
> + * Input args:
> + *	vm - KVM VM
> + *	nr_vcpus - Number of vCPUs for this VM
> + *	gicd_base_gpa - Guest Physical Address of the Distributor region
> + *	gicr_base_gpa - Guest Physical Address of the Redistributor region
> + *
> + * Output args: None
> + *
> + * Return: GIC file-descriptor or negative error code upon failure
> + *
> + * The function creates a vGIC-v3 device and maps the distributor and
> + * redistributor regions of the guest.
> + */
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
> +{
> +	uint64_t redist_attr;
> +	int gic_fd;
> +
> +	TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> +			"Invalid number of CPUs: %u\n", nr_vcpus);

TEST_ASSERT(!list_empty(&vm->vcpus), ...) to ensure we've created vcpus
first. To be really paranoid we could even confirm the number of vcpus in
the list matches nr_vcpus.

> +
> +	gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> +	vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
> +
> +	redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> +	vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +
> +	return gic_fd;
> +}
> -- 
> 2.33.0.153.gba50c8fa24-goog

Otherwise

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

Thanks,
drew

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Raghavendra Rao Ananta]

On Fri, Sep 3, 2021 at 3:00 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> > Implement a simple library to do perform vGIC-v3
>
> s/do//
>
> > setup from a host of view.
>                    ^ point
>
> > This includes creating
> > a vGIC device, setting up distributor and redistributor
> > attributes, and mapping the guest physical addresses.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> >
> > ---
> >  tools/testing/selftests/kvm/Makefile          |  2 +-
> >  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
> >  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
> >  3 files changed, 82 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 5476a8ddef60..8342f65c1d96 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -35,7 +35,7 @@ endif
> >
> >  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
> >  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> > -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> > +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
> >  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
> >
> >  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > new file mode 100644
> > index 000000000000..45bbf238147a
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) host specific defines
> > + */
> > +
> > +#ifndef SELFTEST_KVM_VGIC_H
> > +#define SELFTEST_KVM_VGIC_H
> > +
> > +#include <linux/kvm.h>
> > +
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
> > +
> > +#endif /* SELFTEST_KVM_VGIC_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > new file mode 100644
> > index 000000000000..a0e4b986d335
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) v3 host support
> > + */
> > +
> > +#include <linux/kvm.h>
> > +#include <linux/sizes.h>
> > +
> > +#include "kvm_util.h"
> > +
> > +#define VGIC_V3_GICD_SZ              (SZ_64K)
> > +#define VGIC_V3_GICR_SZ              (2 * SZ_64K)
> > +
> > +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> > +     (((uint64_t)(count) << 52) | \
> > +     ((uint64_t)((base) >> 16) << 16) | \
> > +     ((uint64_t)(flags) << 12) | \
> > +     index)
>
> This could go in vgic.h allowing us to share it in aarch64/vgic_init.c,
> where we already have the same definition.
>
Sure, I will move this.

> > +
> > +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> > +{
> > +     unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> > +
> > +     virt_map(vm, addr, addr, n_pages);
> > +}
> > +
> > +/*
> > + * vGIC-v3 default host setup
> > + *
> > + * Input args:
> > + *   vm - KVM VM
> > + *   nr_vcpus - Number of vCPUs for this VM
> > + *   gicd_base_gpa - Guest Physical Address of the Distributor region
> > + *   gicr_base_gpa - Guest Physical Address of the Redistributor region
> > + *
> > + * Output args: None
> > + *
> > + * Return: GIC file-descriptor or negative error code upon failure
> > + *
> > + * The function creates a vGIC-v3 device and maps the distributor and
> > + * redistributor regions of the guest.
> > + */
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
> > +{
> > +     uint64_t redist_attr;
> > +     int gic_fd;
> > +
> > +     TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> > +                     "Invalid number of CPUs: %u\n", nr_vcpus);
>
> TEST_ASSERT(!list_empty(&vm->vcpus), ...) to ensure we've created vcpus
> first. To be really paranoid we could even confirm the number of vcpus in
> the list matches nr_vcpus.
>
Great idea! I'll add these checks too.

Regards
Raghavendra
> > +
> > +     gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> > +     vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
> > +
> > +     redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> > +     vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> > +                             KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> > +
> > +     return gic_fd;
> > +}
> > --
> > 2.33.0.153.gba50c8fa24-goog
>
> Otherwise
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
> Thanks,
> drew
>

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> Implement a simple library to do perform vGIC-v3
> setup from a host of view. This includes creating
> a vGIC device, setting up distributor and redistributor
> attributes, and mapping the guest physical addresses.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> 
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
>  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 5476a8ddef60..8342f65c1d96 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> new file mode 100644
> index 000000000000..45bbf238147a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ARM Generic Interrupt Controller (GIC) host specific defines
> + */
> +
> +#ifndef SELFTEST_KVM_VGIC_H
> +#define SELFTEST_KVM_VGIC_H
> +
> +#include <linux/kvm.h>
> +
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);

Having seen 5 arguments used in the vgic_v3_setup call in the next patch I
came back here to refresh my memory on that parameter. 'slot' is the one I
forgot, but...

> +
> +#endif /* SELFTEST_KVM_VGIC_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> new file mode 100644
> index 000000000000..a0e4b986d335
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM Generic Interrupt Controller (GIC) v3 host support
> + */
> +
> +#include <linux/kvm.h>
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +
> +#define VGIC_V3_GICD_SZ		(SZ_64K)
> +#define VGIC_V3_GICR_SZ		(2 * SZ_64K)
> +
> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> +	(((uint64_t)(count) << 52) | \
> +	((uint64_t)((base) >> 16) << 16) | \
> +	((uint64_t)(flags) << 12) | \
> +	index)
> +
> +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> +{
> +	unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> +
> +	virt_map(vm, addr, addr, n_pages);
> +}
> +
> +/*
> + * vGIC-v3 default host setup
> + *
> + * Input args:
> + *	vm - KVM VM
> + *	nr_vcpus - Number of vCPUs for this VM
> + *	gicd_base_gpa - Guest Physical Address of the Distributor region
> + *	gicr_base_gpa - Guest Physical Address of the Redistributor region
> + *
> + * Output args: None
> + *
> + * Return: GIC file-descriptor or negative error code upon failure
> + *
> + * The function creates a vGIC-v3 device and maps the distributor and
> + * redistributor regions of the guest.
> + */
> +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> +		uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)

...looks like it was forgotten here too.

Thanks,
drew

> +{
> +	uint64_t redist_attr;
> +	int gic_fd;
> +
> +	TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> +			"Invalid number of CPUs: %u\n", nr_vcpus);
> +
> +	gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> +	vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
> +
> +	redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> +	vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> +
> +	kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +				KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +
> +	return gic_fd;
> +}
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Re: [PATCH v3 10/12] KVM: arm64: selftests: Add host support for vGIC

[Raghavendra Rao Ananta]

On Fri, Sep 3, 2021 at 3:52 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:10PM +0000, Raghavendra Rao Ananta wrote:
> > Implement a simple library to do perform vGIC-v3
> > setup from a host of view. This includes creating
> > a vGIC device, setting up distributor and redistributor
> > attributes, and mapping the guest physical addresses.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> >
> > ---
> >  tools/testing/selftests/kvm/Makefile          |  2 +-
> >  .../selftests/kvm/include/aarch64/vgic.h      | 14 ++++
> >  .../testing/selftests/kvm/lib/aarch64/vgic.c  | 67 +++++++++++++++++++
> >  3 files changed, 82 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 5476a8ddef60..8342f65c1d96 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -35,7 +35,7 @@ endif
> >
> >  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
> >  LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> > -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c
> > +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
> >  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
> >
> >  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/vgic.h b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > new file mode 100644
> > index 000000000000..45bbf238147a
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/include/aarch64/vgic.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) host specific defines
> > + */
> > +
> > +#ifndef SELFTEST_KVM_VGIC_H
> > +#define SELFTEST_KVM_VGIC_H
> > +
> > +#include <linux/kvm.h>
> > +
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa, uint32_t slot);
>
> Having seen 5 arguments used in the vgic_v3_setup call in the next patch I
> came back here to refresh my memory on that parameter. 'slot' is the one I
> forgot, but...
>
> > +
> > +#endif /* SELFTEST_KVM_VGIC_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > new file mode 100644
> > index 000000000000..a0e4b986d335
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) v3 host support
> > + */
> > +
> > +#include <linux/kvm.h>
> > +#include <linux/sizes.h>
> > +
> > +#include "kvm_util.h"
> > +
> > +#define VGIC_V3_GICD_SZ              (SZ_64K)
> > +#define VGIC_V3_GICR_SZ              (2 * SZ_64K)
> > +
> > +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) \
> > +     (((uint64_t)(count) << 52) | \
> > +     ((uint64_t)((base) >> 16) << 16) | \
> > +     ((uint64_t)(flags) << 12) | \
> > +     index)
> > +
> > +static void vgic_v3_map(struct kvm_vm *vm, uint64_t addr, unsigned int size)
> > +{
> > +     unsigned int n_pages = DIV_ROUND_UP(size, vm_get_page_size(vm));
> > +
> > +     virt_map(vm, addr, addr, n_pages);
> > +}
> > +
> > +/*
> > + * vGIC-v3 default host setup
> > + *
> > + * Input args:
> > + *   vm - KVM VM
> > + *   nr_vcpus - Number of vCPUs for this VM
> > + *   gicd_base_gpa - Guest Physical Address of the Distributor region
> > + *   gicr_base_gpa - Guest Physical Address of the Redistributor region
> > + *
> > + * Output args: None
> > + *
> > + * Return: GIC file-descriptor or negative error code upon failure
> > + *
> > + * The function creates a vGIC-v3 device and maps the distributor and
> > + * redistributor regions of the guest.
> > + */
> > +int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus,
> > +             uint64_t gicd_base_gpa, uint64_t gicr_base_gpa)
>
> ...looks like it was forgotten here too.
>
You are right. I had 'slot' to try other mechanisms, but I guess I
forgot to clean it up.
Thanks for catching this.

Regards,
Raghavendra

> Thanks,
> drew
>
> > +{
> > +     uint64_t redist_attr;
> > +     int gic_fd;
> > +
> > +     TEST_ASSERT(nr_vcpus <= KVM_MAX_VCPUS,
> > +                     "Invalid number of CPUs: %u\n", nr_vcpus);
> > +
> > +     gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_DIST, &gicd_base_gpa, true);
> > +     vgic_v3_map(vm, gicd_base_gpa, VGIC_V3_GICD_SZ);
> > +
> > +     redist_attr = REDIST_REGION_ATTR_ADDR(nr_vcpus, gicr_base_gpa, 0, 0);
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +                     KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &redist_attr, true);
> > +     vgic_v3_map(vm, gicr_base_gpa, VGIC_V3_GICR_SZ * nr_vcpus);
> > +
> > +     kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> > +                             KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> > +
> > +     return gic_fd;
> > +}
> > --
> > 2.33.0.153.gba50c8fa24-goog
> >
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> >
>

[PATCH v3 11/12] KVM: arm64: selftests: Add arch_timer test

[Raghavendra Rao Ananta]

Add a KVM selftest to validate the arch_timer functionality.
Primarily, the test sets up periodic timer interrupts and
validates the basic architectural expectations upon its receipt.

The test provides command-line options to configure the period
of the timer, number of iterations, and number of vCPUs.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/arch_timer.c        | 351 ++++++++++++++++++
 3 files changed, 353 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 98053d3afbda..c6058df0cd18 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+/aarch64/arch_timer
 /aarch64/debug-exceptions
 /aarch64/get-reg-list
 /aarch64/psci_cpu_on_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 8342f65c1d96..46d43e706b20 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -84,6 +84,7 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
 TEST_GEN_PROGS_x86_64 += steal_time
 TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
 
+TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
new file mode 100644
index 000000000000..1383f33850e9
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -0,0 +1,351 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * arch_timer.c - Tests the aarch64 timer IRQ functionality
+ *
+ * The test validates both the virtual and physical timer IRQs using
+ * CVAL and TVAL registers. This consitutes the four stages in the test.
+ * The guest's main thread configures the timer interrupt for a stage
+ * and waits for it to fire, with a timeout equal to the timer period.
+ * It asserts that the timeout doesn't exceed the timer period.
+ *
+ * On the other hand, upon receipt of an interrupt, the guest's interrupt
+ * handler validates the interrupt by checking if the architectural state
+ * is in compliance with the specifications.
+ *
+ * The test provides command-line options to configure the timer's
+ * period (-p), number of vCPUs (-n), and iterations per stage (-i).
+ *
+ * Copyright (c) 2021, Google LLC.
+ */
+
+#define _GNU_SOURCE
+
+#include <stdlib.h>
+#include <pthread.h>
+#include <linux/kvm.h>
+#include <linux/sizes.h>
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "delay.h"
+#include "arch_timer.h"
+#include "gic.h"
+#include "vgic.h"
+
+#define NR_VCPUS_DEF			4
+#define NR_TEST_ITERS_DEF		5
+#define TIMER_TEST_PERIOD_MS_DEF	10
+#define TIMER_TEST_ERR_MARGIN_US	100
+
+struct test_args {
+	int nr_vcpus;
+	int nr_iter;
+	int timer_period_ms;
+};
+
+static struct test_args test_args = {
+	.nr_vcpus = NR_VCPUS_DEF,
+	.nr_iter = NR_TEST_ITERS_DEF,
+	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
+};
+
+#define msecs_to_usecs(msec)		((msec) * 1000LL)
+
+#define VTIMER_IRQ			27
+#define PTIMER_IRQ			30
+
+#define GICD_BASE_GPA			0x8000000ULL
+#define GICR_BASE_GPA			0x80A0000ULL
+
+enum guest_stage {
+	GUEST_STAGE_VTIMER_CVAL = 1,
+	GUEST_STAGE_VTIMER_TVAL,
+	GUEST_STAGE_PTIMER_CVAL,
+	GUEST_STAGE_PTIMER_TVAL,
+	GUEST_STAGE_MAX,
+};
+
+/* Sahred variables between host and guest */
+struct test_vcpu_shared_data {
+	int nr_iter;
+	enum guest_stage guest_stage;
+	uint64_t xcnt;
+};
+
+struct test_vcpu {
+	uint32_t vcpuid;
+	pthread_t pt_vcpu_run;
+	struct kvm_vm *vm;
+};
+
+static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
+static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
+
+static void
+guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
+{
+	switch (shared_data->guest_stage) {
+	case GUEST_STAGE_VTIMER_CVAL:
+		timer_set_next_cval_ms(VIRTUAL, test_args.timer_period_ms);
+		shared_data->xcnt = timer_get_cntct(VIRTUAL);
+		timer_set_ctl(VIRTUAL, CTL_ENABLE);
+		break;
+	case GUEST_STAGE_VTIMER_TVAL:
+		timer_set_next_tval_ms(VIRTUAL, test_args.timer_period_ms);
+		shared_data->xcnt = timer_get_cntct(VIRTUAL);
+		timer_set_ctl(VIRTUAL, CTL_ENABLE);
+		break;
+	case GUEST_STAGE_PTIMER_CVAL:
+		timer_set_next_cval_ms(PHYSICAL, test_args.timer_period_ms);
+		shared_data->xcnt = timer_get_cntct(PHYSICAL);
+		timer_set_ctl(PHYSICAL, CTL_ENABLE);
+		break;
+	case GUEST_STAGE_PTIMER_TVAL:
+		timer_set_next_tval_ms(PHYSICAL, test_args.timer_period_ms);
+		shared_data->xcnt = timer_get_cntct(PHYSICAL);
+		timer_set_ctl(PHYSICAL, CTL_ENABLE);
+		break;
+	default:
+		GUEST_ASSERT(0);
+	}
+}
+
+static void guest_validate_irq(unsigned int intid,
+				struct test_vcpu_shared_data *shared_data)
+{
+	enum guest_stage stage = shared_data->guest_stage;
+	uint64_t xcnt = 0, xcnt_diff_us, cval = 0;
+	unsigned long xctl = 0;
+	unsigned int timer_irq = 0;
+
+	if (stage == GUEST_STAGE_VTIMER_CVAL ||
+		stage == GUEST_STAGE_VTIMER_TVAL) {
+		xctl = timer_get_ctl(VIRTUAL);
+		timer_set_ctl(VIRTUAL, CTL_IMASK);
+		xcnt = timer_get_cntct(VIRTUAL);
+		cval = timer_get_cval(VIRTUAL);
+		timer_irq = VTIMER_IRQ;
+	} else if (stage == GUEST_STAGE_PTIMER_CVAL ||
+		stage == GUEST_STAGE_PTIMER_TVAL) {
+		xctl = timer_get_ctl(PHYSICAL);
+		timer_set_ctl(PHYSICAL, CTL_IMASK);
+		xcnt = timer_get_cntct(PHYSICAL);
+		cval = timer_get_cval(PHYSICAL);
+		timer_irq = PTIMER_IRQ;
+	} else {
+		GUEST_ASSERT(0);
+	}
+
+	xcnt_diff_us = cycles_to_usec(xcnt - shared_data->xcnt);
+
+	/* Make sure we are dealing with the correct timer IRQ */
+	GUEST_ASSERT_2(intid == timer_irq, intid, timer_irq);
+
+	/* Basic 'timer codition met' check */
+	GUEST_ASSERT_3(xcnt >= cval, xcnt, cval, xcnt_diff_us);
+	GUEST_ASSERT_1(xctl & CTL_ISTATUS, xctl);
+}
+
+static void guest_irq_handler(struct ex_regs *regs)
+{
+	unsigned int intid = gic_get_and_ack_irq();
+	uint32_t cpu = get_vcpuid();
+	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
+
+	guest_validate_irq(intid, shared_data);
+
+	WRITE_ONCE(shared_data->nr_iter, shared_data->nr_iter + 1);
+
+	gic_set_eoi(intid);
+}
+
+static void guest_run_stage(struct test_vcpu_shared_data *shared_data,
+				enum guest_stage stage)
+{
+	uint32_t irq_iter, config_iter;
+
+	shared_data->guest_stage = stage;
+	shared_data->nr_iter = 0;
+
+	for (config_iter = 0; config_iter < test_args.nr_iter; config_iter++) {
+		/* Setup the next interrupt */
+		guest_configure_timer_action(shared_data);
+
+		/* Setup a timeout for the interrupt to arrive */
+		udelay(msecs_to_usecs(test_args.timer_period_ms) +
+			TIMER_TEST_ERR_MARGIN_US);
+
+		irq_iter = READ_ONCE(shared_data->nr_iter);
+		GUEST_ASSERT_2(config_iter + 1 == irq_iter,
+				config_iter + 1, irq_iter);
+	};
+}
+
+static void guest_code(void)
+{
+	uint32_t cpu = get_vcpuid();
+	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
+
+	local_irq_disable();
+
+	gic_init(GIC_V3, test_args.nr_vcpus,
+		(void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
+
+	timer_set_ctl(VIRTUAL, CTL_IMASK);
+	timer_set_ctl(PHYSICAL, CTL_IMASK);
+
+	gic_irq_enable(VTIMER_IRQ);
+	gic_irq_enable(PTIMER_IRQ);
+	local_irq_enable();
+
+	guest_run_stage(shared_data, GUEST_STAGE_VTIMER_CVAL);
+	guest_run_stage(shared_data, GUEST_STAGE_VTIMER_TVAL);
+	guest_run_stage(shared_data, GUEST_STAGE_PTIMER_CVAL);
+	guest_run_stage(shared_data, GUEST_STAGE_PTIMER_TVAL);
+
+	GUEST_DONE();
+}
+
+static void *test_vcpu_run(void *arg)
+{
+	struct ucall uc;
+	struct test_vcpu *vcpu = arg;
+	struct kvm_vm *vm = vcpu->vm;
+	uint32_t vcpuid = vcpu->vcpuid;
+	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[vcpuid];
+
+	vcpu_run(vm, vcpuid);
+
+	switch (get_ucall(vm, vcpuid, &uc)) {
+	case UCALL_SYNC:
+	case UCALL_DONE:
+		break;
+	case UCALL_ABORT:
+		sync_global_from_guest(vm, *shared_data);
+		TEST_ASSERT(false,
+			"%s at %s:%ld\n\tvalues: %lu, %lu; %lu, vcpu: %u; stage: %u; iter: %u",
+			(const char *)uc.args[0], __FILE__, uc.args[1],
+			uc.args[2], uc.args[3], uc.args[4], vcpuid,
+			shared_data->guest_stage, shared_data->nr_iter);
+		break;
+	default:
+		TEST_FAIL("Unexpected guest exit\n");
+	}
+
+	return NULL;
+}
+
+static void test_run(struct kvm_vm *vm)
+{
+	int i, ret;
+
+	for (i = 0; i < test_args.nr_vcpus; i++) {
+		ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
+				test_vcpu_run, &test_vcpu[i]);
+		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
+	}
+
+	for (i = 0; i < test_args.nr_vcpus; i++)
+		pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
+}
+
+static struct kvm_vm *test_vm_create(void)
+{
+	struct kvm_vm *vm;
+	unsigned int i;
+	int nr_vcpus = test_args.nr_vcpus;
+
+	vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
+
+	vm_init_descriptor_tables(vm);
+	vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
+
+	for (i = 0; i < nr_vcpus; i++) {
+		vcpu_init_descriptor_tables(vm, i);
+
+		test_vcpu[i].vcpuid = i;
+		test_vcpu[i].vm = vm;
+	}
+
+	ucall_init(vm, NULL);
+	vgic_v3_setup(vm, nr_vcpus, GICD_BASE_GPA, GICR_BASE_GPA, 1);
+
+	/* Make all the test's cmdline args visible to the guest */
+	sync_global_to_guest(vm, test_args);
+
+	return vm;
+}
+
+static void test_print_help(char *name)
+{
+	pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
+		name);
+	pr_info("\t-n: Number of vCPUs to configure (default: %u; max: %u)\n",
+		NR_VCPUS_DEF, KVM_MAX_VCPUS);
+	pr_info("\t-i: Number of iterations per stage (default: %u)\n",
+		NR_TEST_ITERS_DEF);
+	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
+		TIMER_TEST_PERIOD_MS_DEF);
+	pr_info("\t-h: print this help screen\n");
+}
+
+static bool parse_args(int argc, char *argv[])
+{
+	int opt;
+
+	while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
+		switch (opt) {
+		case 'n':
+			test_args.nr_vcpus = atoi(optarg);
+			if (test_args.nr_vcpus <= 0) {
+				pr_info("Positive value needed for -n\n");
+				goto err;
+			} else if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
+				pr_info("Max allowed vCPUs: %u\n",
+					KVM_MAX_VCPUS);
+				goto err;
+			}
+			break;
+		case 'i':
+			test_args.nr_iter = atoi(optarg);
+			if (test_args.nr_iter <= 0) {
+				pr_info("Positive value needed for -i\n");
+				goto err;
+			}
+			break;
+		case 'p':
+			test_args.timer_period_ms = atoi(optarg);
+			if (test_args.timer_period_ms <= 0) {
+				pr_info("Positive value needed for -p\n");
+				goto err;
+			}
+			break;
+		case 'h':
+		default:
+			goto err;
+		}
+	}
+
+	return true;
+
+err:
+	test_print_help(argv[0]);
+	return false;
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vm *vm;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	if (!parse_args(argc, argv))
+		exit(KSFT_SKIP);
+
+	vm = test_vm_create();
+	test_run(vm);
+	kvm_vm_free(vm);
+
+	return 0;
+}
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 11/12] KVM: arm64: selftests: Add arch_timer test

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:11PM +0000, Raghavendra Rao Ananta wrote:
> Add a KVM selftest to validate the arch_timer functionality.
> Primarily, the test sets up periodic timer interrupts and
> validates the basic architectural expectations upon its receipt.
> 
> The test provides command-line options to configure the period
> of the timer, number of iterations, and number of vCPUs.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/aarch64/arch_timer.c        | 351 ++++++++++++++++++
>  3 files changed, 353 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 98053d3afbda..c6058df0cd18 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +/aarch64/arch_timer
>  /aarch64/debug-exceptions
>  /aarch64/get-reg-list
>  /aarch64/psci_cpu_on_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 8342f65c1d96..46d43e706b20 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -84,6 +84,7 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
>  TEST_GEN_PROGS_x86_64 += steal_time
>  TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
>  
> +TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>  TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> new file mode 100644
> index 000000000000..1383f33850e9
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -0,0 +1,351 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * arch_timer.c - Tests the aarch64 timer IRQ functionality
> + *
> + * The test validates both the virtual and physical timer IRQs using
> + * CVAL and TVAL registers. This consitutes the four stages in the test.
> + * The guest's main thread configures the timer interrupt for a stage
> + * and waits for it to fire, with a timeout equal to the timer period.
> + * It asserts that the timeout doesn't exceed the timer period.
> + *
> + * On the other hand, upon receipt of an interrupt, the guest's interrupt
> + * handler validates the interrupt by checking if the architectural state
> + * is in compliance with the specifications.
> + *
> + * The test provides command-line options to configure the timer's
> + * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> + *
> + * Copyright (c) 2021, Google LLC.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +#include <pthread.h>
> +#include <linux/kvm.h>
> +#include <linux/sizes.h>
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "delay.h"
> +#include "arch_timer.h"
> +#include "gic.h"
> +#include "vgic.h"
> +
> +#define NR_VCPUS_DEF			4
> +#define NR_TEST_ITERS_DEF		5
> +#define TIMER_TEST_PERIOD_MS_DEF	10
> +#define TIMER_TEST_ERR_MARGIN_US	100
> +
> +struct test_args {
> +	int nr_vcpus;
> +	int nr_iter;
> +	int timer_period_ms;
> +};
> +
> +static struct test_args test_args = {
> +	.nr_vcpus = NR_VCPUS_DEF,
> +	.nr_iter = NR_TEST_ITERS_DEF,
> +	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> +};
> +
> +#define msecs_to_usecs(msec)		((msec) * 1000LL)
> +
> +#define VTIMER_IRQ			27
> +#define PTIMER_IRQ			30
> +
> +#define GICD_BASE_GPA			0x8000000ULL
> +#define GICR_BASE_GPA			0x80A0000ULL
> +
> +enum guest_stage {
> +	GUEST_STAGE_VTIMER_CVAL = 1,
> +	GUEST_STAGE_VTIMER_TVAL,
> +	GUEST_STAGE_PTIMER_CVAL,
> +	GUEST_STAGE_PTIMER_TVAL,
> +	GUEST_STAGE_MAX,
> +};
> +
> +/* Sahred variables between host and guest */

Shared

> +struct test_vcpu_shared_data {
> +	int nr_iter;
> +	enum guest_stage guest_stage;
> +	uint64_t xcnt;
> +};
> +
> +struct test_vcpu {
> +	uint32_t vcpuid;
> +	pthread_t pt_vcpu_run;
> +	struct kvm_vm *vm;
> +};
> +
> +static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
> +static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
> +
> +static void
> +guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
> +{
> +	switch (shared_data->guest_stage) {
> +	case GUEST_STAGE_VTIMER_CVAL:
> +		timer_set_next_cval_ms(VIRTUAL, test_args.timer_period_ms);
> +		shared_data->xcnt = timer_get_cntct(VIRTUAL);
> +		timer_set_ctl(VIRTUAL, CTL_ENABLE);
> +		break;
> +	case GUEST_STAGE_VTIMER_TVAL:
> +		timer_set_next_tval_ms(VIRTUAL, test_args.timer_period_ms);
> +		shared_data->xcnt = timer_get_cntct(VIRTUAL);
> +		timer_set_ctl(VIRTUAL, CTL_ENABLE);
> +		break;
> +	case GUEST_STAGE_PTIMER_CVAL:
> +		timer_set_next_cval_ms(PHYSICAL, test_args.timer_period_ms);
> +		shared_data->xcnt = timer_get_cntct(PHYSICAL);
> +		timer_set_ctl(PHYSICAL, CTL_ENABLE);
> +		break;
> +	case GUEST_STAGE_PTIMER_TVAL:
> +		timer_set_next_tval_ms(PHYSICAL, test_args.timer_period_ms);
> +		shared_data->xcnt = timer_get_cntct(PHYSICAL);
> +		timer_set_ctl(PHYSICAL, CTL_ENABLE);
> +		break;

Since we divide the stages up for vtimer and ptimer, then I'm not sure we
need the wrapper fuctions for timer register get/set with the vtimer and
ptimer switches too.

> +	default:
> +		GUEST_ASSERT(0);
> +	}
> +}
> +
> +static void guest_validate_irq(unsigned int intid,
> +				struct test_vcpu_shared_data *shared_data)
> +{
> +	enum guest_stage stage = shared_data->guest_stage;
> +	uint64_t xcnt = 0, xcnt_diff_us, cval = 0;
> +	unsigned long xctl = 0;
> +	unsigned int timer_irq = 0;
> +
> +	if (stage == GUEST_STAGE_VTIMER_CVAL ||
> +		stage == GUEST_STAGE_VTIMER_TVAL) {
> +		xctl = timer_get_ctl(VIRTUAL);
> +		timer_set_ctl(VIRTUAL, CTL_IMASK);
> +		xcnt = timer_get_cntct(VIRTUAL);
> +		cval = timer_get_cval(VIRTUAL);
> +		timer_irq = VTIMER_IRQ;
> +	} else if (stage == GUEST_STAGE_PTIMER_CVAL ||
> +		stage == GUEST_STAGE_PTIMER_TVAL) {
> +		xctl = timer_get_ctl(PHYSICAL);
> +		timer_set_ctl(PHYSICAL, CTL_IMASK);
> +		xcnt = timer_get_cntct(PHYSICAL);
> +		cval = timer_get_cval(PHYSICAL);
> +		timer_irq = PTIMER_IRQ;
> +	} else {
> +		GUEST_ASSERT(0);
> +	}
> +
> +	xcnt_diff_us = cycles_to_usec(xcnt - shared_data->xcnt);
> +
> +	/* Make sure we are dealing with the correct timer IRQ */
> +	GUEST_ASSERT_2(intid == timer_irq, intid, timer_irq);
> +
> +	/* Basic 'timer codition met' check */

condition

> +	GUEST_ASSERT_3(xcnt >= cval, xcnt, cval, xcnt_diff_us);
> +	GUEST_ASSERT_1(xctl & CTL_ISTATUS, xctl);
> +}
> +
> +static void guest_irq_handler(struct ex_regs *regs)
> +{
> +	unsigned int intid = gic_get_and_ack_irq();
> +	uint32_t cpu = get_vcpuid();
> +	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
> +
> +	guest_validate_irq(intid, shared_data);
> +
> +	WRITE_ONCE(shared_data->nr_iter, shared_data->nr_iter + 1);
> +
> +	gic_set_eoi(intid);
> +}
> +
> +static void guest_run_stage(struct test_vcpu_shared_data *shared_data,
> +				enum guest_stage stage)
> +{
> +	uint32_t irq_iter, config_iter;
> +
> +	shared_data->guest_stage = stage;
> +	shared_data->nr_iter = 0;
> +
> +	for (config_iter = 0; config_iter < test_args.nr_iter; config_iter++) {
> +		/* Setup the next interrupt */
> +		guest_configure_timer_action(shared_data);
> +
> +		/* Setup a timeout for the interrupt to arrive */
> +		udelay(msecs_to_usecs(test_args.timer_period_ms) +
> +			TIMER_TEST_ERR_MARGIN_US);
> +
> +		irq_iter = READ_ONCE(shared_data->nr_iter);
> +		GUEST_ASSERT_2(config_iter + 1 == irq_iter,
> +				config_iter + 1, irq_iter);
> +	};

extra ;

> +}
> +
> +static void guest_code(void)
> +{
> +	uint32_t cpu = get_vcpuid();
> +	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
> +
> +	local_irq_disable();
> +
> +	gic_init(GIC_V3, test_args.nr_vcpus,
> +		(void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
> +
> +	timer_set_ctl(VIRTUAL, CTL_IMASK);
> +	timer_set_ctl(PHYSICAL, CTL_IMASK);
> +
> +	gic_irq_enable(VTIMER_IRQ);
> +	gic_irq_enable(PTIMER_IRQ);
> +	local_irq_enable();
> +
> +	guest_run_stage(shared_data, GUEST_STAGE_VTIMER_CVAL);
> +	guest_run_stage(shared_data, GUEST_STAGE_VTIMER_TVAL);
> +	guest_run_stage(shared_data, GUEST_STAGE_PTIMER_CVAL);
> +	guest_run_stage(shared_data, GUEST_STAGE_PTIMER_TVAL);
> +
> +	GUEST_DONE();
> +}
> +
> +static void *test_vcpu_run(void *arg)
> +{
> +	struct ucall uc;
> +	struct test_vcpu *vcpu = arg;
> +	struct kvm_vm *vm = vcpu->vm;
> +	uint32_t vcpuid = vcpu->vcpuid;
> +	struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[vcpuid];
> +
> +	vcpu_run(vm, vcpuid);
> +
> +	switch (get_ucall(vm, vcpuid, &uc)) {
> +	case UCALL_SYNC:
> +	case UCALL_DONE:
> +		break;
> +	case UCALL_ABORT:
> +		sync_global_from_guest(vm, *shared_data);
> +		TEST_ASSERT(false,

TEST_FAIL(fmt, ...) can be used.

> +			"%s at %s:%ld\n\tvalues: %lu, %lu; %lu, vcpu: %u; stage: %u; iter: %u",
> +			(const char *)uc.args[0], __FILE__, uc.args[1],
> +			uc.args[2], uc.args[3], uc.args[4], vcpuid,
> +			shared_data->guest_stage, shared_data->nr_iter);
> +		break;
> +	default:
> +		TEST_FAIL("Unexpected guest exit\n");
> +	}
> +
> +	return NULL;
> +}
> +
> +static void test_run(struct kvm_vm *vm)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < test_args.nr_vcpus; i++) {
> +		ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
> +				test_vcpu_run, &test_vcpu[i]);
> +		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
> +	}
> +
> +	for (i = 0; i < test_args.nr_vcpus; i++)
> +		pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
> +}
> +
> +static struct kvm_vm *test_vm_create(void)
> +{
> +	struct kvm_vm *vm;
> +	unsigned int i;
> +	int nr_vcpus = test_args.nr_vcpus;
> +
> +	vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
> +
> +	vm_init_descriptor_tables(vm);
> +	vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
> +
> +	for (i = 0; i < nr_vcpus; i++) {
> +		vcpu_init_descriptor_tables(vm, i);
> +
> +		test_vcpu[i].vcpuid = i;
> +		test_vcpu[i].vm = vm;
> +	}
> +
> +	ucall_init(vm, NULL);
> +	vgic_v3_setup(vm, nr_vcpus, GICD_BASE_GPA, GICR_BASE_GPA, 1);
> +
> +	/* Make all the test's cmdline args visible to the guest */
> +	sync_global_to_guest(vm, test_args);
> +
> +	return vm;
> +}
> +
> +static void test_print_help(char *name)
> +{
> +	pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
> +		name);
> +	pr_info("\t-n: Number of vCPUs to configure (default: %u; max: %u)\n",
> +		NR_VCPUS_DEF, KVM_MAX_VCPUS);
> +	pr_info("\t-i: Number of iterations per stage (default: %u)\n",
> +		NR_TEST_ITERS_DEF);
> +	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
> +		TIMER_TEST_PERIOD_MS_DEF);
> +	pr_info("\t-h: print this help screen\n");
> +}
> +
> +static bool parse_args(int argc, char *argv[])
> +{
> +	int opt;
> +
> +	while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
> +		switch (opt) {
> +		case 'n':
> +			test_args.nr_vcpus = atoi(optarg);
> +			if (test_args.nr_vcpus <= 0) {
> +				pr_info("Positive value needed for -n\n");
> +				goto err;
> +			} else if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
> +				pr_info("Max allowed vCPUs: %u\n",
> +					KVM_MAX_VCPUS);
> +				goto err;
> +			}
> +			break;
> +		case 'i':
> +			test_args.nr_iter = atoi(optarg);
> +			if (test_args.nr_iter <= 0) {
> +				pr_info("Positive value needed for -i\n");
> +				goto err;
> +			}
> +			break;
> +		case 'p':
> +			test_args.timer_period_ms = atoi(optarg);
> +			if (test_args.timer_period_ms <= 0) {
> +				pr_info("Positive value needed for -p\n");
> +				goto err;
> +			}
> +			break;
> +		case 'h':
> +		default:
> +			goto err;
> +		}
> +	}
> +
> +	return true;
> +
> +err:
> +	test_print_help(argv[0]);
> +	return false;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +
> +	/* Tell stdout not to buffer its content */
> +	setbuf(stdout, NULL);
> +
> +	if (!parse_args(argc, argv))
> +		exit(KSFT_SKIP);
> +
> +	vm = test_vm_create();
> +	test_run(vm);
> +	kvm_vm_free(vm);
> +
> +	return 0;
> +}
> -- 
> 2.33.0.153.gba50c8fa24-goog

Besides the nits,

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

Thanks,
drew

Re: [PATCH v3 11/12] KVM: arm64: selftests: Add arch_timer test

[Raghavendra Rao Ananta]

On Fri, Sep 3, 2021 at 3:48 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:11PM +0000, Raghavendra Rao Ananta wrote:
> > Add a KVM selftest to validate the arch_timer functionality.
> > Primarily, the test sets up periodic timer interrupts and
> > validates the basic architectural expectations upon its receipt.
> >
> > The test provides command-line options to configure the period
> > of the timer, number of iterations, and number of vCPUs.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> >  tools/testing/selftests/kvm/.gitignore        |   1 +
> >  tools/testing/selftests/kvm/Makefile          |   1 +
> >  .../selftests/kvm/aarch64/arch_timer.c        | 351 ++++++++++++++++++
> >  3 files changed, 353 insertions(+)
> >  create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer.c
> >
> > diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> > index 98053d3afbda..c6058df0cd18 100644
> > --- a/tools/testing/selftests/kvm/.gitignore
> > +++ b/tools/testing/selftests/kvm/.gitignore
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +/aarch64/arch_timer
> >  /aarch64/debug-exceptions
> >  /aarch64/get-reg-list
> >  /aarch64/psci_cpu_on_test
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 8342f65c1d96..46d43e706b20 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -84,6 +84,7 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
> >  TEST_GEN_PROGS_x86_64 += steal_time
> >  TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
> >
> > +TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
> >  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
> >  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> >  TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test
> > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > new file mode 100644
> > index 000000000000..1383f33850e9
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > @@ -0,0 +1,351 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * arch_timer.c - Tests the aarch64 timer IRQ functionality
> > + *
> > + * The test validates both the virtual and physical timer IRQs using
> > + * CVAL and TVAL registers. This consitutes the four stages in the test.
> > + * The guest's main thread configures the timer interrupt for a stage
> > + * and waits for it to fire, with a timeout equal to the timer period.
> > + * It asserts that the timeout doesn't exceed the timer period.
> > + *
> > + * On the other hand, upon receipt of an interrupt, the guest's interrupt
> > + * handler validates the interrupt by checking if the architectural state
> > + * is in compliance with the specifications.
> > + *
> > + * The test provides command-line options to configure the timer's
> > + * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> > + *
> > + * Copyright (c) 2021, Google LLC.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <stdlib.h>
> > +#include <pthread.h>
> > +#include <linux/kvm.h>
> > +#include <linux/sizes.h>
> > +
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "delay.h"
> > +#include "arch_timer.h"
> > +#include "gic.h"
> > +#include "vgic.h"
> > +
> > +#define NR_VCPUS_DEF                 4
> > +#define NR_TEST_ITERS_DEF            5
> > +#define TIMER_TEST_PERIOD_MS_DEF     10
> > +#define TIMER_TEST_ERR_MARGIN_US     100
> > +
> > +struct test_args {
> > +     int nr_vcpus;
> > +     int nr_iter;
> > +     int timer_period_ms;
> > +};
> > +
> > +static struct test_args test_args = {
> > +     .nr_vcpus = NR_VCPUS_DEF,
> > +     .nr_iter = NR_TEST_ITERS_DEF,
> > +     .timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> > +};
> > +
> > +#define msecs_to_usecs(msec)         ((msec) * 1000LL)
> > +
> > +#define VTIMER_IRQ                   27
> > +#define PTIMER_IRQ                   30
> > +
> > +#define GICD_BASE_GPA                        0x8000000ULL
> > +#define GICR_BASE_GPA                        0x80A0000ULL
> > +
> > +enum guest_stage {
> > +     GUEST_STAGE_VTIMER_CVAL = 1,
> > +     GUEST_STAGE_VTIMER_TVAL,
> > +     GUEST_STAGE_PTIMER_CVAL,
> > +     GUEST_STAGE_PTIMER_TVAL,
> > +     GUEST_STAGE_MAX,
> > +};
> > +
> > +/* Sahred variables between host and guest */
>
> Shared
>
> > +struct test_vcpu_shared_data {
> > +     int nr_iter;
> > +     enum guest_stage guest_stage;
> > +     uint64_t xcnt;
> > +};
> > +
> > +struct test_vcpu {
> > +     uint32_t vcpuid;
> > +     pthread_t pt_vcpu_run;
> > +     struct kvm_vm *vm;
> > +};
> > +
> > +static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
> > +static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
> > +
> > +static void
> > +guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
> > +{
> > +     switch (shared_data->guest_stage) {
> > +     case GUEST_STAGE_VTIMER_CVAL:
> > +             timer_set_next_cval_ms(VIRTUAL, test_args.timer_period_ms);
> > +             shared_data->xcnt = timer_get_cntct(VIRTUAL);
> > +             timer_set_ctl(VIRTUAL, CTL_ENABLE);
> > +             break;
> > +     case GUEST_STAGE_VTIMER_TVAL:
> > +             timer_set_next_tval_ms(VIRTUAL, test_args.timer_period_ms);
> > +             shared_data->xcnt = timer_get_cntct(VIRTUAL);
> > +             timer_set_ctl(VIRTUAL, CTL_ENABLE);
> > +             break;
> > +     case GUEST_STAGE_PTIMER_CVAL:
> > +             timer_set_next_cval_ms(PHYSICAL, test_args.timer_period_ms);
> > +             shared_data->xcnt = timer_get_cntct(PHYSICAL);
> > +             timer_set_ctl(PHYSICAL, CTL_ENABLE);
> > +             break;
> > +     case GUEST_STAGE_PTIMER_TVAL:
> > +             timer_set_next_tval_ms(PHYSICAL, test_args.timer_period_ms);
> > +             shared_data->xcnt = timer_get_cntct(PHYSICAL);
> > +             timer_set_ctl(PHYSICAL, CTL_ENABLE);
> > +             break;
>
> Since we divide the stages up for vtimer and ptimer, then I'm not sure we
> need the wrapper fuctions for timer register get/set with the vtimer and
> ptimer switches too.
>
I understand it's a little redundant. But like you saw in the
arch_timer framework patch,
the helper functions to program the cval, or tval, or any others to
follow, can be made very slim.

Thanks for catching the typos. Will fix them.

Regards,
Raghavendra
> > +     default:
> > +             GUEST_ASSERT(0);
> > +     }
> > +}
> > +
> > +static void guest_validate_irq(unsigned int intid,
> > +                             struct test_vcpu_shared_data *shared_data)
> > +{
> > +     enum guest_stage stage = shared_data->guest_stage;
> > +     uint64_t xcnt = 0, xcnt_diff_us, cval = 0;
> > +     unsigned long xctl = 0;
> > +     unsigned int timer_irq = 0;
> > +
> > +     if (stage == GUEST_STAGE_VTIMER_CVAL ||
> > +             stage == GUEST_STAGE_VTIMER_TVAL) {
> > +             xctl = timer_get_ctl(VIRTUAL);
> > +             timer_set_ctl(VIRTUAL, CTL_IMASK);
> > +             xcnt = timer_get_cntct(VIRTUAL);
> > +             cval = timer_get_cval(VIRTUAL);
> > +             timer_irq = VTIMER_IRQ;
> > +     } else if (stage == GUEST_STAGE_PTIMER_CVAL ||
> > +             stage == GUEST_STAGE_PTIMER_TVAL) {
> > +             xctl = timer_get_ctl(PHYSICAL);
> > +             timer_set_ctl(PHYSICAL, CTL_IMASK);
> > +             xcnt = timer_get_cntct(PHYSICAL);
> > +             cval = timer_get_cval(PHYSICAL);
> > +             timer_irq = PTIMER_IRQ;
> > +     } else {
> > +             GUEST_ASSERT(0);
> > +     }
> > +
> > +     xcnt_diff_us = cycles_to_usec(xcnt - shared_data->xcnt);
> > +
> > +     /* Make sure we are dealing with the correct timer IRQ */
> > +     GUEST_ASSERT_2(intid == timer_irq, intid, timer_irq);
> > +
> > +     /* Basic 'timer codition met' check */
>
> condition
>
> > +     GUEST_ASSERT_3(xcnt >= cval, xcnt, cval, xcnt_diff_us);
> > +     GUEST_ASSERT_1(xctl & CTL_ISTATUS, xctl);
> > +}
> > +
> > +static void guest_irq_handler(struct ex_regs *regs)
> > +{
> > +     unsigned int intid = gic_get_and_ack_irq();
> > +     uint32_t cpu = get_vcpuid();
> > +     struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
> > +
> > +     guest_validate_irq(intid, shared_data);
> > +
> > +     WRITE_ONCE(shared_data->nr_iter, shared_data->nr_iter + 1);
> > +
> > +     gic_set_eoi(intid);
> > +}
> > +
> > +static void guest_run_stage(struct test_vcpu_shared_data *shared_data,
> > +                             enum guest_stage stage)
> > +{
> > +     uint32_t irq_iter, config_iter;
> > +
> > +     shared_data->guest_stage = stage;
> > +     shared_data->nr_iter = 0;
> > +
> > +     for (config_iter = 0; config_iter < test_args.nr_iter; config_iter++) {
> > +             /* Setup the next interrupt */
> > +             guest_configure_timer_action(shared_data);
> > +
> > +             /* Setup a timeout for the interrupt to arrive */
> > +             udelay(msecs_to_usecs(test_args.timer_period_ms) +
> > +                     TIMER_TEST_ERR_MARGIN_US);
> > +
> > +             irq_iter = READ_ONCE(shared_data->nr_iter);
> > +             GUEST_ASSERT_2(config_iter + 1 == irq_iter,
> > +                             config_iter + 1, irq_iter);
> > +     };
>
> extra ;
>
> > +}
> > +
> > +static void guest_code(void)
> > +{
> > +     uint32_t cpu = get_vcpuid();
> > +     struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
> > +
> > +     local_irq_disable();
> > +
> > +     gic_init(GIC_V3, test_args.nr_vcpus,
> > +             (void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
> > +
> > +     timer_set_ctl(VIRTUAL, CTL_IMASK);
> > +     timer_set_ctl(PHYSICAL, CTL_IMASK);
> > +
> > +     gic_irq_enable(VTIMER_IRQ);
> > +     gic_irq_enable(PTIMER_IRQ);
> > +     local_irq_enable();
> > +
> > +     guest_run_stage(shared_data, GUEST_STAGE_VTIMER_CVAL);
> > +     guest_run_stage(shared_data, GUEST_STAGE_VTIMER_TVAL);
> > +     guest_run_stage(shared_data, GUEST_STAGE_PTIMER_CVAL);
> > +     guest_run_stage(shared_data, GUEST_STAGE_PTIMER_TVAL);
> > +
> > +     GUEST_DONE();
> > +}
> > +
> > +static void *test_vcpu_run(void *arg)
> > +{
> > +     struct ucall uc;
> > +     struct test_vcpu *vcpu = arg;
> > +     struct kvm_vm *vm = vcpu->vm;
> > +     uint32_t vcpuid = vcpu->vcpuid;
> > +     struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[vcpuid];
> > +
> > +     vcpu_run(vm, vcpuid);
> > +
> > +     switch (get_ucall(vm, vcpuid, &uc)) {
> > +     case UCALL_SYNC:
> > +     case UCALL_DONE:
> > +             break;
> > +     case UCALL_ABORT:
> > +             sync_global_from_guest(vm, *shared_data);
> > +             TEST_ASSERT(false,
>
> TEST_FAIL(fmt, ...) can be used.
>
> > +                     "%s at %s:%ld\n\tvalues: %lu, %lu; %lu, vcpu: %u; stage: %u; iter: %u",
> > +                     (const char *)uc.args[0], __FILE__, uc.args[1],
> > +                     uc.args[2], uc.args[3], uc.args[4], vcpuid,
> > +                     shared_data->guest_stage, shared_data->nr_iter);
> > +             break;
> > +     default:
> > +             TEST_FAIL("Unexpected guest exit\n");
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +static void test_run(struct kvm_vm *vm)
> > +{
> > +     int i, ret;
> > +
> > +     for (i = 0; i < test_args.nr_vcpus; i++) {
> > +             ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
> > +                             test_vcpu_run, &test_vcpu[i]);
> > +             TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
> > +     }
> > +
> > +     for (i = 0; i < test_args.nr_vcpus; i++)
> > +             pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
> > +}
> > +
> > +static struct kvm_vm *test_vm_create(void)
> > +{
> > +     struct kvm_vm *vm;
> > +     unsigned int i;
> > +     int nr_vcpus = test_args.nr_vcpus;
> > +
> > +     vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
> > +
> > +     vm_init_descriptor_tables(vm);
> > +     vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
> > +
> > +     for (i = 0; i < nr_vcpus; i++) {
> > +             vcpu_init_descriptor_tables(vm, i);
> > +
> > +             test_vcpu[i].vcpuid = i;
> > +             test_vcpu[i].vm = vm;
> > +     }
> > +
> > +     ucall_init(vm, NULL);
> > +     vgic_v3_setup(vm, nr_vcpus, GICD_BASE_GPA, GICR_BASE_GPA, 1);
> > +
> > +     /* Make all the test's cmdline args visible to the guest */
> > +     sync_global_to_guest(vm, test_args);
> > +
> > +     return vm;
> > +}
> > +
> > +static void test_print_help(char *name)
> > +{
> > +     pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
> > +             name);
> > +     pr_info("\t-n: Number of vCPUs to configure (default: %u; max: %u)\n",
> > +             NR_VCPUS_DEF, KVM_MAX_VCPUS);
> > +     pr_info("\t-i: Number of iterations per stage (default: %u)\n",
> > +             NR_TEST_ITERS_DEF);
> > +     pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
> > +             TIMER_TEST_PERIOD_MS_DEF);
> > +     pr_info("\t-h: print this help screen\n");
> > +}
> > +
> > +static bool parse_args(int argc, char *argv[])
> > +{
> > +     int opt;
> > +
> > +     while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
> > +             switch (opt) {
> > +             case 'n':
> > +                     test_args.nr_vcpus = atoi(optarg);
> > +                     if (test_args.nr_vcpus <= 0) {
> > +                             pr_info("Positive value needed for -n\n");
> > +                             goto err;
> > +                     } else if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
> > +                             pr_info("Max allowed vCPUs: %u\n",
> > +                                     KVM_MAX_VCPUS);
> > +                             goto err;
> > +                     }
> > +                     break;
> > +             case 'i':
> > +                     test_args.nr_iter = atoi(optarg);
> > +                     if (test_args.nr_iter <= 0) {
> > +                             pr_info("Positive value needed for -i\n");
> > +                             goto err;
> > +                     }
> > +                     break;
> > +             case 'p':
> > +                     test_args.timer_period_ms = atoi(optarg);
> > +                     if (test_args.timer_period_ms <= 0) {
> > +                             pr_info("Positive value needed for -p\n");
> > +                             goto err;
> > +                     }
> > +                     break;
> > +             case 'h':
> > +             default:
> > +                     goto err;
> > +             }
> > +     }
> > +
> > +     return true;
> > +
> > +err:
> > +     test_print_help(argv[0]);
> > +     return false;
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +     struct kvm_vm *vm;
> > +
> > +     /* Tell stdout not to buffer its content */
> > +     setbuf(stdout, NULL);
> > +
> > +     if (!parse_args(argc, argv))
> > +             exit(KSFT_SKIP);
> > +
> > +     vm = test_vm_create();
> > +     test_run(vm);
> > +     kvm_vm_free(vm);
> > +
> > +     return 0;
> > +}
> > --
> > 2.33.0.153.gba50c8fa24-goog
>
> Besides the nits,
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
> Thanks,
> drew
>

[PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Raghavendra Rao Ananta]

Since the timer stack (hardware and KVM) is per-CPU, there
are potential chances for races to occur when the scheduler
decides to migrate a vCPU thread to a different physical CPU.
Hence, include an option to stress-test this part as well by
forcing the vCPUs to migrate across physical CPUs in the
system at a particular rate.

Originally, the bug for the fix with commit 3134cc8beb69d0d
("KVM: arm64: vgic: Resample HW pending state on deactivation")
was discovered using arch_timer test with vCPU migrations and
can be easily reproduced.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index 1383f33850e9..de246c7afab2 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -14,6 +14,8 @@
  *
  * The test provides command-line options to configure the timer's
  * period (-p), number of vCPUs (-n), and iterations per stage (-i).
+ * To stress-test the timer stack even more, an option to migrate the
+ * vCPUs across pCPUs (-m), at a particular rate, is also provided.
  *
  * Copyright (c) 2021, Google LLC.
  */
@@ -24,6 +26,8 @@
 #include <pthread.h>
 #include <linux/kvm.h>
 #include <linux/sizes.h>
+#include <linux/bitmap.h>
+#include <sys/sysinfo.h>
 
 #include "kvm_util.h"
 #include "processor.h"
@@ -41,12 +45,14 @@ struct test_args {
 	int nr_vcpus;
 	int nr_iter;
 	int timer_period_ms;
+	int migration_freq_ms;
 };
 
 static struct test_args test_args = {
 	.nr_vcpus = NR_VCPUS_DEF,
 	.nr_iter = NR_TEST_ITERS_DEF,
 	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
+	.migration_freq_ms = 0,		/* Turn off migrations by default */
 };
 
 #define msecs_to_usecs(msec)		((msec) * 1000LL)
@@ -81,6 +87,9 @@ struct test_vcpu {
 static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
 static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
 
+static unsigned long *vcpu_done_map;
+static pthread_mutex_t vcpu_done_map_lock;
+
 static void
 guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
 {
@@ -216,6 +225,11 @@ static void *test_vcpu_run(void *arg)
 
 	vcpu_run(vm, vcpuid);
 
+	/* Currently, any exit from guest is an indication of completion */
+	pthread_mutex_lock(&vcpu_done_map_lock);
+	set_bit(vcpuid, vcpu_done_map);
+	pthread_mutex_unlock(&vcpu_done_map_lock);
+
 	switch (get_ucall(vm, vcpuid, &uc)) {
 	case UCALL_SYNC:
 	case UCALL_DONE:
@@ -235,9 +249,73 @@ static void *test_vcpu_run(void *arg)
 	return NULL;
 }
 
+static uint32_t test_get_pcpu(void)
+{
+	uint32_t pcpu;
+	unsigned int nproc_conf;
+	cpu_set_t online_cpuset;
+
+	nproc_conf = get_nprocs_conf();
+	sched_getaffinity(0, sizeof(cpu_set_t), &online_cpuset);
+
+	/* Randomly find an available pCPU to place a vCPU on */
+	do {
+		pcpu = rand() % nproc_conf;
+	} while (!CPU_ISSET(pcpu, &online_cpuset));
+
+	return pcpu;
+}
+static int test_migrate_vcpu(struct test_vcpu *vcpu)
+{
+	int ret;
+	cpu_set_t cpuset;
+	uint32_t new_pcpu = test_get_pcpu();
+
+	CPU_ZERO(&cpuset);
+	CPU_SET(new_pcpu, &cpuset);
+	ret = pthread_setaffinity_np(vcpu->pt_vcpu_run,
+					sizeof(cpuset), &cpuset);
+
+	/* Allow the error where the vCPU thread is already finished */
+	TEST_ASSERT(ret == 0 || ret == ESRCH,
+			"Failed to migrate the vCPU:%u to pCPU: %u; ret: %d\n",
+			vcpu->vcpuid, new_pcpu, ret);
+
+	return ret;
+}
+static void *test_vcpu_migration(void *arg)
+{
+	unsigned int i, n_done;
+	bool vcpu_done;
+
+	do {
+		usleep(msecs_to_usecs(test_args.migration_freq_ms));
+
+		for (n_done = 0, i = 0; i < test_args.nr_vcpus; i++) {
+			pthread_mutex_lock(&vcpu_done_map_lock);
+			vcpu_done = test_bit(i, vcpu_done_map);
+			pthread_mutex_unlock(&vcpu_done_map_lock);
+
+			if (vcpu_done) {
+				n_done++;
+				continue;
+			}
+
+			test_migrate_vcpu(&test_vcpu[i]);
+		}
+	} while (test_args.nr_vcpus != n_done);
+
+	return NULL;
+}
+
 static void test_run(struct kvm_vm *vm)
 {
 	int i, ret;
+	pthread_t pt_vcpu_migration;
+
+	pthread_mutex_init(&vcpu_done_map_lock, NULL);
+	vcpu_done_map = bitmap_alloc(test_args.nr_vcpus);
+	TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");
 
 	for (i = 0; i < test_args.nr_vcpus; i++) {
 		ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
@@ -245,8 +323,23 @@ static void test_run(struct kvm_vm *vm)
 		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
 	}
 
+	/* Spawn a thread to control the vCPU migrations */
+	if (test_args.migration_freq_ms) {
+		srand(time(NULL));
+
+		ret = pthread_create(&pt_vcpu_migration, NULL,
+					test_vcpu_migration, NULL);
+		TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
+	}
+
+
 	for (i = 0; i < test_args.nr_vcpus; i++)
 		pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
+
+	if (test_args.migration_freq_ms)
+		pthread_join(pt_vcpu_migration, NULL);
+
+	bitmap_free(vcpu_done_map);
 }
 
 static struct kvm_vm *test_vm_create(void)
@@ -286,6 +379,7 @@ static void test_print_help(char *name)
 		NR_TEST_ITERS_DEF);
 	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
 		TIMER_TEST_PERIOD_MS_DEF);
+	pr_info("\t-m: Frequency (in ms) of vCPUs to migrate to different pCPU. 0 to turn off (default: 0)\n");
 	pr_info("\t-h: print this help screen\n");
 }
 
@@ -293,7 +387,7 @@ static bool parse_args(int argc, char *argv[])
 {
 	int opt;
 
-	while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
+	while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
 		switch (opt) {
 		case 'n':
 			test_args.nr_vcpus = atoi(optarg);
@@ -320,6 +414,13 @@ static bool parse_args(int argc, char *argv[])
 				goto err;
 			}
 			break;
+		case 'm':
+			test_args.migration_freq_ms = atoi(optarg);
+			if (test_args.migration_freq_ms < 0) {
+				pr_info("0 or positive value needed for -m\n");
+				goto err;
+			}
+			break;
 		case 'h':
 		default:
 			goto err;
@@ -343,6 +444,11 @@ int main(int argc, char *argv[])
 	if (!parse_args(argc, argv))
 		exit(KSFT_SKIP);
 
+	if (get_nprocs() < 2) {
+		print_skip("At least two physical CPUs needed for vCPU migration");
+		exit(KSFT_SKIP);
+	}
+
 	vm = test_vm_create();
 	test_run(vm);
 	kvm_vm_free(vm);
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Andrew Jones]

On Wed, Sep 01, 2021 at 09:14:12PM +0000, Raghavendra Rao Ananta wrote:
> Since the timer stack (hardware and KVM) is per-CPU, there
> are potential chances for races to occur when the scheduler
> decides to migrate a vCPU thread to a different physical CPU.
> Hence, include an option to stress-test this part as well by
> forcing the vCPUs to migrate across physical CPUs in the
> system at a particular rate.
> 
> Originally, the bug for the fix with commit 3134cc8beb69d0d
> ("KVM: arm64: vgic: Resample HW pending state on deactivation")
> was discovered using arch_timer test with vCPU migrations and
> can be easily reproduced.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
>  .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index 1383f33850e9..de246c7afab2 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -14,6 +14,8 @@
>   *
>   * The test provides command-line options to configure the timer's
>   * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> + * To stress-test the timer stack even more, an option to migrate the
> + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
>   *
>   * Copyright (c) 2021, Google LLC.
>   */
> @@ -24,6 +26,8 @@
>  #include <pthread.h>
>  #include <linux/kvm.h>
>  #include <linux/sizes.h>
> +#include <linux/bitmap.h>
> +#include <sys/sysinfo.h>
>  
>  #include "kvm_util.h"
>  #include "processor.h"
> @@ -41,12 +45,14 @@ struct test_args {
>  	int nr_vcpus;
>  	int nr_iter;
>  	int timer_period_ms;
> +	int migration_freq_ms;
>  };
>  
>  static struct test_args test_args = {
>  	.nr_vcpus = NR_VCPUS_DEF,
>  	.nr_iter = NR_TEST_ITERS_DEF,
>  	.timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> +	.migration_freq_ms = 0,		/* Turn off migrations by default */

I'd rather we enable good tests like these by default.

>  };
>  
>  #define msecs_to_usecs(msec)		((msec) * 1000LL)
> @@ -81,6 +87,9 @@ struct test_vcpu {
>  static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
>  static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
>  
> +static unsigned long *vcpu_done_map;
> +static pthread_mutex_t vcpu_done_map_lock;
> +
>  static void
>  guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
>  {
> @@ -216,6 +225,11 @@ static void *test_vcpu_run(void *arg)
>  
>  	vcpu_run(vm, vcpuid);
>  
> +	/* Currently, any exit from guest is an indication of completion */
> +	pthread_mutex_lock(&vcpu_done_map_lock);
> +	set_bit(vcpuid, vcpu_done_map);
> +	pthread_mutex_unlock(&vcpu_done_map_lock);
> +
>  	switch (get_ucall(vm, vcpuid, &uc)) {
>  	case UCALL_SYNC:
>  	case UCALL_DONE:
> @@ -235,9 +249,73 @@ static void *test_vcpu_run(void *arg)
>  	return NULL;
>  }
>  
> +static uint32_t test_get_pcpu(void)
> +{
> +	uint32_t pcpu;
> +	unsigned int nproc_conf;
> +	cpu_set_t online_cpuset;
> +
> +	nproc_conf = get_nprocs_conf();
> +	sched_getaffinity(0, sizeof(cpu_set_t), &online_cpuset);
> +
> +	/* Randomly find an available pCPU to place a vCPU on */
> +	do {
> +		pcpu = rand() % nproc_conf;
> +	} while (!CPU_ISSET(pcpu, &online_cpuset));
> +
> +	return pcpu;
> +}
> +static int test_migrate_vcpu(struct test_vcpu *vcpu)
> +{
> +	int ret;
> +	cpu_set_t cpuset;
> +	uint32_t new_pcpu = test_get_pcpu();
> +
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(new_pcpu, &cpuset);
> +	ret = pthread_setaffinity_np(vcpu->pt_vcpu_run,
> +					sizeof(cpuset), &cpuset);
> +
> +	/* Allow the error where the vCPU thread is already finished */
> +	TEST_ASSERT(ret == 0 || ret == ESRCH,
> +			"Failed to migrate the vCPU:%u to pCPU: %u; ret: %d\n",
> +			vcpu->vcpuid, new_pcpu, ret);

It'd be good to collect stats for the two cases so we know how many
vcpus we actually migrated with a successful setaffinity and how many
just completed too early. If our stats don't look good, then we can
adjust our timeouts and frequencies.

> +
> +	return ret;
> +}
> +static void *test_vcpu_migration(void *arg)
> +{
> +	unsigned int i, n_done;
> +	bool vcpu_done;
> +
> +	do {
> +		usleep(msecs_to_usecs(test_args.migration_freq_ms));
> +
> +		for (n_done = 0, i = 0; i < test_args.nr_vcpus; i++) {
> +			pthread_mutex_lock(&vcpu_done_map_lock);
> +			vcpu_done = test_bit(i, vcpu_done_map);
> +			pthread_mutex_unlock(&vcpu_done_map_lock);
> +
> +			if (vcpu_done) {
> +				n_done++;
> +				continue;
> +			}
> +
> +			test_migrate_vcpu(&test_vcpu[i]);
> +		}
> +	} while (test_args.nr_vcpus != n_done);
> +
> +	return NULL;
> +}
> +
>  static void test_run(struct kvm_vm *vm)
>  {
>  	int i, ret;
> +	pthread_t pt_vcpu_migration;
> +
> +	pthread_mutex_init(&vcpu_done_map_lock, NULL);
> +	vcpu_done_map = bitmap_alloc(test_args.nr_vcpus);
> +	TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");
>  
>  	for (i = 0; i < test_args.nr_vcpus; i++) {
>  		ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
> @@ -245,8 +323,23 @@ static void test_run(struct kvm_vm *vm)
>  		TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
>  	}
>  
> +	/* Spawn a thread to control the vCPU migrations */
> +	if (test_args.migration_freq_ms) {
> +		srand(time(NULL));
> +
> +		ret = pthread_create(&pt_vcpu_migration, NULL,
> +					test_vcpu_migration, NULL);
> +		TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
> +	}
> +
> +
>  	for (i = 0; i < test_args.nr_vcpus; i++)
>  		pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
> +
> +	if (test_args.migration_freq_ms)
> +		pthread_join(pt_vcpu_migration, NULL);
> +
> +	bitmap_free(vcpu_done_map);
>  }
>  
>  static struct kvm_vm *test_vm_create(void)
> @@ -286,6 +379,7 @@ static void test_print_help(char *name)
>  		NR_TEST_ITERS_DEF);
>  	pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
>  		TIMER_TEST_PERIOD_MS_DEF);
> +	pr_info("\t-m: Frequency (in ms) of vCPUs to migrate to different pCPU. 0 to turn off (default: 0)\n");
>  	pr_info("\t-h: print this help screen\n");
>  }
>  
> @@ -293,7 +387,7 @@ static bool parse_args(int argc, char *argv[])
>  {
>  	int opt;
>  
> -	while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
> +	while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
>  		switch (opt) {
>  		case 'n':
>  			test_args.nr_vcpus = atoi(optarg);
> @@ -320,6 +414,13 @@ static bool parse_args(int argc, char *argv[])
>  				goto err;
>  			}
>  			break;
> +		case 'm':
> +			test_args.migration_freq_ms = atoi(optarg);
> +			if (test_args.migration_freq_ms < 0) {
> +				pr_info("0 or positive value needed for -m\n");
> +				goto err;
> +			}
> +			break;
>  		case 'h':
>  		default:
>  			goto err;
> @@ -343,6 +444,11 @@ int main(int argc, char *argv[])
>  	if (!parse_args(argc, argv))
>  		exit(KSFT_SKIP);
>  
> +	if (get_nprocs() < 2) {

if (test_args.migration_freq_ms && get_nprocs() < 2)

> +		print_skip("At least two physical CPUs needed for vCPU migration");
> +		exit(KSFT_SKIP);
> +	}
> +
>  	vm = test_vm_create();
>  	test_run(vm);
>  	kvm_vm_free(vm);
> -- 
> 2.33.0.153.gba50c8fa24-goog
>

Thanks,
drew 

Re: [PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Raghavendra Rao Ananta]

On Fri, Sep 3, 2021 at 4:05 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Sep 01, 2021 at 09:14:12PM +0000, Raghavendra Rao Ananta wrote:
> > Since the timer stack (hardware and KVM) is per-CPU, there
> > are potential chances for races to occur when the scheduler
> > decides to migrate a vCPU thread to a different physical CPU.
> > Hence, include an option to stress-test this part as well by
> > forcing the vCPUs to migrate across physical CPUs in the
> > system at a particular rate.
> >
> > Originally, the bug for the fix with commit 3134cc8beb69d0d
> > ("KVM: arm64: vgic: Resample HW pending state on deactivation")
> > was discovered using arch_timer test with vCPU migrations and
> > can be easily reproduced.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> >  .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
> >  1 file changed, 107 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > index 1383f33850e9..de246c7afab2 100644
> > --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > @@ -14,6 +14,8 @@
> >   *
> >   * The test provides command-line options to configure the timer's
> >   * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> > + * To stress-test the timer stack even more, an option to migrate the
> > + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> >   *
> >   * Copyright (c) 2021, Google LLC.
> >   */
> > @@ -24,6 +26,8 @@
> >  #include <pthread.h>
> >  #include <linux/kvm.h>
> >  #include <linux/sizes.h>
> > +#include <linux/bitmap.h>
> > +#include <sys/sysinfo.h>
> >
> >  #include "kvm_util.h"
> >  #include "processor.h"
> > @@ -41,12 +45,14 @@ struct test_args {
> >       int nr_vcpus;
> >       int nr_iter;
> >       int timer_period_ms;
> > +     int migration_freq_ms;
> >  };
> >
> >  static struct test_args test_args = {
> >       .nr_vcpus = NR_VCPUS_DEF,
> >       .nr_iter = NR_TEST_ITERS_DEF,
> >       .timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> > +     .migration_freq_ms = 0,         /* Turn off migrations by default */
>
> I'd rather we enable good tests like these by default.
>
Well, that was my original idea, but I was concerned about the ease
for diagnosing
things since it'll become too noisy. And so I let it as a personal
preference. But I can
include it back and see how it goes.
> >  };
> >
> >  #define msecs_to_usecs(msec)         ((msec) * 1000LL)
> > @@ -81,6 +87,9 @@ struct test_vcpu {
> >  static struct test_vcpu test_vcpu[KVM_MAX_VCPUS];
> >  static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];
> >
> > +static unsigned long *vcpu_done_map;
> > +static pthread_mutex_t vcpu_done_map_lock;
> > +
> >  static void
> >  guest_configure_timer_action(struct test_vcpu_shared_data *shared_data)
> >  {
> > @@ -216,6 +225,11 @@ static void *test_vcpu_run(void *arg)
> >
> >       vcpu_run(vm, vcpuid);
> >
> > +     /* Currently, any exit from guest is an indication of completion */
> > +     pthread_mutex_lock(&vcpu_done_map_lock);
> > +     set_bit(vcpuid, vcpu_done_map);
> > +     pthread_mutex_unlock(&vcpu_done_map_lock);
> > +
> >       switch (get_ucall(vm, vcpuid, &uc)) {
> >       case UCALL_SYNC:
> >       case UCALL_DONE:
> > @@ -235,9 +249,73 @@ static void *test_vcpu_run(void *arg)
> >       return NULL;
> >  }
> >
> > +static uint32_t test_get_pcpu(void)
> > +{
> > +     uint32_t pcpu;
> > +     unsigned int nproc_conf;
> > +     cpu_set_t online_cpuset;
> > +
> > +     nproc_conf = get_nprocs_conf();
> > +     sched_getaffinity(0, sizeof(cpu_set_t), &online_cpuset);
> > +
> > +     /* Randomly find an available pCPU to place a vCPU on */
> > +     do {
> > +             pcpu = rand() % nproc_conf;
> > +     } while (!CPU_ISSET(pcpu, &online_cpuset));
> > +
> > +     return pcpu;
> > +}
> > +static int test_migrate_vcpu(struct test_vcpu *vcpu)
> > +{
> > +     int ret;
> > +     cpu_set_t cpuset;
> > +     uint32_t new_pcpu = test_get_pcpu();
> > +
> > +     CPU_ZERO(&cpuset);
> > +     CPU_SET(new_pcpu, &cpuset);
> > +     ret = pthread_setaffinity_np(vcpu->pt_vcpu_run,
> > +                                     sizeof(cpuset), &cpuset);
> > +
> > +     /* Allow the error where the vCPU thread is already finished */
> > +     TEST_ASSERT(ret == 0 || ret == ESRCH,
> > +                     "Failed to migrate the vCPU:%u to pCPU: %u; ret: %d\n",
> > +                     vcpu->vcpuid, new_pcpu, ret);
>
> It'd be good to collect stats for the two cases so we know how many
> vcpus we actually migrated with a successful setaffinity and how many
> just completed too early. If our stats don't look good, then we can
> adjust our timeouts and frequencies.
>
I can do that, but since we don't attempt to migrate if the migration
thread learns
that the vCPU is already done, I'm guessing we may not hit ESRCH as much.
> > +
> > +     return ret;
> > +}
> > +static void *test_vcpu_migration(void *arg)
> > +{
> > +     unsigned int i, n_done;
> > +     bool vcpu_done;
> > +
> > +     do {
> > +             usleep(msecs_to_usecs(test_args.migration_freq_ms));
> > +
> > +             for (n_done = 0, i = 0; i < test_args.nr_vcpus; i++) {
> > +                     pthread_mutex_lock(&vcpu_done_map_lock);
> > +                     vcpu_done = test_bit(i, vcpu_done_map);
> > +                     pthread_mutex_unlock(&vcpu_done_map_lock);
> > +
> > +                     if (vcpu_done) {
> > +                             n_done++;
> > +                             continue;
> > +                     }
> > +
> > +                     test_migrate_vcpu(&test_vcpu[i]);
> > +             }
> > +     } while (test_args.nr_vcpus != n_done);
> > +
> > +     return NULL;
> > +}
> > +
> >  static void test_run(struct kvm_vm *vm)
> >  {
> >       int i, ret;
> > +     pthread_t pt_vcpu_migration;
> > +
> > +     pthread_mutex_init(&vcpu_done_map_lock, NULL);
> > +     vcpu_done_map = bitmap_alloc(test_args.nr_vcpus);
> > +     TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");
> >
> >       for (i = 0; i < test_args.nr_vcpus; i++) {
> >               ret = pthread_create(&test_vcpu[i].pt_vcpu_run, NULL,
> > @@ -245,8 +323,23 @@ static void test_run(struct kvm_vm *vm)
> >               TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
> >       }
> >
> > +     /* Spawn a thread to control the vCPU migrations */
> > +     if (test_args.migration_freq_ms) {
> > +             srand(time(NULL));
> > +
> > +             ret = pthread_create(&pt_vcpu_migration, NULL,
> > +                                     test_vcpu_migration, NULL);
> > +             TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
> > +     }
> > +
> > +
> >       for (i = 0; i < test_args.nr_vcpus; i++)
> >               pthread_join(test_vcpu[i].pt_vcpu_run, NULL);
> > +
> > +     if (test_args.migration_freq_ms)
> > +             pthread_join(pt_vcpu_migration, NULL);
> > +
> > +     bitmap_free(vcpu_done_map);
> >  }
> >
> >  static struct kvm_vm *test_vm_create(void)
> > @@ -286,6 +379,7 @@ static void test_print_help(char *name)
> >               NR_TEST_ITERS_DEF);
> >       pr_info("\t-p: Periodicity (in ms) of the guest timer (default: %u)\n",
> >               TIMER_TEST_PERIOD_MS_DEF);
> > +     pr_info("\t-m: Frequency (in ms) of vCPUs to migrate to different pCPU. 0 to turn off (default: 0)\n");
> >       pr_info("\t-h: print this help screen\n");
> >  }
> >
> > @@ -293,7 +387,7 @@ static bool parse_args(int argc, char *argv[])
> >  {
> >       int opt;
> >
> > -     while ((opt = getopt(argc, argv, "hn:i:p:")) != -1) {
> > +     while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
> >               switch (opt) {
> >               case 'n':
> >                       test_args.nr_vcpus = atoi(optarg);
> > @@ -320,6 +414,13 @@ static bool parse_args(int argc, char *argv[])
> >                               goto err;
> >                       }
> >                       break;
> > +             case 'm':
> > +                     test_args.migration_freq_ms = atoi(optarg);
> > +                     if (test_args.migration_freq_ms < 0) {
> > +                             pr_info("0 or positive value needed for -m\n");
> > +                             goto err;
> > +                     }
> > +                     break;
> >               case 'h':
> >               default:
> >                       goto err;
> > @@ -343,6 +444,11 @@ int main(int argc, char *argv[])
> >       if (!parse_args(argc, argv))
> >               exit(KSFT_SKIP);
> >
> > +     if (get_nprocs() < 2) {
>
> if (test_args.migration_freq_ms && get_nprocs() < 2)
>
> > +             print_skip("At least two physical CPUs needed for vCPU migration");
> > +             exit(KSFT_SKIP);
> > +     }
> > +
> >       vm = test_vm_create();
> >       test_run(vm);
> >       kvm_vm_free(vm);
> > --
> > 2.33.0.153.gba50c8fa24-goog
> >
>
> Thanks,
> drew
>

Re: [PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Andrew Jones]

On Fri, Sep 03, 2021 at 01:53:27PM -0700, Raghavendra Rao Ananta wrote:
> On Fri, Sep 3, 2021 at 4:05 AM Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Wed, Sep 01, 2021 at 09:14:12PM +0000, Raghavendra Rao Ananta wrote:
> > > Since the timer stack (hardware and KVM) is per-CPU, there
> > > are potential chances for races to occur when the scheduler
> > > decides to migrate a vCPU thread to a different physical CPU.
> > > Hence, include an option to stress-test this part as well by
> > > forcing the vCPUs to migrate across physical CPUs in the
> > > system at a particular rate.
> > >
> > > Originally, the bug for the fix with commit 3134cc8beb69d0d
> > > ("KVM: arm64: vgic: Resample HW pending state on deactivation")
> > > was discovered using arch_timer test with vCPU migrations and
> > > can be easily reproduced.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > ---
> > >  .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
> > >  1 file changed, 107 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > index 1383f33850e9..de246c7afab2 100644
> > > --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > @@ -14,6 +14,8 @@
> > >   *
> > >   * The test provides command-line options to configure the timer's
> > >   * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> > > + * To stress-test the timer stack even more, an option to migrate the
> > > + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> > >   *
> > >   * Copyright (c) 2021, Google LLC.
> > >   */
> > > @@ -24,6 +26,8 @@
> > >  #include <pthread.h>
> > >  #include <linux/kvm.h>
> > >  #include <linux/sizes.h>
> > > +#include <linux/bitmap.h>
> > > +#include <sys/sysinfo.h>
> > >
> > >  #include "kvm_util.h"
> > >  #include "processor.h"
> > > @@ -41,12 +45,14 @@ struct test_args {
> > >       int nr_vcpus;
> > >       int nr_iter;
> > >       int timer_period_ms;
> > > +     int migration_freq_ms;
> > >  };
> > >
> > >  static struct test_args test_args = {
> > >       .nr_vcpus = NR_VCPUS_DEF,
> > >       .nr_iter = NR_TEST_ITERS_DEF,
> > >       .timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> > > +     .migration_freq_ms = 0,         /* Turn off migrations by default */
> >
> > I'd rather we enable good tests like these by default.
> >
> Well, that was my original idea, but I was concerned about the ease
> for diagnosing
> things since it'll become too noisy. And so I let it as a personal
> preference. But I can
> include it back and see how it goes.

Break the tests into two? One run without migration and one with. If
neither work, then we can diagnose the one without first, possibly
avoiding the need to diagnose the one with.

Thanks,
drew

Re: [PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Raghavendra Rao Ananta]

On Sun, Sep 5, 2021 at 11:39 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Sep 03, 2021 at 01:53:27PM -0700, Raghavendra Rao Ananta wrote:
> > On Fri, Sep 3, 2021 at 4:05 AM Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Wed, Sep 01, 2021 at 09:14:12PM +0000, Raghavendra Rao Ananta wrote:
> > > > Since the timer stack (hardware and KVM) is per-CPU, there
> > > > are potential chances for races to occur when the scheduler
> > > > decides to migrate a vCPU thread to a different physical CPU.
> > > > Hence, include an option to stress-test this part as well by
> > > > forcing the vCPUs to migrate across physical CPUs in the
> > > > system at a particular rate.
> > > >
> > > > Originally, the bug for the fix with commit 3134cc8beb69d0d
> > > > ("KVM: arm64: vgic: Resample HW pending state on deactivation")
> > > > was discovered using arch_timer test with vCPU migrations and
> > > > can be easily reproduced.
> > > >
> > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > > ---
> > > >  .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
> > > >  1 file changed, 107 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > index 1383f33850e9..de246c7afab2 100644
> > > > --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > @@ -14,6 +14,8 @@
> > > >   *
> > > >   * The test provides command-line options to configure the timer's
> > > >   * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> > > > + * To stress-test the timer stack even more, an option to migrate the
> > > > + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> > > >   *
> > > >   * Copyright (c) 2021, Google LLC.
> > > >   */
> > > > @@ -24,6 +26,8 @@
> > > >  #include <pthread.h>
> > > >  #include <linux/kvm.h>
> > > >  #include <linux/sizes.h>
> > > > +#include <linux/bitmap.h>
> > > > +#include <sys/sysinfo.h>
> > > >
> > > >  #include "kvm_util.h"
> > > >  #include "processor.h"
> > > > @@ -41,12 +45,14 @@ struct test_args {
> > > >       int nr_vcpus;
> > > >       int nr_iter;
> > > >       int timer_period_ms;
> > > > +     int migration_freq_ms;
> > > >  };
> > > >
> > > >  static struct test_args test_args = {
> > > >       .nr_vcpus = NR_VCPUS_DEF,
> > > >       .nr_iter = NR_TEST_ITERS_DEF,
> > > >       .timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> > > > +     .migration_freq_ms = 0,         /* Turn off migrations by default */
> > >
> > > I'd rather we enable good tests like these by default.
> > >
> > Well, that was my original idea, but I was concerned about the ease
> > for diagnosing
> > things since it'll become too noisy. And so I let it as a personal
> > preference. But I can
> > include it back and see how it goes.
>
> Break the tests into two? One run without migration and one with. If
> neither work, then we can diagnose the one without first, possibly
> avoiding the need to diagnose the one with.
>
Right. I guess that's where the test's migration switch can come in handy.
Let's turn migration ON by default. If we see a failure, we can turn
OFF migration
and run the tests. I'll try to include some verbose printing for diagnosability.

Regards,
Raghavendra
> Thanks,
> drew
>

Re: [PATCH v3 12/12] KVM: arm64: selftests: arch_timer: Support vCPU migration

[Andrew Jones]

On Tue, Sep 07, 2021 at 09:14:54AM -0700, Raghavendra Rao Ananta wrote:
> On Sun, Sep 5, 2021 at 11:39 PM Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Fri, Sep 03, 2021 at 01:53:27PM -0700, Raghavendra Rao Ananta wrote:
> > > On Fri, Sep 3, 2021 at 4:05 AM Andrew Jones <drjones@redhat.com> wrote:
> > > >
> > > > On Wed, Sep 01, 2021 at 09:14:12PM +0000, Raghavendra Rao Ananta wrote:
> > > > > Since the timer stack (hardware and KVM) is per-CPU, there
> > > > > are potential chances for races to occur when the scheduler
> > > > > decides to migrate a vCPU thread to a different physical CPU.
> > > > > Hence, include an option to stress-test this part as well by
> > > > > forcing the vCPUs to migrate across physical CPUs in the
> > > > > system at a particular rate.
> > > > >
> > > > > Originally, the bug for the fix with commit 3134cc8beb69d0d
> > > > > ("KVM: arm64: vgic: Resample HW pending state on deactivation")
> > > > > was discovered using arch_timer test with vCPU migrations and
> > > > > can be easily reproduced.
> > > > >
> > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > > > ---
> > > > >  .../selftests/kvm/aarch64/arch_timer.c        | 108 +++++++++++++++++-
> > > > >  1 file changed, 107 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > > index 1383f33850e9..de246c7afab2 100644
> > > > > --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> > > > > @@ -14,6 +14,8 @@
> > > > >   *
> > > > >   * The test provides command-line options to configure the timer's
> > > > >   * period (-p), number of vCPUs (-n), and iterations per stage (-i).
> > > > > + * To stress-test the timer stack even more, an option to migrate the
> > > > > + * vCPUs across pCPUs (-m), at a particular rate, is also provided.
> > > > >   *
> > > > >   * Copyright (c) 2021, Google LLC.
> > > > >   */
> > > > > @@ -24,6 +26,8 @@
> > > > >  #include <pthread.h>
> > > > >  #include <linux/kvm.h>
> > > > >  #include <linux/sizes.h>
> > > > > +#include <linux/bitmap.h>
> > > > > +#include <sys/sysinfo.h>
> > > > >
> > > > >  #include "kvm_util.h"
> > > > >  #include "processor.h"
> > > > > @@ -41,12 +45,14 @@ struct test_args {
> > > > >       int nr_vcpus;
> > > > >       int nr_iter;
> > > > >       int timer_period_ms;
> > > > > +     int migration_freq_ms;
> > > > >  };
> > > > >
> > > > >  static struct test_args test_args = {
> > > > >       .nr_vcpus = NR_VCPUS_DEF,
> > > > >       .nr_iter = NR_TEST_ITERS_DEF,
> > > > >       .timer_period_ms = TIMER_TEST_PERIOD_MS_DEF,
> > > > > +     .migration_freq_ms = 0,         /* Turn off migrations by default */
> > > >
> > > > I'd rather we enable good tests like these by default.
> > > >
> > > Well, that was my original idea, but I was concerned about the ease
> > > for diagnosing
> > > things since it'll become too noisy. And so I let it as a personal
> > > preference. But I can
> > > include it back and see how it goes.
> >
> > Break the tests into two? One run without migration and one with. If
> > neither work, then we can diagnose the one without first, possibly
> > avoiding the need to diagnose the one with.
> >
> Right. I guess that's where the test's migration switch can come in handy.
> Let's turn migration ON by default. If we see a failure, we can turn
> OFF migration
> and run the tests. I'll try to include some verbose printing for diagnosability.
>

Sounds good. You can also consider using pr_debug if you feel the need to
balance verbosity with diagnostics.

Thanks,
drew

Re: [PATCH v3 00/12] KVM: arm64: selftests: Introduce arch_timer selftest

[Oliver Upton]

On Wed, Sep 01, 2021 at 09:14:00PM +0000, Raghavendra Rao Ananta wrote:
> Hello,
> 
> The patch series adds a KVM selftest to validate the behavior of
> ARM's generic timer (patch-11). The test programs the timer IRQs
> periodically, and for each interrupt, it validates the behaviour
> against the architecture specifications. The test further provides
> a command-line interface to configure the number of vCPUs, the
> period of the timer, and the number of iterations that the test
> has to run for.
> 
> Patch-12 adds an option to randomly migrate the vCPUs to different
> physical CPUs across the system. The bug for the fix provided by
> Marc with commit 3134cc8beb69d0d ("KVM: arm64: vgic: Resample HW
> pending state on deactivation") was discovered using arch_timer
> test with vCPU migrations.
> 
> Since the test heavily depends on interrupts, patch-10 adds a host
> library to setup ARM Generic Interrupt Controller v3 (GICv3). This
> includes creating a vGIC device, setting up distributor and
> redistributor attributes, and mapping the guest physical addresses.
> Symmetrical to this, patch-9 adds a guest library to talk to the vGIC,
> which includes initializing the controller, enabling/disabling the
> interrupts, and so on.
> 
> Furthermore, additional processor utilities such as accessing the MMIO
> (via readl/writel), read/write to assembler unsupported registers,
> basic delay generation, enable/disable local IRQs, and so on, are also
> introduced that the test/GICv3 takes advantage of (patches 1 through 8).
> 
> The patch series, specifically the library support, is derived from the
> kvm-unit-tests and the kernel itself.
> 
> Regards,
> Raghavendra

For later submissions, can you include a lore.kernel.org link to your
older revisions of the series? NBD now, its easy to find in my inbox but
just for future reference.

--
Best,
Oliver

Re: [PATCH v3 00/12] KVM: arm64: selftests: Introduce arch_timer selftest

[Oliver Upton]

+cc Andrew Jones

On Wed, Sep 01, 2021 at 09:14:00PM +0000, Raghavendra Rao Ananta wrote:
> Hello,
> 
> The patch series adds a KVM selftest to validate the behavior of
> ARM's generic timer (patch-11). The test programs the timer IRQs
> periodically, and for each interrupt, it validates the behaviour
> against the architecture specifications. The test further provides
> a command-line interface to configure the number of vCPUs, the
> period of the timer, and the number of iterations that the test
> has to run for.
> 
> Patch-12 adds an option to randomly migrate the vCPUs to different
> physical CPUs across the system. The bug for the fix provided by
> Marc with commit 3134cc8beb69d0d ("KVM: arm64: vgic: Resample HW
> pending state on deactivation") was discovered using arch_timer
> test with vCPU migrations.
> 
> Since the test heavily depends on interrupts, patch-10 adds a host
> library to setup ARM Generic Interrupt Controller v3 (GICv3). This
> includes creating a vGIC device, setting up distributor and
> redistributor attributes, and mapping the guest physical addresses.
> Symmetrical to this, patch-9 adds a guest library to talk to the vGIC,
> which includes initializing the controller, enabling/disabling the
> interrupts, and so on.
> 
> Furthermore, additional processor utilities such as accessing the MMIO
> (via readl/writel), read/write to assembler unsupported registers,
> basic delay generation, enable/disable local IRQs, and so on, are also
> introduced that the test/GICv3 takes advantage of (patches 1 through 8).
> 
> The patch series, specifically the library support, is derived from the
> kvm-unit-tests and the kernel itself.
> 
> Regards,
> Raghavendra
> 
> v2 -> v3:
> 
> - Addressed the comments from Ricardo regarding moving the vGIC host
>   support for selftests to its own library.
> - Added an option (-m) to migrate the guest vCPUs to physical CPUs
>   in the system.
> 
> v1 -> v2:
> 
> Addressed comments from Zenghui in include/aarch64/arch_timer.h:
> - Correct the header description
> - Remove unnecessary inclusion of linux/sizes.h
> - Re-arrange CTL_ defines in ascending order
> - Remove inappropriate 'return' from timer_set_* functions, which
>   returns 'void'.
> 
> Raghavendra Rao Ananta (12):
>   KVM: arm64: selftests: Add MMIO readl/writel support
>   KVM: arm64: selftests: Add write_sysreg_s and read_sysreg_s
>   KVM: arm64: selftests: Add support for cpu_relax
>   KVM: arm64: selftests: Add basic support for arch_timers
>   KVM: arm64: selftests: Add basic support to generate delays
>   KVM: arm64: selftests: Add support to disable and enable local IRQs
>   KVM: arm64: selftests: Add support to get the vcpuid from MPIDR_EL1
>   KVM: arm64: selftests: Add light-weight spinlock support
>   KVM: arm64: selftests: Add basic GICv3 support
>   KVM: arm64: selftests: Add host support for vGIC
>   KVM: arm64: selftests: Add arch_timer test
>   KVM: arm64: selftests: arch_timer: Support vCPU migration
> 
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   3 +-
>  .../selftests/kvm/aarch64/arch_timer.c        | 457 ++++++++++++++++++
>  .../kvm/include/aarch64/arch_timer.h          | 142 ++++++
>  .../selftests/kvm/include/aarch64/delay.h     |  25 +
>  .../selftests/kvm/include/aarch64/gic.h       |  21 +
>  .../selftests/kvm/include/aarch64/processor.h | 140 +++++-
>  .../selftests/kvm/include/aarch64/spinlock.h  |  13 +
>  .../selftests/kvm/include/aarch64/vgic.h      |  14 +
>  tools/testing/selftests/kvm/lib/aarch64/gic.c |  93 ++++
>  .../selftests/kvm/lib/aarch64/gic_private.h   |  21 +
>  .../selftests/kvm/lib/aarch64/gic_v3.c        | 240 +++++++++
>  .../selftests/kvm/lib/aarch64/gic_v3.h        |  70 +++
>  .../selftests/kvm/lib/aarch64/spinlock.c      |  27 ++
>  .../testing/selftests/kvm/lib/aarch64/vgic.c  |  67 +++
>  15 files changed, 1332 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/arch_timer.c
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/arch_timer.h
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/delay.h
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/gic.h
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/spinlock.h
>  create mode 100644 tools/testing/selftests/kvm/include/aarch64/vgic.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic.c
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_private.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/spinlock.c
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vgic.c
> 
> -- 
> 2.33.0.153.gba50c8fa24-goog
>