2020-04-10 23:20:11

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 00/10] KVM: selftests: Add KVM_SET_MEMORY_REGION tests

This is v2-ish of my series to add a "delete" testcase[1], and v5.1 of
Wainer's series to add a "max" testcase[2].

I've only tested on x86_64. I fudged compile testing on !x86_64 by
inverting the ifdefs, e.g. to squish unused var warnings, but by no means
is the code actually tested on other architectures.

I kept Andrew's review for the "max" test. Other than the 1MB->2MB
change (see below), it was basically a straight copy-paste of code.

v1->v2 of delete:
- Drop patch to expose primary memslot. [Peter]
- Add explicit synchronization to MOVE and DELETE tests. [Peter]
- Use list.h instead of open coding linked lists. [Peter]
- Clean up the code and separate the testcases into separate functions.
- Expand GUEST_ASSERT() to allow passing a value back to the host for
printing.
- Move to common KVM, with ifdefs to hide the x86_64-only stuff (which
is a lot at this point).
- Do KVM_SET_NR_MMU_PAGES in the "zero" testcase to get less obscure
behavior for KVM_RUN. [Christian]

v5.1 of max:
- Fix a whitespace issue in vm_get_fd(). [checkpatch]
- Move the code to set_memory_region_test. The only _intended_
functional change is to create 2MB regions instead of 1MB regions.
The only motivation for doing so was to reuse an existing define in
set_memory_region_test.

[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]

Sean Christopherson (8):
KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()
KVM: selftests: Use kernel's list instead of homebrewed replacement
KVM: selftests: Add util to delete memory region
KVM: selftests: Add GUEST_ASSERT variants to pass values to host
KVM: sefltests: Add explicit synchronization to move mem region test
KVM: selftests: Add "delete" testcase to set_memory_region_test
KVM: selftests: Add "zero" testcase to set_memory_region_test
KVM: selftests: Make set_memory_region_test common to all
architectures

Wainer dos Santos Moschetta (2):
selftests: kvm: Add vm_get_fd() in kvm_util
selftests: kvm: Add testcase for creating max number of memslots

tools/testing/selftests/kvm/.gitignore | 2 +-
tools/testing/selftests/kvm/Makefile | 4 +-
.../testing/selftests/kvm/include/kvm_util.h | 28 +-
tools/testing/selftests/kvm/lib/kvm_util.c | 154 +++----
.../selftests/kvm/lib/kvm_util_internal.h | 8 +-
.../selftests/kvm/lib/s390x/processor.c | 5 +-
.../selftests/kvm/set_memory_region_test.c | 403 ++++++++++++++++++
.../kvm/x86_64/set_memory_region_test.c | 141 ------
8 files changed, 520 insertions(+), 225 deletions(-)
create mode 100644 tools/testing/selftests/kvm/set_memory_region_test.c
delete mode 100644 tools/testing/selftests/kvm/x86_64/set_memory_region_test.c

--
2.26.0


2020-04-10 23:20:17

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 04/10] KVM: selftests: Add GUEST_ASSERT variants to pass values to host

Add variants of GUEST_ASSERT to pass values back to the host, e.g. to
help debug/understand a failure when the the cause of the assert isn't
necessarily binary.

It'd probably be possible to auto-calculate the number of arguments and
just have a single GUEST_ASSERT, but there are a limited number of
variants and silently eating arguments could lead to subtle code bugs.

Signed-off-by: Sean Christopherson <[email protected]>
---
.../testing/selftests/kvm/include/kvm_util.h | 25 +++++++++++++++----
1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index d4c3e4d9cd92..e38d91bd8ec1 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -313,11 +313,26 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc);

#define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage)
#define GUEST_DONE() ucall(UCALL_DONE, 0)
-#define GUEST_ASSERT(_condition) do { \
- if (!(_condition)) \
- ucall(UCALL_ABORT, 2, \
- "Failed guest assert: " \
- #_condition, __LINE__); \
+#define __GUEST_ASSERT(_condition, _nargs, _args...) do { \
+ if (!(_condition)) \
+ ucall(UCALL_ABORT, 2 + _nargs, \
+ "Failed guest assert: " \
+ #_condition, __LINE__, _args); \
} while (0)

+#define GUEST_ASSERT(_condition) \
+ __GUEST_ASSERT((_condition), 0, 0)
+
+#define GUEST_ASSERT_1(_condition, arg1) \
+ __GUEST_ASSERT((_condition), 1, (arg1))
+
+#define GUEST_ASSERT_2(_condition, arg1, arg2) \
+ __GUEST_ASSERT((_condition), 2, (arg1), (arg2))
+
+#define GUEST_ASSERT_3(_condition, arg1, arg2, arg3) \
+ __GUEST_ASSERT((_condition), 3, (arg1), (arg2), (arg3))
+
+#define GUEST_ASSERT_4(_condition, arg1, arg2, arg3, arg4) \
+ __GUEST_ASSERT((_condition), 4, (arg1), (arg2), (arg3), (arg4))
+
#endif /* SELFTEST_KVM_UTIL_H */
--
2.26.0

2020-04-10 23:20:42

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH 02/10] KVM: selftests: Use kernel's list instead of homebrewed replacement

Replace the KVM selftests' homebrewed linked lists for vCPUs and memory
regions with the kernel's 'struct list_head'.

Signed-off-by: Sean Christopherson <[email protected]>
---
.../testing/selftests/kvm/include/kvm_util.h | 1 +
tools/testing/selftests/kvm/lib/kvm_util.c | 94 ++++++++-----------
.../selftests/kvm/lib/kvm_util_internal.h | 8 +-
.../selftests/kvm/lib/s390x/processor.c | 5 +-
4 files changed, 48 insertions(+), 60 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index a99b875f50d2..2f329e785c58 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -10,6 +10,7 @@
#include "test_util.h"

#include "asm/kvm.h"
+#include "linux/list.h"
#include "linux/kvm.h"
#include <sys/ioctl.h>

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 9a783c20dd26..105ee9bc09f0 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -161,6 +161,9 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
vm = calloc(1, sizeof(*vm));
TEST_ASSERT(vm != NULL, "Insufficient Memory");

+ INIT_LIST_HEAD(&vm->vcpus);
+ INIT_LIST_HEAD(&vm->userspace_mem_regions);
+
vm->mode = mode;
vm->type = 0;

@@ -258,8 +261,7 @@ void kvm_vm_restart(struct kvm_vm *vmp, int perm)
if (vmp->has_irqchip)
vm_create_irqchip(vmp);

- for (region = vmp->userspace_mem_region_head; region;
- region = region->next) {
+ list_for_each_entry(region, &vmp->userspace_mem_regions, list) {
int ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION, &region->region);
TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed,\n"
" rc: %i errno: %i\n"
@@ -319,8 +321,7 @@ userspace_mem_region_find(struct kvm_vm *vm, uint64_t start, uint64_t end)
{
struct userspace_mem_region *region;

- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
uint64_t existing_start = region->region.guest_phys_addr;
uint64_t existing_end = region->region.guest_phys_addr
+ region->region.memory_size - 1;
@@ -378,11 +379,11 @@ kvm_userspace_memory_region_find(struct kvm_vm *vm, uint64_t start,
*/
struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
{
- struct vcpu *vcpup;
+ struct vcpu *vcpu;

- for (vcpup = vm->vcpu_head; vcpup; vcpup = vcpup->next) {
- if (vcpup->id == vcpuid)
- return vcpup;
+ list_for_each_entry(vcpu, &vm->vcpus, list) {
+ if (vcpu->id == vcpuid)
+ return vcpu;
}

return NULL;
@@ -392,16 +393,15 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
* VM VCPU Remove
*
* Input Args:
- * vm - Virtual Machine
* vcpu - VCPU to remove
*
* Output Args: None
*
* Return: None, TEST_ASSERT failures for all error conditions
*
- * Within the VM specified by vm, removes the VCPU given by vcpuid.
+ * Removes a vCPU from a VM and frees its resources.
*/
-static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
+static void vm_vcpu_rm(struct vcpu *vcpu)
{
int ret;

@@ -412,21 +412,17 @@ static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
TEST_ASSERT(ret == 0, "Close of VCPU fd failed, rc: %i "
"errno: %i", ret, errno);

- if (vcpu->next)
- vcpu->next->prev = vcpu->prev;
- if (vcpu->prev)
- vcpu->prev->next = vcpu->next;
- else
- vm->vcpu_head = vcpu->next;
+ list_del(&vcpu->list);
free(vcpu);
}

void kvm_vm_release(struct kvm_vm *vmp)
{
+ struct vcpu *vcpu, *tmp;
int ret;

- while (vmp->vcpu_head)
- vm_vcpu_rm(vmp, vmp->vcpu_head);
+ list_for_each_entry_safe(vcpu, tmp, &vmp->vcpus, list)
+ vm_vcpu_rm(vcpu);

ret = close(vmp->fd);
TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
@@ -442,15 +438,15 @@ void kvm_vm_release(struct kvm_vm *vmp)
*/
void kvm_vm_free(struct kvm_vm *vmp)
{
+ struct userspace_mem_region *region, *tmp;
int ret;

if (vmp == NULL)
return;

/* Free userspace_mem_regions. */
- while (vmp->userspace_mem_region_head) {
- struct userspace_mem_region *region
- = vmp->userspace_mem_region_head;
+ list_for_each_entry_safe(region, tmp, &vmp->userspace_mem_regions, list) {
+ list_del(&region->list);

region->region.memory_size = 0;
ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION,
@@ -458,7 +454,6 @@ void kvm_vm_free(struct kvm_vm *vmp)
TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed, "
"rc: %i errno: %i", ret, errno);

- vmp->userspace_mem_region_head = region->next;
sparsebit_free(&region->unused_phy_pages);
ret = munmap(region->mmap_start, region->mmap_size);
TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i",
@@ -611,12 +606,10 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
(uint64_t) region->region.memory_size);

/* Confirm no region with the requested slot already exists. */
- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
- if (region->region.slot == slot)
- break;
- }
- if (region != NULL)
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
+ if (region->region.slot != slot)
+ continue;
+
TEST_FAIL("A mem region with the requested slot "
"already exists.\n"
" requested slot: %u paddr: 0x%lx npages: 0x%lx\n"
@@ -625,6 +618,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
region->region.slot,
(uint64_t) region->region.guest_phys_addr,
(uint64_t) region->region.memory_size);
+ }

/* Allocate and initialize new mem region structure. */
region = calloc(1, sizeof(*region));
@@ -685,10 +679,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
guest_paddr, (uint64_t) region->region.memory_size);

/* Add to linked-list of memory regions. */
- if (vm->userspace_mem_region_head)
- vm->userspace_mem_region_head->prev = region;
- region->next = vm->userspace_mem_region_head;
- vm->userspace_mem_region_head = region;
+ list_add(&region->list, &vm->userspace_mem_regions);
}

/*
@@ -711,20 +702,17 @@ memslot2region(struct kvm_vm *vm, uint32_t memslot)
{
struct userspace_mem_region *region;

- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
if (region->region.slot == memslot)
- break;
- }
- if (region == NULL) {
- fprintf(stderr, "No mem region with the requested slot found,\n"
- " requested slot: %u\n", memslot);
- fputs("---- vm dump ----\n", stderr);
- vm_dump(stderr, vm, 2);
- TEST_FAIL("Mem region not found");
+ return region;
}

- return region;
+ fprintf(stderr, "No mem region with the requested slot found,\n"
+ " requested slot: %u\n", memslot);
+ fputs("---- vm dump ----\n", stderr);
+ vm_dump(stderr, vm, 2);
+ TEST_FAIL("Mem region not found");
+ return NULL;
}

/*
@@ -862,10 +850,7 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
"vcpu id: %u errno: %i", vcpuid, errno);

/* Add to linked-list of VCPUs. */
- if (vm->vcpu_head)
- vm->vcpu_head->prev = vcpu;
- vcpu->next = vm->vcpu_head;
- vm->vcpu_head = vcpu;
+ list_add(&vcpu->list, &vm->vcpus);
}

/*
@@ -1058,8 +1043,8 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
{
struct userspace_mem_region *region;
- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
+
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
if ((gpa >= region->region.guest_phys_addr)
&& (gpa <= (region->region.guest_phys_addr
+ region->region.memory_size - 1)))
@@ -1091,8 +1076,8 @@ void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
vm_paddr_t addr_hva2gpa(struct kvm_vm *vm, void *hva)
{
struct userspace_mem_region *region;
- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
+
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
if ((hva >= region->host_mem)
&& (hva <= (region->host_mem
+ region->region.memory_size - 1)))
@@ -1519,8 +1504,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
fprintf(stream, "%*sfd: %i\n", indent, "", vm->fd);
fprintf(stream, "%*spage_size: 0x%x\n", indent, "", vm->page_size);
fprintf(stream, "%*sMem Regions:\n", indent, "");
- for (region = vm->userspace_mem_region_head; region;
- region = region->next) {
+ list_for_each_entry(region, &vm->userspace_mem_regions, list) {
fprintf(stream, "%*sguest_phys: 0x%lx size: 0x%lx "
"host_virt: %p\n", indent + 2, "",
(uint64_t) region->region.guest_phys_addr,
@@ -1539,7 +1523,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
virt_dump(stream, vm, indent + 4);
}
fprintf(stream, "%*sVCPUs:\n", indent, "");
- for (vcpu = vm->vcpu_head; vcpu; vcpu = vcpu->next)
+ list_for_each_entry(vcpu, &vm->vcpus, list)
vcpu_dump(stream, vm, vcpu->id, indent + 2);
}

diff --git a/tools/testing/selftests/kvm/lib/kvm_util_internal.h b/tools/testing/selftests/kvm/lib/kvm_util_internal.h
index ca56a0133127..2ef446520748 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util_internal.h
+++ b/tools/testing/selftests/kvm/lib/kvm_util_internal.h
@@ -13,7 +13,6 @@
#define KVM_DEV_PATH "/dev/kvm"

struct userspace_mem_region {
- struct userspace_mem_region *next, *prev;
struct kvm_userspace_memory_region region;
struct sparsebit *unused_phy_pages;
int fd;
@@ -21,10 +20,11 @@ struct userspace_mem_region {
void *host_mem;
void *mmap_start;
size_t mmap_size;
+ struct list_head list;
};

struct vcpu {
- struct vcpu *next, *prev;
+ struct list_head list;
uint32_t id;
int fd;
struct kvm_run *state;
@@ -41,8 +41,8 @@ struct kvm_vm {
unsigned int pa_bits;
unsigned int va_bits;
uint64_t max_gfn;
- struct vcpu *vcpu_head;
- struct userspace_mem_region *userspace_mem_region_head;
+ struct list_head vcpus;
+ struct list_head userspace_mem_regions;
struct sparsebit *vpages_valid;
struct sparsebit *vpages_mapped;
bool has_irqchip;
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 8d94961bd046..a88c5d665725 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -233,7 +233,10 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)

void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
{
- struct vcpu *vcpu = vm->vcpu_head;
+ struct vcpu *vcpu = vcpu_find(vm, vcpuid);
+
+ if (!vcpu)
+ return;

fprintf(stream, "%*spstate: psw: 0x%.16llx:0x%.16llx\n",
indent, "", vcpu->state->psw_mask, vcpu->state->psw_addr);
--
2.26.0

2020-04-14 16:53:06

by Andrew Jones

[permalink] [raw]
Subject: Re: [PATCH 04/10] KVM: selftests: Add GUEST_ASSERT variants to pass values to host

On Fri, Apr 10, 2020 at 04:17:01PM -0700, Sean Christopherson wrote:
> Add variants of GUEST_ASSERT to pass values back to the host, e.g. to
> help debug/understand a failure when the the cause of the assert isn't
> necessarily binary.
>
> It'd probably be possible to auto-calculate the number of arguments and
> just have a single GUEST_ASSERT, but there are a limited number of
> variants and silently eating arguments could lead to subtle code bugs.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> .../testing/selftests/kvm/include/kvm_util.h | 25 +++++++++++++++----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index d4c3e4d9cd92..e38d91bd8ec1 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -313,11 +313,26 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc);
>
> #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage)
> #define GUEST_DONE() ucall(UCALL_DONE, 0)
> -#define GUEST_ASSERT(_condition) do { \
> - if (!(_condition)) \
> - ucall(UCALL_ABORT, 2, \
> - "Failed guest assert: " \
> - #_condition, __LINE__); \
> +#define __GUEST_ASSERT(_condition, _nargs, _args...) do { \
> + if (!(_condition)) \
> + ucall(UCALL_ABORT, 2 + _nargs, \

Need () around _nargs

> + "Failed guest assert: " \
> + #_condition, __LINE__, _args); \
> } while (0)

We can free up another arg and add __FILE__. Something like the following
(untested):

#include "linux/stringify.h"

#define __GUEST_ASSERT(_condition, _nargs, _args...) do { \
if (!(_condition)) \
ucall(UCALL_ABORT, (_nargs) + 1, \
"Failed guest assert: " \
#_condition " at " __FILE__ ":" __stringify(__LINE__), _args); \
} while (0)

>
> +#define GUEST_ASSERT(_condition) \
> + __GUEST_ASSERT((_condition), 0, 0)
> +
> +#define GUEST_ASSERT_1(_condition, arg1) \
> + __GUEST_ASSERT((_condition), 1, (arg1))
> +
> +#define GUEST_ASSERT_2(_condition, arg1, arg2) \
> + __GUEST_ASSERT((_condition), 2, (arg1), (arg2))
> +
> +#define GUEST_ASSERT_3(_condition, arg1, arg2, arg3) \
> + __GUEST_ASSERT((_condition), 3, (arg1), (arg2), (arg3))
> +
> +#define GUEST_ASSERT_4(_condition, arg1, arg2, arg3, arg4) \
> + __GUEST_ASSERT((_condition), 4, (arg1), (arg2), (arg3), (arg4))
> +

nit: don't need the () around any of the macro params above

> #endif /* SELFTEST_KVM_UTIL_H */
> --
> 2.26.0
>

We could instead add test specific ucalls. To do so, we should first add
UCALL_TEST_SPECIFIC = 32 to the ucall enum, and then tests could extend it

enum {
MY_UCALL_1 = UCALL_TEST_SPECIFIC,
MY_UCALL_2,
};

With appropriately named test specific ucalls it may allow for clearer
code. At least GUEST_ASSERT_<N> wouldn't take on new meanings for each
test.

Thanks,
drew

2020-04-14 16:53:24

by Andrew Jones

[permalink] [raw]
Subject: Re: [PATCH 02/10] KVM: selftests: Use kernel's list instead of homebrewed replacement

On Fri, Apr 10, 2020 at 04:16:59PM -0700, Sean Christopherson wrote:
> Replace the KVM selftests' homebrewed linked lists for vCPUs and memory
> regions with the kernel's 'struct list_head'.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> .../testing/selftests/kvm/include/kvm_util.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 94 ++++++++-----------
> .../selftests/kvm/lib/kvm_util_internal.h | 8 +-
> .../selftests/kvm/lib/s390x/processor.c | 5 +-
> 4 files changed, 48 insertions(+), 60 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index a99b875f50d2..2f329e785c58 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -10,6 +10,7 @@
> #include "test_util.h"
>
> #include "asm/kvm.h"
> +#include "linux/list.h"
> #include "linux/kvm.h"
> #include <sys/ioctl.h>
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 9a783c20dd26..105ee9bc09f0 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -161,6 +161,9 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
> vm = calloc(1, sizeof(*vm));
> TEST_ASSERT(vm != NULL, "Insufficient Memory");
>
> + INIT_LIST_HEAD(&vm->vcpus);
> + INIT_LIST_HEAD(&vm->userspace_mem_regions);
> +
> vm->mode = mode;
> vm->type = 0;
>
> @@ -258,8 +261,7 @@ void kvm_vm_restart(struct kvm_vm *vmp, int perm)
> if (vmp->has_irqchip)
> vm_create_irqchip(vmp);
>
> - for (region = vmp->userspace_mem_region_head; region;
> - region = region->next) {
> + list_for_each_entry(region, &vmp->userspace_mem_regions, list) {
> int ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION, &region->region);
> TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed,\n"
> " rc: %i errno: %i\n"
> @@ -319,8 +321,7 @@ userspace_mem_region_find(struct kvm_vm *vm, uint64_t start, uint64_t end)
> {
> struct userspace_mem_region *region;
>
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> uint64_t existing_start = region->region.guest_phys_addr;
> uint64_t existing_end = region->region.guest_phys_addr
> + region->region.memory_size - 1;
> @@ -378,11 +379,11 @@ kvm_userspace_memory_region_find(struct kvm_vm *vm, uint64_t start,
> */
> struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> {
> - struct vcpu *vcpup;
> + struct vcpu *vcpu;
>
> - for (vcpup = vm->vcpu_head; vcpup; vcpup = vcpup->next) {
> - if (vcpup->id == vcpuid)
> - return vcpup;
> + list_for_each_entry(vcpu, &vm->vcpus, list) {
> + if (vcpu->id == vcpuid)
> + return vcpu;
> }
>
> return NULL;
> @@ -392,16 +393,15 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
> * VM VCPU Remove
> *
> * Input Args:
> - * vm - Virtual Machine
> * vcpu - VCPU to remove
> *
> * Output Args: None
> *
> * Return: None, TEST_ASSERT failures for all error conditions
> *
> - * Within the VM specified by vm, removes the VCPU given by vcpuid.
> + * Removes a vCPU from a VM and frees its resources.
> */
> -static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
> +static void vm_vcpu_rm(struct vcpu *vcpu)
> {
> int ret;
>
> @@ -412,21 +412,17 @@ static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
> TEST_ASSERT(ret == 0, "Close of VCPU fd failed, rc: %i "
> "errno: %i", ret, errno);
>
> - if (vcpu->next)
> - vcpu->next->prev = vcpu->prev;
> - if (vcpu->prev)
> - vcpu->prev->next = vcpu->next;
> - else
> - vm->vcpu_head = vcpu->next;
> + list_del(&vcpu->list);
> free(vcpu);
> }
>
> void kvm_vm_release(struct kvm_vm *vmp)
> {
> + struct vcpu *vcpu, *tmp;
> int ret;
>
> - while (vmp->vcpu_head)
> - vm_vcpu_rm(vmp, vmp->vcpu_head);
> + list_for_each_entry_safe(vcpu, tmp, &vmp->vcpus, list)
> + vm_vcpu_rm(vcpu);
>
> ret = close(vmp->fd);
> TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
> @@ -442,15 +438,15 @@ void kvm_vm_release(struct kvm_vm *vmp)
> */
> void kvm_vm_free(struct kvm_vm *vmp)
> {
> + struct userspace_mem_region *region, *tmp;
> int ret;
>
> if (vmp == NULL)
> return;
>
> /* Free userspace_mem_regions. */
> - while (vmp->userspace_mem_region_head) {
> - struct userspace_mem_region *region
> - = vmp->userspace_mem_region_head;
> + list_for_each_entry_safe(region, tmp, &vmp->userspace_mem_regions, list) {
> + list_del(&region->list);
>
> region->region.memory_size = 0;
> ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION,
> @@ -458,7 +454,6 @@ void kvm_vm_free(struct kvm_vm *vmp)
> TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed, "
> "rc: %i errno: %i", ret, errno);
>
> - vmp->userspace_mem_region_head = region->next;
> sparsebit_free(&region->unused_phy_pages);
> ret = munmap(region->mmap_start, region->mmap_size);
> TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i",
> @@ -611,12 +606,10 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> (uint64_t) region->region.memory_size);
>
> /* Confirm no region with the requested slot already exists. */
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> - if (region->region.slot == slot)
> - break;
> - }
> - if (region != NULL)
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> + if (region->region.slot != slot)
> + continue;
> +
> TEST_FAIL("A mem region with the requested slot "
> "already exists.\n"
> " requested slot: %u paddr: 0x%lx npages: 0x%lx\n"
> @@ -625,6 +618,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> region->region.slot,
> (uint64_t) region->region.guest_phys_addr,
> (uint64_t) region->region.memory_size);
> + }
>
> /* Allocate and initialize new mem region structure. */
> region = calloc(1, sizeof(*region));
> @@ -685,10 +679,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> guest_paddr, (uint64_t) region->region.memory_size);
>
> /* Add to linked-list of memory regions. */
> - if (vm->userspace_mem_region_head)
> - vm->userspace_mem_region_head->prev = region;
> - region->next = vm->userspace_mem_region_head;
> - vm->userspace_mem_region_head = region;
> + list_add(&region->list, &vm->userspace_mem_regions);
> }
>
> /*
> @@ -711,20 +702,17 @@ memslot2region(struct kvm_vm *vm, uint32_t memslot)
> {
> struct userspace_mem_region *region;
>
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> if (region->region.slot == memslot)
> - break;
> - }
> - if (region == NULL) {
> - fprintf(stderr, "No mem region with the requested slot found,\n"
> - " requested slot: %u\n", memslot);
> - fputs("---- vm dump ----\n", stderr);
> - vm_dump(stderr, vm, 2);
> - TEST_FAIL("Mem region not found");
> + return region;
> }
>
> - return region;
> + fprintf(stderr, "No mem region with the requested slot found,\n"
> + " requested slot: %u\n", memslot);
> + fputs("---- vm dump ----\n", stderr);
> + vm_dump(stderr, vm, 2);
> + TEST_FAIL("Mem region not found");
> + return NULL;
> }
>
> /*
> @@ -862,10 +850,7 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
> "vcpu id: %u errno: %i", vcpuid, errno);
>
> /* Add to linked-list of VCPUs. */
> - if (vm->vcpu_head)
> - vm->vcpu_head->prev = vcpu;
> - vcpu->next = vm->vcpu_head;
> - vm->vcpu_head = vcpu;
> + list_add(&vcpu->list, &vm->vcpus);
> }
>
> /*
> @@ -1058,8 +1043,8 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
> void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
> {
> struct userspace_mem_region *region;
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> +
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> if ((gpa >= region->region.guest_phys_addr)
> && (gpa <= (region->region.guest_phys_addr
> + region->region.memory_size - 1)))
> @@ -1091,8 +1076,8 @@ void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
> vm_paddr_t addr_hva2gpa(struct kvm_vm *vm, void *hva)
> {
> struct userspace_mem_region *region;
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> +
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> if ((hva >= region->host_mem)
> && (hva <= (region->host_mem
> + region->region.memory_size - 1)))
> @@ -1519,8 +1504,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
> fprintf(stream, "%*sfd: %i\n", indent, "", vm->fd);
> fprintf(stream, "%*spage_size: 0x%x\n", indent, "", vm->page_size);
> fprintf(stream, "%*sMem Regions:\n", indent, "");
> - for (region = vm->userspace_mem_region_head; region;
> - region = region->next) {
> + list_for_each_entry(region, &vm->userspace_mem_regions, list) {
> fprintf(stream, "%*sguest_phys: 0x%lx size: 0x%lx "
> "host_virt: %p\n", indent + 2, "",
> (uint64_t) region->region.guest_phys_addr,
> @@ -1539,7 +1523,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
> virt_dump(stream, vm, indent + 4);
> }
> fprintf(stream, "%*sVCPUs:\n", indent, "");
> - for (vcpu = vm->vcpu_head; vcpu; vcpu = vcpu->next)
> + list_for_each_entry(vcpu, &vm->vcpus, list)
> vcpu_dump(stream, vm, vcpu->id, indent + 2);
> }
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util_internal.h b/tools/testing/selftests/kvm/lib/kvm_util_internal.h
> index ca56a0133127..2ef446520748 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util_internal.h
> +++ b/tools/testing/selftests/kvm/lib/kvm_util_internal.h
> @@ -13,7 +13,6 @@
> #define KVM_DEV_PATH "/dev/kvm"
>
> struct userspace_mem_region {
> - struct userspace_mem_region *next, *prev;
> struct kvm_userspace_memory_region region;
> struct sparsebit *unused_phy_pages;
> int fd;
> @@ -21,10 +20,11 @@ struct userspace_mem_region {
> void *host_mem;
> void *mmap_start;
> size_t mmap_size;
> + struct list_head list;
> };
>
> struct vcpu {
> - struct vcpu *next, *prev;
> + struct list_head list;
> uint32_t id;
> int fd;
> struct kvm_run *state;
> @@ -41,8 +41,8 @@ struct kvm_vm {
> unsigned int pa_bits;
> unsigned int va_bits;
> uint64_t max_gfn;
> - struct vcpu *vcpu_head;
> - struct userspace_mem_region *userspace_mem_region_head;
> + struct list_head vcpus;
> + struct list_head userspace_mem_regions;
> struct sparsebit *vpages_valid;
> struct sparsebit *vpages_mapped;
> bool has_irqchip;
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 8d94961bd046..a88c5d665725 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -233,7 +233,10 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>
> void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
> {
> - struct vcpu *vcpu = vm->vcpu_head;
> + struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> +
> + if (!vcpu)
> + return;
>
> fprintf(stream, "%*spstate: psw: 0x%.16llx:0x%.16llx\n",
> indent, "", vcpu->state->psw_mask, vcpu->state->psw_addr);
> --
> 2.26.0
>

Reviewed-by: Andrew Jones <[email protected]>

2020-04-16 00:16:48

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 00/10] KVM: selftests: Add KVM_SET_MEMORY_REGION tests

On 11/04/20 01:16, Sean Christopherson wrote:
> This is v2-ish of my series to add a "delete" testcase[1], and v5.1 of
> Wainer's series to add a "max" testcase[2].
>
> I've only tested on x86_64. I fudged compile testing on !x86_64 by
> inverting the ifdefs, e.g. to squish unused var warnings, but by no means
> is the code actually tested on other architectures.
>
> I kept Andrew's review for the "max" test. Other than the 1MB->2MB
> change (see below), it was basically a straight copy-paste of code.
>
> v1->v2 of delete:
> - Drop patch to expose primary memslot. [Peter]
> - Add explicit synchronization to MOVE and DELETE tests. [Peter]
> - Use list.h instead of open coding linked lists. [Peter]
> - Clean up the code and separate the testcases into separate functions.
> - Expand GUEST_ASSERT() to allow passing a value back to the host for
> printing.
> - Move to common KVM, with ifdefs to hide the x86_64-only stuff (which
> is a lot at this point).
> - Do KVM_SET_NR_MMU_PAGES in the "zero" testcase to get less obscure
> behavior for KVM_RUN. [Christian]
>
> v5.1 of max:
> - Fix a whitespace issue in vm_get_fd(). [checkpatch]
> - Move the code to set_memory_region_test. The only _intended_
> functional change is to create 2MB regions instead of 1MB regions.
> The only motivation for doing so was to reuse an existing define in
> set_memory_region_test.
>
> [1] https://lkml.kernel.org/r/[email protected]
> [2] https://lkml.kernel.org/r/[email protected]
>
> Sean Christopherson (8):
> KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()
> KVM: selftests: Use kernel's list instead of homebrewed replacement
> KVM: selftests: Add util to delete memory region
> KVM: selftests: Add GUEST_ASSERT variants to pass values to host
> KVM: sefltests: Add explicit synchronization to move mem region test
> KVM: selftests: Add "delete" testcase to set_memory_region_test
> KVM: selftests: Add "zero" testcase to set_memory_region_test
> KVM: selftests: Make set_memory_region_test common to all
> architectures
>
> Wainer dos Santos Moschetta (2):
> selftests: kvm: Add vm_get_fd() in kvm_util
> selftests: kvm: Add testcase for creating max number of memslots
>
> tools/testing/selftests/kvm/.gitignore | 2 +-
> tools/testing/selftests/kvm/Makefile | 4 +-
> .../testing/selftests/kvm/include/kvm_util.h | 28 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 154 +++----
> .../selftests/kvm/lib/kvm_util_internal.h | 8 +-
> .../selftests/kvm/lib/s390x/processor.c | 5 +-
> .../selftests/kvm/set_memory_region_test.c | 403 ++++++++++++++++++
> .../kvm/x86_64/set_memory_region_test.c | 141 ------
> 8 files changed, 520 insertions(+), 225 deletions(-)
> create mode 100644 tools/testing/selftests/kvm/set_memory_region_test.c
> delete mode 100644 tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
>

Queued, thanks -- while keeping the zero region test x86-only.

Paolo