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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>> + *
>> + * 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
>
