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
