[kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Alexandru Elisei]

This series implements two basic tests for KVM SPE: one that checks that
the reported features match what is specified in the architecture,
implemented in patch #3 ("arm64: spe: Add introspection test"), and another
test that checks that the buffer management interrupt is asserted
correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest
of the patches are either preparatory patches, or a fix, in the case of
patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers").

This series builds on Eric's initial version [1], to which I've added the
buffer tests that I used while developing SPE support for KVM.

KVM SPE support is current in RFC phase, hence why this series is also sent
as RFC. The KVM patches needed for the tests to work can be found at [2].
Userspace support is also necessary, which I've implemented for kvmtool;
this can be found at [3]. This series is also available in a repo [4] to make
testing easier.

[1] https://www.spinics.net/lists/kvm/msg223792.html
[2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3
[3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3
[4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2

Alexandru Elisei (3):
  lib/{bitops,alloc_page}.h: Add missing headers
  lib: arm/arm64: Add function to unmap a page
  am64: spe: Add buffer test

Eric Auger (2):
  arm64: Move get_id_aa64dfr0() in processor.h
  arm64: spe: Add introspection test

 arm/Makefile.arm64        |   1 +
 lib/arm/asm/mmu-api.h     |   1 +
 lib/arm64/asm/processor.h |   5 +
 lib/alloc_page.h          |   2 +
 lib/bitops.h              |   2 +
 lib/libcflat.h            |   1 +
 lib/arm/mmu.c             |  32 +++
 arm/pmu.c                 |   1 -
 arm/spe.c                 | 506 ++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg         |  15 ++
 10 files changed, 565 insertions(+), 1 deletion(-)
 create mode 100644 arm/spe.c

-- 
2.29.1

[kvm-unit-tests RFC PATCH v2 1/5] arm64: Move get_id_aa64dfr0() in processor.h

[Alexandru Elisei]

From: Eric Auger <eric.auger@redhat.com>

SPE support is reported in the ID_AA64DFR0_EL1 register. Move the function
get_id_aa64dfr0() from the pmu test to processor.h so it can be reused by
the SPE tests.

[ Alexandru E: Reworded commit ]

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm64/asm/processor.h | 5 +++++
 arm/pmu.c                 | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 02665b84cc7e..11b756475494 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -88,6 +88,11 @@ static inline uint64_t get_mpidr(void)
 	return read_sysreg(mpidr_el1);
 }
 
+static inline uint64_t get_id_aa64dfr0(void)
+{
+	return read_sysreg(id_aa64dfr0_el1);
+}
+
 #define MPIDR_HWID_BITMASK 0xff00ffffff
 extern int mpidr_to_cpu(uint64_t mpidr);
 
diff --git a/arm/pmu.c b/arm/pmu.c
index 831fb6618279..5406ca3b31ed 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -167,7 +167,6 @@ static void test_overflow_interrupt(void) {}
 #define ID_DFR0_PMU_V3_8_5	0b0110
 #define ID_DFR0_PMU_IMPDEF	0b1111
 
-static inline uint32_t get_id_aa64dfr0(void) { return read_sysreg(id_aa64dfr0_el1); }
 static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
 static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
 static inline uint64_t get_pmccntr(void) { return read_sysreg(pmccntr_el0); }
-- 
2.29.1

[kvm-unit-tests RFC PATCH v2 2/5] lib/{bitops,alloc_page}.h: Add missing headers

[Alexandru Elisei]

bitops.h uses the 'bool' and 'size_t' types, but doesn't include the
stddef.h and stdbool.h headers, where the types are defined. This can cause
the following error when compiling:

In file included from arm/new-test.c:9:
/path/to/kvm-unit-tests/lib/bitops.h:77:15: error: unknown type name 'bool'
   77 | static inline bool is_power_of_2(unsigned long n)
      |               ^~~~
/path/to/kvm-unit-tests/lib/bitops.h:82:38: error: unknown type name 'size_t'
   82 | static inline unsigned int get_order(size_t size)
      |                                      ^~~~~~
/path/to/kvm-unit-tests/lib/bitops.h:24:1: note: 'size_t' is defined in header '<stddef.h>'; did you forget to '#include <stddef.h>'?
   23 | #include <asm/bitops.h>
  +++ |+#include <stddef.h>
   24 |
make: *** [<builtin>: arm/new-test.o] Error 1

The same errors were observed when including alloc_page.h. Fix both files
by including stddef.h and stdbool.h.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/alloc_page.h | 2 ++
 lib/bitops.h     | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 88540d1def06..182862c43363 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -4,6 +4,8 @@
  * This is a simple allocator that provides contiguous physical addresses
  * with byte granularity.
  */
+#include <stdbool.h>
+#include <stddef.h>
 
 #ifndef ALLOC_PAGE_H
 #define ALLOC_PAGE_H 1
diff --git a/lib/bitops.h b/lib/bitops.h
index 308aa86514a8..5aeea0b998b1 100644
--- a/lib/bitops.h
+++ b/lib/bitops.h
@@ -1,5 +1,7 @@
 #ifndef _BITOPS_H_
 #define _BITOPS_H_
+#include <stdbool.h>
+#include <stddef.h>
 
 /*
  * Adapted from
-- 
2.29.1

Re: [kvm-unit-tests RFC PATCH v2 2/5] lib/{bitops, alloc_page}.h: Add missing headers

[Auger Eric]

Hi Alexandru,
On 10/27/20 6:19 PM, Alexandru Elisei wrote:
> bitops.h uses the 'bool' and 'size_t' types, but doesn't include the
> stddef.h and stdbool.h headers, where the types are defined. This can cause
> the following error when compiling:
> 
> In file included from arm/new-test.c:9:
> /path/to/kvm-unit-tests/lib/bitops.h:77:15: error: unknown type name 'bool'
>    77 | static inline bool is_power_of_2(unsigned long n)
>       |               ^~~~
> /path/to/kvm-unit-tests/lib/bitops.h:82:38: error: unknown type name 'size_t'
>    82 | static inline unsigned int get_order(size_t size)
>       |                                      ^~~~~~
> /path/to/kvm-unit-tests/lib/bitops.h:24:1: note: 'size_t' is defined in header '<stddef.h>'; did you forget to '#include <stddef.h>'?
>    23 | #include <asm/bitops.h>
>   +++ |+#include <stddef.h>
>    24 |
> make: *** [<builtin>: arm/new-test.o] Error 1
> 
> The same errors were observed when including alloc_page.h. Fix both files
> by including stddef.h and stdbool.h.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/alloc_page.h | 2 ++
>  lib/bitops.h     | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 88540d1def06..182862c43363 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -4,6 +4,8 @@
>   * This is a simple allocator that provides contiguous physical addresses
>   * with byte granularity.
>   */
> +#include <stdbool.h>
> +#include <stddef.h>
nit: you may move those includes after the #ifndef ALLOC_PAGE_H as it is
usually done.
>  
>  #ifndef ALLOC_PAGE_H
>  #define ALLOC_PAGE_H 1
> diff --git a/lib/bitops.h b/lib/bitops.h
> index 308aa86514a8..5aeea0b998b1 100644
> --- a/lib/bitops.h
> +++ b/lib/bitops.h
> @@ -1,5 +1,7 @@
>  #ifndef _BITOPS_H_
>  #define _BITOPS_H_
> +#include <stdbool.h>
> +#include <stddef.h>
>  
>  /*
>   * Adapted from
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [kvm-unit-tests RFC PATCH v2 2/5] lib/{bitops, alloc_page}.h: Add missing headers

[Alexandru Elisei]

Hi Eric,

On 10/30/20 3:29 PM, Auger Eric wrote:
> Hi Alexandru,
> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>> bitops.h uses the 'bool' and 'size_t' types, but doesn't include the
>> stddef.h and stdbool.h headers, where the types are defined. This can cause
>> the following error when compiling:
>>
>> In file included from arm/new-test.c:9:
>> /path/to/kvm-unit-tests/lib/bitops.h:77:15: error: unknown type name 'bool'
>>    77 | static inline bool is_power_of_2(unsigned long n)
>>       |               ^~~~
>> /path/to/kvm-unit-tests/lib/bitops.h:82:38: error: unknown type name 'size_t'
>>    82 | static inline unsigned int get_order(size_t size)
>>       |                                      ^~~~~~
>> /path/to/kvm-unit-tests/lib/bitops.h:24:1: note: 'size_t' is defined in header '<stddef.h>'; did you forget to '#include <stddef.h>'?
>>    23 | #include <asm/bitops.h>
>>   +++ |+#include <stddef.h>
>>    24 |
>> make: *** [<builtin>: arm/new-test.o] Error 1
>>
>> The same errors were observed when including alloc_page.h. Fix both files
>> by including stddef.h and stdbool.h.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  lib/alloc_page.h | 2 ++
>>  lib/bitops.h     | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
>> index 88540d1def06..182862c43363 100644
>> --- a/lib/alloc_page.h
>> +++ b/lib/alloc_page.h
>> @@ -4,6 +4,8 @@
>>   * This is a simple allocator that provides contiguous physical addresses
>>   * with byte granularity.
>>   */
>> +#include <stdbool.h>
>> +#include <stddef.h>
> nit: you may move those includes after the #ifndef ALLOC_PAGE_H as it is
> usually done.

Oops, you're right, I missed that, will change.

>>  
>>  #ifndef ALLOC_PAGE_H
>>  #define ALLOC_PAGE_H 1
>> diff --git a/lib/bitops.h b/lib/bitops.h
>> index 308aa86514a8..5aeea0b998b1 100644
>> --- a/lib/bitops.h
>> +++ b/lib/bitops.h
>> @@ -1,5 +1,7 @@
>>  #ifndef _BITOPS_H_
>>  #define _BITOPS_H_
>> +#include <stdbool.h>
>> +#include <stddef.h>
>>  
>>  /*
>>   * Adapted from
>>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks,

Alex

[kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Alexandru Elisei]

From: Eric Auger <eric.auger@redhat.com>

Probe the DTB and the ID registers to get information about SPE, then
compare the register fields with the valid values as defined by ARM DDI
0487F.b.

SPE is supported only on AArch64, so make the test exclusive to the
arm64 architecture.

[ Alexandru E: Removed aarch32 compilation support, added DTB probing,
	reworded commit, mostly cosmetic changes to the code ]

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/Makefile.arm64 |   1 +
 lib/libcflat.h     |   1 +
 arm/spe.c          | 172 +++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg  |   7 ++
 4 files changed, 181 insertions(+)
 create mode 100644 arm/spe.c

diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
index dbc7524d3070..94b9c63f0b05 100644
--- a/arm/Makefile.arm64
+++ b/arm/Makefile.arm64
@@ -30,6 +30,7 @@ OBJDIRS += lib/arm64
 tests = $(TEST_DIR)/timer.flat
 tests += $(TEST_DIR)/micro-bench.flat
 tests += $(TEST_DIR)/cache.flat
+tests += $(TEST_DIR)/spe.flat
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
 
diff --git a/lib/libcflat.h b/lib/libcflat.h
index ec0f58b05701..37550c99ffb6 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -37,6 +37,7 @@
 #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
 
 #define SZ_256			(1 << 8)
+#define SZ_2K			(1 << 11)
 #define SZ_4K			(1 << 12)
 #define SZ_8K			(1 << 13)
 #define SZ_16K			(1 << 14)
diff --git a/arm/spe.c b/arm/spe.c
new file mode 100644
index 000000000000..c199cd239194
--- /dev/null
+++ b/arm/spe.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2020, Red Hat Inc, Eric Auger <eric.auger@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 2.1 and
+ * only version 2.1 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
+ * for more details.
+ */
+#include <stdint.h>
+
+#include <bitops.h>
+#include <devicetree.h>
+#include <libcflat.h>
+
+#include <asm/gic.h>
+#include <asm/processor.h>
+#include <asm/sysreg.h>
+
+#define ID_AA64DFR0_PMSVER_SHIFT	32
+#define ID_AA64DFR0_PMSVER_MASK		0xf
+
+#define SYS_PMBIDR_EL1			sys_reg(3, 0, 9, 10, 7)
+#define SYS_PMBIDR_EL1_F_SHIFT		5
+#define SYS_PMBIDR_EL1_P_SHIFT		4
+#define SYS_PMBIDR_EL1_ALIGN_MASK	0xfUL
+#define SYS_PMBIDR_EL1_ALIGN_SHIFT	0
+
+#define SYS_PMSIDR_EL1			sys_reg(3, 0, 9, 9, 7)
+#define SYS_PMSIDR_EL1_FE_SHIFT		0
+#define SYS_PMSIDR_EL1_FT_SHIFT		1
+#define SYS_PMSIDR_EL1_FL_SHIFT		2
+#define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
+#define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
+#define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
+#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
+#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
+#define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
+#define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
+
+struct spe {
+	uint32_t intid;
+	int min_interval;
+	int max_record_size;
+	int countsize;
+	bool fl_cap;
+	bool ft_cap;
+	bool fe_cap;
+	int align;
+};
+static struct spe spe;
+
+static int spe_min_interval(uint8_t interval)
+{
+	switch (interval) {
+	case 0x0:
+		return 256;
+	case 0x2:
+		return 512;
+	case 0x3:
+		return 768;
+	case 0x4:
+		return 1024;
+	case 0x5:
+		return 1536;
+	case 0x6:
+		return 2048;
+	case 0x7:
+		return 3072;
+	case 0x8:
+		return 4096;
+	default:
+		return 0;
+	}
+}
+
+static bool spe_probe(void)
+{
+	const struct fdt_property *prop;
+	const void *fdt = dt_fdt();
+	int node, len;
+	uint32_t *data;
+	uint64_t pmbidr, pmsidr;
+	uint64_t aa64dfr0 = get_id_aa64dfr0();
+	uint8_t pmsver, interval;
+
+	node = fdt_node_offset_by_compatible(fdt, -1, "arm,statistical-profiling-extension-v1");
+	assert(node >= 0);
+	prop = fdt_get_property(fdt, node, "interrupts", &len);
+	assert(prop && len == 3 * sizeof(u32));
+
+	data = (u32 *)prop->data;
+	/* SPE interrupt is required to be a PPI. */
+	assert(fdt32_to_cpu(data[0]) == 1);
+	spe.intid = fdt32_to_cpu(data[1]);
+
+	pmsver = (aa64dfr0 >> ID_AA64DFR0_PMSVER_SHIFT) & ID_AA64DFR0_PMSVER_MASK;
+	if (!pmsver || pmsver > 2) {
+		report_info("Unknown SPE version: 0x%x", pmsver);
+		return false;
+	}
+
+	pmbidr = read_sysreg_s(SYS_PMBIDR_EL1);
+	if (pmbidr & BIT(SYS_PMBIDR_EL1_P_SHIFT)) {
+		report_info("Profiling buffer owned by higher exception level");
+		return false;
+	}
+
+	spe.align = (pmbidr >> SYS_PMBIDR_EL1_ALIGN_SHIFT) & SYS_PMBIDR_EL1_ALIGN_MASK;
+	spe.align = 1 << spe.align;
+
+	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
+
+	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
+	spe.min_interval = spe_min_interval(interval);
+
+	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
+		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
+	spe.max_record_size = 1 << spe.max_record_size;
+
+	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
+			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
+
+	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
+	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
+	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
+
+	return true;
+}
+
+static void spe_test_introspection(void)
+{
+	report_prefix_push("spe-introspection");
+
+	report(spe.align <= SZ_2K, "PMBIDR_E1.Align");
+	report(spe.countsize == 0x2, "PMSIDR_EL1.CountSize");
+	report(spe.max_record_size >= 16 && spe.max_record_size <= 2048,
+			"PMSIDR_EL1 maximum record size");
+	report(spe.min_interval >= 256 && spe.min_interval <= 4096,
+			"PMSIDR_EL1 minimum sampling interval");
+	report(spe.fl_cap && spe.ft_cap && spe.fe_cap, "PMSIDR_EL1 sampling filters");
+
+	report_prefix_pop();
+}
+
+int main(int argc, char *argv[])
+{
+	if (!spe_probe()) {
+		report_skip("SPE not supported");
+		return report_summary();
+	}
+
+	printf("intid:           %u\n", PPI(spe.intid));
+	printf("align: 	         %d\n", spe.align);
+	printf("min_interval:    %d\n", spe.min_interval);
+	printf("max_record_size: %d\n", spe.max_record_size);
+
+	if (argc < 2)
+		report_abort("no test specified");
+
+	report_prefix_push("spe");
+
+	if (strcmp(argv[1], "spe-introspection") == 0)
+		spe_test_introspection();
+	else
+		report_abort("Unknown subtest '%s'", argv[1]);
+
+	return report_summary();
+}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index f776b66ef96d..ad10be123774 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -134,6 +134,13 @@ extra_params = -append 'pmu-overflow-interrupt'
 #groups = pmu
 #accel = tcg
 
+[spe-introspection]
+file = spe.flat
+groups = spe
+arch = arm64
+accel = kvm
+extra_params = -append 'spe-introspection'
+
 # Test GIC emulation
 [gicv2-ipi]
 file = gic.flat
-- 
2.29.1

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Auger Eric]

Hi Alexandru,

On 10/27/20 6:19 PM, Alexandru Elisei wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> Probe the DTB and the ID registers to get information about SPE, then
> compare the register fields with the valid values as defined by ARM DDI
> 0487F.b.
> 
> SPE is supported only on AArch64, so make the test exclusive to the
> arm64 architecture.
> 
> [ Alexandru E: Removed aarch32 compilation support, added DTB probing,
> 	reworded commit, mostly cosmetic changes to the code ]all those changes make sense to me.
+ spe_buffer not allocated in spe_probe() anymore.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/Makefile.arm64 |   1 +
>  lib/libcflat.h     |   1 +
>  arm/spe.c          | 172 +++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg  |   7 ++
>  4 files changed, 181 insertions(+)
>  create mode 100644 arm/spe.c
> 
> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> index dbc7524d3070..94b9c63f0b05 100644
> --- a/arm/Makefile.arm64
> +++ b/arm/Makefile.arm64
> @@ -30,6 +30,7 @@ OBJDIRS += lib/arm64
>  tests = $(TEST_DIR)/timer.flat
>  tests += $(TEST_DIR)/micro-bench.flat
>  tests += $(TEST_DIR)/cache.flat
> +tests += $(TEST_DIR)/spe.flat
>  
>  include $(SRCDIR)/$(TEST_DIR)/Makefile.common
>  
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index ec0f58b05701..37550c99ffb6 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -37,6 +37,7 @@
>  #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
>  
>  #define SZ_256			(1 << 8)
> +#define SZ_2K			(1 << 11)
>  #define SZ_4K			(1 << 12)
>  #define SZ_8K			(1 << 13)
>  #define SZ_16K			(1 << 14)
> diff --git a/arm/spe.c b/arm/spe.c
> new file mode 100644
> index 000000000000..c199cd239194
> --- /dev/null
> +++ b/arm/spe.c
> @@ -0,0 +1,172 @@
> +/*
> + * Copyright (C) 2020, Red Hat Inc, Eric Auger <eric.auger@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 2.1 and
> + * only version 2.1 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
> + * for more details.
> + */
> +#include <stdint.h>
> +
> +#include <bitops.h>
> +#include <devicetree.h>
> +#include <libcflat.h>
> +
> +#include <asm/gic.h>
> +#include <asm/processor.h>
> +#include <asm/sysreg.h>
> +
> +#define ID_AA64DFR0_PMSVER_SHIFT	32
> +#define ID_AA64DFR0_PMSVER_MASK		0xf
> +
> +#define SYS_PMBIDR_EL1			sys_reg(3, 0, 9, 10, 7)
> +#define SYS_PMBIDR_EL1_F_SHIFT		5
> +#define SYS_PMBIDR_EL1_P_SHIFT		4
> +#define SYS_PMBIDR_EL1_ALIGN_MASK	0xfUL
> +#define SYS_PMBIDR_EL1_ALIGN_SHIFT	0
> +
> +#define SYS_PMSIDR_EL1			sys_reg(3, 0, 9, 9, 7)
> +#define SYS_PMSIDR_EL1_FE_SHIFT		0
> +#define SYS_PMSIDR_EL1_FT_SHIFT		1
> +#define SYS_PMSIDR_EL1_FL_SHIFT		2
> +#define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
> +#define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
> +#define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
> +#define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
> +#define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
> +
> +struct spe {
> +	uint32_t intid;
> +	int min_interval;
> +	int max_record_size;
> +	int countsize;
> +	bool fl_cap;
> +	bool ft_cap;
> +	bool fe_cap;
> +	int align;
> +};
> +static struct spe spe;
> +
> +static int spe_min_interval(uint8_t interval)
> +{
> +	switch (interval) {
> +	case 0x0:
> +		return 256;
> +	case 0x2:
> +		return 512;
> +	case 0x3:
> +		return 768;
> +	case 0x4:
> +		return 1024;
> +	case 0x5:
> +		return 1536;
> +	case 0x6:
> +		return 2048;
> +	case 0x7:
> +		return 3072;
> +	case 0x8:
> +		return 4096;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static bool spe_probe(void)
> +{
> +	const struct fdt_property *prop;
> +	const void *fdt = dt_fdt();
> +	int node, len;
> +	uint32_t *data;
> +	uint64_t pmbidr, pmsidr;
> +	uint64_t aa64dfr0 = get_id_aa64dfr0();
> +	uint8_t pmsver, interval;
> +
> +	node = fdt_node_offset_by_compatible(fdt, -1, "arm,statistical-profiling-extension-v1");
> +	assert(node >= 0);
> +	prop = fdt_get_property(fdt, node, "interrupts", &len);
> +	assert(prop && len == 3 * sizeof(u32));
> +
> +	data = (u32 *)prop->data;
> +	/* SPE interrupt is required to be a PPI. */
> +	assert(fdt32_to_cpu(data[0]) == 1);
> +	spe.intid = fdt32_to_cpu(data[1]);
> +
> +	pmsver = (aa64dfr0 >> ID_AA64DFR0_PMSVER_SHIFT) & ID_AA64DFR0_PMSVER_MASK;
> +	if (!pmsver || pmsver > 2) {
> +		report_info("Unknown SPE version: 0x%x", pmsver);
> +		return false;
> +	}
> +
> +	pmbidr = read_sysreg_s(SYS_PMBIDR_EL1);
> +	if (pmbidr & BIT(SYS_PMBIDR_EL1_P_SHIFT)) {
> +		report_info("Profiling buffer owned by higher exception level");
> +		return false;
> +	}
> +
> +	spe.align = (pmbidr >> SYS_PMBIDR_EL1_ALIGN_SHIFT) & SYS_PMBIDR_EL1_ALIGN_MASK;
you can remove ">> SYS_PMBIDR_EL1_ALIGN_SHIFT" here and
SYS_PMBIDR_EL1_ALIGN_SHIFT (0)
> +	spe.align = 1 << spe.align;
> +
> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
> +
> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
> +	spe.min_interval = spe_min_interval(interval);
> +
> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
> +	spe.max_record_size = 1 << spe.max_record_size;
> +
> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
> +
> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);

Why did you remove the report_info() section? I think those human
readable info can be useful.
> +
> +	return true;
> +}
> +
> +static void spe_test_introspection(void)
> +{
> +	report_prefix_push("spe-introspection");
I see you moved this here. gic and pmu tests seem to leave them in the
main, as done originally.
> +
> +	report(spe.align <= SZ_2K, "PMBIDR_E1.Align");
good check added
> +	report(spe.countsize == 0x2, "PMSIDR_EL1.CountSize");
> +	report(spe.max_record_size >= 16 && spe.max_record_size <= 2048,
> +			"PMSIDR_EL1 maximum record size");
> +	report(spe.min_interval >= 256 && spe.min_interval <= 4096,
> +			"PMSIDR_EL1 minimum sampling interval");
> +	report(spe.fl_cap && spe.ft_cap && spe.fe_cap, "PMSIDR_EL1 sampling filters");
indeed all bits read as 1 as the spec says. which is bit weird by the way.
> +
> +	report_prefix_pop();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	if (!spe_probe()) {
> +		report_skip("SPE not supported");
> +		return report_summary();
> +	}
> +
> +	printf("intid:           %u\n", PPI(spe.intid));
> +	printf("align: 	         %d\n", spe.align);
> +	printf("min_interval:    %d\n", spe.min_interval);
> +	printf("max_record_size: %d\n", spe.max_record_size);
> +
> +	if (argc < 2)
> +		report_abort("no test specified");
> +
> +	report_prefix_push("spe");
> +
> +	if (strcmp(argv[1], "spe-introspection") == 0)
> +		spe_test_introspection();
> +	else
> +		report_abort("Unknown subtest '%s'", argv[1]);
> +
> +	return report_summary();
> +}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index f776b66ef96d..ad10be123774 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -134,6 +134,13 @@ extra_params = -append 'pmu-overflow-interrupt'
>  #groups = pmu
>  #accel = tcg
>  
> +[spe-introspection]
> +file = spe.flat
> +groups = spe
> +arch = arm64
> +accel = kvm
> +extra_params = -append 'spe-introspection'
> +
>  # Test GIC emulation
>  [gicv2-ipi]
>  file = gic.flat
> 
Thanks

Eric

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Alexandru Elisei]

Hi Eric,

On 10/30/20 3:29 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> Probe the DTB and the ID registers to get information about SPE, then
>> compare the register fields with the valid values as defined by ARM DDI
>> 0487F.b.
>>
>> SPE is supported only on AArch64, so make the test exclusive to the
>> arm64 architecture.
>>
>> [ Alexandru E: Removed aarch32 compilation support, added DTB probing,
>> 	reworded commit, mostly cosmetic changes to the code ]all those changes make sense to me.
> + spe_buffer not allocated in spe_probe() anymore.

Sure, will add that.

>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/Makefile.arm64 |   1 +
>>  lib/libcflat.h     |   1 +
>>  arm/spe.c          | 172 +++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg  |   7 ++
>>  4 files changed, 181 insertions(+)
>>  create mode 100644 arm/spe.c
>>
>> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
>> index dbc7524d3070..94b9c63f0b05 100644
>> --- a/arm/Makefile.arm64
>> +++ b/arm/Makefile.arm64
>> @@ -30,6 +30,7 @@ OBJDIRS += lib/arm64
>>  tests = $(TEST_DIR)/timer.flat
>>  tests += $(TEST_DIR)/micro-bench.flat
>>  tests += $(TEST_DIR)/cache.flat
>> +tests += $(TEST_DIR)/spe.flat
>>  
>>  include $(SRCDIR)/$(TEST_DIR)/Makefile.common
>>  
>> diff --git a/lib/libcflat.h b/lib/libcflat.h
>> index ec0f58b05701..37550c99ffb6 100644
>> --- a/lib/libcflat.h
>> +++ b/lib/libcflat.h
>> @@ -37,6 +37,7 @@
>>  #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
>>  
>>  #define SZ_256			(1 << 8)
>> +#define SZ_2K			(1 << 11)
>>  #define SZ_4K			(1 << 12)
>>  #define SZ_8K			(1 << 13)
>>  #define SZ_16K			(1 << 14)
>> diff --git a/arm/spe.c b/arm/spe.c
>> new file mode 100644
>> index 000000000000..c199cd239194
>> --- /dev/null
>> +++ b/arm/spe.c
>> @@ -0,0 +1,172 @@
>> +/*
>> + * Copyright (C) 2020, Red Hat Inc, Eric Auger <eric.auger@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Lesser General Public License version 2.1 and
>> + * only version 2.1 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
>> + * for more details.
>> + */
>> +#include <stdint.h>
>> +
>> +#include <bitops.h>
>> +#include <devicetree.h>
>> +#include <libcflat.h>
>> +
>> +#include <asm/gic.h>
>> +#include <asm/processor.h>
>> +#include <asm/sysreg.h>
>> +
>> +#define ID_AA64DFR0_PMSVER_SHIFT	32
>> +#define ID_AA64DFR0_PMSVER_MASK		0xf
>> +
>> +#define SYS_PMBIDR_EL1			sys_reg(3, 0, 9, 10, 7)
>> +#define SYS_PMBIDR_EL1_F_SHIFT		5
>> +#define SYS_PMBIDR_EL1_P_SHIFT		4
>> +#define SYS_PMBIDR_EL1_ALIGN_MASK	0xfUL
>> +#define SYS_PMBIDR_EL1_ALIGN_SHIFT	0
>> +
>> +#define SYS_PMSIDR_EL1			sys_reg(3, 0, 9, 9, 7)
>> +#define SYS_PMSIDR_EL1_FE_SHIFT		0
>> +#define SYS_PMSIDR_EL1_FT_SHIFT		1
>> +#define SYS_PMSIDR_EL1_FL_SHIFT		2
>> +#define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
>> +#define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
>> +#define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
>> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
>> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
>> +#define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
>> +#define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
>> +
>> +struct spe {
>> +	uint32_t intid;
>> +	int min_interval;
>> +	int max_record_size;
>> +	int countsize;
>> +	bool fl_cap;
>> +	bool ft_cap;
>> +	bool fe_cap;
>> +	int align;
>> +};
>> +static struct spe spe;
>> +
>> +static int spe_min_interval(uint8_t interval)
>> +{
>> +	switch (interval) {
>> +	case 0x0:
>> +		return 256;
>> +	case 0x2:
>> +		return 512;
>> +	case 0x3:
>> +		return 768;
>> +	case 0x4:
>> +		return 1024;
>> +	case 0x5:
>> +		return 1536;
>> +	case 0x6:
>> +		return 2048;
>> +	case 0x7:
>> +		return 3072;
>> +	case 0x8:
>> +		return 4096;
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static bool spe_probe(void)
>> +{
>> +	const struct fdt_property *prop;
>> +	const void *fdt = dt_fdt();
>> +	int node, len;
>> +	uint32_t *data;
>> +	uint64_t pmbidr, pmsidr;
>> +	uint64_t aa64dfr0 = get_id_aa64dfr0();
>> +	uint8_t pmsver, interval;
>> +
>> +	node = fdt_node_offset_by_compatible(fdt, -1, "arm,statistical-profiling-extension-v1");
>> +	assert(node >= 0);
>> +	prop = fdt_get_property(fdt, node, "interrupts", &len);
>> +	assert(prop && len == 3 * sizeof(u32));
>> +
>> +	data = (u32 *)prop->data;
>> +	/* SPE interrupt is required to be a PPI. */
>> +	assert(fdt32_to_cpu(data[0]) == 1);
>> +	spe.intid = fdt32_to_cpu(data[1]);
>> +
>> +	pmsver = (aa64dfr0 >> ID_AA64DFR0_PMSVER_SHIFT) & ID_AA64DFR0_PMSVER_MASK;
>> +	if (!pmsver || pmsver > 2) {
>> +		report_info("Unknown SPE version: 0x%x", pmsver);
>> +		return false;
>> +	}
>> +
>> +	pmbidr = read_sysreg_s(SYS_PMBIDR_EL1);
>> +	if (pmbidr & BIT(SYS_PMBIDR_EL1_P_SHIFT)) {
>> +		report_info("Profiling buffer owned by higher exception level");
>> +		return false;
>> +	}
>> +
>> +	spe.align = (pmbidr >> SYS_PMBIDR_EL1_ALIGN_SHIFT) & SYS_PMBIDR_EL1_ALIGN_MASK;
> you can remove ">> SYS_PMBIDR_EL1_ALIGN_SHIFT" here and
> SYS_PMBIDR_EL1_ALIGN_SHIFT (0)

I prefer to keep it to match the way we extract the values for the other fields,
even though I realize it's unnecessary (the compiler will optimize it anyway).

>> +	spe.align = 1 << spe.align;
>> +
>> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
>> +
>> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
>> +	spe.min_interval = spe_min_interval(interval);
>> +
>> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
>> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
>> +	spe.max_record_size = 1 << spe.max_record_size;
>> +
>> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
>> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
>> +
>> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
>> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
>> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
> Why did you remove the report_info() section? I think those human
> readable info can be useful.

I made them part of the test. Since the architecture says they are 1, I think
making sure their value matches is more useful than printing something that the
architecture guarantees.

>> +
>> +	return true;
>> +}
>> +
>> +static void spe_test_introspection(void)
>> +{
>> +	report_prefix_push("spe-introspection");
> I see you moved this here. gic and pmu tests seem to leave them in the
> main, as done originally.

You're right, consistency is good, will put them in main before calling the test
function.

>> +
>> +	report(spe.align <= SZ_2K, "PMBIDR_E1.Align");
> good check added
>> +	report(spe.countsize == 0x2, "PMSIDR_EL1.CountSize");
>> +	report(spe.max_record_size >= 16 && spe.max_record_size <= 2048,
>> +			"PMSIDR_EL1 maximum record size");
>> +	report(spe.min_interval >= 256 && spe.min_interval <= 4096,
>> +			"PMSIDR_EL1 minimum sampling interval");
>> +	report(spe.fl_cap && spe.ft_cap && spe.fe_cap, "PMSIDR_EL1 sampling filters");
> indeed all bits read as 1 as the spec says. which is bit weird by the way.

I think they added the fields because the architects wanted the flexibility to
allow changes in the future.

Thanks,
Alex
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	if (!spe_probe()) {
>> +		report_skip("SPE not supported");
>> +		return report_summary();
>> +	}
>> +
>> +	printf("intid:           %u\n", PPI(spe.intid));
>> +	printf("align: 	         %d\n", spe.align);
>> +	printf("min_interval:    %d\n", spe.min_interval);
>> +	printf("max_record_size: %d\n", spe.max_record_size);
>> +
>> +	if (argc < 2)
>> +		report_abort("no test specified");
>> +
>> +	report_prefix_push("spe");
>> +
>> +	if (strcmp(argv[1], "spe-introspection") == 0)
>> +		spe_test_introspection();
>> +	else
>> +		report_abort("Unknown subtest '%s'", argv[1]);
>> +
>> +	return report_summary();
>> +}
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index f776b66ef96d..ad10be123774 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -134,6 +134,13 @@ extra_params = -append 'pmu-overflow-interrupt'
>>  #groups = pmu
>>  #accel = tcg
>>  
>> +[spe-introspection]
>> +file = spe.flat
>> +groups = spe
>> +arch = arm64
>> +accel = kvm
>> +extra_params = -append 'spe-introspection'
>> +
>>  # Test GIC emulation
>>  [gicv2-ipi]
>>  file = gic.flat
>>
> Thanks
>
> Eric
>

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Auger Eric]

Hi Alexandru,

On 10/30/20 4:59 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 10/30/20 3:29 PM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>>> From: Eric Auger <eric.auger@redhat.com>
>>>
>>> Probe the DTB and the ID registers to get information about SPE, then
>>> compare the register fields with the valid values as defined by ARM DDI
>>> 0487F.b.
>>>
>>> SPE is supported only on AArch64, so make the test exclusive to the
>>> arm64 architecture.
>>>
>>> [ Alexandru E: Removed aarch32 compilation support, added DTB probing,
>>> 	reworded commit, mostly cosmetic changes to the code ]all those changes make sense to me.
all those changes make sense to me (missing returned line above)
>> + spe_buffer not allocated in spe_probe() anymore.
> 
> Sure, will add that.
> 
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  arm/Makefile.arm64 |   1 +
>>>  lib/libcflat.h     |   1 +
>>>  arm/spe.c          | 172 +++++++++++++++++++++++++++++++++++++++++++++
>>>  arm/unittests.cfg  |   7 ++
>>>  4 files changed, 181 insertions(+)
>>>  create mode 100644 arm/spe.c
>>>
>>> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
>>> index dbc7524d3070..94b9c63f0b05 100644
>>> --- a/arm/Makefile.arm64
>>> +++ b/arm/Makefile.arm64
>>> @@ -30,6 +30,7 @@ OBJDIRS += lib/arm64
>>>  tests = $(TEST_DIR)/timer.flat
>>>  tests += $(TEST_DIR)/micro-bench.flat
>>>  tests += $(TEST_DIR)/cache.flat
>>> +tests += $(TEST_DIR)/spe.flat
>>>  
>>>  include $(SRCDIR)/$(TEST_DIR)/Makefile.common
>>>  
>>> diff --git a/lib/libcflat.h b/lib/libcflat.h
>>> index ec0f58b05701..37550c99ffb6 100644
>>> --- a/lib/libcflat.h
>>> +++ b/lib/libcflat.h
>>> @@ -37,6 +37,7 @@
>>>  #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
>>>  
>>>  #define SZ_256			(1 << 8)
>>> +#define SZ_2K			(1 << 11)
>>>  #define SZ_4K			(1 << 12)
>>>  #define SZ_8K			(1 << 13)
>>>  #define SZ_16K			(1 << 14)
>>> diff --git a/arm/spe.c b/arm/spe.c
>>> new file mode 100644
>>> index 000000000000..c199cd239194
>>> --- /dev/null
>>> +++ b/arm/spe.c
>>> @@ -0,0 +1,172 @@
>>> +/*
>>> + * Copyright (C) 2020, Red Hat Inc, Eric Auger <eric.auger@redhat.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU Lesser General Public License version 2.1 and
>>> + * only version 2.1 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
>>> + * for more details.
>>> + */
>>> +#include <stdint.h>
>>> +
>>> +#include <bitops.h>
>>> +#include <devicetree.h>
>>> +#include <libcflat.h>
>>> +
>>> +#include <asm/gic.h>
>>> +#include <asm/processor.h>
>>> +#include <asm/sysreg.h>
>>> +
>>> +#define ID_AA64DFR0_PMSVER_SHIFT	32
>>> +#define ID_AA64DFR0_PMSVER_MASK		0xf
>>> +
>>> +#define SYS_PMBIDR_EL1			sys_reg(3, 0, 9, 10, 7)
>>> +#define SYS_PMBIDR_EL1_F_SHIFT		5
>>> +#define SYS_PMBIDR_EL1_P_SHIFT		4
>>> +#define SYS_PMBIDR_EL1_ALIGN_MASK	0xfUL
>>> +#define SYS_PMBIDR_EL1_ALIGN_SHIFT	0
>>> +
>>> +#define SYS_PMSIDR_EL1			sys_reg(3, 0, 9, 9, 7)
>>> +#define SYS_PMSIDR_EL1_FE_SHIFT		0
>>> +#define SYS_PMSIDR_EL1_FT_SHIFT		1
>>> +#define SYS_PMSIDR_EL1_FL_SHIFT		2
>>> +#define SYS_PMSIDR_EL1_INTERVAL_SHIFT	8
>>> +#define SYS_PMSIDR_EL1_INTERVAL_MASK	0xfUL
>>> +#define SYS_PMSIDR_EL1_MAXSIZE_SHIFT	12
>>> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
>>> +#define SYS_PMSIDR_EL1_MAXSIZE_MASK	0xfUL
>>> +#define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
>>> +#define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
>>> +
>>> +struct spe {
>>> +	uint32_t intid;
>>> +	int min_interval;
>>> +	int max_record_size;
>>> +	int countsize;
>>> +	bool fl_cap;
>>> +	bool ft_cap;
>>> +	bool fe_cap;
>>> +	int align;
>>> +};
>>> +static struct spe spe;
>>> +
>>> +static int spe_min_interval(uint8_t interval)
>>> +{
>>> +	switch (interval) {
>>> +	case 0x0:
>>> +		return 256;
>>> +	case 0x2:
>>> +		return 512;
>>> +	case 0x3:
>>> +		return 768;
>>> +	case 0x4:
>>> +		return 1024;
>>> +	case 0x5:
>>> +		return 1536;
>>> +	case 0x6:
>>> +		return 2048;
>>> +	case 0x7:
>>> +		return 3072;
>>> +	case 0x8:
>>> +		return 4096;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +static bool spe_probe(void)
>>> +{
>>> +	const struct fdt_property *prop;
>>> +	const void *fdt = dt_fdt();
>>> +	int node, len;
>>> +	uint32_t *data;
>>> +	uint64_t pmbidr, pmsidr;
>>> +	uint64_t aa64dfr0 = get_id_aa64dfr0();
>>> +	uint8_t pmsver, interval;
>>> +
>>> +	node = fdt_node_offset_by_compatible(fdt, -1, "arm,statistical-profiling-extension-v1");
>>> +	assert(node >= 0);
>>> +	prop = fdt_get_property(fdt, node, "interrupts", &len);
>>> +	assert(prop && len == 3 * sizeof(u32));
>>> +
>>> +	data = (u32 *)prop->data;
>>> +	/* SPE interrupt is required to be a PPI. */
>>> +	assert(fdt32_to_cpu(data[0]) == 1);
>>> +	spe.intid = fdt32_to_cpu(data[1]);
>>> +
>>> +	pmsver = (aa64dfr0 >> ID_AA64DFR0_PMSVER_SHIFT) & ID_AA64DFR0_PMSVER_MASK;
>>> +	if (!pmsver || pmsver > 2) {
>>> +		report_info("Unknown SPE version: 0x%x", pmsver);
>>> +		return false;
>>> +	}
>>> +
>>> +	pmbidr = read_sysreg_s(SYS_PMBIDR_EL1);
>>> +	if (pmbidr & BIT(SYS_PMBIDR_EL1_P_SHIFT)) {
>>> +		report_info("Profiling buffer owned by higher exception level");
>>> +		return false;
>>> +	}
>>> +
>>> +	spe.align = (pmbidr >> SYS_PMBIDR_EL1_ALIGN_SHIFT) & SYS_PMBIDR_EL1_ALIGN_MASK;
>> you can remove ">> SYS_PMBIDR_EL1_ALIGN_SHIFT" here and
>> SYS_PMBIDR_EL1_ALIGN_SHIFT (0)
> 
> I prefer to keep it to match the way we extract the values for the other fields,
> even though I realize it's unnecessary (the compiler will optimize it anyway).
ok
> 
>>> +	spe.align = 1 << spe.align;
>>> +
>>> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
>>> +
>>> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
>>> +	spe.min_interval = spe_min_interval(interval);
>>> +
>>> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
>>> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
>>> +	spe.max_record_size = 1 << spe.max_record_size;
>>> +
>>> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
>>> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
>>> +
>>> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
>>> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
>>> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
>> Why did you remove the report_info() section? I think those human
>> readable info can be useful.
> 
> I made them part of the test. Since the architecture says they are 1, I think
> making sure their value matches is more useful than printing something that the
> architecture guarantees.
OK for those caps which are always 1 anyway but I was more thinking about

report_info("Align= %d bytes, Min Interval=%d Single record Max Size =
%d bytes", spe.align, spe.min_interval, spe.maxsize);

I'd prefer to keep it.

> 
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static void spe_test_introspection(void)
>>> +{
>>> +	report_prefix_push("spe-introspection");
>> I see you moved this here. gic and pmu tests seem to leave them in the
>> main, as done originally.
> 
> You're right, consistency is good, will put them in main before calling the test
> function.
> 
>>> +
>>> +	report(spe.align <= SZ_2K, "PMBIDR_E1.Align");
>> good check added
>>> +	report(spe.countsize == 0x2, "PMSIDR_EL1.CountSize");
>>> +	report(spe.max_record_size >= 16 && spe.max_record_size <= 2048,
>>> +			"PMSIDR_EL1 maximum record size");
>>> +	report(spe.min_interval >= 256 && spe.min_interval <= 4096,
>>> +			"PMSIDR_EL1 minimum sampling interval");
>>> +	report(spe.fl_cap && spe.ft_cap && spe.fe_cap, "PMSIDR_EL1 sampling filters");
>> indeed all bits read as 1 as the spec says. which is bit weird by the way.
> 
> I think they added the fields because the architects wanted the flexibility to
> allow changes in the future.
OK

Thanks

Eric
> 
> Thanks,
> Alex
>>> +
>>> +	report_prefix_pop();
>>> +}
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> +	if (!spe_probe()) {
>>> +		report_skip("SPE not supported");
>>> +		return report_summary();
>>> +	}
>>> +
>>> +	printf("intid:           %u\n", PPI(spe.intid));
>>> +	printf("align: 	         %d\n", spe.align);
>>> +	printf("min_interval:    %d\n", spe.min_interval);
>>> +	printf("max_record_size: %d\n", spe.max_record_size);
>>> +
>>> +	if (argc < 2)
>>> +		report_abort("no test specified");
>>> +
>>> +	report_prefix_push("spe");
>>> +
>>> +	if (strcmp(argv[1], "spe-introspection") == 0)
>>> +		spe_test_introspection();
>>> +	else
>>> +		report_abort("Unknown subtest '%s'", argv[1]);
>>> +
>>> +	return report_summary();
>>> +}
>>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>>> index f776b66ef96d..ad10be123774 100644
>>> --- a/arm/unittests.cfg
>>> +++ b/arm/unittests.cfg
>>> @@ -134,6 +134,13 @@ extra_params = -append 'pmu-overflow-interrupt'
>>>  #groups = pmu
>>>  #accel = tcg
>>>  
>>> +[spe-introspection]
>>> +file = spe.flat
>>> +groups = spe
>>> +arch = arm64
>>> +accel = kvm
>>> +extra_params = -append 'spe-introspection'
>>> +
>>>  # Test GIC emulation
>>>  [gicv2-ipi]
>>>  file = gic.flat
>>>
>> Thanks
>>
>> Eric
>>
> 

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Alexandru Elisei]

Hi Eric,

On 10/30/20 5:09 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 10/30/20 4:59 PM, Alexandru Elisei wrote:
> [..]
>>>> +	spe.align = 1 << spe.align;
>>>> +
>>>> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
>>>> +
>>>> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
>>>> +	spe.min_interval = spe_min_interval(interval);
>>>> +
>>>> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
>>>> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
>>>> +	spe.max_record_size = 1 << spe.max_record_size;
>>>> +
>>>> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
>>>> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
>>>> +
>>>> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
>>>> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
>>>> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
>>> Why did you remove the report_info() section? I think those human
>>> readable info can be useful.
>> I made them part of the test. Since the architecture says they are 1, I think
>> making sure their value matches is more useful than printing something that the
>> architecture guarantees.
> OK for those caps which are always 1 anyway but I was more thinking about
>
> report_info("Align= %d bytes, Min Interval=%d Single record Max Size =
> %d bytes", spe.align, spe.min_interval, spe.maxsize);
>
> I'd prefer to keep it.

Oh, I think I see what you mean, I chose to print them using printf in main().
This is very similar to what the timer test does, only it does it directly in
main(), instead of calling another function (print_timer_info(), in the case of
the timer test). I can move the printfs to spe_probe() if that's what you prefer.

Thanks,

Alex

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Auger Eric]

Hi Alexandru,

On 10/30/20 6:50 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 10/30/20 5:09 PM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 10/30/20 4:59 PM, Alexandru Elisei wrote:
>> [..]
>>>>> +	spe.align = 1 << spe.align;
>>>>> +
>>>>> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
>>>>> +
>>>>> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
>>>>> +	spe.min_interval = spe_min_interval(interval);
>>>>> +
>>>>> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
>>>>> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
>>>>> +	spe.max_record_size = 1 << spe.max_record_size;
>>>>> +
>>>>> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
>>>>> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
>>>>> +
>>>>> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
>>>>> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
>>>>> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
>>>> Why did you remove the report_info() section? I think those human
>>>> readable info can be useful.
>>> I made them part of the test. Since the architecture says they are 1, I think
>>> making sure their value matches is more useful than printing something that the
>>> architecture guarantees.
>> OK for those caps which are always 1 anyway but I was more thinking about
>>
>> report_info("Align= %d bytes, Min Interval=%d Single record Max Size =
>> %d bytes", spe.align, spe.min_interval, spe.maxsize);
>>
>> I'd prefer to keep it.
> 
> Oh, I think I see what you mean, I chose to print them using printf in main().
> This is very similar to what the timer test does, only it does it directly in
> main(), instead of calling another function (print_timer_info(), in the case of
> the timer test). I can move the printfs to spe_probe() if that's what you prefer.
Ah OK I did not notice. Whatever the place if those traces are there I
am fine.

Thanks

Eric
> 
> Thanks,
> 
> Alex
> 

Re: [kvm-unit-tests RFC PATCH v2 3/5] arm64: spe: Add introspection test

[Andrew Jones]

On Fri, Oct 30, 2020 at 05:50:35PM +0000, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 10/30/20 5:09 PM, Auger Eric wrote:
> > Hi Alexandru,
> >
> > On 10/30/20 4:59 PM, Alexandru Elisei wrote:
> > [..]
> >>>> +	spe.align = 1 << spe.align;
> >>>> +
> >>>> +	pmsidr = read_sysreg_s(SYS_PMSIDR_EL1);
> >>>> +
> >>>> +	interval = (pmsidr >> SYS_PMSIDR_EL1_INTERVAL_SHIFT) & SYS_PMSIDR_EL1_INTERVAL_MASK;
> >>>> +	spe.min_interval = spe_min_interval(interval);
> >>>> +
> >>>> +	spe.max_record_size = (pmsidr >> SYS_PMSIDR_EL1_MAXSIZE_SHIFT) & \
> >>>> +		      SYS_PMSIDR_EL1_MAXSIZE_MASK;
> >>>> +	spe.max_record_size = 1 << spe.max_record_size;
> >>>> +
> >>>> +	spe.countsize = (pmsidr >> SYS_PMSIDR_EL1_COUNTSIZE_SHIFT) & \
> >>>> +			SYS_PMSIDR_EL1_COUNTSIZE_MASK;
> >>>> +
> >>>> +	spe.fl_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FL_SHIFT);
> >>>> +	spe.ft_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FT_SHIFT);
> >>>> +	spe.fe_cap = pmsidr & BIT(SYS_PMSIDR_EL1_FE_SHIFT);
> >>> Why did you remove the report_info() section? I think those human
> >>> readable info can be useful.
> >> I made them part of the test. Since the architecture says they are 1, I think
> >> making sure their value matches is more useful than printing something that the
> >> architecture guarantees.
> > OK for those caps which are always 1 anyway but I was more thinking about
> >
> > report_info("Align= %d bytes, Min Interval=%d Single record Max Size =
> > %d bytes", spe.align, spe.min_interval, spe.maxsize);
> >
> > I'd prefer to keep it.
> 
> Oh, I think I see what you mean, I chose to print them using printf in main().
> This is very similar to what the timer test does, only it does it directly in
> main(), instead of calling another function (print_timer_info(), in the case of
> the timer test). I can move the printfs to spe_probe() if that's what you prefer.

Please use report_info(). When tests fail it's popular to diff the failing
results against the passing results. Some output changes each run, even
in the same environment, and some output really isn't that important. We
should keep important, non-test test results in INFO messages. Then, when
diffing, we can filter out anything without a prefix.

(BTW, I was cc'ed, or maybe bcc'ed, on this series from the beginning.)

Thanks,
drew

> 
> Thanks,
> 
> Alex
> 

[kvm-unit-tests RFC PATCH v2 4/5] lib: arm/arm64: Add function to unmap a page

[Alexandru Elisei]

Being able to cause a stage 1 data abort might be useful for future tests.
Add a function that unmaps a page from the translation tables.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/asm/mmu-api.h |  1 +
 lib/arm/mmu.c         | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 2bbe1faea900..305f77c6501f 100644
--- a/lib/arm/asm/mmu-api.h
+++ b/lib/arm/asm/mmu-api.h
@@ -23,4 +23,5 @@ extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
 			       phys_addr_t phys_start, phys_addr_t phys_end,
 			       pgprot_t prot);
 extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
+extern void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr);
 #endif
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 540a1e842d5b..72ac0be8d146 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -232,3 +232,35 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
 out_flush_tlb:
 	flush_tlb_page(vaddr);
 }
+
+void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr)
+{
+	pgd_t *pgd;
+	pmd_t *pmd;
+	pte_t *pte;
+
+	if (!mmu_enabled())
+		return;
+
+	pgd = pgd_offset(pgtable, vaddr);
+	if (!pgd_valid(*pgd))
+		return;
+
+	pmd = pmd_offset(pgd, vaddr);
+	if (!pmd_valid(*pmd))
+		return;
+
+	if (pmd_huge(*pmd)) {
+		WRITE_ONCE(*pmd, 0);
+		goto out_flush_tlb;
+	} else {
+		pte = pte_offset(pmd, vaddr);
+		if (!pte_valid(*pte))
+			return;
+		WRITE_ONCE(*pte, 0);
+		goto out_flush_tlb;
+	}
+
+out_flush_tlb:
+	flush_tlb_page(vaddr);
+}
-- 
2.29.1

Re: [kvm-unit-tests RFC PATCH v2 4/5] lib: arm/arm64: Add function to unmap a page

[Auger Eric]

Hi Alexandru,

On 10/27/20 6:19 PM, Alexandru Elisei wrote:
> Being able to cause a stage 1 data abort might be useful for future tests.
> Add a function that unmaps a page from the translation tables.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm/asm/mmu-api.h |  1 +
>  lib/arm/mmu.c         | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> index 2bbe1faea900..305f77c6501f 100644
> --- a/lib/arm/asm/mmu-api.h
> +++ b/lib/arm/asm/mmu-api.h
> @@ -23,4 +23,5 @@ extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
>  			       phys_addr_t phys_start, phys_addr_t phys_end,
>  			       pgprot_t prot);
>  extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
> +extern void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr);
>  #endif
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 540a1e842d5b..72ac0be8d146 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -232,3 +232,35 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>  out_flush_tlb:
>  	flush_tlb_page(vaddr);
>  }
> +
> +void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr)
> +{
> +	pgd_t *pgd;
> +	pmd_t *pmd;
> +	pte_t *pte;
> +
> +	if (!mmu_enabled())
> +		return;
> +
> +	pgd = pgd_offset(pgtable, vaddr);
> +	if (!pgd_valid(*pgd))
> +		return;
> +
> +	pmd = pmd_offset(pgd, vaddr);
> +	if (!pmd_valid(*pmd))
> +		return;
> +
> +	if (pmd_huge(*pmd)) {
> +		WRITE_ONCE(*pmd, 0);
> +		goto out_flush_tlb;
> +	} else {
is the else needed?
> +		pte = pte_offset(pmd, vaddr);
> +		if (!pte_valid(*pte))
> +			return;
> +		WRITE_ONCE(*pte, 0);
> +		goto out_flush_tlb;
> +	}
> +
> +out_flush_tlb:
> +	flush_tlb_page(vaddr);
> +}
> 
This code is very similar to mmu_clear_user() besides the bit to invalidate
Just wondering if we couldn't use the same code and pass a bit offset.
It seems the offsets in PMD and PTE are same for USER bit and valid bit.

But maybe this is far-fetched and not worth the sharing.
I see Drew is not in CC, + Drew

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [kvm-unit-tests RFC PATCH v2 4/5] lib: arm/arm64: Add function to unmap a page

[Alexandru Elisei]

Hi Eric,

On 10/30/20 3:46 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>> Being able to cause a stage 1 data abort might be useful for future tests.
>> Add a function that unmaps a page from the translation tables.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  lib/arm/asm/mmu-api.h |  1 +
>>  lib/arm/mmu.c         | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
>> index 2bbe1faea900..305f77c6501f 100644
>> --- a/lib/arm/asm/mmu-api.h
>> +++ b/lib/arm/asm/mmu-api.h
>> @@ -23,4 +23,5 @@ extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
>>  			       phys_addr_t phys_start, phys_addr_t phys_end,
>>  			       pgprot_t prot);
>>  extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
>> +extern void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr);
>>  #endif
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 540a1e842d5b..72ac0be8d146 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -232,3 +232,35 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>  out_flush_tlb:
>>  	flush_tlb_page(vaddr);
>>  }
>> +
>> +void mmu_unmap_page(pgd_t *pgtable, unsigned long vaddr)
>> +{
>> +	pgd_t *pgd;
>> +	pmd_t *pmd;
>> +	pte_t *pte;
>> +
>> +	if (!mmu_enabled())
>> +		return;
>> +
>> +	pgd = pgd_offset(pgtable, vaddr);
>> +	if (!pgd_valid(*pgd))
>> +		return;
>> +
>> +	pmd = pmd_offset(pgd, vaddr);
>> +	if (!pmd_valid(*pmd))
>> +		return;
>> +
>> +	if (pmd_huge(*pmd)) {
>> +		WRITE_ONCE(*pmd, 0);
>> +		goto out_flush_tlb;
>> +	} else {
> is the else needed?

No, not needed. Will remove.

>> +		pte = pte_offset(pmd, vaddr);
>> +		if (!pte_valid(*pte))
>> +			return;
>> +		WRITE_ONCE(*pte, 0);
>> +		goto out_flush_tlb;
>> +	}
>> +
>> +out_flush_tlb:
>> +	flush_tlb_page(vaddr);
>> +}
>>
> This code is very similar to mmu_clear_user() besides the bit to invalidate
> Just wondering if we couldn't use the same code and pass a bit offset.
> It seems the offsets in PMD and PTE are same for USER bit and valid bit.

Yes, I will look into it and see what I can come up with.

>
> But maybe this is far-fetched and not worth the sharing.
> I see Drew is not in CC, + Drew

Yeah... I somehow missed adding Drew to CC for the entire series.

>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks,

Alex

>
> Thanks
>
> Eric
>

[kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test

[Alexandru Elisei]

According to ARM DDI 0487F.b, a profiling buffer management event occurs:

* On a fault.
* On an external abort.
* When the buffer fills.
* When software sets the service bit, PMBSR_EL1.S.

Set up the buffer to trigger the events and check that they are reported
correctly.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/spe.c         | 342 +++++++++++++++++++++++++++++++++++++++++++++-
 arm/unittests.cfg |   8 ++
 2 files changed, 346 insertions(+), 4 deletions(-)

diff --git a/arm/spe.c b/arm/spe.c
index c199cd239194..c185883d079a 100644
--- a/arm/spe.c
+++ b/arm/spe.c
@@ -15,8 +15,10 @@
 #include <bitops.h>
 #include <devicetree.h>
 #include <libcflat.h>
+#include <vmalloc.h>
 
 #include <asm/gic.h>
+#include <asm/mmu.h>
 #include <asm/processor.h>
 #include <asm/sysreg.h>
 
@@ -41,6 +43,44 @@
 #define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
 #define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
 
+#define SYS_PMSCR_EL1			sys_reg(3, 0, 9, 9, 0)
+#define SYS_PMSCR_EL1_E1SPE_SHIFT	1
+#define SYS_PMSCR_EL1_PA_SHIFT		4
+#define SYS_PMSCR_EL1_TS_SHIFT		5
+
+#define SYS_PMSICR_EL1			sys_reg(3, 0, 9, 9, 2)
+
+#define SYS_PMSIRR_EL1			sys_reg(3, 0, 9, 9, 3)
+#define SYS_PMSIRR_EL1_INTERVAL_SHIFT	8
+#define SYS_PMSIRR_EL1_INTERVAL_MASK	0xffffffUL
+
+#define SYS_PMSFCR_EL1			sys_reg(3, 0, 9, 9, 4)
+#define SYS_PMSFCR_EL1_FE_SHIFT		0
+#define SYS_PMSFCR_EL1_FT_SHIFT		1
+#define SYS_PMSFCR_EL1_FL_SHIFT		2
+#define SYS_PMSFCR_EL1_B_SHIFT		16
+#define SYS_PMSFCR_EL1_LD_SHIFT		17
+#define SYS_PMSFCR_EL1_ST_SHIFT		18
+
+#define SYS_PMSEVFR_EL1			sys_reg(3, 0, 9, 9, 5)
+#define SYS_PMSLATFR_EL1		sys_reg(3, 0, 9, 9, 6)
+
+#define SYS_PMBLIMITR_EL1		sys_reg(3, 0, 9, 10, 0)
+#define SYS_PMBLIMITR_EL1_E_SHIFT	0
+
+#define SYS_PMBPTR_EL1			sys_reg(3, 0, 9, 10, 1)
+
+#define SYS_PMBSR_EL1			sys_reg(3, 0, 9, 10, 3)
+#define SYS_PMBSR_EL1_S_SHIFT		17
+#define SYS_PMBSR_EL1_EA_SHIFT		18
+#define SYS_PMBSR_EL1_BSC_BUF_FULL	1
+#define SYS_PMBSR_EL1_EC_SHIFT		26
+#define SYS_PMBSR_EL1_EC_MASK		0x3fUL
+#define SYS_PMBSR_EL1_EC_FAULT_S1	0x24
+#define SYS_PMBSR_EL1_RES0		0x00000000fc0fffffUL
+
+#define psb_csync()			asm volatile("hint #17" : : : "memory")
+
 struct spe {
 	uint32_t intid;
 	int min_interval;
@@ -53,6 +93,15 @@ struct spe {
 };
 static struct spe spe;
 
+struct spe_buffer {
+	uint64_t base;
+	uint64_t limit;
+};
+static struct spe_buffer buffer;
+
+static volatile bool pmbirq_asserted, reassert_irq;
+static volatile uint64_t irq_pmbsr;
+
 static int spe_min_interval(uint8_t interval)
 {
 	switch (interval) {
@@ -131,6 +180,273 @@ static bool spe_probe(void)
 	return true;
 }
 
+/*
+ * Resets and starts a profiling session. Must be called with sampling and
+ * buffer disabled.
+ */
+static void spe_reset_and_start(struct spe_buffer *spe_buffer)
+{
+	uint64_t pmscr;
+
+	assert(spe_buffer->base);
+	assert(spe_buffer->limit > spe_buffer->base);
+
+	write_sysreg_s(spe_buffer->base, SYS_PMBPTR_EL1);
+	/* Change the buffer pointer before PMBLIMITR_EL1. */
+	isb();
+
+	write_sysreg_s(spe_buffer->limit | BIT(SYS_PMBLIMITR_EL1_E_SHIFT),
+		       SYS_PMBLIMITR_EL1);
+	write_sysreg_s(0, SYS_PMBSR_EL1);
+	write_sysreg_s(0, SYS_PMSICR_EL1);
+	/* PMSICR_EL1 must be written to zero before a new sampling session. */
+	isb();
+
+	pmscr = BIT(SYS_PMSCR_EL1_E1SPE_SHIFT) |
+		BIT(SYS_PMSCR_EL1_PA_SHIFT) |
+		BIT(SYS_PMSCR_EL1_TS_SHIFT);
+	write_sysreg_s(pmscr, SYS_PMSCR_EL1);
+	isb();
+}
+
+static void spe_stop_and_drain(void)
+{
+	write_sysreg_s(0, SYS_PMSCR_EL1);
+	isb();
+
+	psb_csync();
+	dsb(nsh);
+	write_sysreg_s(0, SYS_PMBLIMITR_EL1);
+	isb();
+}
+
+static void spe_irq_handler(struct pt_regs *regs)
+{
+	uint32_t intid = gic_read_iar();
+
+	spe_stop_and_drain();
+
+	irq_pmbsr = read_sysreg_s(SYS_PMBSR_EL1);
+
+	if (intid != PPI(spe.intid)) {
+		report_info("Unexpected interrupt: %u", intid);
+		/*
+		 * When we get the interrupt, at least one bit, PMBSR_EL1.S,
+		 * will be set. We can the value zero for an error.
+		 */
+		irq_pmbsr = 0;
+		goto out;
+	}
+
+	if (irq_pmbsr && reassert_irq) {
+		/*
+		 * Don't clear the service bit now, but ack the interrupt so it
+		 * can be handled again.
+		 */
+		gic_write_eoir(intid);
+		reassert_irq = false;
+		irq_pmbsr = 0;
+		return;
+	}
+
+out:
+	write_sysreg_s(0, SYS_PMBSR_EL1);
+	/* Clear PMBSR_EL1 before EOI'ing the interrupt */
+	isb();
+	gic_write_eoir(intid);
+
+	pmbirq_asserted = true;
+}
+
+static void spe_enable_interrupt(void)
+{
+	void *gic_isenabler;
+
+	switch (gic_version()) {
+	case 2:
+		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
+		break;
+	case 3:
+		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
+		break;
+	default:
+		report_abort("Unknown GIC version %d", gic_version());
+	}
+
+	writel(1 << PPI(spe.intid), gic_isenabler);
+}
+
+static void spe_init(void)
+{
+	uint64_t pmsfcr, pmsirr;
+
+	/*
+	 * PMSCR_EL1.E1SPE resets to UNKNOWN value, make sure sampling is
+	 * disabled.
+	 */
+	write_sysreg_s(0, SYS_PMSCR_EL1);
+	isb();
+
+	install_irq_handler(EL1H_IRQ, spe_irq_handler);
+
+	gic_enable_defaults();
+	spe_enable_interrupt();
+	local_irq_enable();
+
+	buffer.base = (u64)memalign(PAGE_SIZE, PAGE_SIZE);
+	assert_msg(buffer.base, "Cannot allocate profiling buffer");
+	buffer.limit = buffer.base + PAGE_SIZE;
+
+	/* Select all operations for sampling */
+	pmsfcr = BIT(SYS_PMSFCR_EL1_FT_SHIFT) |
+		 BIT(SYS_PMSFCR_EL1_B_SHIFT) |
+		 BIT(SYS_PMSFCR_EL1_LD_SHIFT) |
+		 BIT(SYS_PMSFCR_EL1_ST_SHIFT);
+	write_sysreg_s(pmsfcr, SYS_PMSFCR_EL1);
+
+	/*
+	 * From ARM DDI 0487F.b: "[..] Software should set this to a value
+	 * greater than the minimum indicated by PMSIDR_EL1.Interval"
+	 */
+	pmsirr = (spe.min_interval + 1) & SYS_PMSIRR_EL1_INTERVAL_MASK;
+	pmsirr <<= SYS_PMSIRR_EL1_INTERVAL_SHIFT;
+	write_sysreg_s(pmsirr, SYS_PMSIRR_EL1);
+	isb();
+}
+
+static bool spe_test_buffer_service(void)
+{
+	uint64_t expected_pmbsr;
+	int i;
+
+	spe_stop_and_drain();
+
+	reassert_irq = true;
+	pmbirq_asserted = false;
+
+	expected_pmbsr = 0x12345678 | BIT(SYS_PMBSR_EL1_S_SHIFT);
+	expected_pmbsr &= SYS_PMBSR_EL1_RES0;
+
+	/*
+	 * ARM DDI 0487F.b, page D9-2803:
+	 *
+	 * "PMBIRQ is a level-sensitive interrupt request driven by PMBSR_EL1.S.
+	 * This means that a direct write that sets PMBSR_EL1.S to 1 causes the
+	 * interrupt to be asserted, and PMBIRQ remains asserted until software
+	 * clears PMBSR_EL1.S to 0."
+	 */
+	 write_sysreg_s(expected_pmbsr, SYS_PMBSR_EL1);
+	 isb();
+
+	/* Wait for up to 1 second for the GIC to assert the interrupt. */
+	for (i = 0; i < 10; i++) {
+		if (pmbirq_asserted)
+			break;
+		mdelay(100);
+	}
+	reassert_irq = false;
+
+	return irq_pmbsr == expected_pmbsr;
+}
+
+static bool spe_test_buffer_full(void)
+{
+	volatile uint64_t cnt = 0;
+	uint64_t expected_pmbsr, pmbptr;
+
+	spe_stop_and_drain();
+
+	pmbirq_asserted = false;
+	expected_pmbsr = BIT(SYS_PMBSR_EL1_S_SHIFT) | SYS_PMBSR_EL1_BSC_BUF_FULL;
+
+	spe_reset_and_start(&buffer);
+	for (;;) {
+		cnt++;
+		if (pmbirq_asserted)
+			break;
+	}
+
+	pmbptr = read_sysreg_s(SYS_PMBPTR_EL1);
+	/*
+	 * ARM DDI 0487F.b, page D9-2804:
+	 *
+	 * "[..] the Profiling Buffer management event is generated when the PE
+	 * writes past the write limit pointer minus 2^(PMSIDR_EL1.MaxSize)."
+	 */
+	return (pmbptr >= buffer.limit - spe.max_record_size) &&
+		(irq_pmbsr == expected_pmbsr);
+}
+
+static bool spe_test_buffer_stage1_dabt(void)
+{
+	volatile uint64_t cnt = 0;
+	struct spe_buffer dabt_buffer;
+	uint8_t pmbsr_ec;
+
+	spe_stop_and_drain();
+
+	dabt_buffer.base = (u64)memalign(PAGE_SIZE, PAGE_SIZE);
+	assert_msg(dabt_buffer.base, "Cannot allocate profiling buffer");
+	dabt_buffer.limit = dabt_buffer.base + PAGE_SIZE;
+	mmu_unmap_page(current_thread_info()->pgtable, dabt_buffer.base);
+
+	pmbirq_asserted = false;
+
+	spe_reset_and_start(&dabt_buffer);
+	for (;;) {
+		cnt++;
+		if (pmbirq_asserted)
+			break;
+	}
+
+	pmbsr_ec = (irq_pmbsr >> SYS_PMBSR_EL1_EC_SHIFT) & SYS_PMBSR_EL1_EC_MASK;
+	return (irq_pmbsr & BIT(SYS_PMBSR_EL1_S_SHIFT)) &&
+		(pmbsr_ec == SYS_PMBSR_EL1_EC_FAULT_S1);
+}
+
+static bool spe_test_buffer_extabt(void)
+{
+	struct spe_buffer extabt_buffer;
+	volatile uint64_t cnt = 0;
+	phys_addr_t highest_end = 0;
+	struct mem_region *r;
+
+	spe_stop_and_drain();
+
+	/*
+	 * Find a physical address most likely to be absent from the stage 2
+	 * tables. Full explanation in arm/selftest.c, in check_pabt_init().
+	 */
+	for (r = mem_regions; r->end; r++) {
+		if (r->flags & MR_F_IO)
+			continue;
+		if (r->end > highest_end)
+			highest_end = PAGE_ALIGN(r->end);
+	}
+
+	if (mem_region_get_flags(highest_end) != MR_F_UNKNOWN)
+		return false;
+
+	extabt_buffer.base = (u64)vmap(highest_end, PAGE_SIZE);
+	extabt_buffer.limit = extabt_buffer.base + PAGE_SIZE;
+
+	pmbirq_asserted = false;
+
+	report_info("Expecting KVM Stage 2 Data Abort at pmbptr=0x%lx",
+			extabt_buffer.base);
+
+	spe_reset_and_start(&extabt_buffer);
+	for (;;) {
+		cnt++;
+		if (pmbirq_asserted)
+			break;
+	}
+
+	return (irq_pmbsr & BIT(SYS_PMBSR_EL1_S_SHIFT)) &&
+		(irq_pmbsr & BIT(SYS_PMBSR_EL1_EA_SHIFT));
+
+}
+
 static void spe_test_introspection(void)
 {
 	report_prefix_push("spe-introspection");
@@ -146,8 +462,22 @@ static void spe_test_introspection(void)
 	report_prefix_pop();
 }
 
+static void spe_test_buffer(void)
+{
+	report_prefix_push("spe-buffer");
+
+	report(spe_test_buffer_service(), "Service management event");
+	report(spe_test_buffer_full(), "Buffer full management event");
+	report(spe_test_buffer_stage1_dabt(), "Stage 1 data abort management event");
+	report(spe_test_buffer_extabt(), "Buffer external abort management event");
+
+	report_prefix_pop();
+}
+
 int main(int argc, char *argv[])
 {
+	int i;
+
 	if (!spe_probe()) {
 		report_skip("SPE not supported");
 		return report_summary();
@@ -161,12 +491,16 @@ int main(int argc, char *argv[])
 	if (argc < 2)
 		report_abort("no test specified");
 
+	spe_init();
+
 	report_prefix_push("spe");
 
-	if (strcmp(argv[1], "spe-introspection") == 0)
-		spe_test_introspection();
-	else
-		report_abort("Unknown subtest '%s'", argv[1]);
+	for (i = 1; i < argc; i++) {
+		if (strcmp(argv[i], "spe-introspection") == 0)
+			spe_test_introspection();
+		if (strcmp(argv[i], "spe-buffer") == 0)
+			spe_test_buffer();
+	}
 
 	return report_summary();
 }
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ad10be123774..ba21421e81aa 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -141,6 +141,14 @@ arch = arm64
 accel = kvm
 extra_params = -append 'spe-introspection'
 
+[spe-buffer]
+file = spe.flat
+groups = spe
+arch = arm64
+timeout = 10s
+accel = kvm
+extra_params = -append 'spe-buffer'
+
 # Test GIC emulation
 [gicv2-ipi]
 file = gic.flat
-- 
2.29.1

Re: [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test

[Auger Eric]

Hi Alexandru,
On 10/27/20 6:19 PM, Alexandru Elisei wrote:
> According to ARM DDI 0487F.b, a profiling buffer management event occurs:
> 
> * On a fault.
> * On an external abort.
> * When the buffer fills.
> * When software sets the service bit, PMBSR_EL1.S.
Compared to v1 this patch adds 3 new valuable tests. Thank you for that.
Please find technical comments in-line. General comments to be sent
along with the cover letter.
> 
> Set up the buffer to trigger the events and check that they are reported
> correctly.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/spe.c         | 342 +++++++++++++++++++++++++++++++++++++++++++++-
>  arm/unittests.cfg |   8 ++
>  2 files changed, 346 insertions(+), 4 deletions(-)
> 
> diff --git a/arm/spe.c b/arm/spe.c
> index c199cd239194..c185883d079a 100644
> --- a/arm/spe.c
> +++ b/arm/spe.c
> @@ -15,8 +15,10 @@
>  #include <bitops.h>
>  #include <devicetree.h>
>  #include <libcflat.h>
> +#include <vmalloc.h>
>  
>  #include <asm/gic.h>
> +#include <asm/mmu.h>
>  #include <asm/processor.h>
>  #include <asm/sysreg.h>
>  
> @@ -41,6 +43,44 @@
>  #define SYS_PMSIDR_EL1_COUNTSIZE_SHIFT	16
>  #define SYS_PMSIDR_EL1_COUNTSIZE_MASK	0xfUL
>  
> +#define SYS_PMSCR_EL1			sys_reg(3, 0, 9, 9, 0)
> +#define SYS_PMSCR_EL1_E1SPE_SHIFT	1
> +#define SYS_PMSCR_EL1_PA_SHIFT		4
> +#define SYS_PMSCR_EL1_TS_SHIFT		5
> +
> +#define SYS_PMSICR_EL1			sys_reg(3, 0, 9, 9, 2)
> +
> +#define SYS_PMSIRR_EL1			sys_reg(3, 0, 9, 9, 3)
> +#define SYS_PMSIRR_EL1_INTERVAL_SHIFT	8
> +#define SYS_PMSIRR_EL1_INTERVAL_MASK	0xffffffUL
> +
> +#define SYS_PMSFCR_EL1			sys_reg(3, 0, 9, 9, 4)
> +#define SYS_PMSFCR_EL1_FE_SHIFT		0
> +#define SYS_PMSFCR_EL1_FT_SHIFT		1
> +#define SYS_PMSFCR_EL1_FL_SHIFT		2
> +#define SYS_PMSFCR_EL1_B_SHIFT		16
> +#define SYS_PMSFCR_EL1_LD_SHIFT		17
> +#define SYS_PMSFCR_EL1_ST_SHIFT		18
> +
> +#define SYS_PMSEVFR_EL1			sys_reg(3, 0, 9, 9, 5)
> +#define SYS_PMSLATFR_EL1		sys_reg(3, 0, 9, 9, 6)
> +
> +#define SYS_PMBLIMITR_EL1		sys_reg(3, 0, 9, 10, 0)
> +#define SYS_PMBLIMITR_EL1_E_SHIFT	0
> +
> +#define SYS_PMBPTR_EL1			sys_reg(3, 0, 9, 10, 1)
> +
> +#define SYS_PMBSR_EL1			sys_reg(3, 0, 9, 10, 3)
> +#define SYS_PMBSR_EL1_S_SHIFT		17
> +#define SYS_PMBSR_EL1_EA_SHIFT		18
> +#define SYS_PMBSR_EL1_BSC_BUF_FULL	1
> +#define SYS_PMBSR_EL1_EC_SHIFT		26
> +#define SYS_PMBSR_EL1_EC_MASK		0x3fUL
> +#define SYS_PMBSR_EL1_EC_FAULT_S1	0x24
> +#define SYS_PMBSR_EL1_RES0		0x00000000fc0fffffUL
> +
> +#define psb_csync()			asm volatile("hint #17" : : : "memory")
> +
>  struct spe {
>  	uint32_t intid;
>  	int min_interval;
> @@ -53,6 +93,15 @@ struct spe {
>  };
>  static struct spe spe;
>  
> +struct spe_buffer {
> +	uint64_t base;
> +	uint64_t limit;
> +};
> +static struct spe_buffer buffer;
> +
> +static volatile bool pmbirq_asserted, reassert_irq;
> +static volatile uint64_t irq_pmbsr;
> +
>  static int spe_min_interval(uint8_t interval)
>  {
>  	switch (interval) {
> @@ -131,6 +180,273 @@ static bool spe_probe(void)
>  	return true;
>  }
>  
> +/*
> + * Resets and starts a profiling session. Must be called with sampling and
> + * buffer disabled.
> + */
> +static void spe_reset_and_start(struct spe_buffer *spe_buffer)
> +{
> +	uint64_t pmscr;
> +
> +	assert(spe_buffer->base);
> +	assert(spe_buffer->limit > spe_buffer->base);
as you pointed in our discussion, shouldn't we have:
spe_buffer->limit > spe_buffer->base - 2^PMSIDR_EL1.MaxSize
> +
> +	write_sysreg_s(spe_buffer->base, SYS_PMBPTR_EL1);
> +	/* Change the buffer pointer before PMBLIMITR_EL1. */
has become SYS_PMBLIMITR_EL1
> +	isb();
> +
> +	write_sysreg_s(spe_buffer->limit | BIT(SYS_PMBLIMITR_EL1_E_SHIFT),
> +		       SYS_PMBLIMITR_EL1);
> +	write_sysreg_s(0, SYS_PMBSR_EL1);
> +	write_sysreg_s(0, SYS_PMSICR_EL1);
> +	/* PMSICR_EL1 must be written to zero before a new sampling session. */
> +	isb();
> +
> +	pmscr = BIT(SYS_PMSCR_EL1_E1SPE_SHIFT) |
> +		BIT(SYS_PMSCR_EL1_PA_SHIFT) |
> +		BIT(SYS_PMSCR_EL1_TS_SHIFT);
> +	write_sysreg_s(pmscr, SYS_PMSCR_EL1);
> +	isb();
> +}
this helper somehow matches the reset() function in v1 with variations
- no interval reload reg setting to min value
- you do not set the PCT bit (any reason, no timestamp sampling in the
test?). Anyway could remain as a default, does not hurt.
- isb added inbetween the SYS_PMBPTR_EL1 and PMBLIMITR_EL1 which sounds
like a fix
> +
> +static void spe_stop_and_drain(void)
matches the v1 drain() helper except you also stop the profiling.
This was done in my profiling loop function earlier. In my case I reset
PMBLIMITR_EL1.E (I understand from 3.2.1 that either method, reset PMSC
or this bit is ok).
> +{
> +	write_sysreg_s(0, SYS_PMSCR_EL1);
> +	isb();
> +
> +	psb_csync();
> +	dsb(nsh);
> +	write_sysreg_s(0, SYS_PMBLIMITR_EL1);
> +	isb();
> +}
> +
> +static void spe_irq_handler(struct pt_regs *regs)
> +{
> +	uint32_t intid = gic_read_iar();
> +
> +	spe_stop_and_drain();
why do you need to stop the SPE in the handler? This does not look
standard as a behavior?
> +
> +	irq_pmbsr = read_sysreg_s(SYS_PMBSR_EL1);
> +
> +	if (intid != PPI(spe.intid)) {
> +		report_info("Unexpected interrupt: %u", intid);
> +		/*
> +		 * When we get the interrupt, at least one bit, PMBSR_EL1.S,
> +		 * will be set. We can the value zero for an error.
last sentence needs some reword. But anyway the comment looks stale as
here we are handling the case here this is not an SPE IRQ. Maybe put it
below but then also check the PMBSR_EL1.S
> +		 */
> +		irq_pmbsr = 0;
> +		goto out;
> +	}
> +
> +	if (irq_pmbsr && reassert_irq) {
> +		/*
> +		 * Don't clear the service bit now, but ack the interrupt so it
> +		 * can be handled again.
> +		 */
> +		gic_write_eoir(intid);
> +		reassert_irq = false;
> +		irq_pmbsr = 0;
irq_pmbsr = 0 here and above? Looks strange. Don't you want to record
it. in V1 I was decoding the syndrome register with
decode_syndrome_register(), resetting the syndrome reg here and then
post-analyzed the SR. Why did you change that?
> +		return;
> +	}
> +
> +out:
> +	write_sysreg_s(0, SYS_PMBSR_EL1);
> +	/* Clear PMBSR_EL1 before EOI'ing the interrupt */
> +	isb();
> +	gic_write_eoir(intid);
> +
> +	pmbirq_asserted = true;
> +}
> +
> +static void spe_enable_interrupt(void)
gic_enable_irq() helper can be used instead (see v1)
> +{
> +	void *gic_isenabler;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
> +		break;
> +	case 3:
> +		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
> +		break;
> +	default:
> +		report_abort("Unknown GIC version %d", gic_version());
> +	}
> +
> +	writel(1 << PPI(spe.intid), gic_isenabler);

> +}
> +
> +static void spe_init(void)
> +{
> +	uint64_t pmsfcr, pmsirr;
> +
> +	/*
> +	 * PMSCR_EL1.E1SPE resets to UNKNOWN value, make sure sampling is
> +	 * disabled.
> +	 */
> +	write_sysreg_s(0, SYS_PMSCR_EL1);
> +	isb();
> +
> +	install_irq_handler(EL1H_IRQ, spe_irq_handler);
> +
> +	gic_enable_defaults();
> +	spe_enable_interrupt();
> +	local_irq_enable();
> +
> +	buffer.base = (u64)memalign(PAGE_SIZE, PAGE_SIZE);
> +	assert_msg(buffer.base, "Cannot allocate profiling buffer");
> +	buffer.limit = buffer.base + PAGE_SIZE;
> +
> +	/* Select all operations for sampling */
> +	pmsfcr = BIT(SYS_PMSFCR_EL1_FT_SHIFT) |
> +		 BIT(SYS_PMSFCR_EL1_B_SHIFT) |
> +		 BIT(SYS_PMSFCR_EL1_LD_SHIFT) |
> +		 BIT(SYS_PMSFCR_EL1_ST_SHIFT);
> +	write_sysreg_s(pmsfcr, SYS_PMSFCR_EL1);

Besides the buffer allocation, some stuff are already done in the init()
routine so that's a bit a pity to duplicate. That's why I dissociated
the reset from the start in v1.
> +
> +	/*
> +	 * From ARM DDI 0487F.b: "[..] Software should set this to a value
> +	 * greater than the minimum indicated by PMSIDR_EL1.Interval"
> +	 */
> +	pmsirr = (spe.min_interval + 1) & SYS_PMSIRR_EL1_INTERVAL_MASK;
> +	pmsirr <<= SYS_PMSIRR_EL1_INTERVAL_SHIFT;
> +	write_sysreg_s(pmsirr, SYS_PMSIRR_EL1);
> +	isb();
> +}
> +
> +static bool spe_test_buffer_service(void)
> +{
> +	uint64_t expected_pmbsr;
> +	int i;
> +
> +	spe_stop_and_drain();
> +
> +	reassert_irq = true;
> +	pmbirq_asserted = false;
> +
> +	expected_pmbsr = 0x12345678 | BIT(SYS_PMBSR_EL1_S_SHIFT);
what is the point of 0x12345678?
> +	expected_pmbsr &= SYS_PMBSR_EL1_RES0;
> +
> +	/*
> +	 * ARM DDI 0487F.b, page D9-2803:
> +	 *
> +	 * "PMBIRQ is a level-sensitive interrupt request driven by PMBSR_EL1.S.
> +	 * This means that a direct write that sets PMBSR_EL1.S to 1 causes the
> +	 * interrupt to be asserted, and PMBIRQ remains asserted until software
> +	 * clears PMBSR_EL1.S to 0."
> +	 */
> +	 write_sysreg_s(expected_pmbsr, SYS_PMBSR_EL1);
> +	 isb();
> +
> +	/* Wait for up to 1 second for the GIC to assert the interrupt. */
> +	for (i = 0; i < 10; i++) {
> +		if (pmbirq_asserted)
> +			break;
> +		mdelay(100);
> +	}
> +	reassert_irq = false;
> +
> +	return irq_pmbsr == expected_pmbsr;
> +}
> +
> +static bool spe_test_buffer_full(void)
> +{
> +	volatile uint64_t cnt = 0;
> +	uint64_t expected_pmbsr, pmbptr;
> +
> +	spe_stop_and_drain();
> +
> +	pmbirq_asserted = false;
> +	expected_pmbsr = BIT(SYS_PMBSR_EL1_S_SHIFT) | SYS_PMBSR_EL1_BSC_BUF_FULL;
> +
> +	spe_reset_and_start(&buffer);
> +	for (;;) {
> +		cnt++;
> +		if (pmbirq_asserted)
> +			break;
> +	}
what if the interrupt does not occur as expected. You end up relying on
the kut timeout mechanism. Whereas on my test you stop at the end of the
mem_access_loop whatever the occurence of the event.
> +
> +	pmbptr = read_sysreg_s(SYS_PMBPTR_EL1);
> +	/*
> +	 * ARM DDI 0487F.b, page D9-2804:
> +	 *
> +	 * "[..] the Profiling Buffer management event is generated when the PE
> +	 * writes past the write limit pointer minus 2^(PMSIDR_EL1.MaxSize)."
> +	 */
> +	return (pmbptr >= buffer.limit - spe.max_record_size) &&
> +		(irq_pmbsr == expected_pmbsr);
> +}
> +
> +static bool spe_test_buffer_stage1_dabt(void)
> +{
> +	volatile uint64_t cnt = 0;
> +	struct spe_buffer dabt_buffer;
> +	uint8_t pmbsr_ec;
> +
> +	spe_stop_and_drain();
> +
> +	dabt_buffer.base = (u64)memalign(PAGE_SIZE, PAGE_SIZE);
> +	assert_msg(dabt_buffer.base, "Cannot allocate profiling buffer");
> +	dabt_buffer.limit = dabt_buffer.base + PAGE_SIZE;
> +	mmu_unmap_page(current_thread_info()->pgtable, dabt_buffer.base);
> +
> +	pmbirq_asserted = false;
> +
> +	spe_reset_and_start(&dabt_buffer);
> +	for (;;) {
> +		cnt++;
> +		if (pmbirq_asserted)
> +			break;
> +	}
can't you have the 1s timeout as for the service test. Isn't the fault
supposed to happen immediately.
> +
> +	pmbsr_ec = (irq_pmbsr >> SYS_PMBSR_EL1_EC_SHIFT) & SYS_PMBSR_EL1_EC_MASK;
> +	return (irq_pmbsr & BIT(SYS_PMBSR_EL1_S_SHIFT)) &&
> +		(pmbsr_ec == SYS_PMBSR_EL1_EC_FAULT_S1);
> +}
> +
> +static bool spe_test_buffer_extabt(void)
> +{
> +	struct spe_buffer extabt_buffer;
> +	volatile uint64_t cnt = 0;
> +	phys_addr_t highest_end = 0;
> +	struct mem_region *r;
> +
> +	spe_stop_and_drain();
> +
> +	/*
> +	 * Find a physical address most likely to be absent from the stage 2
> +	 * tables. Full explanation in arm/selftest.c, in check_pabt_init().
> +	 */
> +	for (r = mem_regions; r->end; r++) {
> +		if (r->flags & MR_F_IO)
> +			continue;
> +		if (r->end > highest_end)
> +			highest_end = PAGE_ALIGN(r->end);
> +	}
> +
> +	if (mem_region_get_flags(highest_end) != MR_F_UNKNOWN)
> +		return false;
> +
> +	extabt_buffer.base = (u64)vmap(highest_end, PAGE_SIZE);
> +	extabt_buffer.limit = extabt_buffer.base + PAGE_SIZE;
> +
> +	pmbirq_asserted = false;
> +
> +	report_info("Expecting KVM Stage 2 Data Abort at pmbptr=0x%lx",
> +			extabt_buffer.base);
> +
> +	spe_reset_and_start(&extabt_buffer);
> +	for (;;) {
> +		cnt++;
> +		if (pmbirq_asserted)
> +			break;
> +	}
> +
> +	return (irq_pmbsr & BIT(SYS_PMBSR_EL1_S_SHIFT)) &&
> +		(irq_pmbsr & BIT(SYS_PMBSR_EL1_EA_SHIFT));
I still think the decode_syndrome_register I introduced in v1 is useful.
It matches what we have in the PMU tests. Also in the verbose mode it
allows to see what does happen. Then you can post-process the collected
info.
> +
> +}
> +
>  static void spe_test_introspection(void)
>  {
>  	report_prefix_push("spe-introspection");
> @@ -146,8 +462,22 @@ static void spe_test_introspection(void)
>  	report_prefix_pop();
>  }
>  
> +static void spe_test_buffer(void)
> +{
> +	report_prefix_push("spe-buffer");
this rather test buffer events so the name of the test may be updated.
Also this single test may be split into several ones. This may be easier
to analyze upon failures.
> +
> +	report(spe_test_buffer_service(), "Service management event");
> +	report(spe_test_buffer_full(), "Buffer full management event");
> +	report(spe_test_buffer_stage1_dabt(), "Stage 1 data abort management event");
> +	report(spe_test_buffer_extabt(), "Buffer external abort management event");
> +
> +	report_prefix_pop();
> +}
> +
>  int main(int argc, char *argv[])
>  {
> +	int i;
> +
>  	if (!spe_probe()) {
>  		report_skip("SPE not supported");
>  		return report_summary();
> @@ -161,12 +491,16 @@ int main(int argc, char *argv[])
>  	if (argc < 2)
>  		report_abort("no test specified");
>  
> +	spe_init();
> +
>  	report_prefix_push("spe");
>  
> -	if (strcmp(argv[1], "spe-introspection") == 0)
> -		spe_test_introspection();
> -	else
> -		report_abort("Unknown subtest '%s'", argv[1]);
> +	for (i = 1; i < argc; i++) {
> +		if (strcmp(argv[i], "spe-introspection") == 0)
> +			spe_test_introspection();
> +		if (strcmp(argv[i], "spe-buffer") == 0)
> +			spe_test_buffer();
> +	}
>  
>  	return report_summary();
>  }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index ad10be123774..ba21421e81aa 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -141,6 +141,14 @@ arch = arm64
>  accel = kvm
>  extra_params = -append 'spe-introspection'
>  
> +[spe-buffer]
> +file = spe.flat
> +groups = spe
> +arch = arm64
> +timeout = 10s
so this is the time we will wait if service interrupt or buffer fullness
interrupt does not hit.
> +accel = kvm
> +extra_params = -append 'spe-buffer'
> +
>  # Test GIC emulation
>  [gicv2-ipi]
>  file = gic.flat
> 
Thanks

Eric

Re: [kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Alexandru Elisei]

Hi,

Adding Andrew to CC list, somehow I forgot, sorry for that.

Thanks,

Alex

On 10/27/20 5:19 PM, Alexandru Elisei wrote:
> This series implements two basic tests for KVM SPE: one that checks that
> the reported features match what is specified in the architecture,
> implemented in patch #3 ("arm64: spe: Add introspection test"), and another
> test that checks that the buffer management interrupt is asserted
> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest
> of the patches are either preparatory patches, or a fix, in the case of
> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers").
>
> This series builds on Eric's initial version [1], to which I've added the
> buffer tests that I used while developing SPE support for KVM.
>
> KVM SPE support is current in RFC phase, hence why this series is also sent
> as RFC. The KVM patches needed for the tests to work can be found at [2].
> Userspace support is also necessary, which I've implemented for kvmtool;
> this can be found at [3]. This series is also available in a repo [4] to make
> testing easier.
>
> [1] https://www.spinics.net/lists/kvm/msg223792.html
> [2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3
> [3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3
> [4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2
>
> Alexandru Elisei (3):
>   lib/{bitops,alloc_page}.h: Add missing headers
>   lib: arm/arm64: Add function to unmap a page
>   am64: spe: Add buffer test
>
> Eric Auger (2):
>   arm64: Move get_id_aa64dfr0() in processor.h
>   arm64: spe: Add introspection test
>
>  arm/Makefile.arm64        |   1 +
>  lib/arm/asm/mmu-api.h     |   1 +
>  lib/arm64/asm/processor.h |   5 +
>  lib/alloc_page.h          |   2 +
>  lib/bitops.h              |   2 +
>  lib/libcflat.h            |   1 +
>  lib/arm/mmu.c             |  32 +++
>  arm/pmu.c                 |   1 -
>  arm/spe.c                 | 506 ++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg         |  15 ++
>  10 files changed, 565 insertions(+), 1 deletion(-)
>  create mode 100644 arm/spe.c
>

Re: [kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Auger Eric]

Hi Alexandru,

[+ Drew]

On 10/27/20 6:19 PM, Alexandru Elisei wrote:
> This series implements two basic tests for KVM SPE: one that checks that
> the reported features match what is specified in the architecture,
> implemented in patch #3 ("arm64: spe: Add introspection test"), and another
> test that checks that the buffer management interrupt is asserted
> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest
> of the patches are either preparatory patches, or a fix, in the case of
> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers").
> 
> This series builds on Eric's initial version [1], to which I've added the
> buffer tests that I used while developing SPE support for KVM.

As you respin my series, with my prior agreement, I expected to find
most of the code I wrote, obviously with some potential fixes and
adaptations to fit your needs for additional tests.

However, in this v2, two significant v1 patches have disappeared:

1) [3/4] spe: Add profiling buffer test (170 LOC diffstat)
2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat)

They were actually the crux of the original series (the introspection
test being required as a prerequisite but not testing much really ;-).

1) consists in a "spe-buffer" test starting the profiling on a mastered
sequence of instructions (as done for PMU event counters). It introduces
the infra to start the profiling, prepare SPE reset config, the macro
definitions, start/stop/drain, the code under profiling and basically
checks that the buffer was effectively written. We also check we do not
get any spurious event as it is not expected.

=> This test has disappeared and the infra now is diluted in
[kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no
credit is given to my work as my S-o-b has disappeared.

2) consists in a "spe-events" test checking we effectively get the
buffer full event when duly expected. This introduces the infra to
handle interrupts, check the occurence of events by analyzing the
syndrome registers, compare occurences against expected ones. This
largely mimics what we have with PMU tests.

=> This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a
different stop condition, and again the infra is diluted in the same
patch, with large arbitrary changes, without any credit given to my
work. Those changes may explain why you removed my S-o-b but given the
anteriority of my series, this does not look normal to me, in a
community environment.

As discussed privately, this gives me the impression that those two
patches were totally ignored while respinning.

Please could you restructure the series at least to keep the buffer-full
test + infra separate from the new tests and reset a collaborative S-o-b.

Then if you think there are issues wrt the 1st test, "spe-buffer", not
included in this series, please let's discuss and fix/improve but not
simply trash it as is (in an everyone growing spirit).

An alternative is I can take back the ownership of my series and push it
upstream in a standard way. Then either you rebase your new tests on top
of it or I will be happy to do it for you after discussions on the
technical comments.

Thanks

Eric

> 
> KVM SPE support is current in RFC phase, hence why this series is also sent
> as RFC. The KVM patches needed for the tests to work can be found at [2].
> Userspace support is also necessary, which I've implemented for kvmtool;
> this can be found at [3]. This series is also available in a repo [4] to make
> testing easier.
> 
> [1] https://www.spinics.net/lists/kvm/msg223792.html
> [2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3
> [3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3
> [4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2
> 
> Alexandru Elisei (3):
>   lib/{bitops,alloc_page}.h: Add missing headers
>   lib: arm/arm64: Add function to unmap a page
>   am64: spe: Add buffer test
> 
> Eric Auger (2):
>   arm64: Move get_id_aa64dfr0() in processor.h
>   arm64: spe: Add introspection test
> 
>  arm/Makefile.arm64        |   1 +
>  lib/arm/asm/mmu-api.h     |   1 +
>  lib/arm64/asm/processor.h |   5 +
>  lib/alloc_page.h          |   2 +
>  lib/bitops.h              |   2 +
>  lib/libcflat.h            |   1 +
>  lib/arm/mmu.c             |  32 +++
>  arm/pmu.c                 |   1 -
>  arm/spe.c                 | 506 ++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg         |  15 ++
>  10 files changed, 565 insertions(+), 1 deletion(-)
>  create mode 100644 arm/spe.c
> 

Re: [kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Alexandru Elisei]

Hi Eric,

On 10/30/20 6:17 PM, Auger Eric wrote:
> Hi Alexandru,
>
> [+ Drew]
>
> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>> This series implements two basic tests for KVM SPE: one that checks that
>> the reported features match what is specified in the architecture,
>> implemented in patch #3 ("arm64: spe: Add introspection test"), and another
>> test that checks that the buffer management interrupt is asserted
>> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest
>> of the patches are either preparatory patches, or a fix, in the case of
>> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers").
>>
>> This series builds on Eric's initial version [1], to which I've added the
>> buffer tests that I used while developing SPE support for KVM.
> As you respin my series, with my prior agreement, I expected to find
> most of the code I wrote, obviously with some potential fixes and
> adaptations to fit your needs for additional tests.

I believe there has been a misunderstanding. I asked you if I can pickup *some* of
your patches, not all of them.

>
> However, in this v2, two significant v1 patches have disappeared:
>
> 1) [3/4] spe: Add profiling buffer test (170 LOC diffstat)
> 2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat)
>
> They were actually the crux of the original series (the introspection
> test being required as a prerequisite but not testing much really ;-).
>
> 1) consists in a "spe-buffer" test starting the profiling on a mastered
> sequence of instructions (as done for PMU event counters). It introduces
> the infra to start the profiling, prepare SPE reset config, the macro
> definitions, start/stop/drain, the code under profiling and basically
> checks that the buffer was effectively written. We also check we do not
> get any spurious event as it is not expected.
>
> => This test has disappeared and the infra now is diluted in
> [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no
> credit is given to my work as my S-o-b has disappeared.
>
> 2) consists in a "spe-events" test checking we effectively get the
> buffer full event when duly expected. This introduces the infra to
> handle interrupts, check the occurence of events by analyzing the
> syndrome registers, compare occurences against expected ones. This
> largely mimics what we have with PMU tests.
>
> => This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a
> different stop condition, and again the infra is diluted in the same
> patch, with large arbitrary changes, without any credit given to my
> work. Those changes may explain why you removed my S-o-b but given the
> anteriority of my series, this does not look normal to me, in a
> community environment.
>
> As discussed privately, this gives me the impression that those two
> patches were totally ignored while respinning.

Your impression is correct. The buffer test is my original work. No code has been
borrowed from your patches, hence why the differences might look like arbitrary
changes.

I believe there are some correctness issues with your patches, and I decided to
send my own test which I used when developing KVM SPE support instead of rebasing
your tests.

>
> Please could you restructure the series at least to keep the buffer-full
> test + infra separate from the new tests and reset a collaborative S-o-b.
>
> Then if you think there are issues wrt the 1st test, "spe-buffer", not
> included in this series, please let's discuss and fix/improve but not
> simply trash it as is (in an everyone growing spirit).
>
> An alternative is I can take back the ownership of my series and push it
> upstream in a standard way. Then either you rebase your new tests on top
> of it or I will be happy to do it for you after discussions on the
> technical comments.

It was not my intention to make you feel that your contribution is not appreciated
or ignored. Let's work on merging your series first and then I'll rebase and
resend any tests from my series which were not included.

Thanks,
Alex

Re: [kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Auger Eric]

Hi Alexandru,

On 10/30/20 11:31 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 10/30/20 6:17 PM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> [+ Drew]
>>
>> On 10/27/20 6:19 PM, Alexandru Elisei wrote:
>>> This series implements two basic tests for KVM SPE: one that checks that
>>> the reported features match what is specified in the architecture,
>>> implemented in patch #3 ("arm64: spe: Add introspection test"), and another
>>> test that checks that the buffer management interrupt is asserted
>>> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest
>>> of the patches are either preparatory patches, or a fix, in the case of
>>> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers").
>>>
>>> This series builds on Eric's initial version [1], to which I've added the
>>> buffer tests that I used while developing SPE support for KVM.
>> As you respin my series, with my prior agreement, I expected to find
>> most of the code I wrote, obviously with some potential fixes and
>> adaptations to fit your needs for additional tests.
> 
> I believe there has been a misunderstanding. I asked you if I can pickup *some* of
> your patches, not all of them.
> 
>>
>> However, in this v2, two significant v1 patches have disappeared:
>>
>> 1) [3/4] spe: Add profiling buffer test (170 LOC diffstat)
>> 2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat)
>>
>> They were actually the crux of the original series (the introspection
>> test being required as a prerequisite but not testing much really ;-).
>>
>> 1) consists in a "spe-buffer" test starting the profiling on a mastered
>> sequence of instructions (as done for PMU event counters). It introduces
>> the infra to start the profiling, prepare SPE reset config, the macro
>> definitions, start/stop/drain, the code under profiling and basically
>> checks that the buffer was effectively written. We also check we do not
>> get any spurious event as it is not expected.
>>
>> => This test has disappeared and the infra now is diluted in
>> [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no
>> credit is given to my work as my S-o-b has disappeared.
>>
>> 2) consists in a "spe-events" test checking we effectively get the
>> buffer full event when duly expected. This introduces the infra to
>> handle interrupts, check the occurence of events by analyzing the
>> syndrome registers, compare occurences against expected ones. This
>> largely mimics what we have with PMU tests.
>>
>> => This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a
>> different stop condition, and again the infra is diluted in the same
>> patch, with large arbitrary changes, without any credit given to my
>> work. Those changes may explain why you removed my S-o-b but given the
>> anteriority of my series, this does not look normal to me, in a
>> community environment.
>>
>> As discussed privately, this gives me the impression that those two
>> patches were totally ignored while respinning.
> 
> Your impression is correct. The buffer test is my original work. No code has been
> borrowed from your patches, hence why the differences might look like arbitrary
> changes.
> 
> I believe there are some correctness issues with your patches, and I decided to
> send my own test which I used when developing KVM SPE support instead of rebasing
> your tests.
> 
>>
>> Please could you restructure the series at least to keep the buffer-full
>> test + infra separate from the new tests and reset a collaborative S-o-b.
>>
>> Then if you think there are issues wrt the 1st test, "spe-buffer", not
>> included in this series, please let's discuss and fix/improve but not
>> simply trash it as is (in an everyone growing spirit).
>>
>> An alternative is I can take back the ownership of my series and push it
>> upstream in a standard way. Then either you rebase your new tests on top
>> of it or I will be happy to do it for you after discussions on the
>> technical comments.
> 
> It was not my intention to make you feel that your contribution is not appreciated
> or ignored. Let's work on merging your series first and then I'll rebase and
> resend any tests from my series which were not included.

Sure. So let's continue our technical discussion on both your respin and
my original series to identify issues and potential improvements to get
the best of them.

Thanks

Eric
> 
> Thanks,
> Alex
>