Re: [PATCH v9 10/14] KVM: selftests: aarch64: Add aarch64/page_fault_test

[Sean Christopherson <seanjc@google.com>]

On Tue, Oct 11, 2022, Ricardo Koller wrote:
> +/* Returns true to continue the test, and false if it should be skipped. */
> +static bool punch_hole_in_backing_store(struct kvm_vm *vm,
> +					struct userspace_mem_region *region)
> +{
> +	void *hva = (void *)region->region.userspace_addr;
> +	uint64_t paging_size = region->region.memory_size;
> +	int ret, fd = region->fd;
> +
> +	if (fd != -1) {
> +		ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> +				0, paging_size);
> +		TEST_ASSERT(ret == 0, "fallocate failed, errno: %d\n", errno);
> +	} else {
> +		ret = madvise(hva, paging_size, MADV_DONTNEED);
> +		TEST_ASSERT(ret == 0, "madvise failed, errno: %d\n", errno);
> +	}

Uber nit, no need to manually print the errno, TEST_ASSERT() does that automatically
and pretty prints the string too.  I know this because I keep forgetting myself :-).

> +/* Returns true to continue the test, and false if it should be skipped. */
> +static bool handle_cmd(struct kvm_vm *vm, int cmd)
> +{
> +	struct userspace_mem_region *data_region, *pt_region;
> +	bool continue_test = true;
> +
> +	data_region = vm_get_mem_region(vm, MEM_REGION_TEST_DATA);
> +	pt_region = vm_get_mem_region(vm, MEM_REGION_PT);
> +
> +	if (cmd == CMD_SKIP_TEST)
> +		continue_test = false;
> +
> +	if (cmd & CMD_HOLE_PT)
> +		continue_test = punch_hole_in_backing_store(vm, pt_region);
> +	if (cmd & CMD_HOLE_DATA)
> +		continue_test = punch_hole_in_backing_store(vm, data_region);
> +
> +	return continue_test;
> +}

...

> +static void setup_abort_handlers(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
> +		struct test_desc *test)

Align params.

static void setup_abort_handlers(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
				 struct test_desc *test)

> +static void for_each_test_and_guest_mode(
> +		void (*func)(enum vm_guest_mode m, void *a),

If you spin a new version, can you put together a patch or mini-series to add a
typedef for this function pointer?  Then this function doesn't need a funky wrap.
Or alternatively, as follow-up to avoid delaying this series even longer.

E.g.

static void for_each_test_and_guest_mode(guest_mode_test_t func,
					 enum vm_mem_backing_src_type src_type)

diff --git a/tools/testing/selftests/kvm/include/guest_modes.h b/tools/testing/selftests/kvm/include/guest_modes.h
index b691df33e64e..ee7c5c271eb2 100644
--- a/tools/testing/selftests/kvm/include/guest_modes.h
+++ b/tools/testing/selftests/kvm/include/guest_modes.h
@@ -15,7 +15,9 @@ extern struct guest_mode guest_modes[NUM_VM_MODES];
        guest_modes[mode] = (struct guest_mode){ supported, enabled }; \
 })
 
+typedef void (*guest_mode_test_t)(enum vm_guest_mode, void *);
+
 void guest_modes_append_default(void);
-void for_each_guest_mode(void (*func)(enum vm_guest_mode, void *), void *arg);
+void for_each_guest_mode(guest_mode_test_t func, void *arg);
 void guest_modes_help(void);
 void guest_modes_cmdline(const char *arg);



> +		enum vm_mem_backing_src_type src_type)
> +{
> +	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);
> +	}
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	enum vm_mem_backing_src_type src_type;
> +	int opt;
> +
> +	setbuf(stdout, NULL);
> +
> +	src_type = DEFAULT_VM_MEM_SRC;
> +
> +	while ((opt = getopt(argc, argv, "hm:s:")) != -1) {
> +		switch (opt) {
> +		case 'm':
> +			guest_modes_cmdline(optarg);
> +			break;
> +		case 's':
> +			src_type = parse_backing_src_type(optarg);
> +			break;
> +		case 'h':
> +		default:
> +			help(argv[0]);
> +			exit(0);
> +		}
> +	}
> +
> +	for_each_test_and_guest_mode(run_test, src_type);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index c1ddca8db225..5f977528e09c 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -105,11 +105,19 @@ enum {
>  #define ESR_EC_MASK		(ESR_EC_NUM - 1)
>  
>  #define ESR_EC_SVC64		0x15
> +#define ESR_EC_IABT		0x21
> +#define ESR_EC_DABT		0x25
>  #define ESR_EC_HW_BP_CURRENT	0x31
>  #define ESR_EC_SSTEP_CURRENT	0x33
>  #define ESR_EC_WP_CURRENT	0x35
>  #define ESR_EC_BRK_INS		0x3c
>  
> +/* Access flag */
> +#define PTE_AF			(1ULL << 10)
> +
> +/* Access flag update enable/disable */
> +#define TCR_EL1_HA		(1ULL << 39)
> +
>  void aarch64_get_supported_page_sizes(uint32_t ipa,
>  				      bool *ps4k, bool *ps16k, bool *ps64k);
>  
> -- 
> 2.38.0.rc1.362.ged0d419d3c-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm