Re: [kvm-unit-tests PATCH v2 3/3] arm/debugtest: test access to the debug registers for guest

["Alex Bennée" <alex.bennee@linaro.org>]

Andrew Jones <drjones@redhat.com> writes:

> On Thu, May 18, 2017 at 05:02:08PM +0100, Alex Bennée wrote:
>> Currently this is a fairly simple test that reads the debug register
>> state and attempts to write/read-back the watchpoint and breakpoint
>> registers.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  arm/Makefile.common |   1 +
>>  arm/debugtest.c     | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 672 insertions(+)
>>  create mode 100644 arm/debugtest.c
>>
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index 74c7394..1d7dec1 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -15,6 +15,7 @@ tests-common += $(TEST_DIR)/pci-test.flat
>>  tests-common += $(TEST_DIR)/pmu.flat
>>  tests-common += $(TEST_DIR)/gic.flat
>>  tests-common += $(TEST_DIR)/psci.flat
>> +tests-common += $(TEST_DIR)/debugtest.flat
>>
>>  tests-all = $(tests-common) $(tests)
>>  all: $(tests-all)
>> diff --git a/arm/debugtest.c b/arm/debugtest.c
>> new file mode 100644
>> index 0000000..4400bf3
>> --- /dev/null
>> +++ b/arm/debugtest.c
>> @@ -0,0 +1,671 @@
>> +/*
>> + * Test guest access to debug registers.
>> + *
>> + * Copyright (C) 2015, Linaro Ltd, Alex Bennée <alex.bennee@linaro.org>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/psci.h>
>> +#include <asm/smp.h>
>> +#include <asm/cpumask.h>
>> +#include <asm/barrier.h>
>> +#include "asm/sysreg.h"
>> +#include "asm/processor.h"
>
> nit: mixing <> and ""
>
>> +
>> +
>> +
>
> nit: excessive blank lines
>
>> +#if defined(__arm__)
>> +
>> +#define ID_DFR0	__ACCESS_CP15(c0, 0, c1, 2)
>> +
>> +/* __ACCESS_CP14(CRn, Op1, CRm, Op2) */
>> +#define DBGDIDR	__ACCESS_CP14(c0, 0, c0, 0)
>> +#define DBGDSCR	__ACCESS_CP14(c0, 0, c2, 2)
>> +
>> +#define DBGBCR0	__ACCESS_CP14(c0, 0, c0, 5)
>> +#define DBGBCR1	__ACCESS_CP14(c0, 0, c1, 5)
>> +#define DBGBCR2	__ACCESS_CP14(c0, 0, c2, 5)
>> +#define DBGBCR3	__ACCESS_CP14(c0, 0, c3, 5)
>> +#define DBGBCR4	__ACCESS_CP14(c0, 0, c4, 5)
>> +#define DBGBCR5	__ACCESS_CP14(c0, 0, c5, 5)
>> +#define DBGBCR6	__ACCESS_CP14(c0, 0, c6, 5)
>> +#define DBGBCR7	__ACCESS_CP14(c0, 0, c7, 5)
>> +#define DBGBCR8	__ACCESS_CP14(c0, 0, c8, 5)
>> +#define DBGBCR9	__ACCESS_CP14(c0, 0, c9, 5)
>> +#define DBGBCR10	__ACCESS_CP14(c0, 0, c10, 5)
>> +#define DBGBCR11	__ACCESS_CP14(c0, 0, c11, 5)
>> +#define DBGBCR12	__ACCESS_CP14(c0, 0, c12, 5)
>> +#define DBGBCR13	__ACCESS_CP14(c0, 0, c13, 5)
>> +#define DBGBCR14	__ACCESS_CP14(c0, 0, c14, 5)
>> +#define DBGBCR15	__ACCESS_CP14(c0, 0, c15, 5)
>> +#define DBGBVR0	__ACCESS_CP14(c0, 0, c0, 4)
>> +#define DBGBVR1	__ACCESS_CP14(c0, 0, c1, 4)
>> +#define DBGBVR2	__ACCESS_CP14(c0, 0, c2, 4)
>> +#define DBGBVR3	__ACCESS_CP14(c0, 0, c3, 4)
>> +#define DBGBVR4	__ACCESS_CP14(c0, 0, c4, 4)
>> +#define DBGBVR5	__ACCESS_CP14(c0, 0, c5, 4)
>> +#define DBGBVR6	__ACCESS_CP14(c0, 0, c6, 4)
>> +#define DBGBVR7	__ACCESS_CP14(c0, 0, c7, 4)
>> +#define DBGBVR8	__ACCESS_CP14(c0, 0, c8, 4)
>> +#define DBGBVR9	__ACCESS_CP14(c0, 0, c9, 4)
>> +#define DBGBVR10	__ACCESS_CP14(c0, 0, c10, 4)
>> +#define DBGBVR11	__ACCESS_CP14(c0, 0, c11, 4)
>> +#define DBGBVR12	__ACCESS_CP14(c0, 0, c12, 4)
>> +#define DBGBVR13	__ACCESS_CP14(c0, 0, c13, 4)
>> +#define DBGBVR14	__ACCESS_CP14(c0, 0, c14, 4)
>> +#define DBGBVR15	__ACCESS_CP14(c0, 0, c15, 4)
>> +#define DBGWCR0	__ACCESS_CP14(c0, 0, c0, 7)
>> +#define DBGWCR1	__ACCESS_CP14(c0, 0, c1, 7)
>> +#define DBGWCR2	__ACCESS_CP14(c0, 0, c2, 7)
>> +#define DBGWCR3	__ACCESS_CP14(c0, 0, c3, 7)
>> +#define DBGWCR4	__ACCESS_CP14(c0, 0, c4, 7)
>> +#define DBGWCR5	__ACCESS_CP14(c0, 0, c5, 7)
>> +#define DBGWCR6	__ACCESS_CP14(c0, 0, c6, 7)
>> +#define DBGWCR7	__ACCESS_CP14(c0, 0, c7, 7)
>> +#define DBGWCR8	__ACCESS_CP14(c0, 0, c8, 7)
>> +#define DBGWCR9	__ACCESS_CP14(c0, 0, c9, 7)
>> +#define DBGWCR10	__ACCESS_CP14(c0, 0, c10, 7)
>> +#define DBGWCR11	__ACCESS_CP14(c0, 0, c11, 7)
>> +#define DBGWCR12	__ACCESS_CP14(c0, 0, c12, 7)
>> +#define DBGWCR13	__ACCESS_CP14(c0, 0, c13, 7)
>> +#define DBGWCR14	__ACCESS_CP14(c0, 0, c14, 7)
>> +#define DBGWCR15	__ACCESS_CP14(c0, 0, c15, 7)
>> +#define DBGWVR0	__ACCESS_CP14(c0, 0, c0, 6)
>> +#define DBGWVR1	__ACCESS_CP14(c0, 0, c1, 6)
>> +#define DBGWVR2	__ACCESS_CP14(c0, 0, c2, 6)
>> +#define DBGWVR3	__ACCESS_CP14(c0, 0, c3, 6)
>> +#define DBGWVR4	__ACCESS_CP14(c0, 0, c4, 6)
>> +#define DBGWVR5	__ACCESS_CP14(c0, 0, c5, 6)
>> +#define DBGWVR6	__ACCESS_CP14(c0, 0, c6, 6)
>> +#define DBGWVR7	__ACCESS_CP14(c0, 0, c7, 6)
>> +#define DBGWVR8	__ACCESS_CP14(c0, 0, c8, 6)
>> +#define DBGWVR9	__ACCESS_CP14(c0, 0, c9, 6)
>> +#define DBGWVR10	__ACCESS_CP14(c0, 0, c10, 6)
>> +#define DBGWVR11	__ACCESS_CP14(c0, 0, c11, 6)
>> +#define DBGWVR12	__ACCESS_CP14(c0, 0, c12, 6)
>> +#define DBGWVR13	__ACCESS_CP14(c0, 0, c13, 6)
>> +#define DBGWVR14	__ACCESS_CP14(c0, 0, c14, 6)
>> +#define DBGWVR15	__ACCESS_CP14(c0, 0, c15, 6)
>> +
>> +#define read_debug_bcr(reg)	read_sysreg(DBGBCR ## reg)
>> +#define read_debug_bvr(reg)	read_sysreg(DBGBVR ## reg)
>> +#define read_debug_wcr(reg)	read_sysreg(DBGWCR ## reg)
>> +#define read_debug_wvr(reg)	read_sysreg(DBGWVR ## reg)
>> +
>> +#define write_debug_bcr(val, reg) write_sysreg(val, DBGBCR ## reg)
>> +#define write_debug_bvr(val, reg) write_sysreg(val, DBGBVR ## reg)
>> +#define write_debug_wcr(val, reg) write_sysreg(val, DBGWCR ## reg)
>> +#define write_debug_wvr(val, reg) write_sysreg(val, DBGWVR ## reg)
>> +
>> +#define PRIxDBGADDR PRIx32
>> +
>> +struct dbgregs {
>> +	uint32_t dbgbcr;
>> +	uint32_t dbgbvr;
>> +	uint32_t dbgwcr;
>> +	uint32_t dbgwvr;
>> +};
>> +
>> +#elif defined(__aarch64__)
>> +
>> +#define read_debug_bcr(reg)	read_sysreg(dbgbcr ## reg ## _el1)
>> +#define read_debug_bvr(reg)	read_sysreg(dbgbvr ## reg ## _el1)
>> +#define read_debug_wcr(reg)	read_sysreg(dbgwcr ## reg ## _el1)
>> +#define read_debug_wvr(reg)	read_sysreg(dbgwvr ## reg ## _el1)
>> +
>> +#define write_debug_bcr(val, reg) write_sysreg(val, dbgbcr ## reg ## _el1)
>> +#define write_debug_bvr(val, reg) write_sysreg(val, dbgbvr ## reg ## _el1)
>> +#define write_debug_wcr(val, reg) write_sysreg(val, dbgwcr ## reg ## _el1)
>> +#define write_debug_wvr(val, reg) write_sysreg(val, dbgwvr ## reg ## _el1)
>> +
>> +#define PRIxDBGADDR PRIx64
>> +
>> +struct dbgregs {
>> +	uint32_t dbgbcr;
>> +	uint64_t dbgbvr;
>> +	uint32_t dbgwcr;
>> +	uint64_t dbgwvr;
>> +};
>> +
>> +#endif
>> +
>> +#define MIN(a, b)		((a) < (b) ? (a) : (b))
>
> MIN() is already defined in libcflat.h
>
>> +
>> +static cpumask_t smp_test_complete;
>> +static int nbp, nwp;
>> +
>> +static void read_dbgb(int n, struct dbgregs *array)
>> +{
>> +	switch (n-1) {
>> +	case 15:
>> +		array[15].dbgbcr = read_debug_bcr(15);
>> +		array[15].dbgbvr = read_debug_bvr(15);
>> +	case 14:
>> +		array[14].dbgbcr = read_debug_bcr(14);
>> +		array[14].dbgbvr = read_debug_bvr(14);
>> +	case 13:
>> +		array[13].dbgbcr = read_debug_bcr(13);
>> +		array[13].dbgbvr = read_debug_bvr(13);
>> +	case 12:
>> +		array[12].dbgbcr = read_debug_bcr(12);
>> +		array[12].dbgbvr = read_debug_bvr(12);
>> +	case 11:
>> +		array[11].dbgbcr = read_debug_bcr(11);
>> +		array[11].dbgbvr = read_debug_bvr(11);
>> +	case 10:
>> +		array[10].dbgbcr = read_debug_bcr(10);
>> +		array[10].dbgbvr = read_debug_bvr(10);
>> +	case 9:
>> +		array[9].dbgbcr = read_debug_bcr(9);
>> +		array[9].dbgbvr = read_debug_bvr(9);
>> +	case 8:
>> +		array[8].dbgbcr = read_debug_bcr(8);
>> +		array[8].dbgbvr = read_debug_bvr(8);
>> +	case 7:
>> +		array[7].dbgbcr = read_debug_bcr(7);
>> +		array[7].dbgbvr = read_debug_bvr(7);
>> +	case 6:
>> +		array[6].dbgbcr = read_debug_bcr(6);
>> +		array[6].dbgbvr = read_debug_bvr(6);
>> +	case 5:
>> +		array[5].dbgbcr = read_debug_bcr(5);
>> +		array[5].dbgbvr = read_debug_bvr(5);
>> +	case 4:
>> +		array[4].dbgbcr = read_debug_bcr(4);
>> +		array[4].dbgbvr = read_debug_bvr(4);
>> +	case 3:
>> +		array[3].dbgbcr = read_debug_bcr(3);
>> +		array[3].dbgbvr = read_debug_bvr(3);
>> +	case 2:
>> +		array[2].dbgbcr = read_debug_bcr(2);
>> +		array[2].dbgbvr = read_debug_bvr(2);
>> +	case 1:
>> +		array[1].dbgbcr = read_debug_bcr(1);
>> +		array[1].dbgbvr = read_debug_bvr(1);
>> +	case 0:
>> +		array[0].dbgbcr = read_debug_bcr(0);
>> +		array[0].dbgbvr = read_debug_bvr(0);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static void write_dbgb(int n, struct dbgregs *array)
>> +{
>> +	switch (n-1) {
>> +	case 15:
>> +		write_debug_bcr(array[15].dbgbcr, 15);
>> +		write_debug_bvr(array[15].dbgbvr, 15);
>> +	case 14:
>> +		write_debug_bcr(array[14].dbgbcr, 14);
>> +		write_debug_bvr(array[14].dbgbvr, 14);
>> +	case 13:
>> +		write_debug_bcr(array[13].dbgbcr, 13);
>> +		write_debug_bvr(array[13].dbgbvr, 13);
>> +	case 12:
>> +		write_debug_bcr(array[12].dbgbcr, 12);
>> +		write_debug_bvr(array[12].dbgbvr, 12);
>> +	case 11:
>> +		write_debug_bcr(array[11].dbgbcr, 11);
>> +		write_debug_bvr(array[11].dbgbvr, 11);
>> +	case 10:
>> +		write_debug_bcr(array[10].dbgbcr, 10);
>> +		write_debug_bvr(array[10].dbgbvr, 10);
>> +	case 9:
>> +		write_debug_bcr(array[9].dbgbcr, 9);
>> +		write_debug_bvr(array[9].dbgbvr, 9);
>> +	case 8:
>> +		write_debug_bcr(array[8].dbgbcr, 8);
>> +		write_debug_bvr(array[8].dbgbvr, 8);
>> +	case 7:
>> +		write_debug_bcr(array[7].dbgbcr, 7);
>> +		write_debug_bvr(array[7].dbgbvr, 7);
>> +	case 6:
>> +		write_debug_bcr(array[6].dbgbcr, 6);
>> +		write_debug_bvr(array[6].dbgbvr, 6);
>> +	case 5:
>> +		write_debug_bcr(array[5].dbgbcr, 5);
>> +		write_debug_bvr(array[5].dbgbvr, 5);
>> +	case 4:
>> +		write_debug_bcr(array[4].dbgbcr, 4);
>> +		write_debug_bvr(array[4].dbgbvr, 4);
>> +	case 3:
>> +		write_debug_bcr(array[3].dbgbcr, 3);
>> +		write_debug_bvr(array[3].dbgbvr, 3);
>> +	case 2:
>> +		write_debug_bcr(array[2].dbgbcr, 2);
>> +		write_debug_bvr(array[2].dbgbvr, 2);
>> +	case 1:
>> +		write_debug_bcr(array[1].dbgbcr, 1);
>> +		write_debug_bvr(array[1].dbgbvr, 1);
>> +	case 0:
>> +		write_debug_bcr(array[0].dbgbcr, 0);
>> +		write_debug_bvr(array[0].dbgbvr, 0);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static void read_dbgw(int n, struct dbgregs *array)
>> +{
>> +	switch (n-1) {
>> +	case 15:
>> +		array[15].dbgwcr = read_debug_wcr(15);
>> +		array[15].dbgwvr = read_debug_wvr(15);
>> +	case 14:
>> +		array[14].dbgwcr = read_debug_wcr(14);
>> +		array[14].dbgwvr = read_debug_wvr(14);
>> +	case 13:
>> +		array[13].dbgwcr = read_debug_wcr(13);
>> +		array[13].dbgwvr = read_debug_wvr(13);
>> +	case 12:
>> +		array[12].dbgwcr = read_debug_wcr(12);
>> +		array[12].dbgwvr = read_debug_wvr(12);
>> +	case 11:
>> +		array[11].dbgwcr = read_debug_wcr(11);
>> +		array[11].dbgwvr = read_debug_wvr(11);
>> +	case 10:
>> +		array[10].dbgwcr = read_debug_wcr(10);
>> +		array[10].dbgwvr = read_debug_wvr(10);
>> +	case 9:
>> +		array[9].dbgwcr = read_debug_wcr(9);
>> +		array[9].dbgwvr = read_debug_wvr(9);
>> +	case 8:
>> +		array[8].dbgwcr = read_debug_wcr(8);
>> +		array[8].dbgwvr = read_debug_wvr(8);
>> +	case 7:
>> +		array[7].dbgwcr = read_debug_wcr(7);
>> +		array[7].dbgwvr = read_debug_wvr(7);
>> +	case 6:
>> +		array[6].dbgwcr = read_debug_wcr(6);
>> +		array[6].dbgwvr = read_debug_wvr(6);
>> +	case 5:
>> +		array[5].dbgwcr = read_debug_wcr(5);
>> +		array[5].dbgwvr = read_debug_wvr(5);
>> +	case 4:
>> +		array[4].dbgwcr = read_debug_wcr(4);
>> +		array[4].dbgwvr = read_debug_wvr(4);
>> +	case 3:
>> +		array[3].dbgwcr = read_debug_wcr(3);
>> +		array[3].dbgwvr = read_debug_wvr(3);
>> +	case 2:
>> +		array[2].dbgwcr = read_debug_wcr(2);
>> +		array[2].dbgwvr = read_debug_wvr(2);
>> +	case 1:
>> +		array[1].dbgwcr = read_debug_wcr(1);
>> +		array[1].dbgwvr = read_debug_wvr(1);
>> +	case 0:
>> +		array[0].dbgwcr = read_debug_wcr(0);
>> +		array[0].dbgwvr = read_debug_wvr(0);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static void write_dbgw(int n, struct dbgregs *array)
>> +{
>> +	switch (n-1) {
>> +	case 15:
>> +		write_debug_wcr(array[15].dbgwcr, 15);
>> +		write_debug_wvr(array[15].dbgwvr, 15);
>> +	case 14:
>> +		write_debug_wcr(array[14].dbgwcr, 14);
>> +		write_debug_wvr(array[14].dbgwvr, 14);
>> +	case 13:
>> +		write_debug_wcr(array[13].dbgwcr, 13);
>> +		write_debug_wvr(array[13].dbgwvr, 13);
>> +	case 12:
>> +		write_debug_wcr(array[12].dbgwcr, 12);
>> +		write_debug_wvr(array[12].dbgwvr, 12);
>> +	case 11:
>> +		write_debug_wcr(array[11].dbgwcr, 11);
>> +		write_debug_wvr(array[11].dbgwvr, 11);
>> +	case 10:
>> +		write_debug_wcr(array[10].dbgwcr, 10);
>> +		write_debug_wvr(array[10].dbgwvr, 10);
>> +	case 9:
>> +		write_debug_wcr(array[9].dbgwcr, 9);
>> +		write_debug_wvr(array[9].dbgwvr, 9);
>> +	case 8:
>> +		write_debug_wcr(array[8].dbgwcr, 8);
>> +		write_debug_wvr(array[8].dbgwvr, 8);
>> +	case 7:
>> +		write_debug_wcr(array[7].dbgwcr, 7);
>> +		write_debug_wvr(array[7].dbgwvr, 7);
>> +	case 6:
>> +		write_debug_wcr(array[6].dbgwcr, 6);
>> +		write_debug_wvr(array[6].dbgwvr, 6);
>> +	case 5:
>> +		write_debug_wcr(array[5].dbgwcr, 5);
>> +		write_debug_wvr(array[5].dbgwvr, 5);
>> +	case 4:
>> +		write_debug_wcr(array[4].dbgwcr, 4);
>> +		write_debug_wvr(array[4].dbgwvr, 4);
>> +	case 3:
>> +		write_debug_wcr(array[3].dbgwcr, 3);
>> +		write_debug_wvr(array[3].dbgwvr, 3);
>> +	case 2:
>> +		write_debug_wcr(array[2].dbgwcr, 2);
>> +		write_debug_wvr(array[2].dbgwvr, 2);
>> +	case 1:
>> +		write_debug_wcr(array[1].dbgwcr, 1);
>> +		write_debug_wvr(array[1].dbgwvr, 1);
>> +	case 0:
>> +		write_debug_wcr(array[0].dbgwcr, 0);
>> +		write_debug_wvr(array[0].dbgwvr, 0);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>
> Any reason not to implement the above functions with
> loops? E.g.
>
>  while (--n >= 0) {
>    write_debug_wcr(array[n].dbgwcr, n);
>    write_debug_wvr(array[n].dbgwvr, n);
>  }

Well we are using macro pasting here so we need to instantiate all
possible asm fragments.

>
>> +
>> +static void dump_regs(int cpu, struct dbgregs *array)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nbp; i++)
>> +		printf("CPU%d: B%d 0x%08x:0x%" PRIxDBGADDR "\n", cpu, i,
>> +			array[i].dbgbcr, array[i].dbgbvr);
>> +	for (i = 0; i < nwp; i++)
>> +		printf("CPU%d: W%d 0x%08x:0x%" PRIxDBGADDR "\n", cpu, i,
>> +			array[i].dbgwcr, array[i].dbgwvr);
>> +}
>> +
>> +static int compare_regs(struct dbgregs *initial, struct dbgregs *current)
>> +{
>> +	int i, errors = 0;
>> +
>> +	for (i = 0; i < nbp; i++) {
>> +		if (initial[i].dbgbcr != current[i].dbgbcr) {
>> +			printf("ERROR DBGBCR%d: %" PRIx32 "!=%" PRIx32 "\n",
>> +				i, initial[i].dbgbcr, current[i].dbgbcr);
>> +			errors++;
>> +		}
>> +		if (initial[i].dbgbvr != current[i].dbgbvr) {
>> +			printf("ERROR DBGBVR%d: %" PRIxDBGADDR "!=%" PRIxDBGADDR "\n",
>> +				i, initial[i].dbgbvr, current[i].dbgbvr);
>> +			errors++;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < nwp; i++) {
>> +		if (initial[i].dbgwcr != current[i].dbgwcr) {
>> +			printf("ERROR DBGWCR%d: %" PRIx32 "!=%" PRIx32 "\n",
>> +				i, initial[i].dbgwcr, current[i].dbgwcr);
>> +			errors++;
>> +		}
>> +		if (initial[i].dbgwvr != current[i].dbgwvr) {
>> +			printf("ERROR DBWVR%d: %" PRIxDBGADDR "!=%" PRIxDBGADDR "\n",
>> +				i, initial[i].dbgwvr, current[i].dbgwvr);
>> +			errors++;
>> +		}
>> +	}
>
> Shouldn't these error messages also output the CPU?
>
>> +
>> +	return errors;
>> +}
>> +
>> +static void test_debug_regs(void)
>> +{
>> +	int errors = 0;
>> +	int cpu = smp_processor_id();
>> +	int i;
>> +	struct dbgregs initial[16], current[16];
>> +
>> +	printf("CPU%d online\n", cpu);
>> +
>> +	printf("Reading Initial Values\n");
>> +	read_dbgb(nbp, &initial[0]);
>> +	read_dbgw(nwp, &initial[0]);
>> +	dump_regs(cpu, &initial[0]);
>> +
>> +	printf("Setting simple BPs and WPs\n");
>> +	for (i = 0; i < nbp; i++) {
>> +		initial[i].dbgbcr = (0x3 << 1) | (0xf << 5) ; /* E=0 */
>> +		initial[i].dbgbvr = ~0x3;
>> +	}
>> +	for (i = 0; i < nwp; i++) {
>> +		initial[i].dbgwcr = (0x3 << 1) | (0x3 << 3); /* E=0 */
>> +		initial[i].dbgwvr = ~0x7;
>> +	}
>> +	write_dbgb(nbp, &initial[0]);
>> +	write_dbgw(nwp, &initial[0]);
>> +
>> +	printf("Reading values back\n");
>
> The above informational printfs will print for each cpu and in whatever
> order the cpus are run, potentially giving strangely ordered output. Do
> we really need them?
>
>> +	read_dbgb(nbp, &current[0]);
>> +	read_dbgw(nwp, &current[0]);
>> +
>> +	errors += compare_regs(&initial[0], &current[0]);
>> +
>> +	report("CPU%d: Done - Errors: %d\n", errors == 0, cpu, errors);
>> +
>> +	if (errors)
>> +		dump_regs(cpu, &current[0]);
>> +
>> +	cpumask_set_cpu(cpu, &smp_test_complete);
>> +	if (cpu != 0)
>> +		halt();
>> +}
>> +
>> +static char *split_var(char *s, long *val)
>> +{
>> +	char *p;
>> +
>> +	p = strchr(s, '=');
>> +	if (!p)
>> +		return NULL;
>> +
>> +	*val = atol(p+1);
>> +	*p = '\0';
>> +
>> +	return s;
>> +}
>
> We have parse_keyval() in lib/util.c. It isn't an awesome design,
> but it's similar to this old split_var() does. I think Radim has
> plans to improve on it, at which time we'll fix this test up too.
> In short, let's use parse_keyval here for now and not add this
> function.
>
>> +
>> +#if defined(__arm__)
>> +static void read_debug_feature_regs(void)
>> +{
>> +	uint32_t id_dfr0 = read_sysreg(ID_DFR0);
>> +	uint32_t dbgdidr = read_sysreg(DBGDIDR);
>> +	uint32_t dbgdscr = read_sysreg(DBGDSCR);
>> +	char *pmu, *dm_m, *mmt_m, *cpt_m, *mmd_m, *cpsd_m, *cpd_m;
>> +
>> +	printf("ID_DFR0: 0x%" PRIx32 "\n", id_dfr0);
>> +
>> +	/* PMU support */
>> +	switch ((id_dfr0 >> 24) & 0xf) {
>> +	case 0:
>> +		pmu = "No PMUv2";
>> +		break;
>> +	case 1:
>> +		pmu = "PMUv1";
>> +		break;
>> +	case 2:
>> +		pmu = "PMUv2";
>> +		break;
>> +	case 3:
>> +		pmu = "No PMU Extensions";
>> +		break;
>> +	default:
>> +		pmu = "Invalid";
>> +		break;
>> +	}
>> +	printf("  PMU: %s\n", pmu);
>> +
>> +	/* M-profile debug model */
>> +	switch ((id_dfr0 >> 20) & 0xf) {
>> +	case 0:
>> +		dm_m = "Not supported";
>> +		break;
>> +	case 1:
>> +		dm_m = "M-profile debug with memory mapped access";
>> +		break;
>> +	default:
>> +		dm_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  M-profile debug: %s\n", dm_m);
>> +
>> +	/* Memory mapped trace model */
>> +	switch ((id_dfr0 >> 16) & 0xf) {
>> +	case 0:
>> +		mmt_m = "Not supported";
>> +		break;
>> +	case 1:
>> +		mmt_m = "Supported with memory mapped access";
>> +		break;
>> +	default:
>> +		mmt_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  Trace Model: %s\n", mmt_m);
>> +
>> +	/* Co-processor trace model */
>> +	switch ((id_dfr0 >> 12) & 0xf) {
>> +	case 0:
>> +		cpt_m = "Not supported";
>> +		break;
>> +	case 1:
>> +		cpt_m = "Supported with CP14access";
>> +		break;
>> +	default:
>> +		cpt_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  Trace Model: %s\n", cpt_m);
>> +
>> +	/* Memory mapped debug model, A and R profiles */
>> +	switch ((id_dfr0 >> 8) & 0xf) {
>> +	case 0:
>> +		mmd_m = "Not supported or pre-ARMv6";
>> +		break;
>> +	case 4:
>> +		mmd_m = "Supported v7 Debug Architecture";
>> +		break;
>> +	case 5:
>> +		mmd_m = "Supported v7.1 Debug Architecture";
>> +		break;
>> +	default:
>> +		mmd_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  Debug Model: %s\n", mmd_m);
>> +
>> +	/* Co-processor secure debug model */
>> +	switch ((id_dfr0 >> 4) & 0xf) {
>> +	case 0:
>> +		cpsd_m = "Not supported";
>> +		break;
>> +	case 2:
>> +		cpsd_m = "Supported v6 Debug Architecture";
>> +		break;
>> +	case 3:
>> +		cpsd_m = "Supported v6.1 Debug Architecture";
>> +		break;
>> +	case 4:
>> +		cpsd_m = "Supported v7 Debug Architecture";
>> +		break;
>> +	case 5:
>> +		cpsd_m = "Supported v7.1 Debug Architecture";
>> +		break;
>> +	default:
>> +		cpsd_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  Co-processor Secure Debug Model: %s\n", cpsd_m);
>> +
>> +	/* Co-processor debug model */
>> +	switch ((id_dfr0 >> 0) & 0xf) {
>> +	case 0:
>> +		cpd_m = "Not supported";
>> +		break;
>> +	case 2:
>> +		cpd_m = "Supported v6 Debug Architecture";
>> +		break;
>> +	case 3:
>> +		cpd_m = "Supported v6.1 Debug Architecture";
>> +		break;
>> +	case 4:
>> +		cpd_m = "Supported v7 Debug Architecture";
>> +		break;
>> +	case 5:
>> +		cpd_m = "Supported v7.1 Debug Architecture";
>> +		break;
>> +	default:
>> +		cpd_m = "Invalid";
>> +		break;
>> +	}
>> +	printf("  Co-processor Debug Model: %s\n", cpd_m);
>> +
>> +	/* DBGIDR, num registers */
>> +	nwp = (dbgdidr >> 28) + 1;
>> +	/* BRPs == 0 is reserved, else it is BRPs + 1 */
>> +	nbp = ((dbgdidr >> 24) & 0xf);
>> +	if (nbp > 0)
>> +		nbp += 1;
>> +
>> +	printf("DBGDIDR: 0x%" PRIx32 " nwp:%d nbp:%d\n", dbgdidr, nwp, nbp);
>> +
> nit: extra blank line
>> +	printf("DBGDSCR: 0x%" PRIx32 "\n", dbgdscr);
>
> You might want to consider report_info() instead of printfs here.
>
>> +
>> +}
>> +#elif defined(__aarch64__)
>> +static void read_debug_feature_regs(void)
>> +{
>> +	uint64_t id = read_sysreg(id_aa64dfr0_el1);
>> +
>> +	/* Check the number of break/watch points */
>> +	nbp = ((id >> 12) & 0xf);
>> +	if (nbp > 0)
>> +		nbp -= 1;
>> +
>> +	nwp = ((id >> 20) & 0xf);
>> +	if (nwp > 0)
>> +		nwp -= 1;
>> +
>> +	printf("ID_AA64_DFR0_EL1: 0x%" PRIx64 " nwp:%d nbp:%d\n", id, nwp, nbp);
>
> report_info()?
>
>> +}
>> +#endif
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	int cpu, i;
>> +
>> +	read_debug_feature_regs();
>> +
>> +	for (i = 0; i < argc; ++i) {
>
> should start at i = 1, argv[0] is program name
>
>> +		char *var;
>> +		long val;
>> +
>> +		var = split_var(argv[i], &val);
>> +
>> +		if (var) {
>> +			int tmp;
>> +
>> +			if (strcmp(var, "nbp") == 0) {
>> +				tmp = MIN(nbp, val);
>> +				nbp = tmp;
>> +			}
>> +			if (strcmp(var, "nwp") == 0) {
>> +				tmp = MIN(nwp, val);
>> +				nwp = tmp;
>> +			}
>> +		}
>> +	}
>
> Should probably report_info() the values of nbp and nwp
> here as they're variable and have the MIN() thing and all
> modifying them.
>
>> +
>> +	for_each_present_cpu(cpu) {
>> +		if (cpu == 0)
>> +			continue;
>> +		smp_boot_secondary(cpu, test_debug_regs);
>> +	}
>> +
>> +	test_debug_regs();
>> +
>> +	while (!cpumask_full(&smp_test_complete))
>> +		cpu_relax();
>> +
>> +	return report_summary();
>> +}
>> --
>> 2.13.0
>>
>
> Thanks,
> drew


--
Alex Bennée