Re: [PATCH v3 09/13] KVM: selftests: aarch64: Add aarch64/page_fault_test

[Oliver Upton <oupton@google.com>]

Hi Ricardo,

On Thu, Apr 07, 2022 at 05:41:16PM -0700, Ricardo Koller wrote:
> diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
> new file mode 100644
> index 000000000000..04fc6007f630
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c

[...]

> +/* Guest virtual addresses that point to the test page and its PTE. */
> +#define TEST_GVA				0xc0000000
> +#define TEST_EXEC_GVA				0xc0000008
> +#define TEST_PTE_GVA				0xd0000000
> +#define TEST_DATA				0x0123456789ABCDEF
> +
> +#define CMD_NONE				(0)
> +#define CMD_SKIP_TEST				(1ULL << 1)
> +#define CMD_HOLE_PT				(1ULL << 2)
> +#define CMD_HOLE_TEST				(1ULL << 3)
> +
> +#define PREPARE_FN_NR				10
> +#define CHECK_FN_NR				10
> +
> +uint64_t pte_gpa;
> +
> +enum { PT, TEST, NR_MEMSLOTS};

nit: fix formatting (just use newlines for each value).

> +struct memslot_desc {
> +	void *hva;
> +	uint64_t gpa;
> +	uint64_t size;
> +	uint64_t guest_pages;
> +	uint64_t backing_pages;
> +	enum vm_mem_backing_src_type src_type;
> +	uint32_t idx;
> +} memslot[NR_MEMSLOTS] = {
> +	{
> +		.idx = TEST_PT_SLOT_INDEX,
> +		.backing_pages = PT_MEMSLOT_BACKING_SRC_NPAGES,
> +	},
> +	{
> +		.idx = TEST_MEM_SLOT_INDEX,
> +		.backing_pages = TEST_MEMSLOT_BACKING_SRC_NPAGES,

nit: initialize fields in the order they appear in the struct.

[...]

> +static void guest_write64(void)
> +{
> +	uint64_t val;
> +
> +	WRITE_ONCE(*((uint64_t *)TEST_GVA), TEST_DATA);
> +	val = READ_ONCE(*(uint64_t *)TEST_GVA);

nit: you could proabably avoid casting with a global pointer.

  static uint64_t *guest_test_memory = (uint64_t *)TEST_GVA;

[...]

> +static void guest_test_check(struct test_desc *test)
> +{
> +	void (*check_fn)(void);
> +	int i;
> +
> +	for (i = 0; i < CHECK_FN_NR; i++) {
> +		check_fn = test->guest_test_check[i];
> +		if (!check_fn)
> +			continue;
> +		check_fn();

nit:

  if (check_fn)
          check_fn();

One less line :)

> +	}
> +}
> +
> +static void guest_code(struct test_desc *test)
> +{
> +	if (!test->guest_test)
> +		test->guest_test = guest_nop;

In other cases you've chosen to skip function calls if NULL. Is there a
need to call something here that I've missed or could you just do:

  if (test->guest_test)
          test->guest_test();

below?

> +	if (!guest_prepare(test))
> +		GUEST_SYNC(CMD_SKIP_TEST);
> +
> +	GUEST_SYNC(test->mem_mark_cmd);
> +	test->guest_test();
> +
> +	guest_test_check(test);
> +	GUEST_DONE();
> +}

[...]

> +
> +#define SNAME(s)			#s
> +#define SCAT2(a, b)			SNAME(a ## _ ## b)
> +#define SCAT3(a, b, c)			SCAT2(a, SCAT2(b, c))
> +
> +#define _CHECK(_test)			_CHECK_##_test
> +#define _PREPARE(_test)			_PREPARE_##_test
> +#define _PREPARE_guest_read64		guest_prepare_nop
> +#define _PREPARE_guest_ld_preidx	guest_prepare_nop
> +#define _PREPARE_guest_write64		guest_prepare_nop
> +#define _PREPARE_guest_st_preidx	guest_prepare_nop
> +#define _PREPARE_guest_exec		guest_prepare_nop
> +#define _PREPARE_guest_at		guest_prepare_nop

Since you check for NULL in guest_prepare(), could you just use NULL for
these and drop guest_prepare_nop()?

> +#define _PREPARE_guest_dc_zva		guest_check_dc_zva
> +#define _PREPARE_guest_cas		guest_check_lse
> +
> +/* With or without access flag checks */
> +#define _PREPARE_with_af		guest_set_ha, guest_clear_pte_af
> +#define _PREPARE_no_af			guest_prepare_nop
> +#define _CHECK_with_af			guest_check_pte_af
> +#define _CHECK_no_af			guest_check_nop
> +
> +/* Performs an access and checks that no faults (no events) were triggered. */
> +#define TEST_ACCESS(_access, _with_af, _mark_cmd)				\
> +{										\
> +	.name			= SCAT3(_access, _with_af, #_mark_cmd),		\
> +	.guest_prepare		= { _PREPARE(_with_af),				\
> +				    _PREPARE(_access) },			\
> +	.mem_mark_cmd		= _mark_cmd,					\
> +	.guest_test		= _access,					\
> +	.guest_test_check	= { _CHECK(_with_af) },				\
> +	.expected_events	= { 0 },					\
> +}
> +
> +static struct test_desc tests[] = {
> +	/* Check that HW is setting the Access Flag (AF) (sanity checks). */
> +	TEST_ACCESS(guest_read64, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_ld_preidx, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_cas, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_write64, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_st_preidx, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_dc_zva, with_af, CMD_NONE),
> +	TEST_ACCESS(guest_exec, with_af, CMD_NONE),
> +
> +	/*
> +	 * Accessing a hole in the test memslot (punched with fallocate or
> +	 * madvise) shouldn't fault (more sanity checks).
> +	 */
> +	TEST_ACCESS(guest_read64, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_cas, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_ld_preidx, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_write64, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_st_preidx, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_at, no_af, CMD_HOLE_TEST),
> +	TEST_ACCESS(guest_dc_zva, no_af, CMD_HOLE_TEST),
> +
> +	{ 0 },

nit: no comma, we don't want to ever add anything after the sentinel I
presume.

> +};
> +
> +static void for_each_test_and_guest_mode(
> +		void (*func)(enum vm_guest_mode m, void *a),
> +		enum vm_mem_backing_src_type src_type)
> +{

Could you avoid the forward declaration and instead move definitions
around to satisfy the compiler?

> +	struct test_desc *t;
> +
> +	for (t = &tests[0]; t->name; t++) {
> +		if (t->skip)
> +			continue;
> +
> +		struct test_params p = {
> +			.src_type = src_type,
> +			.test_desc = t,
> +		};
> +
> +		for_each_guest_mode(run_test, &p);
> +	}
> +}
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 16753a1f28e3..cb5849fd8fd1 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -125,6 +125,12 @@ enum {
>  #define ESR_EC_WP_CURRENT	0x35
>  #define ESR_EC_BRK_INS		0x3c
>  
> +/* Access flag */
> +#define PTE_AF			(1ULL << 10)
> +
> +/* Acces flag update enable/disable */

typo: Access

--
Thanks,
Oliver