[kvm-unit-tests PATCH v4 00/10] arm/arm64: Various fixes

[kvm-unit-tests PATCH v4 00/10] arm/arm64: Various fixes

[Alexandru Elisei]

These are the patches that were left unmerged from the previous version of
the series, plus a few new patches. Patch #1 "Makefile: Use
no-stack-protector compiler options" is straightforward and came about
because of a compile error I experienced on RockPro64.

Patches #3 and #5 are the result of Andre's comments on the previous
version. When adding ISBs after register writes I noticed in the ARM ARM
that a read of the timer counter value can be reordered, and patch #4
tries to avoid that.

Patch #7 is also the result of a review comment. For the GIC tests, we wait
up to 5 seconds for the interrupt to be asserted. However, the GIC tests
can use more than one CPU, which is not the case with the timer test. And
waiting for the GIC to assert the interrupt can happen up to 6 times (8
times after patch #9), so I figured that a timeout of 10 seconds for the
test is acceptable.

Patch #8 tries to improve the way we test how the timer generates the
interrupt. If the GIC asserts the timer interrupt, but the device itself is
not generating it, that's a pretty big problem.

Ran the same tests as before:

- with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
  (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).

- with qemu, on an arm64 host kernel:
    a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
       GICv2 (Seattle).
    b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.

Changes:
* Patches #1, #3, #4, #5, #7, #8 are new.
* For patch #2, as per Drew's suggestion, I changed the entry point to halt
  because the test is supposed to test if CPU_ON is successful.
* Removed the ISB from patch #6 because that was fixed by #3.
* Moved the architecture dependent function init_dcache_line_size to
  cpu_init in lib/arm/setup.c as per Drew's suggestion.

Alexandru Elisei (10):
  Makefile: Use no-stack-protector compiler options
  arm/arm64: psci: Don't run C code without stack or vectors
  arm64: timer: Add ISB after register writes
  arm64: timer: Add ISB before reading the counter value
  arm64: timer: Make irq_received volatile
  arm64: timer: EOIR the interrupt after masking the timer
  arm64: timer: Wait for the GIC to sample timer interrupt state
  arm64: timer: Check the timer interrupt state
  arm64: timer: Test behavior when timer disabled or masked
  arm/arm64: Perform dcache clean + invalidate after turning MMU off

 Makefile                  |  4 +-
 lib/arm/asm/processor.h   | 13 +++++++
 lib/arm64/asm/processor.h | 12 ++++++
 lib/arm/setup.c           |  8 ++++
 arm/cstart.S              | 22 +++++++++++
 arm/cstart64.S            | 23 ++++++++++++
 arm/psci.c                | 15 ++++++--
 arm/timer.c               | 79 ++++++++++++++++++++++++++++++++-------
 arm/unittests.cfg         |  2 +-
 9 files changed, 158 insertions(+), 20 deletions(-)

-- 
2.20.1

[kvm-unit-tests PATCH v4 01/10] Makefile: Use no-stack-protector compiler options

[Alexandru Elisei]

Let's fix the typos so that the -fno-stack-protector and
-fno-stack-protector-all compiler options are actually used.

Tested by compiling for arm64, x86_64 and ppc64 little endian. Before the
patch, the arguments were missing from the gcc invocation; after the patch,
they were present. Also fixes a compilation error that I was seeing with
aarch64 gcc version 9.2.0, where the linker was complaining about an
undefined reference to the symbol __stack_chk_guard.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Drew Jones <drjones@redhat.com>
CC: Laurent Vivier <lvivier@redhat.com>
CC: Thomas Huth <thuth@redhat.com>
CC: David Hildenbrand <david@redhat.com>
CC: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 767b6c6a51d0..754ed65ecd2f 100644
--- a/Makefile
+++ b/Makefile
@@ -55,8 +55,8 @@ COMMON_CFLAGS += -Wignored-qualifiers -Werror
 
 frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer
 fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "")
-fnostack_protector := $(call cc-option, -fno-stack-protector, "")
-fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
+fno_stack_protector := $(call cc-option, -fno-stack-protector, "")
+fno_stack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
 wno_frame_address := $(call cc-option, -Wno-frame-address, "")
 fno_pic := $(call cc-option, -fno-pic, "")
 no_pie := $(call cc-option, -no-pie, "")
-- 
2.20.1

Re: [kvm-unit-tests PATCH v4 01/10] Makefile: Use no-stack-protector compiler options

[Thomas Huth]

On 31/01/2020 17.37, Alexandru Elisei wrote:
> Let's fix the typos so that the -fno-stack-protector and
> -fno-stack-protector-all compiler options are actually used.
> 
> Tested by compiling for arm64, x86_64 and ppc64 little endian. Before the
> patch, the arguments were missing from the gcc invocation; after the patch,
> they were present. Also fixes a compilation error that I was seeing with
> aarch64 gcc version 9.2.0, where the linker was complaining about an
> undefined reference to the symbol __stack_chk_guard.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Drew Jones <drjones@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>
> CC: David Hildenbrand <david@redhat.com>
> CC: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 767b6c6a51d0..754ed65ecd2f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -55,8 +55,8 @@ COMMON_CFLAGS += -Wignored-qualifiers -Werror
>  
>  frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer
>  fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "")
> -fnostack_protector := $(call cc-option, -fno-stack-protector, "")
> -fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
> +fno_stack_protector := $(call cc-option, -fno-stack-protector, "")
> +fno_stack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
>  wno_frame_address := $(call cc-option, -Wno-frame-address, "")
>  fno_pic := $(call cc-option, -fno-pic, "")
>  no_pie := $(call cc-option, -no-pie, "")

Ouch, very well spotted.

Fixes: e5c73790f5f0 ("build: don't reevaluate cc-option shell command")

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v4 01/10] Makefile: Use no-stack-protector compiler options

[Laurent Vivier]

On 31/01/2020 17:37, Alexandru Elisei wrote:
> Let's fix the typos so that the -fno-stack-protector and
> -fno-stack-protector-all compiler options are actually used.
> 
> Tested by compiling for arm64, x86_64 and ppc64 little endian. Before the
> patch, the arguments were missing from the gcc invocation; after the patch,
> they were present. Also fixes a compilation error that I was seeing with
> aarch64 gcc version 9.2.0, where the linker was complaining about an
> undefined reference to the symbol __stack_chk_guard.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Drew Jones <drjones@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>
> CC: David Hildenbrand <david@redhat.com>
> CC: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 767b6c6a51d0..754ed65ecd2f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -55,8 +55,8 @@ COMMON_CFLAGS += -Wignored-qualifiers -Werror
>  
>  frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer
>  fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "")
> -fnostack_protector := $(call cc-option, -fno-stack-protector, "")
> -fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
> +fno_stack_protector := $(call cc-option, -fno-stack-protector, "")
> +fno_stack_protector_all := $(call cc-option, -fno-stack-protector-all, "")
>  wno_frame_address := $(call cc-option, -Wno-frame-address, "")
>  fno_pic := $(call cc-option, -fno-pic, "")
>  no_pie := $(call cc-option, -no-pie, "")
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[kvm-unit-tests PATCH v4 02/10] arm/arm64: psci: Don't run C code without stack or vectors

[Alexandru Elisei]

The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
done by setting the entry point for the CPU_ON call to the physical address
of the C function cpu_psci_cpu_die.

The compiler is well within its rights to use the stack when generating
code for cpu_psci_cpu_die.  However, because no stack initialization has
been done, the stack pointer is zero, as set by KVM when creating the VCPU.
This causes a data abort without a change in exception level. The VBAR_EL1
register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
and we end up trying to fetch instructions from address 0x200.

At this point, a stage 2 instruction abort is generated which is taken to
KVM. KVM interprets this as an instruction fetch from an I/O region, and
injects a prefetch abort into the guest. Prefetch abort is a synchronous
exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
which is...  0x200. The VCPU ends up in an infinite loop causing a prefetch
abort while fetching the instruction to service the said abort.

To avoid all of this, lets use the assembly function halt as the CPU_ON
entry address. Also, expand the check to test that we only get
PSCI_RET_SUCCESS exactly once, as we're never offlining the CPU during the
test.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/psci.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arm/psci.c b/arm/psci.c
index 5c1accb6cea4..ffc09a2e9858 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -79,13 +79,14 @@ static void cpu_on_secondary_entry(void)
 	cpumask_set_cpu(cpu, &cpu_on_ready);
 	while (!cpu_on_start)
 		cpu_relax();
-	cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+	cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
 	cpumask_set_cpu(cpu, &cpu_on_done);
 }
 
 static bool psci_cpu_on_test(void)
 {
 	bool failed = false;
+	int ret_success = 0;
 	int cpu;
 
 	cpumask_set_cpu(1, &cpu_on_ready);
@@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
 	cpu_on_start = 1;
 	smp_mb();
 
-	cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+	cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
 	cpumask_set_cpu(0, &cpu_on_done);
 
 	while (!cpumask_full(&cpu_on_done))
@@ -113,12 +114,19 @@ static bool psci_cpu_on_test(void)
 	for_each_present_cpu(cpu) {
 		if (cpu == 1)
 			continue;
-		if (cpu_on_ret[cpu] != PSCI_RET_SUCCESS && cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
+		if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) {
+			ret_success++;
+		} else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
 			report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]);
 			failed = true;
 		}
 	}
 
+	if (ret_success != 1) {
+		report_info("got %d CPU_ON success", ret_success);
+		failed = true;
+	}
+
 	return !failed;
 }
 
-- 
2.20.1

[kvm-unit-tests PATCH v4 03/10] arm64: timer: Add ISB after register writes

[Alexandru Elisei]

From ARM DDI 0487E.a glossary, the section "Context synchronization
event":

"All direct and indirect writes to System registers that are made before
the Context synchronization event affect any instruction, including a
direct read, that appears in program order after the instruction causing
the Context synchronization event."

The ISB instruction is a context synchronization event [1]. Add an ISB
after all register writes, to make sure that the writes have been
completed when we try to test their effects.

[1] ARM DDI 0487E.a, section C6.2.96

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index f390e8e65d31..c6ea108cfa4b 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -41,6 +41,7 @@ static u64 read_vtimer_cval(void)
 static void write_vtimer_cval(u64 val)
 {
 	write_sysreg(val, cntv_cval_el0);
+	isb();
 }
 
 static s32 read_vtimer_tval(void)
@@ -51,6 +52,7 @@ static s32 read_vtimer_tval(void)
 static void write_vtimer_tval(s32 val)
 {
 	write_sysreg(val, cntv_tval_el0);
+	isb();
 }
 
 static u64 read_vtimer_ctl(void)
@@ -61,6 +63,7 @@ static u64 read_vtimer_ctl(void)
 static void write_vtimer_ctl(u64 val)
 {
 	write_sysreg(val, cntv_ctl_el0);
+	isb();
 }
 
 static u64 read_ptimer_counter(void)
@@ -76,6 +79,7 @@ static u64 read_ptimer_cval(void)
 static void write_ptimer_cval(u64 val)
 {
 	write_sysreg(val, cntp_cval_el0);
+	isb();
 }
 
 static s32 read_ptimer_tval(void)
@@ -86,6 +90,7 @@ static s32 read_ptimer_tval(void)
 static void write_ptimer_tval(s32 val)
 {
 	write_sysreg(val, cntp_tval_el0);
+	isb();
 }
 
 static u64 read_ptimer_ctl(void)
@@ -96,6 +101,7 @@ static u64 read_ptimer_ctl(void)
 static void write_ptimer_ctl(u64 val)
 {
 	write_sysreg(val, cntp_ctl_el0);
+	isb();
 }
 
 struct timer_info {
@@ -181,7 +187,6 @@ static bool test_cval_10msec(struct timer_info *info)
 	before_timer = info->read_counter();
 	info->write_cval(before_timer + time_10ms);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	isb();
 
 	/* Wait for the timer to fire */
 	while (!(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS))
@@ -217,11 +222,9 @@ static void test_timer(struct timer_info *info)
 	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	isb();
 	report(!gic_timer_pending(info), "not pending before");
 
 	info->write_cval(now - 1);
-	isb();
 	report(gic_timer_pending(info), "interrupt signal pending");
 
 	/* Disable the timer again and prepare to take interrupts */
-- 
2.20.1

[kvm-unit-tests PATCH v4 04/10] arm64: timer: Add ISB before reading the counter value

[Alexandru Elisei]

Reads of the physical counter and the virtual counter registers "can occur
speculatively and out of order relative to other instructions executed on
the same PE" [1, 2].

There is no theoretical limit to the number of instructions that the CPU
can reorder and we use the counter value to program the timer to fire in
the future. Add an ISB before reading the counter to make sure the read
instruction is not reordered too long in the past with regard to the
instruction that programs the timer alarm, thus causing the timer to fire
unexpectedly. This matches what Linux does (see
arch/arm64/include/asm/arch_timer.h).

Because we use the counter value to program the timer, we create a register
dependency [3] between the value that we read and the value that we write to
CVAL and thus we don't need a barrier after the read. Linux does things
differently because the read needs to be ordered with regard to a memory
load (more information in commit 75a19a0202db ("arm64: arch_timer: Ensure
counter register reads occur with seqlock held")).

This also matches what we already do in get_cntvct from
lib/arm{,64}/asm/processor.h.

[1] ARM DDI 0487E.a, section D11.2.1
[2] ARM DDI 0487E.a, section D11.2.2
[3] ARM DDI 0486E.a, section B2.3.2

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arm/timer.c b/arm/timer.c
index c6ea108cfa4b..e758e84855c3 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -30,6 +30,7 @@ static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
 
 static u64 read_vtimer_counter(void)
 {
+	isb();
 	return read_sysreg(cntvct_el0);
 }
 
@@ -68,6 +69,7 @@ static void write_vtimer_ctl(u64 val)
 
 static u64 read_ptimer_counter(void)
 {
+	isb();
 	return read_sysreg(cntpct_el0);
 }
 
-- 
2.20.1

[kvm-unit-tests PATCH v4 05/10] arm64: timer: Make irq_received volatile

[Alexandru Elisei]

The irq_received field is modified by the interrupt handler. Make it
volatile so that the compiler doesn't reorder accesses with regard to
the instruction that will be causing the interrupt.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/timer.c b/arm/timer.c
index e758e84855c3..82f891147b35 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -109,7 +109,7 @@ static void write_ptimer_ctl(u64 val)
 struct timer_info {
 	u32 irq;
 	u32 irq_flags;
-	bool irq_received;
+	volatile bool irq_received;
 	u64 (*read_counter)(void);
 	u64 (*read_cval)(void);
 	void (*write_cval)(u64);
-- 
2.20.1

[kvm-unit-tests PATCH v4 06/10] arm64: timer: EOIR the interrupt after masking the timer

[Alexandru Elisei]

Writing to the EOIR register before masking the HW mapped timer
interrupt can cause taking another timer interrupt immediately after
exception return. This doesn't happen all the time, because KVM
reevaluates the state of pending HW mapped level sensitive interrupts on
each guest exit. If the second interrupt is pending and a guest exit
occurs after masking the timer interrupt and before the ERET (which
restores PSTATE.I), then KVM removes it.

Move the write after the IMASK bit has been set to prevent this from
happening.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 82f891147b35..b6f9dd10162d 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -157,19 +157,20 @@ static void irq_handler(struct pt_regs *regs)
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
-	if (irqnr != GICC_INT_SPURIOUS)
-		gic_write_eoir(irqstat);
-
 	if (irqnr == PPI(vtimer_info.irq)) {
 		info = &vtimer_info;
 	} else if (irqnr == PPI(ptimer_info.irq)) {
 		info = &ptimer_info;
 	} else {
+		if (irqnr != GICC_INT_SPURIOUS)
+			gic_write_eoir(irqstat);
 		report_info("Unexpected interrupt: %d\n", irqnr);
 		return;
 	}
 
 	info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
+	gic_write_eoir(irqstat);
+
 	info->irq_received = true;
 }
 
-- 
2.20.1

[kvm-unit-tests PATCH v4 07/10] arm64: timer: Wait for the GIC to sample timer interrupt state

[Alexandru Elisei]

There is a delay between the timer asserting the interrupt and the GIC
sampling the interrupt state. Let's take that into account when we are
checking if the timer interrupt is pending (or not) at the GIC level.

An interrupt can be pending or active and pending [1,2]. Let's be precise
and check that the interrupt is actually pending, not active and pending.

[1] ARM IHI 0048B.b, section 1.4.1
[2] ARM IHI 0069E, section 1.2.2

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c       | 43 ++++++++++++++++++++++++++++++++++++++-----
 arm/unittests.cfg |  2 +-
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index b6f9dd10162d..ba7e8c6a90ed 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -8,6 +8,7 @@
 #include <libcflat.h>
 #include <devicetree.h>
 #include <errata.h>
+#include <asm/delay.h>
 #include <asm/processor.h>
 #include <asm/gic.h>
 #include <asm/io.h>
@@ -16,6 +17,14 @@
 #define ARCH_TIMER_CTL_IMASK   (1 << 1)
 #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
 
+enum gic_state {
+	GIC_STATE_INACTIVE,
+	GIC_STATE_PENDING,
+	GIC_STATE_ACTIVE,
+	GIC_STATE_ACTIVE_PENDING,
+};
+
+static void *gic_isactiver;
 static void *gic_ispendr;
 static void *gic_isenabler;
 static void *gic_icenabler;
@@ -174,9 +183,28 @@ static void irq_handler(struct pt_regs *regs)
 	info->irq_received = true;
 }
 
-static bool gic_timer_pending(struct timer_info *info)
+static enum gic_state gic_timer_state(struct timer_info *info)
 {
-	return readl(gic_ispendr) & (1 << PPI(info->irq));
+	enum gic_state state = GIC_STATE_INACTIVE;
+	int i;
+	bool pending, active;
+
+	/* Wait for up to 1s for the GIC to sample the interrupt. */
+	for (i = 0; i < 10; i++) {
+		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
+		active = readl(gic_isactiver) & (1 << PPI(info->irq));
+		if (!active && !pending)
+			state = GIC_STATE_INACTIVE;
+		if (pending)
+			state = GIC_STATE_PENDING;
+		if (active)
+			state = GIC_STATE_ACTIVE;
+		if (active && pending)
+			state = GIC_STATE_ACTIVE_PENDING;
+		mdelay(100);
+	}
+
+	return state;
 }
 
 static bool test_cval_10msec(struct timer_info *info)
@@ -225,15 +253,18 @@ static void test_timer(struct timer_info *info)
 	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	report(!gic_timer_pending(info), "not pending before");
+	report(gic_timer_state(info) == GIC_STATE_INACTIVE,
+			"not pending before");
 
 	info->write_cval(now - 1);
-	report(gic_timer_pending(info), "interrupt signal pending");
+	report(gic_timer_state(info) == GIC_STATE_PENDING,
+			"interrupt signal pending");
 
 	/* Disable the timer again and prepare to take interrupts */
 	info->write_ctl(0);
 	set_timer_irq_enabled(info, true);
-	report(!gic_timer_pending(info), "interrupt signal no longer pending");
+	report(gic_timer_state(info) == GIC_STATE_INACTIVE,
+			"interrupt signal no longer pending");
 
 	report(test_cval_10msec(info), "latency within 10 ms");
 	report(info->irq_received, "interrupt received");
@@ -307,11 +338,13 @@ static void test_init(void)
 
 	switch (gic_version()) {
 	case 2:
+		gic_isactiver = gicv2_dist_base() + GICD_ISACTIVER;
 		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
 		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
 		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
 		break;
 	case 3:
+		gic_isactiver = gicv3_sgi_base() + GICD_ISACTIVER;
 		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
 		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
 		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index daeb5a09ad39..1f1bb24d9d13 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -132,7 +132,7 @@ groups = psci
 [timer]
 file = timer.flat
 groups = timer
-timeout = 2s
+timeout = 8s
 arch = arm64
 
 # Exit tests
-- 
2.20.1

[kvm-unit-tests PATCH v4 08/10] arm64: timer: Check the timer interrupt state

[Alexandru Elisei]

We check that the interrupt is pending (or not) at the GIC level, but we
don't check if the timer is asserting it (or not). Let's make sure we don't
run into a strange situation where the two devices' states aren't
synchronized.

Coincidently, the "interrupt signal no longer pending" test fails for
non-emulated timers (i.e, the virtual timer on a non-vhe host) if the
host kernel doesn't have patch 16e604a437c89 ("KVM: arm/arm64: vgic:
Reevaluate level sensitive interrupts on enable").

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index ba7e8c6a90ed..35038f2bae57 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -183,6 +183,13 @@ static void irq_handler(struct pt_regs *regs)
 	info->irq_received = true;
 }
 
+/* Check that the timer condition is met. */
+static bool timer_pending(struct timer_info *info)
+{
+	return (info->read_ctl() & ARCH_TIMER_CTL_ENABLE) &&
+		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
+}
+
 static enum gic_state gic_timer_state(struct timer_info *info)
 {
 	enum gic_state state = GIC_STATE_INACTIVE;
@@ -220,7 +227,7 @@ static bool test_cval_10msec(struct timer_info *info)
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
 
 	/* Wait for the timer to fire */
-	while (!(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS))
+	while (!timer_pending(info))
 		;
 
 	/* It fired, check how long it took */
@@ -253,17 +260,17 @@ static void test_timer(struct timer_info *info)
 	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	report(gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
 			"not pending before");
 
 	info->write_cval(now - 1);
-	report(gic_timer_state(info) == GIC_STATE_PENDING,
+	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
 			"interrupt signal pending");
 
 	/* Disable the timer again and prepare to take interrupts */
 	info->write_ctl(0);
 	set_timer_irq_enabled(info, true);
-	report(gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
 			"interrupt signal no longer pending");
 
 	report(test_cval_10msec(info), "latency within 10 ms");
-- 
2.20.1

[kvm-unit-tests PATCH v4 09/10] arm64: timer: Test behavior when timer disabled or masked

[Alexandru Elisei]

When the timer is disabled (the *_CTL_EL0.ENABLE bit is clear) or the
timer interrupt is masked at the timer level (the *_CTL_EL0.IMASK bit is
set), timer interrupts must not be pending or asserted by the VGIC.
However, only when the timer interrupt is masked, we can still check
that the timer condition is met by reading the *_CTL_EL0.ISTATUS bit.

This test was used to discover a bug and test the fix introduced by KVM
commit 16e604a437c8 ("KVM: arm/arm64: vgic: Reevaluate level sensitive
interrupts on enable").

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/timer.c       | 7 +++++++
 arm/unittests.cfg | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arm/timer.c b/arm/timer.c
index 35038f2bae57..dea364f5355d 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -269,10 +269,17 @@ static void test_timer(struct timer_info *info)
 
 	/* Disable the timer again and prepare to take interrupts */
 	info->write_ctl(0);
+	info->irq_received = false;
 	set_timer_irq_enabled(info, true);
+	report(!info->irq_received, "no interrupt when timer is disabled");
 	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
 			"interrupt signal no longer pending");
 
+	info->write_cval(now - 1);
+	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
+	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+			"interrupt signal not pending");
+
 	report(test_cval_10msec(info), "latency within 10 ms");
 	report(info->irq_received, "interrupt received");
 
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 1f1bb24d9d13..017958d28ffd 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -132,7 +132,7 @@ groups = psci
 [timer]
 file = timer.flat
 groups = timer
-timeout = 8s
+timeout = 10s
 arch = arm64
 
 # Exit tests
-- 
2.20.1

[kvm-unit-tests PATCH v4 10/10] arm/arm64: Perform dcache clean + invalidate after turning MMU off

[Alexandru Elisei]

When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
or to Strongly-Ordered memory on arm [2]. This means that the accesses are
non-cacheable.

Perform a dcache clean to PoC so we can read the newer values from the
cache after we turn the MMU off, instead of the stale values from memory.

Perform an invalidation so we can access the data written to memory after
we turn the MMU back on. This prevents reading back the stale values we
cleaned from the cache when we turned the MMU off.

Data caches are PIPT and the VAs are translated using the current
translation tables, or an identity mapping (what Arm calls a "flat
mapping") when the MMU is off [1, 2]. Do the clean + invalidate when the
MMU is off so we don't depend on the current translation tables and we can
make sure that the operation applies to the entire physical memory.

The patch was tested by hacking arm/selftest.c:

+#include <alloc_page.h>
+#include <asm/mmu.h>
 int main(int argc, char **argv)
 {
+	int *x = alloc_page();
+
 	report_prefix_push("selftest");

+	*x = 0x42;
+	mmu_disable();
+	report(*x == 0x42, "read back value written with MMU on");
+	*x = 0x50;
+	mmu_enable(current_thread_info()->pgtable);
+	report(*x == 0x50, "read back value written with MMU off");
+
 	if (argc < 2)
 		report_abort("no test specified");

Without the fix, the first report fails, and the test usually hangs before
the second report. This is because mmu_enable pushes the LR register on the
stack when the MMU is off, which means that the value will be written to
memory.  However, after asm_mmu_enable, the MMU is enabled, and we read it
back from the dcache, thus getting garbage.

With the fix, the two reports pass.

[1] ARM DDI 0487E.a, section D5.2.9
[2] ARM DDI 0406C.d, section B3.2.1

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/asm/processor.h   | 13 +++++++++++++
 lib/arm64/asm/processor.h | 12 ++++++++++++
 lib/arm/setup.c           |  8 ++++++++
 arm/cstart.S              | 22 ++++++++++++++++++++++
 arm/cstart64.S            | 23 +++++++++++++++++++++++
 5 files changed, 78 insertions(+)

diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index a8c4628da818..1e1132dafd2b 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -9,6 +9,11 @@
 #include <asm/sysreg.h>
 #include <asm/barrier.h>
 
+#define CTR_DMINLINE_SHIFT	16
+#define CTR_DMINLINE_MASK	(0xf << 16)
+#define CTR_DMINLINE(x)	\
+	(((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
+
 enum vector {
 	EXCPTN_RST,
 	EXCPTN_UND,
@@ -64,6 +69,7 @@ extern bool is_user(void);
 
 #define CNTVCT		__ACCESS_CP15_64(1, c14)
 #define CNTFRQ		__ACCESS_CP15(c14, 0, c0, 0)
+#define CTR		__ACCESS_CP15(c0, 0, c0, 1)
 
 static inline u64 get_cntvct(void)
 {
@@ -76,4 +82,11 @@ static inline u32 get_cntfrq(void)
 	return read_sysreg(CNTFRQ);
 }
 
+static inline u32 get_ctr(void)
+{
+	return read_sysreg(CTR);
+}
+
+extern u32 dcache_line_size;
+
 #endif /* _ASMARM_PROCESSOR_H_ */
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1d9223f728a5..02665b84cc7e 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -16,6 +16,11 @@
 #define SCTLR_EL1_A	(1 << 1)
 #define SCTLR_EL1_M	(1 << 0)
 
+#define CTR_DMINLINE_SHIFT	16
+#define CTR_DMINLINE_MASK	(0xf << 16)
+#define CTR_DMINLINE(x)	\
+	(((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
+
 #ifndef __ASSEMBLY__
 #include <asm/ptrace.h>
 #include <asm/esr.h>
@@ -105,5 +110,12 @@ static inline u32 get_cntfrq(void)
 	return read_sysreg(cntfrq_el0);
 }
 
+static inline u64 get_ctr(void)
+{
+	return read_sysreg(ctr_el0);
+}
+
+extern u32 dcache_line_size;
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMARM64_PROCESSOR_H_ */
diff --git a/lib/arm/setup.c b/lib/arm/setup.c
index 385e135f4865..418b4e58a5f8 100644
--- a/lib/arm/setup.c
+++ b/lib/arm/setup.c
@@ -20,6 +20,7 @@
 #include <asm/thread_info.h>
 #include <asm/setup.h>
 #include <asm/page.h>
+#include <asm/processor.h>
 #include <asm/smp.h>
 
 #include "io.h"
@@ -38,6 +39,8 @@ static struct mem_region __initial_mem_regions[NR_INITIAL_MEM_REGIONS + 1];
 struct mem_region *mem_regions = __initial_mem_regions;
 phys_addr_t __phys_offset, __phys_end;
 
+u32 dcache_line_size;
+
 int mpidr_to_cpu(uint64_t mpidr)
 {
 	int i;
@@ -66,6 +69,11 @@ static void cpu_init(void)
 	ret = dt_for_each_cpu_node(cpu_set, NULL);
 	assert(ret == 0);
 	set_cpu_online(0, true);
+	/*
+	 * DminLine is log2 of the number of words in the smallest cache line; a
+	 * word is 4 bytes.
+	 */
+	dcache_line_size = 1 << (CTR_DMINLINE(get_ctr()) + 2);
 }
 
 unsigned int mem_region_get_flags(phys_addr_t paddr)
diff --git a/arm/cstart.S b/arm/cstart.S
index e54e380e0d53..ef936ae2f874 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -197,6 +197,20 @@ asm_mmu_enable:
 
 	mov     pc, lr
 
+.macro dcache_clean_inval domain, start, end, tmp1, tmp2
+	ldr	\tmp1, =dcache_line_size
+	ldr	\tmp1, [\tmp1]
+	sub	\tmp2, \tmp1, #1
+	bic	\start, \start, \tmp2
+9998:
+	/* DCCIMVAC */
+	mcr	p15, 0, \start, c7, c14, 1
+	add	\start, \start, \tmp1
+	cmp	\start, \end
+	blo	9998b
+	dsb	\domain
+.endm
+
 .globl asm_mmu_disable
 asm_mmu_disable:
 	/* SCTLR */
@@ -204,6 +218,14 @@ asm_mmu_disable:
 	bic	r0, #CR_M
 	mcr	p15, 0, r0, c1, c0, 0
 	isb
+
+	ldr	r0, =__phys_offset
+	ldr	r0, [r0]
+	ldr	r1, =__phys_end
+	ldr	r1, [r1]
+	dcache_clean_inval sy, r0, r1, r2, r3
+	isb
+
 	mov     pc, lr
 
 /*
diff --git a/arm/cstart64.S b/arm/cstart64.S
index e5a561ea2e39..ffdd49f73ddd 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -193,12 +193,35 @@ asm_mmu_enable:
 
 	ret
 
+/* Taken with small changes from arch/arm64/incluse/asm/assembler.h */
+.macro dcache_by_line_op op, domain, start, end, tmp1, tmp2
+	adrp	\tmp1, dcache_line_size
+	ldr	\tmp1, [\tmp1, :lo12:dcache_line_size]
+	sub	\tmp2, \tmp1, #1
+	bic	\start, \start, \tmp2
+9998:
+	dc	\op , \start
+	add	\start, \start, \tmp1
+	cmp	\start, \end
+	b.lo	9998b
+	dsb	\domain
+.endm
+
 .globl asm_mmu_disable
 asm_mmu_disable:
 	mrs	x0, sctlr_el1
 	bic	x0, x0, SCTLR_EL1_M
 	msr	sctlr_el1, x0
 	isb
+
+	/* Clean + invalidate the entire memory */
+	adrp	x0, __phys_offset
+	ldr	x0, [x0, :lo12:__phys_offset]
+	adrp	x1, __phys_end
+	ldr	x1, [x1, :lo12:__phys_end]
+	dcache_by_line_op civac, sy, x0, x1, x2, x3
+	isb
+
 	ret
 
 /*
-- 
2.20.1

Re: [kvm-unit-tests PATCH v4 00/10] arm/arm64: Various fixes

[Andrew Jones]

On Fri, Jan 31, 2020 at 04:37:18PM +0000, Alexandru Elisei wrote:
> These are the patches that were left unmerged from the previous version of
> the series, plus a few new patches. Patch #1 "Makefile: Use
> no-stack-protector compiler options" is straightforward and came about
> because of a compile error I experienced on RockPro64.
> 
> Patches #3 and #5 are the result of Andre's comments on the previous
> version. When adding ISBs after register writes I noticed in the ARM ARM
> that a read of the timer counter value can be reordered, and patch #4
> tries to avoid that.
> 
> Patch #7 is also the result of a review comment. For the GIC tests, we wait
> up to 5 seconds for the interrupt to be asserted. However, the GIC tests
> can use more than one CPU, which is not the case with the timer test. And
> waiting for the GIC to assert the interrupt can happen up to 6 times (8
> times after patch #9), so I figured that a timeout of 10 seconds for the
> test is acceptable.
> 
> Patch #8 tries to improve the way we test how the timer generates the
> interrupt. If the GIC asserts the timer interrupt, but the device itself is
> not generating it, that's a pretty big problem.
> 
> Ran the same tests as before:
> 
> - with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
>   (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).
> 
> - with qemu, on an arm64 host kernel:
>     a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
>        GICv2 (Seattle).
>     b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.
> 
> Changes:
> * Patches #1, #3, #4, #5, #7, #8 are new.
> * For patch #2, as per Drew's suggestion, I changed the entry point to halt
>   because the test is supposed to test if CPU_ON is successful.
> * Removed the ISB from patch #6 because that was fixed by #3.
> * Moved the architecture dependent function init_dcache_line_size to
>   cpu_init in lib/arm/setup.c as per Drew's suggestion.
> 
> Alexandru Elisei (10):
>   Makefile: Use no-stack-protector compiler options
>   arm/arm64: psci: Don't run C code without stack or vectors
>   arm64: timer: Add ISB after register writes
>   arm64: timer: Add ISB before reading the counter value
>   arm64: timer: Make irq_received volatile
>   arm64: timer: EOIR the interrupt after masking the timer
>   arm64: timer: Wait for the GIC to sample timer interrupt state
>   arm64: timer: Check the timer interrupt state
>   arm64: timer: Test behavior when timer disabled or masked
>   arm/arm64: Perform dcache clean + invalidate after turning MMU off
> 
>  Makefile                  |  4 +-
>  lib/arm/asm/processor.h   | 13 +++++++
>  lib/arm64/asm/processor.h | 12 ++++++
>  lib/arm/setup.c           |  8 ++++
>  arm/cstart.S              | 22 +++++++++++
>  arm/cstart64.S            | 23 ++++++++++++
>  arm/psci.c                | 15 ++++++--
>  arm/timer.c               | 79 ++++++++++++++++++++++++++++++++-------
>  arm/unittests.cfg         |  2 +-
>  9 files changed, 158 insertions(+), 20 deletions(-)
> 
> -- 
> 2.20.1
>

The series looks good to me. The first patch probably could have been
posted separately, but I'll try to test the whole series tomorrow. If
all looks well, I'll prepare a pull request for Paolo.

Thanks,
drew 

Re: [kvm-unit-tests PATCH v4 00/10] arm/arm64: Various fixes

[Alexandru Elisei]

Hi Andrew,

On 2/3/20 6:59 PM, Andrew Jones wrote:
> On Fri, Jan 31, 2020 at 04:37:18PM +0000, Alexandru Elisei wrote:
>> These are the patches that were left unmerged from the previous version of
>> the series, plus a few new patches. Patch #1 "Makefile: Use
>> no-stack-protector compiler options" is straightforward and came about
>> because of a compile error I experienced on RockPro64.
>>
>> Patches #3 and #5 are the result of Andre's comments on the previous
>> version. When adding ISBs after register writes I noticed in the ARM ARM
>> that a read of the timer counter value can be reordered, and patch #4
>> tries to avoid that.
>>
>> Patch #7 is also the result of a review comment. For the GIC tests, we wait
>> up to 5 seconds for the interrupt to be asserted. However, the GIC tests
>> can use more than one CPU, which is not the case with the timer test. And
>> waiting for the GIC to assert the interrupt can happen up to 6 times (8
>> times after patch #9), so I figured that a timeout of 10 seconds for the
>> test is acceptable.
>>
>> Patch #8 tries to improve the way we test how the timer generates the
>> interrupt. If the GIC asserts the timer interrupt, but the device itself is
>> not generating it, that's a pretty big problem.
>>
>> Ran the same tests as before:
>>
>> - with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
>>   (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).
>>
>> - with qemu, on an arm64 host kernel:
>>     a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
>>        GICv2 (Seattle).
>>     b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.
>>
>> Changes:
>> * Patches #1, #3, #4, #5, #7, #8 are new.
>> * For patch #2, as per Drew's suggestion, I changed the entry point to halt
>>   because the test is supposed to test if CPU_ON is successful.
>> * Removed the ISB from patch #6 because that was fixed by #3.
>> * Moved the architecture dependent function init_dcache_line_size to
>>   cpu_init in lib/arm/setup.c as per Drew's suggestion.
>>
>> Alexandru Elisei (10):
>>   Makefile: Use no-stack-protector compiler options
>>   arm/arm64: psci: Don't run C code without stack or vectors
>>   arm64: timer: Add ISB after register writes
>>   arm64: timer: Add ISB before reading the counter value
>>   arm64: timer: Make irq_received volatile
>>   arm64: timer: EOIR the interrupt after masking the timer
>>   arm64: timer: Wait for the GIC to sample timer interrupt state
>>   arm64: timer: Check the timer interrupt state
>>   arm64: timer: Test behavior when timer disabled or masked
>>   arm/arm64: Perform dcache clean + invalidate after turning MMU off
>>
>>  Makefile                  |  4 +-
>>  lib/arm/asm/processor.h   | 13 +++++++
>>  lib/arm64/asm/processor.h | 12 ++++++
>>  lib/arm/setup.c           |  8 ++++
>>  arm/cstart.S              | 22 +++++++++++
>>  arm/cstart64.S            | 23 ++++++++++++
>>  arm/psci.c                | 15 ++++++--
>>  arm/timer.c               | 79 ++++++++++++++++++++++++++++++++-------
>>  arm/unittests.cfg         |  2 +-
>>  9 files changed, 158 insertions(+), 20 deletions(-)
>>
>> -- 
>> 2.20.1
>>
> The series looks good to me. The first patch probably could have been
> posted separately, but I'll try to test the whole series tomorrow. If

Noted, next time I will try to do a better job separating the patches. I found the
bug while testing the arm64 fixes, and I was getting ready to send the patches, so
I just figured I'll send it as part of the series.

> all looks well, I'll prepare a pull request for Paolo.

Thank you very much for taking the time to review the patches! Much appreciated.

Thanks,
Alex
>
> Thanks,
> drew 
>