[PATCH 0/6] KVM: selftests: memslot_perf_test: aarch64 cleanup/fixes

[PATCH 0/6] KVM: selftests: memslot_perf_test: aarch64 cleanup/fixes

[Gavin Shan]

kvm/selftests/memslots_perf_test doesn't work with 64KB-page-size-host
and 4KB-page-size-guest on aarch64. In the implementation, the host and
guest page size have been hardcoded to 4KB. It's ovbiously not working
on aarch64 which supports 4KB, 16KB, 64KB individually on host and guest.

This series tries to fix it. After the series is applied, the test runs
successfully with 64KB-page-size-host and 4KB-page-size-guest.

   # ./memslots_perf_tests -v -s 512

Since we're here, the code is cleaned up a bit as PATCH[1-3] do. The
other patches are fixes to handle the mismatched host/guest page
sized.

Gavin Shan (6):
  KVM: selftests: memslot_perf_test: Use data->nslots in prepare_vm()
  KVM: selftests: memslot_perf_test: Consolidate loop conditions in
    prepare_vm()
  KVM: selftests: memslot_perf_test: Probe memory slots for once
  KVM: selftests: memslot_perf_test: Support variable guest page size
  KVM: selftests: memslot_perf_test: Consolidate memory sizes
  KVM: selftests: memslot_perf_test: Report optimal memory slots

 .../testing/selftests/kvm/memslot_perf_test.c | 286 +++++++++++-------
 1 file changed, 183 insertions(+), 103 deletions(-)

-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/6] KVM: selftests: memslot_perf_test: aarch64 cleanup/fixes

[Gavin Shan]

kvm/selftests/memslots_perf_test doesn't work with 64KB-page-size-host
and 4KB-page-size-guest on aarch64. In the implementation, the host and
guest page size have been hardcoded to 4KB. It's ovbiously not working
on aarch64 which supports 4KB, 16KB, 64KB individually on host and guest.

This series tries to fix it. After the series is applied, the test runs
successfully with 64KB-page-size-host and 4KB-page-size-guest.

   # ./memslots_perf_tests -v -s 512

Since we're here, the code is cleaned up a bit as PATCH[1-3] do. The
other patches are fixes to handle the mismatched host/guest page
sized.

Gavin Shan (6):
  KVM: selftests: memslot_perf_test: Use data->nslots in prepare_vm()
  KVM: selftests: memslot_perf_test: Consolidate loop conditions in
    prepare_vm()
  KVM: selftests: memslot_perf_test: Probe memory slots for once
  KVM: selftests: memslot_perf_test: Support variable guest page size
  KVM: selftests: memslot_perf_test: Consolidate memory sizes
  KVM: selftests: memslot_perf_test: Report optimal memory slots

 .../testing/selftests/kvm/memslot_perf_test.c | 286 +++++++++++-------
 1 file changed, 183 insertions(+), 103 deletions(-)

-- 
2.23.0

[PATCH 1/6] KVM: selftests: memslot_perf_test: Use data->nslots in prepare_vm()

[Gavin Shan]

In prepare_vm(), 'data->nslots' is assigned with 'max_mem_slots - 1'
at the beginning, meaning they are interchangeable.

Use 'data->nslots' isntead of 'max_mem_slots - 1'. With this, it
becomes easier to move the logic of probing number of slots into
upper layer in subsequent patches.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/memslot_perf_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 44995446d942..231cc8449c2e 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -280,14 +280,14 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	ucall_init(data->vm, NULL);
 
 	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
-		max_mem_slots - 1, data->pages_per_slot, rempages);
+		data->nslots, data->pages_per_slot, rempages);
 
 	clock_gettime(CLOCK_MONOTONIC, &tstart);
-	for (slot = 1, guest_addr = MEM_GPA; slot < max_mem_slots; slot++) {
+	for (slot = 1, guest_addr = MEM_GPA; slot <= data->nslots; slot++) {
 		uint64_t npages;
 
 		npages = data->pages_per_slot;
-		if (slot == max_mem_slots - 1)
+		if (slot == data->nslots)
 			npages += rempages;
 
 		vm_userspace_mem_region_add(data->vm, VM_MEM_SRC_ANONYMOUS,
@@ -297,12 +297,12 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
-	for (slot = 0, guest_addr = MEM_GPA; slot < max_mem_slots - 1; slot++) {
+	for (slot = 0, guest_addr = MEM_GPA; slot < data->nslots; slot++) {
 		uint64_t npages;
 		uint64_t gpa;
 
 		npages = data->pages_per_slot;
-		if (slot == max_mem_slots - 2)
+		if (slot == data->nslots - 1)
 			npages += rempages;
 
 		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr,
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/6] KVM: selftests: memslot_perf_test: Use data->nslots in prepare_vm()

[Gavin Shan]

In prepare_vm(), 'data->nslots' is assigned with 'max_mem_slots - 1'
at the beginning, meaning they are interchangeable.

Use 'data->nslots' isntead of 'max_mem_slots - 1'. With this, it
becomes easier to move the logic of probing number of slots into
upper layer in subsequent patches.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/memslot_perf_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 44995446d942..231cc8449c2e 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -280,14 +280,14 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	ucall_init(data->vm, NULL);
 
 	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
-		max_mem_slots - 1, data->pages_per_slot, rempages);
+		data->nslots, data->pages_per_slot, rempages);
 
 	clock_gettime(CLOCK_MONOTONIC, &tstart);
-	for (slot = 1, guest_addr = MEM_GPA; slot < max_mem_slots; slot++) {
+	for (slot = 1, guest_addr = MEM_GPA; slot <= data->nslots; slot++) {
 		uint64_t npages;
 
 		npages = data->pages_per_slot;
-		if (slot == max_mem_slots - 1)
+		if (slot == data->nslots)
 			npages += rempages;
 
 		vm_userspace_mem_region_add(data->vm, VM_MEM_SRC_ANONYMOUS,
@@ -297,12 +297,12 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
-	for (slot = 0, guest_addr = MEM_GPA; slot < max_mem_slots - 1; slot++) {
+	for (slot = 0, guest_addr = MEM_GPA; slot < data->nslots; slot++) {
 		uint64_t npages;
 		uint64_t gpa;
 
 		npages = data->pages_per_slot;
-		if (slot == max_mem_slots - 2)
+		if (slot == data->nslots - 1)
 			npages += rempages;
 
 		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr,
-- 
2.23.0

[PATCH 2/6] KVM: selftests: memslot_perf_test: Consolidate loop conditions in prepare_vm()

[Gavin Shan]

There are two loops in prepare_vm(), which have different conditions.
'slot' is treated as meory slot index in the first loop, but index of
the host virtual address array in the second loop. It makes it a bit
hard to understand the code.

Change the usage of 'slot' in the second loop, to treat it as the
memory slot index either.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/memslot_perf_test.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 231cc8449c2e..dcb492b3f27b 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -297,21 +297,20 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
-	for (slot = 0, guest_addr = MEM_GPA; slot < data->nslots; slot++) {
+	for (slot = 1, guest_addr = MEM_GPA; slot <= data->nslots; slot++) {
 		uint64_t npages;
 		uint64_t gpa;
 
 		npages = data->pages_per_slot;
-		if (slot == data->nslots - 1)
+		if (slot == data->nslots)
 			npages += rempages;
 
-		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr,
-					 slot + 1);
+		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr, slot);
 		TEST_ASSERT(gpa == guest_addr,
 			    "vm_phy_pages_alloc() failed\n");
 
-		data->hva_slots[slot] = addr_gpa2hva(data->vm, guest_addr);
-		memset(data->hva_slots[slot], 0, npages * 4096);
+		data->hva_slots[slot - 1] = addr_gpa2hva(data->vm, guest_addr);
+		memset(data->hva_slots[slot - 1], 0, npages * 4096);
 
 		guest_addr += npages * 4096;
 	}
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/6] KVM: selftests: memslot_perf_test: Consolidate loop conditions in prepare_vm()

[Gavin Shan]

There are two loops in prepare_vm(), which have different conditions.
'slot' is treated as meory slot index in the first loop, but index of
the host virtual address array in the second loop. It makes it a bit
hard to understand the code.

Change the usage of 'slot' in the second loop, to treat it as the
memory slot index either.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/memslot_perf_test.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 231cc8449c2e..dcb492b3f27b 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -297,21 +297,20 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
-	for (slot = 0, guest_addr = MEM_GPA; slot < data->nslots; slot++) {
+	for (slot = 1, guest_addr = MEM_GPA; slot <= data->nslots; slot++) {
 		uint64_t npages;
 		uint64_t gpa;
 
 		npages = data->pages_per_slot;
-		if (slot == data->nslots - 1)
+		if (slot == data->nslots)
 			npages += rempages;
 
-		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr,
-					 slot + 1);
+		gpa = vm_phy_pages_alloc(data->vm, npages, guest_addr, slot);
 		TEST_ASSERT(gpa == guest_addr,
 			    "vm_phy_pages_alloc() failed\n");
 
-		data->hva_slots[slot] = addr_gpa2hva(data->vm, guest_addr);
-		memset(data->hva_slots[slot], 0, npages * 4096);
+		data->hva_slots[slot - 1] = addr_gpa2hva(data->vm, guest_addr);
+		memset(data->hva_slots[slot - 1], 0, npages * 4096);
 
 		guest_addr += npages * 4096;
 	}
-- 
2.23.0

[PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Gavin Shan]

prepare_vm() is called in every iteration and run. The allowed memory
slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
free and unnecessary.

Move the probing logic for the allowed memory slots to parse_args()
for once, which is upper layer of prepare_vm().

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index dcb492b3f27b..d5aa9148f96f 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		       void *guest_code, uint64_t mempages,
 		       struct timespec *slot_runtime)
 {
-	uint32_t max_mem_slots;
 	uint64_t rempages;
 	uint64_t guest_addr;
 	uint32_t slot;
 	struct timespec tstart;
 	struct sync_area *sync;
 
-	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
-	TEST_ASSERT(max_mem_slots > 1,
-		    "KVM_CAP_NR_MEMSLOTS should be greater than 1");
-	TEST_ASSERT(nslots > 1 || nslots == -1,
-		    "Slot count cap should be greater than 1");
-	if (nslots != -1)
-		max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
-	pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
-
 	TEST_ASSERT(mempages > 1,
 		    "Can't test without any memory");
 
 	data->npages = mempages;
-	data->nslots = max_mem_slots - 1;
+	data->nslots = nslots;
 	data->pages_per_slot = mempages / data->nslots;
 	if (!data->pages_per_slot) {
 		*maxslots = mempages + 1;
@@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
 			break;
 		case 's':
 			targs->nslots = atoi(optarg);
-			if (targs->nslots <= 0 && targs->nslots != -1) {
-				pr_info("Slot count cap has to be positive or -1 for no cap\n");
+			if (targs->nslots <= 1 && targs->nslots != -1) {
+				pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
 				return false;
 			}
 			break;
@@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
 		return false;
 	}
 
+	/* Memory slot 0 is reserved */
+	if (targs->nslots == -1) {
+		targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
+		if (targs->nslots < 1) {
+			pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
+			return false;
+		}
+	} else {
+		targs->nslots--;
+	}
+
+	pr_info_v("Number of memory slots: %d\n", targs->nslots);
+
 	return true;
 }
 
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Gavin Shan]

prepare_vm() is called in every iteration and run. The allowed memory
slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
free and unnecessary.

Move the probing logic for the allowed memory slots to parse_args()
for once, which is upper layer of prepare_vm().

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index dcb492b3f27b..d5aa9148f96f 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		       void *guest_code, uint64_t mempages,
 		       struct timespec *slot_runtime)
 {
-	uint32_t max_mem_slots;
 	uint64_t rempages;
 	uint64_t guest_addr;
 	uint32_t slot;
 	struct timespec tstart;
 	struct sync_area *sync;
 
-	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
-	TEST_ASSERT(max_mem_slots > 1,
-		    "KVM_CAP_NR_MEMSLOTS should be greater than 1");
-	TEST_ASSERT(nslots > 1 || nslots == -1,
-		    "Slot count cap should be greater than 1");
-	if (nslots != -1)
-		max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
-	pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
-
 	TEST_ASSERT(mempages > 1,
 		    "Can't test without any memory");
 
 	data->npages = mempages;
-	data->nslots = max_mem_slots - 1;
+	data->nslots = nslots;
 	data->pages_per_slot = mempages / data->nslots;
 	if (!data->pages_per_slot) {
 		*maxslots = mempages + 1;
@@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
 			break;
 		case 's':
 			targs->nslots = atoi(optarg);
-			if (targs->nslots <= 0 && targs->nslots != -1) {
-				pr_info("Slot count cap has to be positive or -1 for no cap\n");
+			if (targs->nslots <= 1 && targs->nslots != -1) {
+				pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
 				return false;
 			}
 			break;
@@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
 		return false;
 	}
 
+	/* Memory slot 0 is reserved */
+	if (targs->nslots == -1) {
+		targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
+		if (targs->nslots < 1) {
+			pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
+			return false;
+		}
+	} else {
+		targs->nslots--;
+	}
+
+	pr_info_v("Number of memory slots: %d\n", targs->nslots);
+
 	return true;
 }
 
-- 
2.23.0

Re: [PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> prepare_vm() is called in every iteration and run. The allowed memory
> slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
> free and unnecessary.
> 
> Move the probing logic for the allowed memory slots to parse_args()
> for once, which is upper layer of prepare_vm().
> 
> No functional change intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
>   1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index dcb492b3f27b..d5aa9148f96f 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>   		       void *guest_code, uint64_t mempages,
>   		       struct timespec *slot_runtime)
>   {
> -	uint32_t max_mem_slots;
>   	uint64_t rempages;
>   	uint64_t guest_addr;
>   	uint32_t slot;
>   	struct timespec tstart;
>   	struct sync_area *sync;
>   
> -	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
> -	TEST_ASSERT(max_mem_slots > 1,
> -		    "KVM_CAP_NR_MEMSLOTS should be greater than 1");
> -	TEST_ASSERT(nslots > 1 || nslots == -1,
> -		    "Slot count cap should be greater than 1");
> -	if (nslots != -1)
> -		max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
> -	pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
> -
>   	TEST_ASSERT(mempages > 1,
>   		    "Can't test without any memory");
>   
>   	data->npages = mempages;
> -	data->nslots = max_mem_slots - 1;
> +	data->nslots = nslots;
>   	data->pages_per_slot = mempages / data->nslots;
>   	if (!data->pages_per_slot) {
>   		*maxslots = mempages + 1;
> @@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
>   			break;
>   		case 's':
>   			targs->nslots = atoi(optarg);
> -			if (targs->nslots <= 0 && targs->nslots != -1) {
> -				pr_info("Slot count cap has to be positive or -1 for no cap\n");
> +			if (targs->nslots <= 1 && targs->nslots != -1) {
> +				pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
>   				return false;
>   			}
>   			break;
> @@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
>   		return false;
>   	}
>   
> +	/* Memory slot 0 is reserved */
> +	if (targs->nslots == -1) {
> +		targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
> +		if (targs->nslots < 1) {
> +			pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
> +			return false;
> +		}
> +	} else {
> +		targs->nslots--;
> +	}
> +
> +	pr_info_v("Number of memory slots: %d\n", targs->nslots);
> +

Can't see any capping of the command line provided slot count to
KVM_CAP_NR_MEMSLOTS value, like the old code did.

>   	return true;
>   }
>   

Thanks,
Maciej

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> prepare_vm() is called in every iteration and run. The allowed memory
> slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
> free and unnecessary.
> 
> Move the probing logic for the allowed memory slots to parse_args()
> for once, which is upper layer of prepare_vm().
> 
> No functional change intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
>   1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index dcb492b3f27b..d5aa9148f96f 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>   		       void *guest_code, uint64_t mempages,
>   		       struct timespec *slot_runtime)
>   {
> -	uint32_t max_mem_slots;
>   	uint64_t rempages;
>   	uint64_t guest_addr;
>   	uint32_t slot;
>   	struct timespec tstart;
>   	struct sync_area *sync;
>   
> -	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
> -	TEST_ASSERT(max_mem_slots > 1,
> -		    "KVM_CAP_NR_MEMSLOTS should be greater than 1");
> -	TEST_ASSERT(nslots > 1 || nslots == -1,
> -		    "Slot count cap should be greater than 1");
> -	if (nslots != -1)
> -		max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
> -	pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
> -
>   	TEST_ASSERT(mempages > 1,
>   		    "Can't test without any memory");
>   
>   	data->npages = mempages;
> -	data->nslots = max_mem_slots - 1;
> +	data->nslots = nslots;
>   	data->pages_per_slot = mempages / data->nslots;
>   	if (!data->pages_per_slot) {
>   		*maxslots = mempages + 1;
> @@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
>   			break;
>   		case 's':
>   			targs->nslots = atoi(optarg);
> -			if (targs->nslots <= 0 && targs->nslots != -1) {
> -				pr_info("Slot count cap has to be positive or -1 for no cap\n");
> +			if (targs->nslots <= 1 && targs->nslots != -1) {
> +				pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
>   				return false;
>   			}
>   			break;
> @@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
>   		return false;
>   	}
>   
> +	/* Memory slot 0 is reserved */
> +	if (targs->nslots == -1) {
> +		targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
> +		if (targs->nslots < 1) {
> +			pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
> +			return false;
> +		}
> +	} else {
> +		targs->nslots--;
> +	}
> +
> +	pr_info_v("Number of memory slots: %d\n", targs->nslots);
> +

Can't see any capping of the command line provided slot count to
KVM_CAP_NR_MEMSLOTS value, like the old code did.

>   	return true;
>   }
>   

Thanks,
Maciej

Re: [PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Gavin Shan]

On 10/18/22 1:34 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> prepare_vm() is called in every iteration and run. The allowed memory
>> slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
>> free and unnecessary.
>>
>> Move the probing logic for the allowed memory slots to parse_args()
>> for once, which is upper layer of prepare_vm().
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
>>   1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index dcb492b3f27b..d5aa9148f96f 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>>                  void *guest_code, uint64_t mempages,
>>                  struct timespec *slot_runtime)
>>   {
>> -    uint32_t max_mem_slots;
>>       uint64_t rempages;
>>       uint64_t guest_addr;
>>       uint32_t slot;
>>       struct timespec tstart;
>>       struct sync_area *sync;
>> -    max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
>> -    TEST_ASSERT(max_mem_slots > 1,
>> -            "KVM_CAP_NR_MEMSLOTS should be greater than 1");
>> -    TEST_ASSERT(nslots > 1 || nslots == -1,
>> -            "Slot count cap should be greater than 1");
>> -    if (nslots != -1)
>> -        max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
>> -    pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
>> -
>>       TEST_ASSERT(mempages > 1,
>>               "Can't test without any memory");
>>       data->npages = mempages;
>> -    data->nslots = max_mem_slots - 1;
>> +    data->nslots = nslots;
>>       data->pages_per_slot = mempages / data->nslots;
>>       if (!data->pages_per_slot) {
>>           *maxslots = mempages + 1;
>> @@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
>>               break;
>>           case 's':
>>               targs->nslots = atoi(optarg);
>> -            if (targs->nslots <= 0 && targs->nslots != -1) {
>> -                pr_info("Slot count cap has to be positive or -1 for no cap\n");
>> +            if (targs->nslots <= 1 && targs->nslots != -1) {
>> +                pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
>>                   return false;
>>               }
>>               break;
>> @@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
>>           return false;
>>       }
>> +    /* Memory slot 0 is reserved */
>> +    if (targs->nslots == -1) {
>> +        targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
>> +        if (targs->nslots < 1) {
>> +            pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
>> +            return false;
>> +        }
>> +    } else {
>> +        targs->nslots--;
>> +    }
>> +
>> +    pr_info_v("Number of memory slots: %d\n", targs->nslots);
>> +
> 
> Can't see any capping of the command line provided slot count to
> KVM_CAP_NR_MEMSLOTS value, like the old code did.
> 

Indeed. I wanted to avoid extra variable @max_mem_slots and the
capping is missed. I will fix it up in next revision.

>>       return true;
>>   }

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/6] KVM: selftests: memslot_perf_test: Probe memory slots for once

[Gavin Shan]

On 10/18/22 1:34 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> prepare_vm() is called in every iteration and run. The allowed memory
>> slots (KVM_CAP_NR_MEMSLOTS) are probed for multiple times. It's not
>> free and unnecessary.
>>
>> Move the probing logic for the allowed memory slots to parse_args()
>> for once, which is upper layer of prepare_vm().
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c | 29 ++++++++++---------
>>   1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index dcb492b3f27b..d5aa9148f96f 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -245,27 +245,17 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>>                  void *guest_code, uint64_t mempages,
>>                  struct timespec *slot_runtime)
>>   {
>> -    uint32_t max_mem_slots;
>>       uint64_t rempages;
>>       uint64_t guest_addr;
>>       uint32_t slot;
>>       struct timespec tstart;
>>       struct sync_area *sync;
>> -    max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
>> -    TEST_ASSERT(max_mem_slots > 1,
>> -            "KVM_CAP_NR_MEMSLOTS should be greater than 1");
>> -    TEST_ASSERT(nslots > 1 || nslots == -1,
>> -            "Slot count cap should be greater than 1");
>> -    if (nslots != -1)
>> -        max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
>> -    pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
>> -
>>       TEST_ASSERT(mempages > 1,
>>               "Can't test without any memory");
>>       data->npages = mempages;
>> -    data->nslots = max_mem_slots - 1;
>> +    data->nslots = nslots;
>>       data->pages_per_slot = mempages / data->nslots;
>>       if (!data->pages_per_slot) {
>>           *maxslots = mempages + 1;
>> @@ -885,8 +875,8 @@ static bool parse_args(int argc, char *argv[],
>>               break;
>>           case 's':
>>               targs->nslots = atoi(optarg);
>> -            if (targs->nslots <= 0 && targs->nslots != -1) {
>> -                pr_info("Slot count cap has to be positive or -1 for no cap\n");
>> +            if (targs->nslots <= 1 && targs->nslots != -1) {
>> +                pr_info("Slot count cap must be larger than 1 or -1 for no cap\n");
>>                   return false;
>>               }
>>               break;
>> @@ -932,6 +922,19 @@ static bool parse_args(int argc, char *argv[],
>>           return false;
>>       }
>> +    /* Memory slot 0 is reserved */
>> +    if (targs->nslots == -1) {
>> +        targs->nslots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS) - 1;
>> +        if (targs->nslots < 1) {
>> +            pr_info("KVM_CAP_NR_MEMSLOTS should be greater than 1\n");
>> +            return false;
>> +        }
>> +    } else {
>> +        targs->nslots--;
>> +    }
>> +
>> +    pr_info_v("Number of memory slots: %d\n", targs->nslots);
>> +
> 
> Can't see any capping of the command line provided slot count to
> KVM_CAP_NR_MEMSLOTS value, like the old code did.
> 

Indeed. I wanted to avoid extra variable @max_mem_slots and the
capping is missed. I will fix it up in next revision.

>>       return true;
>>   }

Thanks,
Gavin

[PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

The test case is obviously broken on aarch64 because non-4KB guest
page size is supported. The guest page size on aarch64 could be 4KB,
16KB or 64KB.

This supports variable guest page size, mostly for aarch64.

  - The host determines the guest page size when virtual machine is
    created. The value is also passed to guest through the synchronization
    area.

  - The number of guest pages are unknown until the virtual machine
    is to be created. So all the related macros are dropped. Instead,
    their values are dynamically calculated based on the guest page
    size.

  - The static checks on memory sizes and pages becomes dependent
    on guest page size, which is unknown until the virtual machine
    is about to be created. So all the static checks are converted
    to dynamic checks, done in check_memory_sizes().

  - As the address passed to madvise() should be aligned to host page,
    the size of page chunk is automatically selected, other than one
    page.

  - All other changes included in this patch are almost mechanical
    replacing '4096' with 'guest_page_size'.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
 1 file changed, 115 insertions(+), 76 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index d5aa9148f96f..d587bd952ff9 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -26,14 +26,11 @@
 #include <processor.h>
 
 #define MEM_SIZE		((512U << 20) + 4096)
-#define MEM_SIZE_PAGES		(MEM_SIZE / 4096)
 #define MEM_GPA		0x10000000UL
 #define MEM_AUX_GPA		MEM_GPA
 #define MEM_SYNC_GPA		MEM_AUX_GPA
 #define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
 #define MEM_TEST_SIZE		(MEM_SIZE - 4096)
-static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
-static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
 
 /*
  * 32 MiB is max size that gets well over 100 iterations on 509 slots.
@@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
  * limited resolution).
  */
 #define MEM_SIZE_MAP		((32U << 20) + 4096)
-#define MEM_SIZE_MAP_PAGES	(MEM_SIZE_MAP / 4096)
 #define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
-#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
-static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
 
 /*
  * 128 MiB is min size that fills 32k slots with at least one page in each
  * while at the same time gets 100+ iterations in such test
+ *
+ * 2 MiB chunk size like a typical huge page
  */
 #define MEM_TEST_UNMAP_SIZE		(128U << 20)
-#define MEM_TEST_UNMAP_SIZE_PAGES	(MEM_TEST_UNMAP_SIZE / 4096)
-/* 2 MiB chunk size like a typical huge page */
-#define MEM_TEST_UNMAP_CHUNK_PAGES	(2U << (20 - 12))
-static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
-	      "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
-	      "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
-	      (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
-	      "invalid unmap test region size");
+#define MEM_TEST_UNMAP_CHUNK_SIZE	(2U << 20)
 
 /*
  * For the move active test the middle of the test area is placed on
@@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
  * for the total size of 25 pages.
  * Hence, the maximum size here is 50 pages.
  */
-#define MEM_TEST_MOVE_SIZE_PAGES	(50)
-#define MEM_TEST_MOVE_SIZE		(MEM_TEST_MOVE_SIZE_PAGES * 4096)
+#define MEM_TEST_MOVE_SIZE		0x32000
 #define MEM_TEST_MOVE_GPA_DEST		(MEM_GPA + MEM_SIZE)
 static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
 	      "invalid move test region size");
@@ -100,6 +83,7 @@ struct vm_data {
 };
 
 struct sync_area {
+	uint32_t    guest_page_size;
 	atomic_bool start_flag;
 	atomic_bool exit_flag;
 	atomic_bool sync_flag;
@@ -192,14 +176,15 @@ static void *vm_gpa2hva(struct vm_data *data, uint64_t gpa, uint64_t *rempages)
 	uint64_t gpage, pgoffs;
 	uint32_t slot, slotoffs;
 	void *base;
+	uint32_t guest_page_size = data->vm->page_size;
 
 	TEST_ASSERT(gpa >= MEM_GPA, "Too low gpa to translate");
-	TEST_ASSERT(gpa < MEM_GPA + data->npages * 4096,
+	TEST_ASSERT(gpa < MEM_GPA + data->npages * guest_page_size,
 		    "Too high gpa to translate");
 	gpa -= MEM_GPA;
 
-	gpage = gpa / 4096;
-	pgoffs = gpa % 4096;
+	gpage = gpa / guest_page_size;
+	pgoffs = gpa % guest_page_size;
 	slot = min(gpage / data->pages_per_slot, (uint64_t)data->nslots - 1);
 	slotoffs = gpage - (slot * data->pages_per_slot);
 
@@ -217,14 +202,16 @@ static void *vm_gpa2hva(struct vm_data *data, uint64_t gpa, uint64_t *rempages)
 	}
 
 	base = data->hva_slots[slot];
-	return (uint8_t *)base + slotoffs * 4096 + pgoffs;
+	return (uint8_t *)base + slotoffs * guest_page_size + pgoffs;
 }
 
 static uint64_t vm_slot2gpa(struct vm_data *data, uint32_t slot)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+
 	TEST_ASSERT(slot < data->nslots, "Too high slot number");
 
-	return MEM_GPA + slot * data->pages_per_slot * 4096;
+	return MEM_GPA + slot * data->pages_per_slot * guest_page_size;
 }
 
 static struct vm_data *alloc_vm(void)
@@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
 }
 
 static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
-		       void *guest_code, uint64_t mempages,
+		       void *guest_code, uint64_t mem_size,
 		       struct timespec *slot_runtime)
 {
-	uint64_t rempages;
+	uint64_t mempages, rempages;
 	uint64_t guest_addr;
-	uint32_t slot;
+	uint32_t slot, guest_page_size;
 	struct timespec tstart;
 	struct sync_area *sync;
 
-	TEST_ASSERT(mempages > 1,
-		    "Can't test without any memory");
+	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+	mempages = mem_size / guest_page_size;
+
+	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
+	ucall_init(data->vm, NULL);
 
 	data->npages = mempages;
+	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
 	data->nslots = nslots;
-	data->pages_per_slot = mempages / data->nslots;
+	data->pages_per_slot = data->npages / data->nslots;
 	if (!data->pages_per_slot) {
-		*maxslots = mempages + 1;
+		*maxslots = data->npages + 1;
 		return false;
 	}
 
-	rempages = mempages % data->nslots;
+	rempages = data->npages % data->nslots;
 	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
 	TEST_ASSERT(data->hva_slots, "malloc() fail");
 
-	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
-	ucall_init(data->vm, NULL);
-
 	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
 		data->nslots, data->pages_per_slot, rempages);
 
@@ -283,7 +271,7 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		vm_userspace_mem_region_add(data->vm, VM_MEM_SRC_ANONYMOUS,
 					    guest_addr, slot, npages,
 					    0);
-		guest_addr += npages * 4096;
+		guest_addr += npages * guest_page_size;
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
@@ -300,12 +288,12 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 			    "vm_phy_pages_alloc() failed\n");
 
 		data->hva_slots[slot - 1] = addr_gpa2hva(data->vm, guest_addr);
-		memset(data->hva_slots[slot - 1], 0, npages * 4096);
+		memset(data->hva_slots[slot - 1], 0, npages * guest_page_size);
 
-		guest_addr += npages * 4096;
+		guest_addr += npages * guest_page_size;
 	}
 
-	virt_map(data->vm, MEM_GPA, MEM_GPA, mempages);
+	virt_map(data->vm, MEM_GPA, MEM_GPA, data->npages);
 
 	sync = (typeof(sync))vm_gpa2hva(data, MEM_SYNC_GPA, NULL);
 	atomic_init(&sync->start_flag, false);
@@ -404,6 +392,7 @@ static bool guest_perform_sync(void)
 static void guest_code_test_memslot_move(void)
 {
 	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
 	uintptr_t base = (typeof(base))READ_ONCE(sync->move_area_ptr);
 
 	GUEST_SYNC(0);
@@ -414,7 +403,7 @@ static void guest_code_test_memslot_move(void)
 		uintptr_t ptr;
 
 		for (ptr = base; ptr < base + MEM_TEST_MOVE_SIZE;
-		     ptr += 4096)
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		/*
@@ -432,6 +421,7 @@ static void guest_code_test_memslot_move(void)
 static void guest_code_test_memslot_map(void)
 {
 	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
 
 	GUEST_SYNC(0);
 
@@ -441,14 +431,16 @@ static void guest_code_test_memslot_map(void)
 		uintptr_t ptr;
 
 		for (ptr = MEM_TEST_GPA;
-		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2;
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		if (!guest_perform_sync())
 			break;
 
 		for (ptr = MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2;
-		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE;
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_2;
 
 		if (!guest_perform_sync())
@@ -495,6 +487,9 @@ static void guest_code_test_memslot_unmap(void)
 
 static void guest_code_test_memslot_rw(void)
 {
+	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
+
 	GUEST_SYNC(0);
 
 	guest_spin_until_start();
@@ -503,14 +498,14 @@ static void guest_code_test_memslot_rw(void)
 		uintptr_t ptr;
 
 		for (ptr = MEM_TEST_GPA;
-		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		if (!guest_perform_sync())
 			break;
 
-		for (ptr = MEM_TEST_GPA + 4096 / 2;
-		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += 4096) {
+		for (ptr = MEM_TEST_GPA + page_size / 2;
+		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += page_size) {
 			uint64_t val = *(uint64_t *)ptr;
 
 			GUEST_ASSERT_1(val == MEM_TEST_VAL_2, val);
@@ -528,6 +523,8 @@ static bool test_memslot_move_prepare(struct vm_data *data,
 				      struct sync_area *sync,
 				      uint64_t *maxslots, bool isactive)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t move_pages = MEM_TEST_MOVE_SIZE / guest_page_size;
 	uint64_t movesrcgpa, movetestgpa;
 
 	movesrcgpa = vm_slot2gpa(data, data->nslots - 1);
@@ -536,7 +533,7 @@ static bool test_memslot_move_prepare(struct vm_data *data,
 		uint64_t lastpages;
 
 		vm_gpa2hva(data, movesrcgpa, &lastpages);
-		if (lastpages < MEM_TEST_MOVE_SIZE_PAGES / 2) {
+		if (lastpages < move_pages / 2) {
 			*maxslots = 0;
 			return false;
 		}
@@ -582,8 +579,9 @@ static void test_memslot_do_unmap(struct vm_data *data,
 				  uint64_t offsp, uint64_t count)
 {
 	uint64_t gpa, ctr;
+	uint32_t guest_page_size = data->vm->page_size;
 
-	for (gpa = MEM_TEST_GPA + offsp * 4096, ctr = 0; ctr < count; ) {
+	for (gpa = MEM_TEST_GPA + offsp * guest_page_size, ctr = 0; ctr < count; ) {
 		uint64_t npages;
 		void *hva;
 		int ret;
@@ -591,12 +589,12 @@ static void test_memslot_do_unmap(struct vm_data *data,
 		hva = vm_gpa2hva(data, gpa, &npages);
 		TEST_ASSERT(npages, "Empty memory slot at gptr 0x%"PRIx64, gpa);
 		npages = min(npages, count - ctr);
-		ret = madvise(hva, npages * 4096, MADV_DONTNEED);
+		ret = madvise(hva, npages * guest_page_size, MADV_DONTNEED);
 		TEST_ASSERT(!ret,
 			    "madvise(%p, MADV_DONTNEED) on VM memory should not fail for gptr 0x%"PRIx64,
 			    hva, gpa);
 		ctr += npages;
-		gpa += npages * 4096;
+		gpa += npages * guest_page_size;
 	}
 	TEST_ASSERT(ctr == count,
 		    "madvise(MADV_DONTNEED) should exactly cover all of the requested area");
@@ -607,11 +605,12 @@ static void test_memslot_map_unmap_check(struct vm_data *data,
 {
 	uint64_t gpa;
 	uint64_t *val;
+	uint32_t guest_page_size = data->vm->page_size;
 
 	if (!map_unmap_verify)
 		return;
 
-	gpa = MEM_TEST_GPA + offsp * 4096;
+	gpa = MEM_TEST_GPA + offsp * guest_page_size;
 	val = (typeof(val))vm_gpa2hva(data, gpa, NULL);
 	TEST_ASSERT(*val == valexp,
 		    "Guest written values should read back correctly before unmap (%"PRIu64" vs %"PRIu64" @ %"PRIx64")",
@@ -621,12 +620,14 @@ static void test_memslot_map_unmap_check(struct vm_data *data,
 
 static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_pages = MEM_TEST_MAP_SIZE / guest_page_size;
+
 	/*
 	 * Unmap the second half of the test area while guest writes to (maps)
 	 * the first half.
 	 */
-	test_memslot_do_unmap(data, MEM_TEST_MAP_SIZE_PAGES / 2,
-			      MEM_TEST_MAP_SIZE_PAGES / 2);
+	test_memslot_do_unmap(data, guest_pages / 2, guest_pages / 2);
 
 	/*
 	 * Wait for the guest to finish writing the first half of the test
@@ -637,10 +638,8 @@ static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 	 */
 	host_perform_sync(sync);
 	test_memslot_map_unmap_check(data, 0, MEM_TEST_VAL_1);
-	test_memslot_map_unmap_check(data,
-				     MEM_TEST_MAP_SIZE_PAGES / 2 - 1,
-				     MEM_TEST_VAL_1);
-	test_memslot_do_unmap(data, 0, MEM_TEST_MAP_SIZE_PAGES / 2);
+	test_memslot_map_unmap_check(data, guest_pages / 2 - 1, MEM_TEST_VAL_1);
+	test_memslot_do_unmap(data, 0, guest_pages / 2);
 
 
 	/*
@@ -653,16 +652,16 @@ static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 	 * the test area.
 	 */
 	host_perform_sync(sync);
-	test_memslot_map_unmap_check(data, MEM_TEST_MAP_SIZE_PAGES / 2,
-				     MEM_TEST_VAL_2);
-	test_memslot_map_unmap_check(data, MEM_TEST_MAP_SIZE_PAGES - 1,
-				     MEM_TEST_VAL_2);
+	test_memslot_map_unmap_check(data, guest_pages / 2, MEM_TEST_VAL_2);
+	test_memslot_map_unmap_check(data, guest_pages - 1, MEM_TEST_VAL_2);
 }
 
 static void test_memslot_unmap_loop_common(struct vm_data *data,
 					   struct sync_area *sync,
 					   uint64_t chunk)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_pages = MEM_TEST_UNMAP_SIZE / guest_page_size;
 	uint64_t ctr;
 
 	/*
@@ -674,42 +673,49 @@ static void test_memslot_unmap_loop_common(struct vm_data *data,
 	 */
 	host_perform_sync(sync);
 	test_memslot_map_unmap_check(data, 0, MEM_TEST_VAL_1);
-	for (ctr = 0; ctr < MEM_TEST_UNMAP_SIZE_PAGES / 2; ctr += chunk)
+	for (ctr = 0; ctr < guest_pages / 2; ctr += chunk)
 		test_memslot_do_unmap(data, ctr, chunk);
 
 	/* Likewise, but for the opposite host / guest areas */
 	host_perform_sync(sync);
-	test_memslot_map_unmap_check(data, MEM_TEST_UNMAP_SIZE_PAGES / 2,
-				     MEM_TEST_VAL_2);
-	for (ctr = MEM_TEST_UNMAP_SIZE_PAGES / 2;
-	     ctr < MEM_TEST_UNMAP_SIZE_PAGES; ctr += chunk)
+	test_memslot_map_unmap_check(data, guest_pages / 2, MEM_TEST_VAL_2);
+	for (ctr = guest_pages / 2; ctr < guest_pages; ctr += chunk)
 		test_memslot_do_unmap(data, ctr, chunk);
 }
 
 static void test_memslot_unmap_loop(struct vm_data *data,
 				    struct sync_area *sync)
 {
-	test_memslot_unmap_loop_common(data, sync, 1);
+	uint32_t host_page_size = getpagesize();
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_chunk_pages = guest_page_size >= host_page_size ?
+					1 : host_page_size / guest_page_size;
+
+	test_memslot_unmap_loop_common(data, sync, guest_chunk_pages);
 }
 
 static void test_memslot_unmap_loop_chunked(struct vm_data *data,
 					    struct sync_area *sync)
 {
-	test_memslot_unmap_loop_common(data, sync, MEM_TEST_UNMAP_CHUNK_PAGES);
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_chunk_pages = MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size;
+
+	test_memslot_unmap_loop_common(data, sync, guest_chunk_pages);
 }
 
 static void test_memslot_rw_loop(struct vm_data *data, struct sync_area *sync)
 {
 	uint64_t gptr;
+	uint32_t guest_page_size = data->vm->page_size;
 
-	for (gptr = MEM_TEST_GPA + 4096 / 2;
-	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += 4096)
+	for (gptr = MEM_TEST_GPA + guest_page_size / 2;
+	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += guest_page_size)
 		*(uint64_t *)vm_gpa2hva(data, gptr, NULL) = MEM_TEST_VAL_2;
 
 	host_perform_sync(sync);
 
 	for (gptr = MEM_TEST_GPA;
-	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += 4096) {
+	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += guest_page_size) {
 		uint64_t *vptr = (typeof(vptr))vm_gpa2hva(data, gptr, NULL);
 		uint64_t val = *vptr;
 
@@ -738,7 +744,7 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 			 struct timespec *slot_runtime,
 			 struct timespec *guest_runtime)
 {
-	uint64_t mem_size = tdata->mem_size ? : MEM_SIZE_PAGES;
+	uint64_t mem_size = tdata->mem_size ? : MEM_SIZE;
 	struct vm_data *data;
 	struct sync_area *sync;
 	struct timespec tstart;
@@ -753,6 +759,7 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 
 	sync = (typeof(sync))vm_gpa2hva(data, MEM_SYNC_GPA, NULL);
 
+	sync->guest_page_size = data->vm->page_size;
 	if (tdata->prepare &&
 	    !tdata->prepare(data, sync, maxslots)) {
 		ret = false;
@@ -786,19 +793,19 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 static const struct test_data tests[] = {
 	{
 		.name = "map",
-		.mem_size = MEM_SIZE_MAP_PAGES,
+		.mem_size = MEM_SIZE_MAP,
 		.guest_code = guest_code_test_memslot_map,
 		.loop = test_memslot_map_loop,
 	},
 	{
 		.name = "unmap",
-		.mem_size = MEM_TEST_UNMAP_SIZE_PAGES + 1,
+		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop,
 	},
 	{
 		.name = "unmap chunked",
-		.mem_size = MEM_TEST_UNMAP_SIZE_PAGES + 1,
+		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop_chunked,
 	},
@@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
 		pr_info("%d: %s\n", ctr, tests[ctr].name);
 }
 
+static bool check_memory_sizes(void)
+{
+	uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+
+	if (MEM_SIZE % guest_page_size ||
+	    MEM_TEST_SIZE % guest_page_size) {
+		pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
+		return false;
+	}
+
+	if (MEM_SIZE_MAP % guest_page_size		||
+	    MEM_TEST_MAP_SIZE % guest_page_size		||
+	    (MEM_TEST_MAP_SIZE / guest_page_size) <= 2	||
+	    (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
+		pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
+		return false;
+	}
+
+	if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE		||
+	    MEM_TEST_UNMAP_SIZE % guest_page_size	||
+	    (MEM_TEST_UNMAP_SIZE / guest_page_size) %
+	    (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
+		pr_info("invalid MEM_TEST_UNMAP_SIZE or MEM_TEST_UNMAP_CHUNK_SIZE\n");
+		return false;
+	}
+
+	return true;
+}
+
 static bool parse_args(int argc, char *argv[],
 		       struct test_args *targs)
 {
@@ -1012,6 +1048,9 @@ int main(int argc, char *argv[])
 	/* Tell stdout not to buffer its content */
 	setbuf(stdout, NULL);
 
+	if (!check_memory_sizes())
+		return -1;
+
 	if (!parse_args(argc, argv, &targs))
 		return -1;
 
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

The test case is obviously broken on aarch64 because non-4KB guest
page size is supported. The guest page size on aarch64 could be 4KB,
16KB or 64KB.

This supports variable guest page size, mostly for aarch64.

  - The host determines the guest page size when virtual machine is
    created. The value is also passed to guest through the synchronization
    area.

  - The number of guest pages are unknown until the virtual machine
    is to be created. So all the related macros are dropped. Instead,
    their values are dynamically calculated based on the guest page
    size.

  - The static checks on memory sizes and pages becomes dependent
    on guest page size, which is unknown until the virtual machine
    is about to be created. So all the static checks are converted
    to dynamic checks, done in check_memory_sizes().

  - As the address passed to madvise() should be aligned to host page,
    the size of page chunk is automatically selected, other than one
    page.

  - All other changes included in this patch are almost mechanical
    replacing '4096' with 'guest_page_size'.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
 1 file changed, 115 insertions(+), 76 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index d5aa9148f96f..d587bd952ff9 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -26,14 +26,11 @@
 #include <processor.h>
 
 #define MEM_SIZE		((512U << 20) + 4096)
-#define MEM_SIZE_PAGES		(MEM_SIZE / 4096)
 #define MEM_GPA		0x10000000UL
 #define MEM_AUX_GPA		MEM_GPA
 #define MEM_SYNC_GPA		MEM_AUX_GPA
 #define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
 #define MEM_TEST_SIZE		(MEM_SIZE - 4096)
-static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
-static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
 
 /*
  * 32 MiB is max size that gets well over 100 iterations on 509 slots.
@@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
  * limited resolution).
  */
 #define MEM_SIZE_MAP		((32U << 20) + 4096)
-#define MEM_SIZE_MAP_PAGES	(MEM_SIZE_MAP / 4096)
 #define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
-#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
-static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
 
 /*
  * 128 MiB is min size that fills 32k slots with at least one page in each
  * while at the same time gets 100+ iterations in such test
+ *
+ * 2 MiB chunk size like a typical huge page
  */
 #define MEM_TEST_UNMAP_SIZE		(128U << 20)
-#define MEM_TEST_UNMAP_SIZE_PAGES	(MEM_TEST_UNMAP_SIZE / 4096)
-/* 2 MiB chunk size like a typical huge page */
-#define MEM_TEST_UNMAP_CHUNK_PAGES	(2U << (20 - 12))
-static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
-	      "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
-	      "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
-	      (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
-	      "invalid unmap test region size");
+#define MEM_TEST_UNMAP_CHUNK_SIZE	(2U << 20)
 
 /*
  * For the move active test the middle of the test area is placed on
@@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
  * for the total size of 25 pages.
  * Hence, the maximum size here is 50 pages.
  */
-#define MEM_TEST_MOVE_SIZE_PAGES	(50)
-#define MEM_TEST_MOVE_SIZE		(MEM_TEST_MOVE_SIZE_PAGES * 4096)
+#define MEM_TEST_MOVE_SIZE		0x32000
 #define MEM_TEST_MOVE_GPA_DEST		(MEM_GPA + MEM_SIZE)
 static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
 	      "invalid move test region size");
@@ -100,6 +83,7 @@ struct vm_data {
 };
 
 struct sync_area {
+	uint32_t    guest_page_size;
 	atomic_bool start_flag;
 	atomic_bool exit_flag;
 	atomic_bool sync_flag;
@@ -192,14 +176,15 @@ static void *vm_gpa2hva(struct vm_data *data, uint64_t gpa, uint64_t *rempages)
 	uint64_t gpage, pgoffs;
 	uint32_t slot, slotoffs;
 	void *base;
+	uint32_t guest_page_size = data->vm->page_size;
 
 	TEST_ASSERT(gpa >= MEM_GPA, "Too low gpa to translate");
-	TEST_ASSERT(gpa < MEM_GPA + data->npages * 4096,
+	TEST_ASSERT(gpa < MEM_GPA + data->npages * guest_page_size,
 		    "Too high gpa to translate");
 	gpa -= MEM_GPA;
 
-	gpage = gpa / 4096;
-	pgoffs = gpa % 4096;
+	gpage = gpa / guest_page_size;
+	pgoffs = gpa % guest_page_size;
 	slot = min(gpage / data->pages_per_slot, (uint64_t)data->nslots - 1);
 	slotoffs = gpage - (slot * data->pages_per_slot);
 
@@ -217,14 +202,16 @@ static void *vm_gpa2hva(struct vm_data *data, uint64_t gpa, uint64_t *rempages)
 	}
 
 	base = data->hva_slots[slot];
-	return (uint8_t *)base + slotoffs * 4096 + pgoffs;
+	return (uint8_t *)base + slotoffs * guest_page_size + pgoffs;
 }
 
 static uint64_t vm_slot2gpa(struct vm_data *data, uint32_t slot)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+
 	TEST_ASSERT(slot < data->nslots, "Too high slot number");
 
-	return MEM_GPA + slot * data->pages_per_slot * 4096;
+	return MEM_GPA + slot * data->pages_per_slot * guest_page_size;
 }
 
 static struct vm_data *alloc_vm(void)
@@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
 }
 
 static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
-		       void *guest_code, uint64_t mempages,
+		       void *guest_code, uint64_t mem_size,
 		       struct timespec *slot_runtime)
 {
-	uint64_t rempages;
+	uint64_t mempages, rempages;
 	uint64_t guest_addr;
-	uint32_t slot;
+	uint32_t slot, guest_page_size;
 	struct timespec tstart;
 	struct sync_area *sync;
 
-	TEST_ASSERT(mempages > 1,
-		    "Can't test without any memory");
+	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+	mempages = mem_size / guest_page_size;
+
+	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
+	ucall_init(data->vm, NULL);
 
 	data->npages = mempages;
+	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
 	data->nslots = nslots;
-	data->pages_per_slot = mempages / data->nslots;
+	data->pages_per_slot = data->npages / data->nslots;
 	if (!data->pages_per_slot) {
-		*maxslots = mempages + 1;
+		*maxslots = data->npages + 1;
 		return false;
 	}
 
-	rempages = mempages % data->nslots;
+	rempages = data->npages % data->nslots;
 	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
 	TEST_ASSERT(data->hva_slots, "malloc() fail");
 
-	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
-	ucall_init(data->vm, NULL);
-
 	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
 		data->nslots, data->pages_per_slot, rempages);
 
@@ -283,7 +271,7 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		vm_userspace_mem_region_add(data->vm, VM_MEM_SRC_ANONYMOUS,
 					    guest_addr, slot, npages,
 					    0);
-		guest_addr += npages * 4096;
+		guest_addr += npages * guest_page_size;
 	}
 	*slot_runtime = timespec_elapsed(tstart);
 
@@ -300,12 +288,12 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 			    "vm_phy_pages_alloc() failed\n");
 
 		data->hva_slots[slot - 1] = addr_gpa2hva(data->vm, guest_addr);
-		memset(data->hva_slots[slot - 1], 0, npages * 4096);
+		memset(data->hva_slots[slot - 1], 0, npages * guest_page_size);
 
-		guest_addr += npages * 4096;
+		guest_addr += npages * guest_page_size;
 	}
 
-	virt_map(data->vm, MEM_GPA, MEM_GPA, mempages);
+	virt_map(data->vm, MEM_GPA, MEM_GPA, data->npages);
 
 	sync = (typeof(sync))vm_gpa2hva(data, MEM_SYNC_GPA, NULL);
 	atomic_init(&sync->start_flag, false);
@@ -404,6 +392,7 @@ static bool guest_perform_sync(void)
 static void guest_code_test_memslot_move(void)
 {
 	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
 	uintptr_t base = (typeof(base))READ_ONCE(sync->move_area_ptr);
 
 	GUEST_SYNC(0);
@@ -414,7 +403,7 @@ static void guest_code_test_memslot_move(void)
 		uintptr_t ptr;
 
 		for (ptr = base; ptr < base + MEM_TEST_MOVE_SIZE;
-		     ptr += 4096)
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		/*
@@ -432,6 +421,7 @@ static void guest_code_test_memslot_move(void)
 static void guest_code_test_memslot_map(void)
 {
 	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
 
 	GUEST_SYNC(0);
 
@@ -441,14 +431,16 @@ static void guest_code_test_memslot_map(void)
 		uintptr_t ptr;
 
 		for (ptr = MEM_TEST_GPA;
-		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2;
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		if (!guest_perform_sync())
 			break;
 
 		for (ptr = MEM_TEST_GPA + MEM_TEST_MAP_SIZE / 2;
-		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_MAP_SIZE;
+		     ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_2;
 
 		if (!guest_perform_sync())
@@ -495,6 +487,9 @@ static void guest_code_test_memslot_unmap(void)
 
 static void guest_code_test_memslot_rw(void)
 {
+	struct sync_area *sync = (typeof(sync))MEM_SYNC_GPA;
+	uint32_t page_size = (typeof(page_size))READ_ONCE(sync->guest_page_size);
+
 	GUEST_SYNC(0);
 
 	guest_spin_until_start();
@@ -503,14 +498,14 @@ static void guest_code_test_memslot_rw(void)
 		uintptr_t ptr;
 
 		for (ptr = MEM_TEST_GPA;
-		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += 4096)
+		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += page_size)
 			*(uint64_t *)ptr = MEM_TEST_VAL_1;
 
 		if (!guest_perform_sync())
 			break;
 
-		for (ptr = MEM_TEST_GPA + 4096 / 2;
-		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += 4096) {
+		for (ptr = MEM_TEST_GPA + page_size / 2;
+		     ptr < MEM_TEST_GPA + MEM_TEST_SIZE; ptr += page_size) {
 			uint64_t val = *(uint64_t *)ptr;
 
 			GUEST_ASSERT_1(val == MEM_TEST_VAL_2, val);
@@ -528,6 +523,8 @@ static bool test_memslot_move_prepare(struct vm_data *data,
 				      struct sync_area *sync,
 				      uint64_t *maxslots, bool isactive)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t move_pages = MEM_TEST_MOVE_SIZE / guest_page_size;
 	uint64_t movesrcgpa, movetestgpa;
 
 	movesrcgpa = vm_slot2gpa(data, data->nslots - 1);
@@ -536,7 +533,7 @@ static bool test_memslot_move_prepare(struct vm_data *data,
 		uint64_t lastpages;
 
 		vm_gpa2hva(data, movesrcgpa, &lastpages);
-		if (lastpages < MEM_TEST_MOVE_SIZE_PAGES / 2) {
+		if (lastpages < move_pages / 2) {
 			*maxslots = 0;
 			return false;
 		}
@@ -582,8 +579,9 @@ static void test_memslot_do_unmap(struct vm_data *data,
 				  uint64_t offsp, uint64_t count)
 {
 	uint64_t gpa, ctr;
+	uint32_t guest_page_size = data->vm->page_size;
 
-	for (gpa = MEM_TEST_GPA + offsp * 4096, ctr = 0; ctr < count; ) {
+	for (gpa = MEM_TEST_GPA + offsp * guest_page_size, ctr = 0; ctr < count; ) {
 		uint64_t npages;
 		void *hva;
 		int ret;
@@ -591,12 +589,12 @@ static void test_memslot_do_unmap(struct vm_data *data,
 		hva = vm_gpa2hva(data, gpa, &npages);
 		TEST_ASSERT(npages, "Empty memory slot at gptr 0x%"PRIx64, gpa);
 		npages = min(npages, count - ctr);
-		ret = madvise(hva, npages * 4096, MADV_DONTNEED);
+		ret = madvise(hva, npages * guest_page_size, MADV_DONTNEED);
 		TEST_ASSERT(!ret,
 			    "madvise(%p, MADV_DONTNEED) on VM memory should not fail for gptr 0x%"PRIx64,
 			    hva, gpa);
 		ctr += npages;
-		gpa += npages * 4096;
+		gpa += npages * guest_page_size;
 	}
 	TEST_ASSERT(ctr == count,
 		    "madvise(MADV_DONTNEED) should exactly cover all of the requested area");
@@ -607,11 +605,12 @@ static void test_memslot_map_unmap_check(struct vm_data *data,
 {
 	uint64_t gpa;
 	uint64_t *val;
+	uint32_t guest_page_size = data->vm->page_size;
 
 	if (!map_unmap_verify)
 		return;
 
-	gpa = MEM_TEST_GPA + offsp * 4096;
+	gpa = MEM_TEST_GPA + offsp * guest_page_size;
 	val = (typeof(val))vm_gpa2hva(data, gpa, NULL);
 	TEST_ASSERT(*val == valexp,
 		    "Guest written values should read back correctly before unmap (%"PRIu64" vs %"PRIu64" @ %"PRIx64")",
@@ -621,12 +620,14 @@ static void test_memslot_map_unmap_check(struct vm_data *data,
 
 static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_pages = MEM_TEST_MAP_SIZE / guest_page_size;
+
 	/*
 	 * Unmap the second half of the test area while guest writes to (maps)
 	 * the first half.
 	 */
-	test_memslot_do_unmap(data, MEM_TEST_MAP_SIZE_PAGES / 2,
-			      MEM_TEST_MAP_SIZE_PAGES / 2);
+	test_memslot_do_unmap(data, guest_pages / 2, guest_pages / 2);
 
 	/*
 	 * Wait for the guest to finish writing the first half of the test
@@ -637,10 +638,8 @@ static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 	 */
 	host_perform_sync(sync);
 	test_memslot_map_unmap_check(data, 0, MEM_TEST_VAL_1);
-	test_memslot_map_unmap_check(data,
-				     MEM_TEST_MAP_SIZE_PAGES / 2 - 1,
-				     MEM_TEST_VAL_1);
-	test_memslot_do_unmap(data, 0, MEM_TEST_MAP_SIZE_PAGES / 2);
+	test_memslot_map_unmap_check(data, guest_pages / 2 - 1, MEM_TEST_VAL_1);
+	test_memslot_do_unmap(data, 0, guest_pages / 2);
 
 
 	/*
@@ -653,16 +652,16 @@ static void test_memslot_map_loop(struct vm_data *data, struct sync_area *sync)
 	 * the test area.
 	 */
 	host_perform_sync(sync);
-	test_memslot_map_unmap_check(data, MEM_TEST_MAP_SIZE_PAGES / 2,
-				     MEM_TEST_VAL_2);
-	test_memslot_map_unmap_check(data, MEM_TEST_MAP_SIZE_PAGES - 1,
-				     MEM_TEST_VAL_2);
+	test_memslot_map_unmap_check(data, guest_pages / 2, MEM_TEST_VAL_2);
+	test_memslot_map_unmap_check(data, guest_pages - 1, MEM_TEST_VAL_2);
 }
 
 static void test_memslot_unmap_loop_common(struct vm_data *data,
 					   struct sync_area *sync,
 					   uint64_t chunk)
 {
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_pages = MEM_TEST_UNMAP_SIZE / guest_page_size;
 	uint64_t ctr;
 
 	/*
@@ -674,42 +673,49 @@ static void test_memslot_unmap_loop_common(struct vm_data *data,
 	 */
 	host_perform_sync(sync);
 	test_memslot_map_unmap_check(data, 0, MEM_TEST_VAL_1);
-	for (ctr = 0; ctr < MEM_TEST_UNMAP_SIZE_PAGES / 2; ctr += chunk)
+	for (ctr = 0; ctr < guest_pages / 2; ctr += chunk)
 		test_memslot_do_unmap(data, ctr, chunk);
 
 	/* Likewise, but for the opposite host / guest areas */
 	host_perform_sync(sync);
-	test_memslot_map_unmap_check(data, MEM_TEST_UNMAP_SIZE_PAGES / 2,
-				     MEM_TEST_VAL_2);
-	for (ctr = MEM_TEST_UNMAP_SIZE_PAGES / 2;
-	     ctr < MEM_TEST_UNMAP_SIZE_PAGES; ctr += chunk)
+	test_memslot_map_unmap_check(data, guest_pages / 2, MEM_TEST_VAL_2);
+	for (ctr = guest_pages / 2; ctr < guest_pages; ctr += chunk)
 		test_memslot_do_unmap(data, ctr, chunk);
 }
 
 static void test_memslot_unmap_loop(struct vm_data *data,
 				    struct sync_area *sync)
 {
-	test_memslot_unmap_loop_common(data, sync, 1);
+	uint32_t host_page_size = getpagesize();
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_chunk_pages = guest_page_size >= host_page_size ?
+					1 : host_page_size / guest_page_size;
+
+	test_memslot_unmap_loop_common(data, sync, guest_chunk_pages);
 }
 
 static void test_memslot_unmap_loop_chunked(struct vm_data *data,
 					    struct sync_area *sync)
 {
-	test_memslot_unmap_loop_common(data, sync, MEM_TEST_UNMAP_CHUNK_PAGES);
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t guest_chunk_pages = MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size;
+
+	test_memslot_unmap_loop_common(data, sync, guest_chunk_pages);
 }
 
 static void test_memslot_rw_loop(struct vm_data *data, struct sync_area *sync)
 {
 	uint64_t gptr;
+	uint32_t guest_page_size = data->vm->page_size;
 
-	for (gptr = MEM_TEST_GPA + 4096 / 2;
-	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += 4096)
+	for (gptr = MEM_TEST_GPA + guest_page_size / 2;
+	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += guest_page_size)
 		*(uint64_t *)vm_gpa2hva(data, gptr, NULL) = MEM_TEST_VAL_2;
 
 	host_perform_sync(sync);
 
 	for (gptr = MEM_TEST_GPA;
-	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += 4096) {
+	     gptr < MEM_TEST_GPA + MEM_TEST_SIZE; gptr += guest_page_size) {
 		uint64_t *vptr = (typeof(vptr))vm_gpa2hva(data, gptr, NULL);
 		uint64_t val = *vptr;
 
@@ -738,7 +744,7 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 			 struct timespec *slot_runtime,
 			 struct timespec *guest_runtime)
 {
-	uint64_t mem_size = tdata->mem_size ? : MEM_SIZE_PAGES;
+	uint64_t mem_size = tdata->mem_size ? : MEM_SIZE;
 	struct vm_data *data;
 	struct sync_area *sync;
 	struct timespec tstart;
@@ -753,6 +759,7 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 
 	sync = (typeof(sync))vm_gpa2hva(data, MEM_SYNC_GPA, NULL);
 
+	sync->guest_page_size = data->vm->page_size;
 	if (tdata->prepare &&
 	    !tdata->prepare(data, sync, maxslots)) {
 		ret = false;
@@ -786,19 +793,19 @@ static bool test_execute(int nslots, uint64_t *maxslots,
 static const struct test_data tests[] = {
 	{
 		.name = "map",
-		.mem_size = MEM_SIZE_MAP_PAGES,
+		.mem_size = MEM_SIZE_MAP,
 		.guest_code = guest_code_test_memslot_map,
 		.loop = test_memslot_map_loop,
 	},
 	{
 		.name = "unmap",
-		.mem_size = MEM_TEST_UNMAP_SIZE_PAGES + 1,
+		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop,
 	},
 	{
 		.name = "unmap chunked",
-		.mem_size = MEM_TEST_UNMAP_SIZE_PAGES + 1,
+		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop_chunked,
 	},
@@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
 		pr_info("%d: %s\n", ctr, tests[ctr].name);
 }
 
+static bool check_memory_sizes(void)
+{
+	uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+
+	if (MEM_SIZE % guest_page_size ||
+	    MEM_TEST_SIZE % guest_page_size) {
+		pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
+		return false;
+	}
+
+	if (MEM_SIZE_MAP % guest_page_size		||
+	    MEM_TEST_MAP_SIZE % guest_page_size		||
+	    (MEM_TEST_MAP_SIZE / guest_page_size) <= 2	||
+	    (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
+		pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
+		return false;
+	}
+
+	if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE		||
+	    MEM_TEST_UNMAP_SIZE % guest_page_size	||
+	    (MEM_TEST_UNMAP_SIZE / guest_page_size) %
+	    (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
+		pr_info("invalid MEM_TEST_UNMAP_SIZE or MEM_TEST_UNMAP_CHUNK_SIZE\n");
+		return false;
+	}
+
+	return true;
+}
+
 static bool parse_args(int argc, char *argv[],
 		       struct test_args *targs)
 {
@@ -1012,6 +1048,9 @@ int main(int argc, char *argv[])
 	/* Tell stdout not to buffer its content */
 	setbuf(stdout, NULL);
 
+	if (!check_memory_sizes())
+		return -1;
+
 	if (!parse_args(argc, argv, &targs))
 		return -1;
 
-- 
2.23.0

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> The test case is obviously broken on aarch64 because non-4KB guest
> page size is supported. The guest page size on aarch64 could be 4KB,
> 16KB or 64KB.
> 
> This supports variable guest page size, mostly for aarch64.
> 
>    - The host determines the guest page size when virtual machine is
>      created. The value is also passed to guest through the synchronization
>      area.
> 
>    - The number of guest pages are unknown until the virtual machine
>      is to be created. So all the related macros are dropped. Instead,
>      their values are dynamically calculated based on the guest page
>      size.
> 
>    - The static checks on memory sizes and pages becomes dependent
>      on guest page size, which is unknown until the virtual machine
>      is about to be created. So all the static checks are converted
>      to dynamic checks, done in check_memory_sizes().
> 
>    - As the address passed to madvise() should be aligned to host page,
>      the size of page chunk is automatically selected, other than one
>      page.
> 
>    - All other changes included in this patch are almost mechanical
>      replacing '4096' with 'guest_page_size'.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>   1 file changed, 115 insertions(+), 76 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index d5aa9148f96f..d587bd952ff9 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -26,14 +26,11 @@
>   #include <processor.h>
>   
>   #define MEM_SIZE		((512U << 20) + 4096)
> -#define MEM_SIZE_PAGES		(MEM_SIZE / 4096)
>   #define MEM_GPA		0x10000000UL
>   #define MEM_AUX_GPA		MEM_GPA
>   #define MEM_SYNC_GPA		MEM_AUX_GPA
>   #define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
>   #define MEM_TEST_SIZE		(MEM_SIZE - 4096)
> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>   
>   /*
>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>    * limited resolution).
>    */
>   #define MEM_SIZE_MAP		((32U << 20) + 4096)
> -#define MEM_SIZE_MAP_PAGES	(MEM_SIZE_MAP / 4096)
>   #define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>   
>   /*
>    * 128 MiB is min size that fills 32k slots with at least one page in each
>    * while at the same time gets 100+ iterations in such test
> + *
> + * 2 MiB chunk size like a typical huge page
>    */
>   #define MEM_TEST_UNMAP_SIZE		(128U << 20)
> -#define MEM_TEST_UNMAP_SIZE_PAGES	(MEM_TEST_UNMAP_SIZE / 4096)
> -/* 2 MiB chunk size like a typical huge page */
> -#define MEM_TEST_UNMAP_CHUNK_PAGES	(2U << (20 - 12))
> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
> -	      "invalid unmap test region size");
> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
> -	      "invalid unmap test region size");
> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
> -	      (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
> -	      "invalid unmap test region size");
> +#define MEM_TEST_UNMAP_CHUNK_SIZE	(2U << 20)
>   
>   /*
>    * For the move active test the middle of the test area is placed on
> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>    * for the total size of 25 pages.
>    * Hence, the maximum size here is 50 pages.
>    */
> -#define MEM_TEST_MOVE_SIZE_PAGES	(50)
> -#define MEM_TEST_MOVE_SIZE		(MEM_TEST_MOVE_SIZE_PAGES * 4096)
> +#define MEM_TEST_MOVE_SIZE		0x32000

The above number seems less readable than an explicit value of 50 pages.

In addition to that, it's 50 pages only with 4k page size, so at least
the comment above needs to be updated to reflect this fact.

>   #define MEM_TEST_MOVE_GPA_DEST		(MEM_GPA + MEM_SIZE)
>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>   	      "invalid move test region size");
(...)
> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>   }
>   
>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
> -		       void *guest_code, uint64_t mempages,
> +		       void *guest_code, uint64_t mem_size,
>   		       struct timespec *slot_runtime)
>   {
> -	uint64_t rempages;
> +	uint64_t mempages, rempages;
>   	uint64_t guest_addr;
> -	uint32_t slot;
> +	uint32_t slot, guest_page_size;
>   	struct timespec tstart;
>   	struct sync_area *sync;
>   
> -	TEST_ASSERT(mempages > 1,
> -		    "Can't test without any memory");
> +	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
> +	mempages = mem_size / guest_page_size;
> +
> +	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
> +	ucall_init(data->vm, NULL);
>

TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
here would catch the case if someone accidentally modifies
__vm_create_with_one_vcpu() to use other page size than specified for
VM_MODE_DEFAULT.

>   	data->npages = mempages;
> +	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>   	data->nslots = nslots;
> -	data->pages_per_slot = mempages / data->nslots;
> +	data->pages_per_slot = data->npages / data->nslots;
>   	if (!data->pages_per_slot) {
> -		*maxslots = mempages + 1;
> +		*maxslots = data->npages + 1;
>   		return false;
>   	}
>   
> -	rempages = mempages % data->nslots;
> +	rempages = data->npages % data->nslots;
>   	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>   	TEST_ASSERT(data->hva_slots, "malloc() fail");
>   
> -	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
> -	ucall_init(data->vm, NULL);
> -
>   	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>   		data->nslots, data->pages_per_slot, rempages);
>   
(...)
> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>   		pr_info("%d: %s\n", ctr, tests[ctr].name);
>   }
>   
> +static bool check_memory_sizes(void)
> +{
> +	uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
> +
> +	if (MEM_SIZE % guest_page_size ||
> +	    MEM_TEST_SIZE % guest_page_size) {
> +		pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
> +		return false;
> +	}
> +
> +	if (MEM_SIZE_MAP % guest_page_size		||
> +	    MEM_TEST_MAP_SIZE % guest_page_size		||
> +	    (MEM_TEST_MAP_SIZE / guest_page_size) <= 2	||
> +	    (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
> +		pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
> +		return false;
> +	}
> +
> +	if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE		||
> +	    MEM_TEST_UNMAP_SIZE % guest_page_size	||
> +	    (MEM_TEST_UNMAP_SIZE / guest_page_size) %
> +	    (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {

This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
to match the old static_assert().

Thanks,
Maciej

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> The test case is obviously broken on aarch64 because non-4KB guest
> page size is supported. The guest page size on aarch64 could be 4KB,
> 16KB or 64KB.
> 
> This supports variable guest page size, mostly for aarch64.
> 
>    - The host determines the guest page size when virtual machine is
>      created. The value is also passed to guest through the synchronization
>      area.
> 
>    - The number of guest pages are unknown until the virtual machine
>      is to be created. So all the related macros are dropped. Instead,
>      their values are dynamically calculated based on the guest page
>      size.
> 
>    - The static checks on memory sizes and pages becomes dependent
>      on guest page size, which is unknown until the virtual machine
>      is about to be created. So all the static checks are converted
>      to dynamic checks, done in check_memory_sizes().
> 
>    - As the address passed to madvise() should be aligned to host page,
>      the size of page chunk is automatically selected, other than one
>      page.
> 
>    - All other changes included in this patch are almost mechanical
>      replacing '4096' with 'guest_page_size'.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>   1 file changed, 115 insertions(+), 76 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index d5aa9148f96f..d587bd952ff9 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -26,14 +26,11 @@
>   #include <processor.h>
>   
>   #define MEM_SIZE		((512U << 20) + 4096)
> -#define MEM_SIZE_PAGES		(MEM_SIZE / 4096)
>   #define MEM_GPA		0x10000000UL
>   #define MEM_AUX_GPA		MEM_GPA
>   #define MEM_SYNC_GPA		MEM_AUX_GPA
>   #define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
>   #define MEM_TEST_SIZE		(MEM_SIZE - 4096)
> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>   
>   /*
>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>    * limited resolution).
>    */
>   #define MEM_SIZE_MAP		((32U << 20) + 4096)
> -#define MEM_SIZE_MAP_PAGES	(MEM_SIZE_MAP / 4096)
>   #define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>   
>   /*
>    * 128 MiB is min size that fills 32k slots with at least one page in each
>    * while at the same time gets 100+ iterations in such test
> + *
> + * 2 MiB chunk size like a typical huge page
>    */
>   #define MEM_TEST_UNMAP_SIZE		(128U << 20)
> -#define MEM_TEST_UNMAP_SIZE_PAGES	(MEM_TEST_UNMAP_SIZE / 4096)
> -/* 2 MiB chunk size like a typical huge page */
> -#define MEM_TEST_UNMAP_CHUNK_PAGES	(2U << (20 - 12))
> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
> -	      "invalid unmap test region size");
> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
> -	      "invalid unmap test region size");
> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
> -	      (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
> -	      "invalid unmap test region size");
> +#define MEM_TEST_UNMAP_CHUNK_SIZE	(2U << 20)
>   
>   /*
>    * For the move active test the middle of the test area is placed on
> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>    * for the total size of 25 pages.
>    * Hence, the maximum size here is 50 pages.
>    */
> -#define MEM_TEST_MOVE_SIZE_PAGES	(50)
> -#define MEM_TEST_MOVE_SIZE		(MEM_TEST_MOVE_SIZE_PAGES * 4096)
> +#define MEM_TEST_MOVE_SIZE		0x32000

The above number seems less readable than an explicit value of 50 pages.

In addition to that, it's 50 pages only with 4k page size, so at least
the comment above needs to be updated to reflect this fact.

>   #define MEM_TEST_MOVE_GPA_DEST		(MEM_GPA + MEM_SIZE)
>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>   	      "invalid move test region size");
(...)
> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>   }
>   
>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
> -		       void *guest_code, uint64_t mempages,
> +		       void *guest_code, uint64_t mem_size,
>   		       struct timespec *slot_runtime)
>   {
> -	uint64_t rempages;
> +	uint64_t mempages, rempages;
>   	uint64_t guest_addr;
> -	uint32_t slot;
> +	uint32_t slot, guest_page_size;
>   	struct timespec tstart;
>   	struct sync_area *sync;
>   
> -	TEST_ASSERT(mempages > 1,
> -		    "Can't test without any memory");
> +	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
> +	mempages = mem_size / guest_page_size;
> +
> +	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
> +	ucall_init(data->vm, NULL);
>

TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
here would catch the case if someone accidentally modifies
__vm_create_with_one_vcpu() to use other page size than specified for
VM_MODE_DEFAULT.

>   	data->npages = mempages;
> +	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>   	data->nslots = nslots;
> -	data->pages_per_slot = mempages / data->nslots;
> +	data->pages_per_slot = data->npages / data->nslots;
>   	if (!data->pages_per_slot) {
> -		*maxslots = mempages + 1;
> +		*maxslots = data->npages + 1;
>   		return false;
>   	}
>   
> -	rempages = mempages % data->nslots;
> +	rempages = data->npages % data->nslots;
>   	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>   	TEST_ASSERT(data->hva_slots, "malloc() fail");
>   
> -	data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
> -	ucall_init(data->vm, NULL);
> -
>   	pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>   		data->nslots, data->pages_per_slot, rempages);
>   
(...)
> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>   		pr_info("%d: %s\n", ctr, tests[ctr].name);
>   }
>   
> +static bool check_memory_sizes(void)
> +{
> +	uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
> +
> +	if (MEM_SIZE % guest_page_size ||
> +	    MEM_TEST_SIZE % guest_page_size) {
> +		pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
> +		return false;
> +	}
> +
> +	if (MEM_SIZE_MAP % guest_page_size		||
> +	    MEM_TEST_MAP_SIZE % guest_page_size		||
> +	    (MEM_TEST_MAP_SIZE / guest_page_size) <= 2	||
> +	    (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
> +		pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
> +		return false;
> +	}
> +
> +	if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE		||
> +	    MEM_TEST_UNMAP_SIZE % guest_page_size	||
> +	    (MEM_TEST_UNMAP_SIZE / guest_page_size) %
> +	    (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {

This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
to match the old static_assert().

Thanks,
Maciej

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> The test case is obviously broken on aarch64 because non-4KB guest
>> page size is supported. The guest page size on aarch64 could be 4KB,
>> 16KB or 64KB.
>>
>> This supports variable guest page size, mostly for aarch64.
>>
>>    - The host determines the guest page size when virtual machine is
>>      created. The value is also passed to guest through the synchronization
>>      area.
>>
>>    - The number of guest pages are unknown until the virtual machine
>>      is to be created. So all the related macros are dropped. Instead,
>>      their values are dynamically calculated based on the guest page
>>      size.
>>
>>    - The static checks on memory sizes and pages becomes dependent
>>      on guest page size, which is unknown until the virtual machine
>>      is about to be created. So all the static checks are converted
>>      to dynamic checks, done in check_memory_sizes().
>>
>>    - As the address passed to madvise() should be aligned to host page,
>>      the size of page chunk is automatically selected, other than one
>>      page.
>>
>>    - All other changes included in this patch are almost mechanical
>>      replacing '4096' with 'guest_page_size'.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index d5aa9148f96f..d587bd952ff9 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -26,14 +26,11 @@
>>   #include <processor.h>
>>   #define MEM_SIZE        ((512U << 20) + 4096)
>> -#define MEM_SIZE_PAGES        (MEM_SIZE / 4096)
>>   #define MEM_GPA        0x10000000UL
>>   #define MEM_AUX_GPA        MEM_GPA
>>   #define MEM_SYNC_GPA        MEM_AUX_GPA
>>   #define MEM_TEST_GPA        (MEM_AUX_GPA + 4096)
>>   #define MEM_TEST_SIZE        (MEM_SIZE - 4096)
>> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
>> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>   /*
>>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
>> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>    * limited resolution).
>>    */
>>   #define MEM_SIZE_MAP        ((32U << 20) + 4096)
>> -#define MEM_SIZE_MAP_PAGES    (MEM_SIZE_MAP / 4096)
>>   #define MEM_TEST_MAP_SIZE    (MEM_SIZE_MAP - 4096)
>> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
>> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>>   /*
>>    * 128 MiB is min size that fills 32k slots with at least one page in each
>>    * while at the same time gets 100+ iterations in such test
>> + *
>> + * 2 MiB chunk size like a typical huge page
>>    */
>>   #define MEM_TEST_UNMAP_SIZE        (128U << 20)
>> -#define MEM_TEST_UNMAP_SIZE_PAGES    (MEM_TEST_UNMAP_SIZE / 4096)
>> -/* 2 MiB chunk size like a typical huge page */
>> -#define MEM_TEST_UNMAP_CHUNK_PAGES    (2U << (20 - 12))
>> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
>> -          "invalid unmap test region size");
>> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
>> -          "invalid unmap test region size");
>> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>> -          (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
>> -          "invalid unmap test region size");
>> +#define MEM_TEST_UNMAP_CHUNK_SIZE    (2U << 20)
>>   /*
>>    * For the move active test the middle of the test area is placed on
>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>    * for the total size of 25 pages.
>>    * Hence, the maximum size here is 50 pages.
>>    */
>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>> +#define MEM_TEST_MOVE_SIZE        0x32000
> 
> The above number seems less readable than an explicit value of 50 pages.
> 
> In addition to that, it's 50 pages only with 4k page size, so at least
> the comment above needs to be updated to reflect this fact.
> 

Yeah, I will change the comments like below in next revision.

  /*
   * When running this test with 32k memslots, actually 32763 excluding
   * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
   * The last slot contains 0x19000 bytes memory. Hence, the maximum size
   * here is 0x32000 bytes.
   */

>>   #define MEM_TEST_MOVE_GPA_DEST        (MEM_GPA + MEM_SIZE)
>>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>>             "invalid move test region size");
> (...)
>> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>>   }
>>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>> -               void *guest_code, uint64_t mempages,
>> +               void *guest_code, uint64_t mem_size,
>>                  struct timespec *slot_runtime)
>>   {
>> -    uint64_t rempages;
>> +    uint64_t mempages, rempages;
>>       uint64_t guest_addr;
>> -    uint32_t slot;
>> +    uint32_t slot, guest_page_size;
>>       struct timespec tstart;
>>       struct sync_area *sync;
>> -    TEST_ASSERT(mempages > 1,
>> -            "Can't test without any memory");
>> +    guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>> +    mempages = mem_size / guest_page_size;
>> +
>> +    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>> +    ucall_init(data->vm, NULL);
>>
> 
> TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
> here would catch the case if someone accidentally modifies
> __vm_create_with_one_vcpu() to use other page size than specified for
> VM_MODE_DEFAULT.
> 

Sure, it's not harmful at least.

>>       data->npages = mempages;
>> +    TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>>       data->nslots = nslots;
>> -    data->pages_per_slot = mempages / data->nslots;
>> +    data->pages_per_slot = data->npages / data->nslots;
>>       if (!data->pages_per_slot) {
>> -        *maxslots = mempages + 1;
>> +        *maxslots = data->npages + 1;
>>           return false;
>>       }
>> -    rempages = mempages % data->nslots;
>> +    rempages = data->npages % data->nslots;
>>       data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>>       TEST_ASSERT(data->hva_slots, "malloc() fail");
>> -    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>> -    ucall_init(data->vm, NULL);
>> -
>>       pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>>           data->nslots, data->pages_per_slot, rempages);
> (...)
>> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>>           pr_info("%d: %s\n", ctr, tests[ctr].name);
>>   }
>> +static bool check_memory_sizes(void)
>> +{
>> +    uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>> +
>> +    if (MEM_SIZE % guest_page_size ||
>> +        MEM_TEST_SIZE % guest_page_size) {
>> +        pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
>> +        return false;
>> +    }
>> +
>> +    if (MEM_SIZE_MAP % guest_page_size        ||
>> +        MEM_TEST_MAP_SIZE % guest_page_size        ||
>> +        (MEM_TEST_MAP_SIZE / guest_page_size) <= 2    ||
>> +        (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
>> +        pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
>> +        return false;
>> +    }
>> +
>> +    if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE        ||
>> +        MEM_TEST_UNMAP_SIZE % guest_page_size    ||
>> +        (MEM_TEST_UNMAP_SIZE / guest_page_size) %
>> +        (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
> 
> This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
> to match the old static_assert().
> 

Nice catch! I will fix it up in next revision :)

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> The test case is obviously broken on aarch64 because non-4KB guest
>> page size is supported. The guest page size on aarch64 could be 4KB,
>> 16KB or 64KB.
>>
>> This supports variable guest page size, mostly for aarch64.
>>
>>    - The host determines the guest page size when virtual machine is
>>      created. The value is also passed to guest through the synchronization
>>      area.
>>
>>    - The number of guest pages are unknown until the virtual machine
>>      is to be created. So all the related macros are dropped. Instead,
>>      their values are dynamically calculated based on the guest page
>>      size.
>>
>>    - The static checks on memory sizes and pages becomes dependent
>>      on guest page size, which is unknown until the virtual machine
>>      is about to be created. So all the static checks are converted
>>      to dynamic checks, done in check_memory_sizes().
>>
>>    - As the address passed to madvise() should be aligned to host page,
>>      the size of page chunk is automatically selected, other than one
>>      page.
>>
>>    - All other changes included in this patch are almost mechanical
>>      replacing '4096' with 'guest_page_size'.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index d5aa9148f96f..d587bd952ff9 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -26,14 +26,11 @@
>>   #include <processor.h>
>>   #define MEM_SIZE        ((512U << 20) + 4096)
>> -#define MEM_SIZE_PAGES        (MEM_SIZE / 4096)
>>   #define MEM_GPA        0x10000000UL
>>   #define MEM_AUX_GPA        MEM_GPA
>>   #define MEM_SYNC_GPA        MEM_AUX_GPA
>>   #define MEM_TEST_GPA        (MEM_AUX_GPA + 4096)
>>   #define MEM_TEST_SIZE        (MEM_SIZE - 4096)
>> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
>> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>   /*
>>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
>> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>    * limited resolution).
>>    */
>>   #define MEM_SIZE_MAP        ((32U << 20) + 4096)
>> -#define MEM_SIZE_MAP_PAGES    (MEM_SIZE_MAP / 4096)
>>   #define MEM_TEST_MAP_SIZE    (MEM_SIZE_MAP - 4096)
>> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
>> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
>> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>>   /*
>>    * 128 MiB is min size that fills 32k slots with at least one page in each
>>    * while at the same time gets 100+ iterations in such test
>> + *
>> + * 2 MiB chunk size like a typical huge page
>>    */
>>   #define MEM_TEST_UNMAP_SIZE        (128U << 20)
>> -#define MEM_TEST_UNMAP_SIZE_PAGES    (MEM_TEST_UNMAP_SIZE / 4096)
>> -/* 2 MiB chunk size like a typical huge page */
>> -#define MEM_TEST_UNMAP_CHUNK_PAGES    (2U << (20 - 12))
>> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
>> -          "invalid unmap test region size");
>> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
>> -          "invalid unmap test region size");
>> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>> -          (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
>> -          "invalid unmap test region size");
>> +#define MEM_TEST_UNMAP_CHUNK_SIZE    (2U << 20)
>>   /*
>>    * For the move active test the middle of the test area is placed on
>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>    * for the total size of 25 pages.
>>    * Hence, the maximum size here is 50 pages.
>>    */
>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>> +#define MEM_TEST_MOVE_SIZE        0x32000
> 
> The above number seems less readable than an explicit value of 50 pages.
> 
> In addition to that, it's 50 pages only with 4k page size, so at least
> the comment above needs to be updated to reflect this fact.
> 

Yeah, I will change the comments like below in next revision.

  /*
   * When running this test with 32k memslots, actually 32763 excluding
   * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
   * The last slot contains 0x19000 bytes memory. Hence, the maximum size
   * here is 0x32000 bytes.
   */

>>   #define MEM_TEST_MOVE_GPA_DEST        (MEM_GPA + MEM_SIZE)
>>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>>             "invalid move test region size");
> (...)
>> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>>   }
>>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>> -               void *guest_code, uint64_t mempages,
>> +               void *guest_code, uint64_t mem_size,
>>                  struct timespec *slot_runtime)
>>   {
>> -    uint64_t rempages;
>> +    uint64_t mempages, rempages;
>>       uint64_t guest_addr;
>> -    uint32_t slot;
>> +    uint32_t slot, guest_page_size;
>>       struct timespec tstart;
>>       struct sync_area *sync;
>> -    TEST_ASSERT(mempages > 1,
>> -            "Can't test without any memory");
>> +    guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>> +    mempages = mem_size / guest_page_size;
>> +
>> +    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>> +    ucall_init(data->vm, NULL);
>>
> 
> TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
> here would catch the case if someone accidentally modifies
> __vm_create_with_one_vcpu() to use other page size than specified for
> VM_MODE_DEFAULT.
> 

Sure, it's not harmful at least.

>>       data->npages = mempages;
>> +    TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>>       data->nslots = nslots;
>> -    data->pages_per_slot = mempages / data->nslots;
>> +    data->pages_per_slot = data->npages / data->nslots;
>>       if (!data->pages_per_slot) {
>> -        *maxslots = mempages + 1;
>> +        *maxslots = data->npages + 1;
>>           return false;
>>       }
>> -    rempages = mempages % data->nslots;
>> +    rempages = data->npages % data->nslots;
>>       data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>>       TEST_ASSERT(data->hva_slots, "malloc() fail");
>> -    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>> -    ucall_init(data->vm, NULL);
>> -
>>       pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>>           data->nslots, data->pages_per_slot, rempages);
> (...)
>> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>>           pr_info("%d: %s\n", ctr, tests[ctr].name);
>>   }
>> +static bool check_memory_sizes(void)
>> +{
>> +    uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>> +
>> +    if (MEM_SIZE % guest_page_size ||
>> +        MEM_TEST_SIZE % guest_page_size) {
>> +        pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
>> +        return false;
>> +    }
>> +
>> +    if (MEM_SIZE_MAP % guest_page_size        ||
>> +        MEM_TEST_MAP_SIZE % guest_page_size        ||
>> +        (MEM_TEST_MAP_SIZE / guest_page_size) <= 2    ||
>> +        (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
>> +        pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
>> +        return false;
>> +    }
>> +
>> +    if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE        ||
>> +        MEM_TEST_UNMAP_SIZE % guest_page_size    ||
>> +        (MEM_TEST_UNMAP_SIZE / guest_page_size) %
>> +        (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
> 
> This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
> to match the old static_assert().
> 

Nice catch! I will fix it up in next revision :)

Thanks,
Gavin

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 8:46 AM, Gavin Shan wrote:
> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>> On 14.10.2022 09:19, Gavin Shan wrote:
>>> The test case is obviously broken on aarch64 because non-4KB guest
>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>> 16KB or 64KB.
>>>
>>> This supports variable guest page size, mostly for aarch64.
>>>
>>>    - The host determines the guest page size when virtual machine is
>>>      created. The value is also passed to guest through the synchronization
>>>      area.
>>>
>>>    - The number of guest pages are unknown until the virtual machine
>>>      is to be created. So all the related macros are dropped. Instead,
>>>      their values are dynamically calculated based on the guest page
>>>      size.
>>>
>>>    - The static checks on memory sizes and pages becomes dependent
>>>      on guest page size, which is unknown until the virtual machine
>>>      is about to be created. So all the static checks are converted
>>>      to dynamic checks, done in check_memory_sizes().
>>>
>>>    - As the address passed to madvise() should be aligned to host page,
>>>      the size of page chunk is automatically selected, other than one
>>>      page.
>>>
>>>    - All other changes included in this patch are almost mechanical
>>>      replacing '4096' with 'guest_page_size'.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> index d5aa9148f96f..d587bd952ff9 100644
>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> @@ -26,14 +26,11 @@
>>>   #include <processor.h>
>>>   #define MEM_SIZE        ((512U << 20) + 4096)
>>> -#define MEM_SIZE_PAGES        (MEM_SIZE / 4096)
>>>   #define MEM_GPA        0x10000000UL
>>>   #define MEM_AUX_GPA        MEM_GPA
>>>   #define MEM_SYNC_GPA        MEM_AUX_GPA
>>>   #define MEM_TEST_GPA        (MEM_AUX_GPA + 4096)
>>>   #define MEM_TEST_SIZE        (MEM_SIZE - 4096)
>>> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
>>> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>>   /*
>>>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
>>> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>>    * limited resolution).
>>>    */
>>>   #define MEM_SIZE_MAP        ((32U << 20) + 4096)
>>> -#define MEM_SIZE_MAP_PAGES    (MEM_SIZE_MAP / 4096)
>>>   #define MEM_TEST_MAP_SIZE    (MEM_SIZE_MAP - 4096)
>>> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
>>> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>>>   /*
>>>    * 128 MiB is min size that fills 32k slots with at least one page in each
>>>    * while at the same time gets 100+ iterations in such test
>>> + *
>>> + * 2 MiB chunk size like a typical huge page
>>>    */
>>>   #define MEM_TEST_UNMAP_SIZE        (128U << 20)
>>> -#define MEM_TEST_UNMAP_SIZE_PAGES    (MEM_TEST_UNMAP_SIZE / 4096)
>>> -/* 2 MiB chunk size like a typical huge page */
>>> -#define MEM_TEST_UNMAP_CHUNK_PAGES    (2U << (20 - 12))
>>> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
>>> -          "invalid unmap test region size");
>>> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
>>> -          "invalid unmap test region size");
>>> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>> -          (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
>>> -          "invalid unmap test region size");
>>> +#define MEM_TEST_UNMAP_CHUNK_SIZE    (2U << 20)
>>>   /*
>>>    * For the move active test the middle of the test area is placed on
>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>    * for the total size of 25 pages.
>>>    * Hence, the maximum size here is 50 pages.
>>>    */
>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>
>> The above number seems less readable than an explicit value of 50 pages.
>>
>> In addition to that, it's 50 pages only with 4k page size, so at least
>> the comment above needs to be updated to reflect this fact.
>>
> 
> Yeah, I will change the comments like below in next revision.
> 
>   /*
>    * When running this test with 32k memslots, actually 32763 excluding
>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>    * here is 0x32000 bytes.
>    */
> 

I will replace those numbers with readable ones like below :)

/*
  * When running this test with 32k memslots, actually 32763 excluding
  * the reserved memory slot 0, the memory for each slot is 16KB. The
  * last slot contains 100KB memory with the remaining 84KB. Hence,
  * the maximum size is double of that (200KB)
  */

>>>   #define MEM_TEST_MOVE_GPA_DEST        (MEM_GPA + MEM_SIZE)
>>>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>>>             "invalid move test region size");
>> (...)
>>> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>>>   }
>>>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>>> -               void *guest_code, uint64_t mempages,
>>> +               void *guest_code, uint64_t mem_size,
>>>                  struct timespec *slot_runtime)
>>>   {
>>> -    uint64_t rempages;
>>> +    uint64_t mempages, rempages;
>>>       uint64_t guest_addr;
>>> -    uint32_t slot;
>>> +    uint32_t slot, guest_page_size;
>>>       struct timespec tstart;
>>>       struct sync_area *sync;
>>> -    TEST_ASSERT(mempages > 1,
>>> -            "Can't test without any memory");
>>> +    guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>>> +    mempages = mem_size / guest_page_size;
>>> +
>>> +    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>>> +    ucall_init(data->vm, NULL);
>>>
>>
>> TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
>> here would catch the case if someone accidentally modifies
>> __vm_create_with_one_vcpu() to use other page size than specified for
>> VM_MODE_DEFAULT.
>>
> 
> Sure, it's not harmful at least.
> 
>>>       data->npages = mempages;
>>> +    TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>>>       data->nslots = nslots;
>>> -    data->pages_per_slot = mempages / data->nslots;
>>> +    data->pages_per_slot = data->npages / data->nslots;
>>>       if (!data->pages_per_slot) {
>>> -        *maxslots = mempages + 1;
>>> +        *maxslots = data->npages + 1;
>>>           return false;
>>>       }
>>> -    rempages = mempages % data->nslots;
>>> +    rempages = data->npages % data->nslots;
>>>       data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>>>       TEST_ASSERT(data->hva_slots, "malloc() fail");
>>> -    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>>> -    ucall_init(data->vm, NULL);
>>> -
>>>       pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>>>           data->nslots, data->pages_per_slot, rempages);
>> (...)
>>> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>>>           pr_info("%d: %s\n", ctr, tests[ctr].name);
>>>   }
>>> +static bool check_memory_sizes(void)
>>> +{
>>> +    uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>>> +
>>> +    if (MEM_SIZE % guest_page_size ||
>>> +        MEM_TEST_SIZE % guest_page_size) {
>>> +        pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    if (MEM_SIZE_MAP % guest_page_size        ||
>>> +        MEM_TEST_MAP_SIZE % guest_page_size        ||
>>> +        (MEM_TEST_MAP_SIZE / guest_page_size) <= 2    ||
>>> +        (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
>>> +        pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE        ||
>>> +        MEM_TEST_UNMAP_SIZE % guest_page_size    ||
>>> +        (MEM_TEST_UNMAP_SIZE / guest_page_size) %
>>> +        (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
>>
>> This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
>> to match the old static_assert().
>>
> 
> Nice catch! I will fix it up in next revision :)
> 

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 8:46 AM, Gavin Shan wrote:
> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>> On 14.10.2022 09:19, Gavin Shan wrote:
>>> The test case is obviously broken on aarch64 because non-4KB guest
>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>> 16KB or 64KB.
>>>
>>> This supports variable guest page size, mostly for aarch64.
>>>
>>>    - The host determines the guest page size when virtual machine is
>>>      created. The value is also passed to guest through the synchronization
>>>      area.
>>>
>>>    - The number of guest pages are unknown until the virtual machine
>>>      is to be created. So all the related macros are dropped. Instead,
>>>      their values are dynamically calculated based on the guest page
>>>      size.
>>>
>>>    - The static checks on memory sizes and pages becomes dependent
>>>      on guest page size, which is unknown until the virtual machine
>>>      is about to be created. So all the static checks are converted
>>>      to dynamic checks, done in check_memory_sizes().
>>>
>>>    - As the address passed to madvise() should be aligned to host page,
>>>      the size of page chunk is automatically selected, other than one
>>>      page.
>>>
>>>    - All other changes included in this patch are almost mechanical
>>>      replacing '4096' with 'guest_page_size'.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> index d5aa9148f96f..d587bd952ff9 100644
>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> @@ -26,14 +26,11 @@
>>>   #include <processor.h>
>>>   #define MEM_SIZE        ((512U << 20) + 4096)
>>> -#define MEM_SIZE_PAGES        (MEM_SIZE / 4096)
>>>   #define MEM_GPA        0x10000000UL
>>>   #define MEM_AUX_GPA        MEM_GPA
>>>   #define MEM_SYNC_GPA        MEM_AUX_GPA
>>>   #define MEM_TEST_GPA        (MEM_AUX_GPA + 4096)
>>>   #define MEM_TEST_SIZE        (MEM_SIZE - 4096)
>>> -static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
>>> -static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>>   /*
>>>    * 32 MiB is max size that gets well over 100 iterations on 509 slots.
>>> @@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
>>>    * limited resolution).
>>>    */
>>>   #define MEM_SIZE_MAP        ((32U << 20) + 4096)
>>> -#define MEM_SIZE_MAP_PAGES    (MEM_SIZE_MAP / 4096)
>>>   #define MEM_TEST_MAP_SIZE    (MEM_SIZE_MAP - 4096)
>>> -#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
>>> -static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
>>> -static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
>>>   /*
>>>    * 128 MiB is min size that fills 32k slots with at least one page in each
>>>    * while at the same time gets 100+ iterations in such test
>>> + *
>>> + * 2 MiB chunk size like a typical huge page
>>>    */
>>>   #define MEM_TEST_UNMAP_SIZE        (128U << 20)
>>> -#define MEM_TEST_UNMAP_SIZE_PAGES    (MEM_TEST_UNMAP_SIZE / 4096)
>>> -/* 2 MiB chunk size like a typical huge page */
>>> -#define MEM_TEST_UNMAP_CHUNK_PAGES    (2U << (20 - 12))
>>> -static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
>>> -          "invalid unmap test region size");
>>> -static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
>>> -          "invalid unmap test region size");
>>> -static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>> -          (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
>>> -          "invalid unmap test region size");
>>> +#define MEM_TEST_UNMAP_CHUNK_SIZE    (2U << 20)
>>>   /*
>>>    * For the move active test the middle of the test area is placed on
>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>    * for the total size of 25 pages.
>>>    * Hence, the maximum size here is 50 pages.
>>>    */
>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>
>> The above number seems less readable than an explicit value of 50 pages.
>>
>> In addition to that, it's 50 pages only with 4k page size, so at least
>> the comment above needs to be updated to reflect this fact.
>>
> 
> Yeah, I will change the comments like below in next revision.
> 
>   /*
>    * When running this test with 32k memslots, actually 32763 excluding
>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>    * here is 0x32000 bytes.
>    */
> 

I will replace those numbers with readable ones like below :)

/*
  * When running this test with 32k memslots, actually 32763 excluding
  * the reserved memory slot 0, the memory for each slot is 16KB. The
  * last slot contains 100KB memory with the remaining 84KB. Hence,
  * the maximum size is double of that (200KB)
  */

>>>   #define MEM_TEST_MOVE_GPA_DEST        (MEM_GPA + MEM_SIZE)
>>>   static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
>>>             "invalid move test region size");
>> (...)
>>> @@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
>>>   }
>>>   static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
>>> -               void *guest_code, uint64_t mempages,
>>> +               void *guest_code, uint64_t mem_size,
>>>                  struct timespec *slot_runtime)
>>>   {
>>> -    uint64_t rempages;
>>> +    uint64_t mempages, rempages;
>>>       uint64_t guest_addr;
>>> -    uint32_t slot;
>>> +    uint32_t slot, guest_page_size;
>>>       struct timespec tstart;
>>>       struct sync_area *sync;
>>> -    TEST_ASSERT(mempages > 1,
>>> -            "Can't test without any memory");
>>> +    guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>>> +    mempages = mem_size / guest_page_size;
>>> +
>>> +    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>>> +    ucall_init(data->vm, NULL);
>>>
>>
>> TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
>> here would catch the case if someone accidentally modifies
>> __vm_create_with_one_vcpu() to use other page size than specified for
>> VM_MODE_DEFAULT.
>>
> 
> Sure, it's not harmful at least.
> 
>>>       data->npages = mempages;
>>> +    TEST_ASSERT(data->npages > 1, "Can't test without any memory");
>>>       data->nslots = nslots;
>>> -    data->pages_per_slot = mempages / data->nslots;
>>> +    data->pages_per_slot = data->npages / data->nslots;
>>>       if (!data->pages_per_slot) {
>>> -        *maxslots = mempages + 1;
>>> +        *maxslots = data->npages + 1;
>>>           return false;
>>>       }
>>> -    rempages = mempages % data->nslots;
>>> +    rempages = data->npages % data->nslots;
>>>       data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
>>>       TEST_ASSERT(data->hva_slots, "malloc() fail");
>>> -    data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
>>> -    ucall_init(data->vm, NULL);
>>> -
>>>       pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
>>>           data->nslots, data->pages_per_slot, rempages);
>> (...)
>>> @@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
>>>           pr_info("%d: %s\n", ctr, tests[ctr].name);
>>>   }
>>> +static bool check_memory_sizes(void)
>>> +{
>>> +    uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
>>> +
>>> +    if (MEM_SIZE % guest_page_size ||
>>> +        MEM_TEST_SIZE % guest_page_size) {
>>> +        pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    if (MEM_SIZE_MAP % guest_page_size        ||
>>> +        MEM_TEST_MAP_SIZE % guest_page_size        ||
>>> +        (MEM_TEST_MAP_SIZE / guest_page_size) <= 2    ||
>>> +        (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
>>> +        pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE        ||
>>> +        MEM_TEST_UNMAP_SIZE % guest_page_size    ||
>>> +        (MEM_TEST_UNMAP_SIZE / guest_page_size) %
>>> +        (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
>>
>> This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
>> to match the old static_assert().
>>
> 
> Nice catch! I will fix it up in next revision :)
> 

Thanks,
Gavin

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 18.10.2022 02:51, Gavin Shan wrote:
> On 10/18/22 8:46 AM, Gavin Shan wrote:
>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>> 16KB or 64KB.
>>>>
>>>> This supports variable guest page size, mostly for aarch64.
>>>>
>>>>    - The host determines the guest page size when virtual machine is
>>>>      created. The value is also passed to guest through the synchronization
>>>>      area.
>>>>
>>>>    - The number of guest pages are unknown until the virtual machine
>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>      their values are dynamically calculated based on the guest page
>>>>      size.
>>>>
>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>      on guest page size, which is unknown until the virtual machine
>>>>      is about to be created. So all the static checks are converted
>>>>      to dynamic checks, done in check_memory_sizes().
>>>>
>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>      the size of page chunk is automatically selected, other than one
>>>>      page.
>>>>
>>>>    - All other changes included in this patch are almost mechanical
>>>>      replacing '4096' with 'guest_page_size'.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
(...)
>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>    * for the total size of 25 pages.
>>>>    * Hence, the maximum size here is 50 pages.
>>>>    */
>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>
>>> The above number seems less readable than an explicit value of 50 pages.
>>>
>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>> the comment above needs to be updated to reflect this fact.
>>>
>>
>> Yeah, I will change the comments like below in next revision.
>>
>>   /*
>>    * When running this test with 32k memslots, actually 32763 excluding
>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>    * here is 0x32000 bytes.
>>    */
>>
> 
> I will replace those numbers with readable ones like below :)
> 
> /*
>   * When running this test with 32k memslots, actually 32763 excluding
>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>   * the maximum size is double of that (200KB)
>   */

Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
defined as 3.

As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
this arch has 32766 slot available for the test memory.

Quick calculations show that this will result in 112 KiB of memory in
the last slot for 4 KiB page size (while for 64 KiB page size the
maximum slot count for this test is 8192 anyway - not counting slot 0).

> Thanks,
> Gavin
> 

Thanks,
Maciej

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 18.10.2022 02:51, Gavin Shan wrote:
> On 10/18/22 8:46 AM, Gavin Shan wrote:
>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>> 16KB or 64KB.
>>>>
>>>> This supports variable guest page size, mostly for aarch64.
>>>>
>>>>    - The host determines the guest page size when virtual machine is
>>>>      created. The value is also passed to guest through the synchronization
>>>>      area.
>>>>
>>>>    - The number of guest pages are unknown until the virtual machine
>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>      their values are dynamically calculated based on the guest page
>>>>      size.
>>>>
>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>      on guest page size, which is unknown until the virtual machine
>>>>      is about to be created. So all the static checks are converted
>>>>      to dynamic checks, done in check_memory_sizes().
>>>>
>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>      the size of page chunk is automatically selected, other than one
>>>>      page.
>>>>
>>>>    - All other changes included in this patch are almost mechanical
>>>>      replacing '4096' with 'guest_page_size'.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
(...)
>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>    * for the total size of 25 pages.
>>>>    * Hence, the maximum size here is 50 pages.
>>>>    */
>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>
>>> The above number seems less readable than an explicit value of 50 pages.
>>>
>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>> the comment above needs to be updated to reflect this fact.
>>>
>>
>> Yeah, I will change the comments like below in next revision.
>>
>>   /*
>>    * When running this test with 32k memslots, actually 32763 excluding
>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>    * here is 0x32000 bytes.
>>    */
>>
> 
> I will replace those numbers with readable ones like below :)
> 
> /*
>   * When running this test with 32k memslots, actually 32763 excluding
>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>   * the maximum size is double of that (200KB)
>   */

Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
defined as 3.

As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
this arch has 32766 slot available for the test memory.

Quick calculations show that this will result in 112 KiB of memory in
the last slot for 4 KiB page size (while for 64 KiB page size the
maximum slot count for this test is 8192 anyway - not counting slot 0).

> Thanks,
> Gavin
> 

Thanks,
Maciej

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
> On 18.10.2022 02:51, Gavin Shan wrote:
>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>> 16KB or 64KB.
>>>>>
>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>
>>>>>    - The host determines the guest page size when virtual machine is
>>>>>      created. The value is also passed to guest through the synchronization
>>>>>      area.
>>>>>
>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>      their values are dynamically calculated based on the guest page
>>>>>      size.
>>>>>
>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>      is about to be created. So all the static checks are converted
>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>
>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>      the size of page chunk is automatically selected, other than one
>>>>>      page.
>>>>>
>>>>>    - All other changes included in this patch are almost mechanical
>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> (...)
>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>    * for the total size of 25 pages.
>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>    */
>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>
>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>
>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>> the comment above needs to be updated to reflect this fact.
>>>>
>>>
>>> Yeah, I will change the comments like below in next revision.
>>>
>>>   /*
>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>    * here is 0x32000 bytes.
>>>    */
>>>
>>
>> I will replace those numbers with readable ones like below :)
>>
>> /*
>>   * When running this test with 32k memslots, actually 32763 excluding
>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>   * the maximum size is double of that (200KB)
>>   */
> 
> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
> defined as 3.
> 
> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
> this arch has 32766 slot available for the test memory.
> 
> Quick calculations show that this will result in 112 KiB of memory in
> the last slot for 4 KiB page size (while for 64 KiB page size the
> maximum slot count for this test is 8192 anyway - not counting slot 0).
> 

It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
In this particular patch, we still have (512MB+4KB). How about to change
like below in this patch. In next patch, it's adjusted accordingly after
we have (512MB+64KB).

(1) In this patch, the comment is changed to as below

     /*
      * We have different number of memory slots, excluding the reserved
      * memory slot 0, on various architectures and configurations. The
      * memory size in this test is calculated by doubling the maximal
      * memory size in last memory slot, with alignment to the largest
      * supported page size (64KB).
      *
      * architecture   slots    memory-per-slot    memory-on-last-slot
      * --------------------------------------------------------------
      * x86-4KB        32763    16KB               100KB
      * arm64-4KB      32766    16KB               52KB
      * arm64-64KB     8192     64KB               64KB
      */
     #define MEM_TEST_MOVE_SIZE	0x40000           /* 256KB */

(2) In the next patch, where we have (512MB+64KB) after the various
     memory sizes are consolidated, It is adjusted accordingly as below.

     /*
      * We have different number of memory slots, excluding the reserved
      * memory slot 0, on various architectures and configurations. The
      * memory size in this test is calculated by doubling the maximal
      * memory size in last memory slot, with alignment to the largest
      * supported page size (64KB).
      *
      * architecture   slots    memory-per-slot    memory-on-last-slot
      * --------------------------------------------------------------
      * x86-4KB        32763    16KB               160KB
      * arm64-4KB      32766    16KB               112KB
      * arm64-64KB     8192     64KB               128KB
      */
     #define MEM_TEST_MOVE_SIZE	0x50000           /* 320KB */

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
> On 18.10.2022 02:51, Gavin Shan wrote:
>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>> 16KB or 64KB.
>>>>>
>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>
>>>>>    - The host determines the guest page size when virtual machine is
>>>>>      created. The value is also passed to guest through the synchronization
>>>>>      area.
>>>>>
>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>      their values are dynamically calculated based on the guest page
>>>>>      size.
>>>>>
>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>      is about to be created. So all the static checks are converted
>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>
>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>      the size of page chunk is automatically selected, other than one
>>>>>      page.
>>>>>
>>>>>    - All other changes included in this patch are almost mechanical
>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> (...)
>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>    * for the total size of 25 pages.
>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>    */
>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>
>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>
>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>> the comment above needs to be updated to reflect this fact.
>>>>
>>>
>>> Yeah, I will change the comments like below in next revision.
>>>
>>>   /*
>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>    * here is 0x32000 bytes.
>>>    */
>>>
>>
>> I will replace those numbers with readable ones like below :)
>>
>> /*
>>   * When running this test with 32k memslots, actually 32763 excluding
>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>   * the maximum size is double of that (200KB)
>>   */
> 
> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
> defined as 3.
> 
> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
> this arch has 32766 slot available for the test memory.
> 
> Quick calculations show that this will result in 112 KiB of memory in
> the last slot for 4 KiB page size (while for 64 KiB page size the
> maximum slot count for this test is 8192 anyway - not counting slot 0).
> 

It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
In this particular patch, we still have (512MB+4KB). How about to change
like below in this patch. In next patch, it's adjusted accordingly after
we have (512MB+64KB).

(1) In this patch, the comment is changed to as below

     /*
      * We have different number of memory slots, excluding the reserved
      * memory slot 0, on various architectures and configurations. The
      * memory size in this test is calculated by doubling the maximal
      * memory size in last memory slot, with alignment to the largest
      * supported page size (64KB).
      *
      * architecture   slots    memory-per-slot    memory-on-last-slot
      * --------------------------------------------------------------
      * x86-4KB        32763    16KB               100KB
      * arm64-4KB      32766    16KB               52KB
      * arm64-64KB     8192     64KB               64KB
      */
     #define MEM_TEST_MOVE_SIZE	0x40000           /* 256KB */

(2) In the next patch, where we have (512MB+64KB) after the various
     memory sizes are consolidated, It is adjusted accordingly as below.

     /*
      * We have different number of memory slots, excluding the reserved
      * memory slot 0, on various architectures and configurations. The
      * memory size in this test is calculated by doubling the maximal
      * memory size in last memory slot, with alignment to the largest
      * supported page size (64KB).
      *
      * architecture   slots    memory-per-slot    memory-on-last-slot
      * --------------------------------------------------------------
      * x86-4KB        32763    16KB               160KB
      * arm64-4KB      32766    16KB               112KB
      * arm64-64KB     8192     64KB               128KB
      */
     #define MEM_TEST_MOVE_SIZE	0x50000           /* 320KB */

Thanks,
Gavin

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 19.10.2022 02:26, Gavin Shan wrote:
> On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
>> On 18.10.2022 02:51, Gavin Shan wrote:
>>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>>> 16KB or 64KB.
>>>>>>
>>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>>
>>>>>>    - The host determines the guest page size when virtual machine is
>>>>>>      created. The value is also passed to guest through the synchronization
>>>>>>      area.
>>>>>>
>>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>>      their values are dynamically calculated based on the guest page
>>>>>>      size.
>>>>>>
>>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>>      is about to be created. So all the static checks are converted
>>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>>
>>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>>      the size of page chunk is automatically selected, other than one
>>>>>>      page.
>>>>>>
>>>>>>    - All other changes included in this patch are almost mechanical
>>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>> ---
>>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> (...)
>>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>>    * for the total size of 25 pages.
>>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>>    */
>>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>>
>>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>>
>>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>>> the comment above needs to be updated to reflect this fact.
>>>>>
>>>>
>>>> Yeah, I will change the comments like below in next revision.
>>>>
>>>>   /*
>>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>>    * here is 0x32000 bytes.
>>>>    */
>>>>
>>>
>>> I will replace those numbers with readable ones like below :)
>>>
>>> /*
>>>   * When running this test with 32k memslots, actually 32763 excluding
>>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>>   * the maximum size is double of that (200KB)
>>>   */
>>
>> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
>> defined as 3.
>>
>> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
>> this arch has 32766 slot available for the test memory.
>>
>> Quick calculations show that this will result in 112 KiB of memory in
>> the last slot for 4 KiB page size (while for 64 KiB page size the
>> maximum slot count for this test is 8192 anyway - not counting slot 0).
>>
> 
> It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
> In this particular patch, we still have (512MB+4KB). How about to change
> like below in this patch. In next patch, it's adjusted accordingly after
> we have (512MB+64KB).

My review comment above referred to the final MEM_SIZE value after the
whole series, so 512 MiB + 64 KiB.

I placed that review comment on patch 4 since it's the only patch in this
series that modified the code comment about MEM_TEST_MOVE_SIZE.

> 
> (1) In this patch, the comment is changed to as below
> 
>      /*
>       * We have different number of memory slots, excluding the reserved
>       * memory slot 0, on various architectures and configurations. The
>       * memory size in this test is calculated by doubling the maximal
>       * memory size in last memory slot, with alignment to the largest
>       * supported page size (64KB).
>       *
>       * architecture   slots    memory-per-slot    memory-on-last-slot
>       * --------------------------------------------------------------
>       * x86-4KB        32763    16KB               100KB
>       * arm64-4KB      32766    16KB               52KB
>       * arm64-64KB     8192     64KB               64KB
>       */
>      #define MEM_TEST_MOVE_SIZE    0x40000           /* 256KB */
>
> (2) In the next patch, where we have (512MB+64KB) after the various
>      memory sizes are consolidated, It is adjusted accordingly as below.
> 
>      /*
>       * We have different number of memory slots, excluding the reserved
>       * memory slot 0, on various architectures and configurations. The
>       * memory size in this test is calculated by doubling the maximal
>       * memory size in last memory slot, with alignment to the largest
>       * supported page size (64KB).
>       *
>       * architecture   slots    memory-per-slot    memory-on-last-slot
>       * --------------------------------------------------------------
>       * x86-4KB        32763    16KB               160KB
>       * arm64-4KB      32766    16KB               112KB
>       * arm64-64KB     8192     64KB               128KB
>       */
>      #define MEM_TEST_MOVE_SIZE    0x50000           /* 320KB */

Now MEM_TEST_MOVE_SIZE is too high for arm64-4KB and arm64-64KB cases
(it needs 160 KiB in the last slot but has less available in these two
cases).

Using a test size of 192 KiB instead seems like a small difference
from the original size of 200 KiB, while still being aligned to
64 KiB.

The move benchmarks runtime difference on x86-4KB with this size
(compared to sizes of 200 KiB and 320 KiB) seems to be negligible.

Since it's an odd number of 64 KiB pages (3) the code that halves
this number of pages will need to be adjusted to operate on raw
sizes instead.

I can see a single block of code that will need such adjustment:
> if (lastpages < move_pages / 2) {
>         *maxslots = 0;
>         return false;
> }   

Similar remark goes for the case (1) above, where you'll probably need
to use 64 KiB test area size (it's only an intermediate form of code
before the final patch changes this value so it's fine if it doesn't
perform as well as the final form of the code).

> Thanks,
> Gavin
> 

Thanks,
Maciej

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Maciej S. Szmigiero]

On 19.10.2022 02:26, Gavin Shan wrote:
> On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
>> On 18.10.2022 02:51, Gavin Shan wrote:
>>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>>> 16KB or 64KB.
>>>>>>
>>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>>
>>>>>>    - The host determines the guest page size when virtual machine is
>>>>>>      created. The value is also passed to guest through the synchronization
>>>>>>      area.
>>>>>>
>>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>>      their values are dynamically calculated based on the guest page
>>>>>>      size.
>>>>>>
>>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>>      is about to be created. So all the static checks are converted
>>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>>
>>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>>      the size of page chunk is automatically selected, other than one
>>>>>>      page.
>>>>>>
>>>>>>    - All other changes included in this patch are almost mechanical
>>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>> ---
>>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> (...)
>>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>>    * for the total size of 25 pages.
>>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>>    */
>>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>>
>>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>>
>>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>>> the comment above needs to be updated to reflect this fact.
>>>>>
>>>>
>>>> Yeah, I will change the comments like below in next revision.
>>>>
>>>>   /*
>>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>>    * here is 0x32000 bytes.
>>>>    */
>>>>
>>>
>>> I will replace those numbers with readable ones like below :)
>>>
>>> /*
>>>   * When running this test with 32k memslots, actually 32763 excluding
>>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>>   * the maximum size is double of that (200KB)
>>>   */
>>
>> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
>> defined as 3.
>>
>> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
>> this arch has 32766 slot available for the test memory.
>>
>> Quick calculations show that this will result in 112 KiB of memory in
>> the last slot for 4 KiB page size (while for 64 KiB page size the
>> maximum slot count for this test is 8192 anyway - not counting slot 0).
>>
> 
> It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
> In this particular patch, we still have (512MB+4KB). How about to change
> like below in this patch. In next patch, it's adjusted accordingly after
> we have (512MB+64KB).

My review comment above referred to the final MEM_SIZE value after the
whole series, so 512 MiB + 64 KiB.

I placed that review comment on patch 4 since it's the only patch in this
series that modified the code comment about MEM_TEST_MOVE_SIZE.

> 
> (1) In this patch, the comment is changed to as below
> 
>      /*
>       * We have different number of memory slots, excluding the reserved
>       * memory slot 0, on various architectures and configurations. The
>       * memory size in this test is calculated by doubling the maximal
>       * memory size in last memory slot, with alignment to the largest
>       * supported page size (64KB).
>       *
>       * architecture   slots    memory-per-slot    memory-on-last-slot
>       * --------------------------------------------------------------
>       * x86-4KB        32763    16KB               100KB
>       * arm64-4KB      32766    16KB               52KB
>       * arm64-64KB     8192     64KB               64KB
>       */
>      #define MEM_TEST_MOVE_SIZE    0x40000           /* 256KB */
>
> (2) In the next patch, where we have (512MB+64KB) after the various
>      memory sizes are consolidated, It is adjusted accordingly as below.
> 
>      /*
>       * We have different number of memory slots, excluding the reserved
>       * memory slot 0, on various architectures and configurations. The
>       * memory size in this test is calculated by doubling the maximal
>       * memory size in last memory slot, with alignment to the largest
>       * supported page size (64KB).
>       *
>       * architecture   slots    memory-per-slot    memory-on-last-slot
>       * --------------------------------------------------------------
>       * x86-4KB        32763    16KB               160KB
>       * arm64-4KB      32766    16KB               112KB
>       * arm64-64KB     8192     64KB               128KB
>       */
>      #define MEM_TEST_MOVE_SIZE    0x50000           /* 320KB */

Now MEM_TEST_MOVE_SIZE is too high for arm64-4KB and arm64-64KB cases
(it needs 160 KiB in the last slot but has less available in these two
cases).

Using a test size of 192 KiB instead seems like a small difference
from the original size of 200 KiB, while still being aligned to
64 KiB.

The move benchmarks runtime difference on x86-4KB with this size
(compared to sizes of 200 KiB and 320 KiB) seems to be negligible.

Since it's an odd number of 64 KiB pages (3) the code that halves
this number of pages will need to be adjusted to operate on raw
sizes instead.

I can see a single block of code that will need such adjustment:
> if (lastpages < move_pages / 2) {
>         *maxslots = 0;
>         return false;
> }   

Similar remark goes for the case (1) above, where you'll probably need
to use 64 KiB test area size (it's only an intermediate form of code
before the final patch changes this value so it's fine if it doesn't
perform as well as the final form of the code).

> Thanks,
> Gavin
> 

Thanks,
Maciej

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/20/22 4:18 AM, Maciej S. Szmigiero wrote:
> On 19.10.2022 02:26, Gavin Shan wrote:
>> On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
>>> On 18.10.2022 02:51, Gavin Shan wrote:
>>>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>>>> 16KB or 64KB.
>>>>>>>
>>>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>>>
>>>>>>>    - The host determines the guest page size when virtual machine is
>>>>>>>      created. The value is also passed to guest through the synchronization
>>>>>>>      area.
>>>>>>>
>>>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>>>      their values are dynamically calculated based on the guest page
>>>>>>>      size.
>>>>>>>
>>>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>>>      is about to be created. So all the static checks are converted
>>>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>>>
>>>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>>>      the size of page chunk is automatically selected, other than one
>>>>>>>      page.
>>>>>>>
>>>>>>>    - All other changes included in this patch are almost mechanical
>>>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>>>
>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>>> ---
>>>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> (...)
>>>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>>>    * for the total size of 25 pages.
>>>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>>>    */
>>>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>>>
>>>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>>>
>>>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>>>> the comment above needs to be updated to reflect this fact.
>>>>>>
>>>>>
>>>>> Yeah, I will change the comments like below in next revision.
>>>>>
>>>>>   /*
>>>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>>>    * here is 0x32000 bytes.
>>>>>    */
>>>>>
>>>>
>>>> I will replace those numbers with readable ones like below :)
>>>>
>>>> /*
>>>>   * When running this test with 32k memslots, actually 32763 excluding
>>>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>>>   * the maximum size is double of that (200KB)
>>>>   */
>>>
>>> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
>>> defined as 3.
>>>
>>> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
>>> this arch has 32766 slot available for the test memory.
>>>
>>> Quick calculations show that this will result in 112 KiB of memory in
>>> the last slot for 4 KiB page size (while for 64 KiB page size the
>>> maximum slot count for this test is 8192 anyway - not counting slot 0).
>>>
>>
>> It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
>> In this particular patch, we still have (512MB+4KB). How about to change
>> like below in this patch. In next patch, it's adjusted accordingly after
>> we have (512MB+64KB).
> 
> My review comment above referred to the final MEM_SIZE value after the
> whole series, so 512 MiB + 64 KiB.
> 
> I placed that review comment on patch 4 since it's the only patch in this
> series that modified the code comment about MEM_TEST_MOVE_SIZE.
> 
>>
>> (1) In this patch, the comment is changed to as below
>>
>>      /*
>>       * We have different number of memory slots, excluding the reserved
>>       * memory slot 0, on various architectures and configurations. The
>>       * memory size in this test is calculated by doubling the maximal
>>       * memory size in last memory slot, with alignment to the largest
>>       * supported page size (64KB).
>>       *
>>       * architecture   slots    memory-per-slot    memory-on-last-slot
>>       * --------------------------------------------------------------
>>       * x86-4KB        32763    16KB               100KB
>>       * arm64-4KB      32766    16KB               52KB
>>       * arm64-64KB     8192     64KB               64KB
>>       */
>>      #define MEM_TEST_MOVE_SIZE    0x40000           /* 256KB */
>>
>> (2) In the next patch, where we have (512MB+64KB) after the various
>>      memory sizes are consolidated, It is adjusted accordingly as below.
>>
>>      /*
>>       * We have different number of memory slots, excluding the reserved
>>       * memory slot 0, on various architectures and configurations. The
>>       * memory size in this test is calculated by doubling the maximal
>>       * memory size in last memory slot, with alignment to the largest
>>       * supported page size (64KB).
>>       *
>>       * architecture   slots    memory-per-slot    memory-on-last-slot
>>       * --------------------------------------------------------------
>>       * x86-4KB        32763    16KB               160KB
>>       * arm64-4KB      32766    16KB               112KB
>>       * arm64-64KB     8192     64KB               128KB
>>       */
>>      #define MEM_TEST_MOVE_SIZE    0x50000           /* 320KB */
> 
> Now MEM_TEST_MOVE_SIZE is too high for arm64-4KB and arm64-64KB cases
> (it needs 160 KiB in the last slot but has less available in these two
> cases).
> 
> Using a test size of 192 KiB instead seems like a small difference
> from the original size of 200 KiB, while still being aligned to
> 64 KiB.
> 
> The move benchmarks runtime difference on x86-4KB with this size
> (compared to sizes of 200 KiB and 320 KiB) seems to be negligible.
> 
> Since it's an odd number of 64 KiB pages (3) the code that halves
> this number of pages will need to be adjusted to operate on raw
> sizes instead.
> 
> I can see a single block of code that will need such adjustment:
>> if (lastpages < move_pages / 2) {
>>         *maxslots = 0;
>>         return false;
>> } 
> 
> Similar remark goes for the case (1) above, where you'll probably need
> to use 64 KiB test area size (it's only an intermediate form of code
> before the final patch changes this value so it's fine if it doesn't
> perform as well as the final form of the code).
> 

Maciej, all your comments make sense to me. It really took me some times
to do the calculation. I just posted v3 to address all your comments.
Hopefully, there is nothing missed. Please go ahead to review v3 directly
when you get a chance.

    v3: https://lore.kernel.org/kvmarm/20221020071209.559062-1-gshan@redhat.com/T/#t

In v3, the comments about MEM_TEST_MOVE_SIZE is fixed in PATCH[v3 4/6],
but it's 64KB. In PATCH[v3 5/6], it's fixed up to 192KB and memory size
is used for the comparison in test_memslot_move_prepare().

Thanks,
Gavin


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size

[Gavin Shan]

On 10/20/22 4:18 AM, Maciej S. Szmigiero wrote:
> On 19.10.2022 02:26, Gavin Shan wrote:
>> On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote:
>>> On 18.10.2022 02:51, Gavin Shan wrote:
>>>> On 10/18/22 8:46 AM, Gavin Shan wrote:
>>>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
>>>>>> On 14.10.2022 09:19, Gavin Shan wrote:
>>>>>>> The test case is obviously broken on aarch64 because non-4KB guest
>>>>>>> page size is supported. The guest page size on aarch64 could be 4KB,
>>>>>>> 16KB or 64KB.
>>>>>>>
>>>>>>> This supports variable guest page size, mostly for aarch64.
>>>>>>>
>>>>>>>    - The host determines the guest page size when virtual machine is
>>>>>>>      created. The value is also passed to guest through the synchronization
>>>>>>>      area.
>>>>>>>
>>>>>>>    - The number of guest pages are unknown until the virtual machine
>>>>>>>      is to be created. So all the related macros are dropped. Instead,
>>>>>>>      their values are dynamically calculated based on the guest page
>>>>>>>      size.
>>>>>>>
>>>>>>>    - The static checks on memory sizes and pages becomes dependent
>>>>>>>      on guest page size, which is unknown until the virtual machine
>>>>>>>      is about to be created. So all the static checks are converted
>>>>>>>      to dynamic checks, done in check_memory_sizes().
>>>>>>>
>>>>>>>    - As the address passed to madvise() should be aligned to host page,
>>>>>>>      the size of page chunk is automatically selected, other than one
>>>>>>>      page.
>>>>>>>
>>>>>>>    - All other changes included in this patch are almost mechanical
>>>>>>>      replacing '4096' with 'guest_page_size'.
>>>>>>>
>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>>> ---
>>>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
>>>>>>>   1 file changed, 115 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>>> index d5aa9148f96f..d587bd952ff9 100644
>>>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>>>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>>> (...)
>>>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
>>>>>>>    * for the total size of 25 pages.
>>>>>>>    * Hence, the maximum size here is 50 pages.
>>>>>>>    */
>>>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50)
>>>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096)
>>>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000
>>>>>>
>>>>>> The above number seems less readable than an explicit value of 50 pages.
>>>>>>
>>>>>> In addition to that, it's 50 pages only with 4k page size, so at least
>>>>>> the comment above needs to be updated to reflect this fact.
>>>>>>
>>>>>
>>>>> Yeah, I will change the comments like below in next revision.
>>>>>
>>>>>   /*
>>>>>    * When running this test with 32k memslots, actually 32763 excluding
>>>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
>>>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size
>>>>>    * here is 0x32000 bytes.
>>>>>    */
>>>>>
>>>>
>>>> I will replace those numbers with readable ones like below :)
>>>>
>>>> /*
>>>>   * When running this test with 32k memslots, actually 32763 excluding
>>>>   * the reserved memory slot 0, the memory for each slot is 16KB. The
>>>>   * last slot contains 100KB memory with the remaining 84KB. Hence,
>>>>   * the maximum size is double of that (200KB)
>>>>   */
>>>
>>> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS
>>> defined as 3.
>>>
>>> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so
>>> this arch has 32766 slot available for the test memory.
>>>
>>> Quick calculations show that this will result in 112 KiB of memory in
>>> the last slot for 4 KiB page size (while for 64 KiB page size the
>>> maximum slot count for this test is 8192 anyway - not counting slot 0).
>>>
>>
>> It seems your calculation had (512MB+64KB), instead of (512MB+4KB).
>> In this particular patch, we still have (512MB+4KB). How about to change
>> like below in this patch. In next patch, it's adjusted accordingly after
>> we have (512MB+64KB).
> 
> My review comment above referred to the final MEM_SIZE value after the
> whole series, so 512 MiB + 64 KiB.
> 
> I placed that review comment on patch 4 since it's the only patch in this
> series that modified the code comment about MEM_TEST_MOVE_SIZE.
> 
>>
>> (1) In this patch, the comment is changed to as below
>>
>>      /*
>>       * We have different number of memory slots, excluding the reserved
>>       * memory slot 0, on various architectures and configurations. The
>>       * memory size in this test is calculated by doubling the maximal
>>       * memory size in last memory slot, with alignment to the largest
>>       * supported page size (64KB).
>>       *
>>       * architecture   slots    memory-per-slot    memory-on-last-slot
>>       * --------------------------------------------------------------
>>       * x86-4KB        32763    16KB               100KB
>>       * arm64-4KB      32766    16KB               52KB
>>       * arm64-64KB     8192     64KB               64KB
>>       */
>>      #define MEM_TEST_MOVE_SIZE    0x40000           /* 256KB */
>>
>> (2) In the next patch, where we have (512MB+64KB) after the various
>>      memory sizes are consolidated, It is adjusted accordingly as below.
>>
>>      /*
>>       * We have different number of memory slots, excluding the reserved
>>       * memory slot 0, on various architectures and configurations. The
>>       * memory size in this test is calculated by doubling the maximal
>>       * memory size in last memory slot, with alignment to the largest
>>       * supported page size (64KB).
>>       *
>>       * architecture   slots    memory-per-slot    memory-on-last-slot
>>       * --------------------------------------------------------------
>>       * x86-4KB        32763    16KB               160KB
>>       * arm64-4KB      32766    16KB               112KB
>>       * arm64-64KB     8192     64KB               128KB
>>       */
>>      #define MEM_TEST_MOVE_SIZE    0x50000           /* 320KB */
> 
> Now MEM_TEST_MOVE_SIZE is too high for arm64-4KB and arm64-64KB cases
> (it needs 160 KiB in the last slot but has less available in these two
> cases).
> 
> Using a test size of 192 KiB instead seems like a small difference
> from the original size of 200 KiB, while still being aligned to
> 64 KiB.
> 
> The move benchmarks runtime difference on x86-4KB with this size
> (compared to sizes of 200 KiB and 320 KiB) seems to be negligible.
> 
> Since it's an odd number of 64 KiB pages (3) the code that halves
> this number of pages will need to be adjusted to operate on raw
> sizes instead.
> 
> I can see a single block of code that will need such adjustment:
>> if (lastpages < move_pages / 2) {
>>         *maxslots = 0;
>>         return false;
>> } 
> 
> Similar remark goes for the case (1) above, where you'll probably need
> to use 64 KiB test area size (it's only an intermediate form of code
> before the final patch changes this value so it's fine if it doesn't
> perform as well as the final form of the code).
> 

Maciej, all your comments make sense to me. It really took me some times
to do the calculation. I just posted v3 to address all your comments.
Hopefully, there is nothing missed. Please go ahead to review v3 directly
when you get a chance.

    v3: https://lore.kernel.org/kvmarm/20221020071209.559062-1-gshan@redhat.com/T/#t

In v3, the comments about MEM_TEST_MOVE_SIZE is fixed in PATCH[v3 4/6],
but it's 64KB. In PATCH[v3 5/6], it's fixed up to 192KB and memory size
is used for the comparison in test_memslot_move_prepare().

Thanks,
Gavin

[PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
should be aligned to host page size, which can be 64KB on aarch64. So it's
wrong by passing additional fixed 4KB memory area to various tests.

Fix it by passing additional fixed 64KB memory area to various tests. After
it's applied, the following command works fine on 64KB-page-size-host and
4KB-page-size-guest.

  # ./memslot_perf_test -v -s 512

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index d587bd952ff9..e6d34744b45d 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -25,12 +25,14 @@
 #include <kvm_util.h>
 #include <processor.h>
 
-#define MEM_SIZE		((512U << 20) + 4096)
-#define MEM_GPA		0x10000000UL
+#define MEM_EXTRA_SIZE		0x10000
+
+#define MEM_SIZE		((512U << 20) + MEM_EXTRA_SIZE)
+#define MEM_GPA			0x10000000UL
 #define MEM_AUX_GPA		MEM_GPA
 #define MEM_SYNC_GPA		MEM_AUX_GPA
-#define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
-#define MEM_TEST_SIZE		(MEM_SIZE - 4096)
+#define MEM_TEST_GPA		(MEM_AUX_GPA + MEM_EXTRA_SIZE)
+#define MEM_TEST_SIZE		(MEM_SIZE - MEM_EXTRA_SIZE)
 
 /*
  * 32 MiB is max size that gets well over 100 iterations on 509 slots.
@@ -38,8 +40,8 @@
  * 8194 slots in use can then be tested (although with slightly
  * limited resolution).
  */
-#define MEM_SIZE_MAP		((32U << 20) + 4096)
-#define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
+#define MEM_SIZE_MAP		((32U << 20) + MEM_EXTRA_SIZE)
+#define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - MEM_EXTRA_SIZE)
 
 /*
  * 128 MiB is min size that fills 32k slots with at least one page in each
@@ -799,13 +801,13 @@ static const struct test_data tests[] = {
 	},
 	{
 		.name = "unmap",
-		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
+		.mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop,
 	},
 	{
 		.name = "unmap chunked",
-		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
+		.mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop_chunked,
 	},
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
should be aligned to host page size, which can be 64KB on aarch64. So it's
wrong by passing additional fixed 4KB memory area to various tests.

Fix it by passing additional fixed 64KB memory area to various tests. After
it's applied, the following command works fine on 64KB-page-size-host and
4KB-page-size-guest.

  # ./memslot_perf_test -v -s 512

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index d587bd952ff9..e6d34744b45d 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -25,12 +25,14 @@
 #include <kvm_util.h>
 #include <processor.h>
 
-#define MEM_SIZE		((512U << 20) + 4096)
-#define MEM_GPA		0x10000000UL
+#define MEM_EXTRA_SIZE		0x10000
+
+#define MEM_SIZE		((512U << 20) + MEM_EXTRA_SIZE)
+#define MEM_GPA			0x10000000UL
 #define MEM_AUX_GPA		MEM_GPA
 #define MEM_SYNC_GPA		MEM_AUX_GPA
-#define MEM_TEST_GPA		(MEM_AUX_GPA + 4096)
-#define MEM_TEST_SIZE		(MEM_SIZE - 4096)
+#define MEM_TEST_GPA		(MEM_AUX_GPA + MEM_EXTRA_SIZE)
+#define MEM_TEST_SIZE		(MEM_SIZE - MEM_EXTRA_SIZE)
 
 /*
  * 32 MiB is max size that gets well over 100 iterations on 509 slots.
@@ -38,8 +40,8 @@
  * 8194 slots in use can then be tested (although with slightly
  * limited resolution).
  */
-#define MEM_SIZE_MAP		((32U << 20) + 4096)
-#define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - 4096)
+#define MEM_SIZE_MAP		((32U << 20) + MEM_EXTRA_SIZE)
+#define MEM_TEST_MAP_SIZE	(MEM_SIZE_MAP - MEM_EXTRA_SIZE)
 
 /*
  * 128 MiB is min size that fills 32k slots with at least one page in each
@@ -799,13 +801,13 @@ static const struct test_data tests[] = {
 	},
 	{
 		.name = "unmap",
-		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
+		.mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop,
 	},
 	{
 		.name = "unmap chunked",
-		.mem_size = MEM_TEST_UNMAP_SIZE + 4096,
+		.mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE,
 		.guest_code = guest_code_test_memslot_unmap,
 		.loop = test_memslot_unmap_loop_chunked,
 	},
-- 
2.23.0

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
> should be aligned to host page size, which can be 64KB on aarch64. So it's
> wrong by passing additional fixed 4KB memory area to various tests.
> 
> Fix it by passing additional fixed 64KB memory area to various tests. After
> it's applied, the following command works fine on 64KB-page-size-host and
> 4KB-page-size-guest.
> 
>    # ./memslot_perf_test -v -s 512
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index d587bd952ff9..e6d34744b45d 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -25,12 +25,14 @@
>   #include <kvm_util.h>
>   #include <processor.h>
>   
> -#define MEM_SIZE		((512U << 20) + 4096)
> -#define MEM_GPA		0x10000000UL
> +#define MEM_EXTRA_SIZE		0x10000

So the biggest page size supported right now is 64 KiB - it would be
good to have an assert somewhere to explicitly check for this
(regardless of implicit checks present in other calculations).

Also, an expression like "(64 << 10)" is more readable than a "1"
with a tail of zeroes (it's easy to add one zero too many or be one
zero short).

Thanks,
Maciej

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Maciej S. Szmigiero]

On 14.10.2022 09:19, Gavin Shan wrote:
> The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
> should be aligned to host page size, which can be 64KB on aarch64. So it's
> wrong by passing additional fixed 4KB memory area to various tests.
> 
> Fix it by passing additional fixed 64KB memory area to various tests. After
> it's applied, the following command works fine on 64KB-page-size-host and
> 4KB-page-size-guest.
> 
>    # ./memslot_perf_test -v -s 512
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index d587bd952ff9..e6d34744b45d 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -25,12 +25,14 @@
>   #include <kvm_util.h>
>   #include <processor.h>
>   
> -#define MEM_SIZE		((512U << 20) + 4096)
> -#define MEM_GPA		0x10000000UL
> +#define MEM_EXTRA_SIZE		0x10000

So the biggest page size supported right now is 64 KiB - it would be
good to have an assert somewhere to explicitly check for this
(regardless of implicit checks present in other calculations).

Also, an expression like "(64 << 10)" is more readable than a "1"
with a tail of zeroes (it's easy to add one zero too many or be one
zero short).

Thanks,
Maciej

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Sean Christopherson]

On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > +#define MEM_EXTRA_SIZE		0x10000
>
> Also, an expression like "(64 << 10)" is more readable than a "1"
> with a tail of zeroes (it's easy to add one zero too many or be one
> zero short).

+1 to not open coding raw numbers.

I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
16KB, 64K, 2MB, 1GB, etc...

Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
math off of those.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Sean Christopherson]

On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > +#define MEM_EXTRA_SIZE		0x10000
>
> Also, an expression like "(64 << 10)" is more readable than a "1"
> with a tail of zeroes (it's easy to add one zero too many or be one
> zero short).

+1 to not open coding raw numbers.

I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
16KB, 64K, 2MB, 1GB, etc...

Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
math off of those.

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 6:08 AM, Sean Christopherson wrote:
> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>> +#define MEM_EXTRA_SIZE		0x10000
>>
>> Also, an expression like "(64 << 10)" is more readable than a "1"
>> with a tail of zeroes (it's easy to add one zero too many or be one
>> zero short).
> 
> +1 to not open coding raw numbers.
> 
> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> 16KB, 64K, 2MB, 1GB, etc...
> 
> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> math off of those.
> 

Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
if it looks good to you?

     #define KB         (1UL << 10)
     #define MB         (1UL << 20)
     #define GB         (1UL << 30)
     #define TB         (1UL << 40)

     /* Base page and huge page size */
     #define SIZE_4KB   (  4 * KB)
     #define SIZE_16KB  ( 16 * KB)
     #define SIZE_64KB  ( 64 * KB)
     #define SIZE_2MB   (  2 * MB)
     #define SIZE_32MB  ( 32 * MB)
     #define SIZE_512MB (512 * MB)
     #define SIZE_1GB   (  1 * GB)
     #define SIZE_16GB  ( 16 * GB)

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 6:08 AM, Sean Christopherson wrote:
> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>> +#define MEM_EXTRA_SIZE		0x10000
>>
>> Also, an expression like "(64 << 10)" is more readable than a "1"
>> with a tail of zeroes (it's easy to add one zero too many or be one
>> zero short).
> 
> +1 to not open coding raw numbers.
> 
> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> 16KB, 64K, 2MB, 1GB, etc...
> 
> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> math off of those.
> 

Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
if it looks good to you?

     #define KB         (1UL << 10)
     #define MB         (1UL << 20)
     #define GB         (1UL << 30)
     #define TB         (1UL << 40)

     /* Base page and huge page size */
     #define SIZE_4KB   (  4 * KB)
     #define SIZE_16KB  ( 16 * KB)
     #define SIZE_64KB  ( 64 * KB)
     #define SIZE_2MB   (  2 * MB)
     #define SIZE_32MB  ( 32 * MB)
     #define SIZE_512MB (512 * MB)
     #define SIZE_1GB   (  1 * GB)
     #define SIZE_16GB  ( 16 * GB)

Thanks,
Gavin

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Maciej S. Szmigiero]

On 18.10.2022 00:51, Gavin Shan wrote:
> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>
>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>> zero short).
>>
>> +1 to not open coding raw numbers.
>>
>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>> 16KB, 64K, 2MB, 1GB, etc...
>>
>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>> math off of those.
>>
> 
> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
> if it looks good to you?
> 
>      #define KB         (1UL << 10)
>      #define MB         (1UL << 20)
>      #define GB         (1UL << 30)
>      #define TB         (1UL << 40)
> 
>      /* Base page and huge page size */
>      #define SIZE_4KB   (  4 * KB)
>      #define SIZE_16KB  ( 16 * KB)
>      #define SIZE_64KB  ( 64 * KB)
>      #define SIZE_2MB   (  2 * MB)
>      #define SIZE_32MB  ( 32 * MB)
>      #define SIZE_512MB (512 * MB)
>      #define SIZE_1GB   (  1 * GB)
>      #define SIZE_16GB  ( 16 * GB)

FYI, QEMU uses KiB, MiB, GiB, etc., see [1].

> Thanks,
> Gavin
> 

Thanks,
Maciej

[1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Maciej S. Szmigiero]

On 18.10.2022 00:51, Gavin Shan wrote:
> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>
>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>> zero short).
>>
>> +1 to not open coding raw numbers.
>>
>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>> 16KB, 64K, 2MB, 1GB, etc...
>>
>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>> math off of those.
>>
> 
> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
> if it looks good to you?
> 
>      #define KB         (1UL << 10)
>      #define MB         (1UL << 20)
>      #define GB         (1UL << 30)
>      #define TB         (1UL << 40)
> 
>      /* Base page and huge page size */
>      #define SIZE_4KB   (  4 * KB)
>      #define SIZE_16KB  ( 16 * KB)
>      #define SIZE_64KB  ( 64 * KB)
>      #define SIZE_2MB   (  2 * MB)
>      #define SIZE_32MB  ( 32 * MB)
>      #define SIZE_512MB (512 * MB)
>      #define SIZE_1GB   (  1 * GB)
>      #define SIZE_16GB  ( 16 * GB)

FYI, QEMU uses KiB, MiB, GiB, etc., see [1].

> Thanks,
> Gavin
> 

Thanks,
Maciej

[1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
> On 18.10.2022 00:51, Gavin Shan wrote:
>> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>>
>>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>>> zero short).
>>>
>>> +1 to not open coding raw numbers.
>>>
>>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>>> 16KB, 64K, 2MB, 1GB, etc...
>>>
>>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>>> math off of those.
>>>
>>
>> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
>> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
>> if it looks good to you?
>>
>>      #define KB         (1UL << 10)
>>      #define MB         (1UL << 20)
>>      #define GB         (1UL << 30)
>>      #define TB         (1UL << 40)
>>
>>      /* Base page and huge page size */
>>      #define SIZE_4KB   (  4 * KB)
>>      #define SIZE_16KB  ( 16 * KB)
>>      #define SIZE_64KB  ( 64 * KB)
>>      #define SIZE_2MB   (  2 * MB)
>>      #define SIZE_32MB  ( 32 * MB)
>>      #define SIZE_512MB (512 * MB)
>>      #define SIZE_1GB   (  1 * GB)
>>      #define SIZE_16GB  ( 16 * GB)
> 
> FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
> 

Right. I checked QEMU's definitions and it makes sense to use
KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
our tests don't use that large memory.

> 
> [1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD
> 

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
> On 18.10.2022 00:51, Gavin Shan wrote:
>> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>>
>>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>>> zero short).
>>>
>>> +1 to not open coding raw numbers.
>>>
>>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>>> 16KB, 64K, 2MB, 1GB, etc...
>>>
>>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>>> math off of those.
>>>
>>
>> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
>> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
>> if it looks good to you?
>>
>>      #define KB         (1UL << 10)
>>      #define MB         (1UL << 20)
>>      #define GB         (1UL << 30)
>>      #define TB         (1UL << 40)
>>
>>      /* Base page and huge page size */
>>      #define SIZE_4KB   (  4 * KB)
>>      #define SIZE_16KB  ( 16 * KB)
>>      #define SIZE_64KB  ( 64 * KB)
>>      #define SIZE_2MB   (  2 * MB)
>>      #define SIZE_32MB  ( 32 * MB)
>>      #define SIZE_512MB (512 * MB)
>>      #define SIZE_1GB   (  1 * GB)
>>      #define SIZE_16GB  ( 16 * GB)
> 
> FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
> 

Right. I checked QEMU's definitions and it makes sense to use
KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
our tests don't use that large memory.

> 
> [1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD
> 

Thanks,
Gavin

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Sean Christopherson]

On Tue, Oct 18, 2022, Gavin Shan wrote:
> On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
> > On 18.10.2022 00:51, Gavin Shan wrote:
> > > On 10/18/22 6:08 AM, Sean Christopherson wrote:
> > > > On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > > > > > +#define MEM_EXTRA_SIZE        0x10000
> > > > > 
> > > > > Also, an expression like "(64 << 10)" is more readable than a "1"
> > > > > with a tail of zeroes (it's easy to add one zero too many or be one
> > > > > zero short).
> > > > 
> > > > +1 to not open coding raw numbers.
> > > > 
> > > > I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> > > > 16KB, 64K, 2MB, 1GB, etc...
> > > > 
> > > > Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> > > > math off of those.
> > > > 
> > > 
> > > Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
> > > right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
> > > if it looks good to you?
> > > 
> > >      #define KB         (1UL << 10)
> > >      #define MB         (1UL << 20)
> > >      #define GB         (1UL << 30)
> > >      #define TB         (1UL << 40)

Any objection to prefixing these with SIZE_ as well?  IMO it's worth burning the
extra five characters to make it all but impossible to misinterpret code.

> > >      /* Base page and huge page size */
> > >      #define SIZE_4KB   (  4 * KB)
> > >      #define SIZE_16KB  ( 16 * KB)
> > >      #define SIZE_64KB  ( 64 * KB)
> > >      #define SIZE_2MB   (  2 * MB)
> > >      #define SIZE_32MB  ( 32 * MB)
> > >      #define SIZE_512MB (512 * MB)
> > >      #define SIZE_1GB   (  1 * GB)
> > >      #define SIZE_16GB  ( 16 * GB)
> > 
> > FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
> > 
> 
> Right. I checked QEMU's definitions and it makes sense to use
> KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
> our tests don't use that large memory.

Ha!  I had typed out KiB, etc... but then thought, "nah, I'm being silly".  KiB
and friends work for me.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Sean Christopherson]

On Tue, Oct 18, 2022, Gavin Shan wrote:
> On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
> > On 18.10.2022 00:51, Gavin Shan wrote:
> > > On 10/18/22 6:08 AM, Sean Christopherson wrote:
> > > > On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > > > > > +#define MEM_EXTRA_SIZE        0x10000
> > > > > 
> > > > > Also, an expression like "(64 << 10)" is more readable than a "1"
> > > > > with a tail of zeroes (it's easy to add one zero too many or be one
> > > > > zero short).
> > > > 
> > > > +1 to not open coding raw numbers.
> > > > 
> > > > I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> > > > 16KB, 64K, 2MB, 1GB, etc...
> > > > 
> > > > Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> > > > math off of those.
> > > > 
> > > 
> > > Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
> > > right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
> > > if it looks good to you?
> > > 
> > >      #define KB         (1UL << 10)
> > >      #define MB         (1UL << 20)
> > >      #define GB         (1UL << 30)
> > >      #define TB         (1UL << 40)

Any objection to prefixing these with SIZE_ as well?  IMO it's worth burning the
extra five characters to make it all but impossible to misinterpret code.

> > >      /* Base page and huge page size */
> > >      #define SIZE_4KB   (  4 * KB)
> > >      #define SIZE_16KB  ( 16 * KB)
> > >      #define SIZE_64KB  ( 64 * KB)
> > >      #define SIZE_2MB   (  2 * MB)
> > >      #define SIZE_32MB  ( 32 * MB)
> > >      #define SIZE_512MB (512 * MB)
> > >      #define SIZE_1GB   (  1 * GB)
> > >      #define SIZE_16GB  ( 16 * GB)
> > 
> > FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
> > 
> 
> Right. I checked QEMU's definitions and it makes sense to use
> KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
> our tests don't use that large memory.

Ha!  I had typed out KiB, etc... but then thought, "nah, I'm being silly".  KiB
and friends work for me.

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 7:32 AM, Sean Christopherson wrote:
> On Tue, Oct 18, 2022, Gavin Shan wrote:
>> On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
>>> On 18.10.2022 00:51, Gavin Shan wrote:
>>>> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>>>>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>>>>
>>>>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>>>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>>>>> zero short).
>>>>>
>>>>> +1 to not open coding raw numbers.
>>>>>
>>>>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>>>>> 16KB, 64K, 2MB, 1GB, etc...
>>>>>
>>>>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>>>>> math off of those.
>>>>>
>>>>
>>>> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
>>>> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
>>>> if it looks good to you?
>>>>
>>>>       #define KB         (1UL << 10)
>>>>       #define MB         (1UL << 20)
>>>>       #define GB         (1UL << 30)
>>>>       #define TB         (1UL << 40)
> 
> Any objection to prefixing these with SIZE_ as well?  IMO it's worth burning the
> extra five characters to make it all but impossible to misinterpret code.
> 

'SIZE_' prefix works for me either.

>>>>       /* Base page and huge page size */
>>>>       #define SIZE_4KB   (  4 * KB)
>>>>       #define SIZE_16KB  ( 16 * KB)
>>>>       #define SIZE_64KB  ( 64 * KB)
>>>>       #define SIZE_2MB   (  2 * MB)
>>>>       #define SIZE_32MB  ( 32 * MB)
>>>>       #define SIZE_512MB (512 * MB)
>>>>       #define SIZE_1GB   (  1 * GB)
>>>>       #define SIZE_16GB  ( 16 * GB)
>>>
>>> FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
>>>
>>
>> Right. I checked QEMU's definitions and it makes sense to use
>> KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
>> our tests don't use that large memory.
> 
> Ha!  I had typed out KiB, etc... but then thought, "nah, I'm being silly".  KiB
> and friends work for me.
> 

Thanks for your confirm, Sean.

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 7:32 AM, Sean Christopherson wrote:
> On Tue, Oct 18, 2022, Gavin Shan wrote:
>> On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote:
>>> On 18.10.2022 00:51, Gavin Shan wrote:
>>>> On 10/18/22 6:08 AM, Sean Christopherson wrote:
>>>>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>>>>> +#define MEM_EXTRA_SIZE        0x10000
>>>>>>
>>>>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>>>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>>>>> zero short).
>>>>>
>>>>> +1 to not open coding raw numbers.
>>>>>
>>>>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>>>>> 16KB, 64K, 2MB, 1GB, etc...
>>>>>
>>>>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>>>>> math off of those.
>>>>>
>>>>
>>>> Ok. I will have one separate patch to define those sizes in kvm_util_base.h,
>>>> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know
>>>> if it looks good to you?
>>>>
>>>>       #define KB         (1UL << 10)
>>>>       #define MB         (1UL << 20)
>>>>       #define GB         (1UL << 30)
>>>>       #define TB         (1UL << 40)
> 
> Any objection to prefixing these with SIZE_ as well?  IMO it's worth burning the
> extra five characters to make it all but impossible to misinterpret code.
> 

'SIZE_' prefix works for me either.

>>>>       /* Base page and huge page size */
>>>>       #define SIZE_4KB   (  4 * KB)
>>>>       #define SIZE_16KB  ( 16 * KB)
>>>>       #define SIZE_64KB  ( 64 * KB)
>>>>       #define SIZE_2MB   (  2 * MB)
>>>>       #define SIZE_32MB  ( 32 * MB)
>>>>       #define SIZE_512MB (512 * MB)
>>>>       #define SIZE_1GB   (  1 * GB)
>>>>       #define SIZE_16GB  ( 16 * GB)
>>>
>>> FYI, QEMU uses KiB, MiB, GiB, etc., see [1].
>>>
>>
>> Right. I checked QEMU's definitions and it makes sense to use
>> KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because
>> our tests don't use that large memory.
> 
> Ha!  I had typed out KiB, etc... but then thought, "nah, I'm being silly".  KiB
> and friends work for me.
> 

Thanks for your confirm, Sean.

Thanks,
Gavin

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Oliver Upton]

On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote:
> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > > +#define MEM_EXTRA_SIZE		0x10000
> >
> > Also, an expression like "(64 << 10)" is more readable than a "1"
> > with a tail of zeroes (it's easy to add one zero too many or be one
> > zero short).
> 
> +1 to not open coding raw numbers.
> 
> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> 16KB, 64K, 2MB, 1GB, etc...
> 
> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> math off of those.

I mean I love boilerplate as much as the next guy, but we can just use
tools/include/linux/sizes.h

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Oliver Upton]

On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote:
> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
> > > +#define MEM_EXTRA_SIZE		0x10000
> >
> > Also, an expression like "(64 << 10)" is more readable than a "1"
> > with a tail of zeroes (it's easy to add one zero too many or be one
> > zero short).
> 
> +1 to not open coding raw numbers.
> 
> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
> 16KB, 64K, 2MB, 1GB, etc...
> 
> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
> math off of those.

I mean I love boilerplate as much as the next guy, but we can just use
tools/include/linux/sizes.h

--
Thanks,
Oliver

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 3:47 PM, Oliver Upton wrote:
> On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote:
>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>> +#define MEM_EXTRA_SIZE		0x10000
>>>
>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>> zero short).
>>
>> +1 to not open coding raw numbers.
>>
>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>> 16KB, 64K, 2MB, 1GB, etc...
>>
>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>> math off of those.
> 
> I mean I love boilerplate as much as the next guy, but we can just use
> tools/include/linux/sizes.h
> 

Nice point, I didn't realize we already had 'tools/include/linux/sizes.h'.
The suggested macros (KiB, MiB, GiB, TiB and their variants) have been added
to PATCH[v2 5/6]. I think it's reasonable to use 'tools/include/linux/sizes.h'
directly instead of reinventing the wheel.

I will go ahead to use 'tools/include/linux/sizes.h' directly in v3 if nobody
objects. I would like to receive comments on v2 before I'm going to post v3.

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 3:47 PM, Oliver Upton wrote:
> On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote:
>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote:
>>>> +#define MEM_EXTRA_SIZE		0x10000
>>>
>>> Also, an expression like "(64 << 10)" is more readable than a "1"
>>> with a tail of zeroes (it's easy to add one zero too many or be one
>>> zero short).
>>
>> +1 to not open coding raw numbers.
>>
>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB,
>> 16KB, 64K, 2MB, 1GB, etc...
>>
>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do
>> math off of those.
> 
> I mean I love boilerplate as much as the next guy, but we can just use
> tools/include/linux/sizes.h
> 

Nice point, I didn't realize we already had 'tools/include/linux/sizes.h'.
The suggested macros (KiB, MiB, GiB, TiB and their variants) have been added
to PATCH[v2 5/6]. I think it's reasonable to use 'tools/include/linux/sizes.h'
directly instead of reinventing the wheel.

I will go ahead to use 'tools/include/linux/sizes.h' directly in v3 if nobody
objects. I would like to receive comments on v2 before I'm going to post v3.

Thanks,
Gavin

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 5:36 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
>> should be aligned to host page size, which can be 64KB on aarch64. So it's
>> wrong by passing additional fixed 4KB memory area to various tests.
>>
>> Fix it by passing additional fixed 64KB memory area to various tests. After
>> it's applied, the following command works fine on 64KB-page-size-host and
>> 4KB-page-size-guest.
>>
>>    # ./memslot_perf_test -v -s 512
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index d587bd952ff9..e6d34744b45d 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -25,12 +25,14 @@
>>   #include <kvm_util.h>
>>   #include <processor.h>
>> -#define MEM_SIZE        ((512U << 20) + 4096)
>> -#define MEM_GPA        0x10000000UL
>> +#define MEM_EXTRA_SIZE        0x10000
> 
> So the biggest page size supported right now is 64 KiB - it would be
> good to have an assert somewhere to explicitly check for this
> (regardless of implicit checks present in other calculations).
> 
> Also, an expression like "(64 << 10)" is more readable than a "1"
> with a tail of zeroes (it's easy to add one zero too many or be one
> zero short).
> 

Yes, it makes sense to me. Lets add check in check_memory_sizes(), which
was added in the previous patch, to fail early if host/guest page size
exceeds 64KB.

    if (host_page_size > SIZE_64KiB || guest_page_size > SIZE_64KiB) {
       pr_info("Unsupported page size on host (0x%x) or guest (0x%x)\n",
               host_page_size, guest_page_size);
    }


For the macros, I think all of us agree on KiB, MiB, GiB, TiB and
their variants :)

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

[Gavin Shan]

On 10/18/22 5:36 AM, Maciej S. Szmigiero wrote:
> On 14.10.2022 09:19, Gavin Shan wrote:
>> The addresses and sizes passed to madvise() and vm_userspace_mem_region_add()
>> should be aligned to host page size, which can be 64KB on aarch64. So it's
>> wrong by passing additional fixed 4KB memory area to various tests.
>>
>> Fix it by passing additional fixed 64KB memory area to various tests. After
>> it's applied, the following command works fine on 64KB-page-size-host and
>> 4KB-page-size-guest.
>>
>>    # ./memslot_perf_test -v -s 512
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   .../testing/selftests/kvm/memslot_perf_test.c  | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
>> index d587bd952ff9..e6d34744b45d 100644
>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
>> @@ -25,12 +25,14 @@
>>   #include <kvm_util.h>
>>   #include <processor.h>
>> -#define MEM_SIZE        ((512U << 20) + 4096)
>> -#define MEM_GPA        0x10000000UL
>> +#define MEM_EXTRA_SIZE        0x10000
> 
> So the biggest page size supported right now is 64 KiB - it would be
> good to have an assert somewhere to explicitly check for this
> (regardless of implicit checks present in other calculations).
> 
> Also, an expression like "(64 << 10)" is more readable than a "1"
> with a tail of zeroes (it's easy to add one zero too many or be one
> zero short).
> 

Yes, it makes sense to me. Lets add check in check_memory_sizes(), which
was added in the previous patch, to fail early if host/guest page size
exceeds 64KB.

    if (host_page_size > SIZE_64KiB || guest_page_size > SIZE_64KiB) {
       pr_info("Unsupported page size on host (0x%x) or guest (0x%x)\n",
               host_page_size, guest_page_size);
    }


For the macros, I think all of us agree on KiB, MiB, GiB, TiB and
their variants :)

Thanks,
Gavin

[PATCH 6/6] KVM: selftests: memslot_perf_test: Report optimal memory slots

[Gavin Shan]

The memory area in each slot should be aligned to host page size.
Otherwise, the test will failure. For example, the following command
fails with the following messages with 64KB-page-size-host and
4KB-pae-size-guest. It's not user friendly. Lets do something to report
the optimal memory slots, instead of failing the test.

  # ./memslot_perf_test -v -s 1000
  Number of memory slots: 999
  Testing map performance with 1 runs, 5 seconds each
  Adding slots 1..999, each slot with 8 pages + 216 extra pages last
  ==== Test Assertion Failure ====
    lib/kvm_util.c:824: vm_adjust_num_guest_pages(vm->mode, npages) == npages
    pid=19872 tid=19872 errno=0 - Success
       1  0x00000000004065b3: vm_userspace_mem_region_add at kvm_util.c:822
       2  0x0000000000401d6b: prepare_vm at memslot_perf_test.c:273
       3  (inlined by) test_execute at memslot_perf_test.c:756
       4  (inlined by) test_loop at memslot_perf_test.c:994
       5  (inlined by) main at memslot_perf_test.c:1073
       6  0x0000ffff7ebb4383: ?? ??:0
       7  0x00000000004021ff: _start at :?
    Number of guest pages is not compatible with the host. Try npages=16

Report the optimal memory slots instead of failing the test when
the memory area in each slot isn't aligned to host page size. With
this applied, the optimal memory slots is reported.

  # ./memslot_perf_test -v -s 1000
  # ./memslot_perf_test -v -s 1000
  Number of memory slots: 999
  Testing map performance with 1 runs, 5 seconds each
  Memslot count too high for this test, decrease the cap (max is 514)

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 45 +++++++++++++++++--
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index e6d34744b45d..bec65803f220 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -230,16 +230,52 @@ static struct vm_data *alloc_vm(void)
 	return data;
 }
 
+static bool check_slot_pages(uint32_t host_page_size, uint32_t guest_page_size,
+			     uint64_t pages_per_slot, uint64_t rempages)
+{
+	if (!pages_per_slot)
+		return false;
+
+	if ((pages_per_slot * guest_page_size) % host_page_size)
+		return false;
+
+	if ((rempages * guest_page_size) % host_page_size)
+		return false;
+
+	return true;
+}
+
+
+static uint64_t get_max_slots(struct vm_data *data, uint32_t host_page_size)
+{
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t mempages, pages_per_slot, rempages;
+	uint64_t slots;
+
+	mempages = data->npages;
+	slots = data->nslots;
+	while (--slots > 1) {
+		pages_per_slot = mempages / slots;
+		rempages = mempages % pages_per_slot;
+		if (check_slot_pages(host_page_size, guest_page_size,
+				     pages_per_slot, rempages))
+			return slots + 1;	/* slot 0 is reserved */
+	}
+
+	return 0;
+}
+
 static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		       void *guest_code, uint64_t mem_size,
 		       struct timespec *slot_runtime)
 {
 	uint64_t mempages, rempages;
 	uint64_t guest_addr;
-	uint32_t slot, guest_page_size;
+	uint32_t slot, host_page_size, guest_page_size;
 	struct timespec tstart;
 	struct sync_area *sync;
 
+	host_page_size = getpagesize();
 	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
 	mempages = mem_size / guest_page_size;
 
@@ -250,12 +286,13 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
 	data->nslots = nslots;
 	data->pages_per_slot = data->npages / data->nslots;
-	if (!data->pages_per_slot) {
-		*maxslots = data->npages + 1;
+	rempages = data->npages % data->nslots;
+	if (!check_slot_pages(host_page_size, guest_page_size,
+			      data->pages_per_slot, rempages)) {
+		*maxslots = get_max_slots(data, host_page_size);
 		return false;
 	}
 
-	rempages = data->npages % data->nslots;
 	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
 	TEST_ASSERT(data->hva_slots, "malloc() fail");
 
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 6/6] KVM: selftests: memslot_perf_test: Report optimal memory slots

[Gavin Shan]

The memory area in each slot should be aligned to host page size.
Otherwise, the test will failure. For example, the following command
fails with the following messages with 64KB-page-size-host and
4KB-pae-size-guest. It's not user friendly. Lets do something to report
the optimal memory slots, instead of failing the test.

  # ./memslot_perf_test -v -s 1000
  Number of memory slots: 999
  Testing map performance with 1 runs, 5 seconds each
  Adding slots 1..999, each slot with 8 pages + 216 extra pages last
  ==== Test Assertion Failure ====
    lib/kvm_util.c:824: vm_adjust_num_guest_pages(vm->mode, npages) == npages
    pid=19872 tid=19872 errno=0 - Success
       1  0x00000000004065b3: vm_userspace_mem_region_add at kvm_util.c:822
       2  0x0000000000401d6b: prepare_vm at memslot_perf_test.c:273
       3  (inlined by) test_execute at memslot_perf_test.c:756
       4  (inlined by) test_loop at memslot_perf_test.c:994
       5  (inlined by) main at memslot_perf_test.c:1073
       6  0x0000ffff7ebb4383: ?? ??:0
       7  0x00000000004021ff: _start at :?
    Number of guest pages is not compatible with the host. Try npages=16

Report the optimal memory slots instead of failing the test when
the memory area in each slot isn't aligned to host page size. With
this applied, the optimal memory slots is reported.

  # ./memslot_perf_test -v -s 1000
  # ./memslot_perf_test -v -s 1000
  Number of memory slots: 999
  Testing map performance with 1 runs, 5 seconds each
  Memslot count too high for this test, decrease the cap (max is 514)

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 .../testing/selftests/kvm/memslot_perf_test.c | 45 +++++++++++++++++--
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index e6d34744b45d..bec65803f220 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -230,16 +230,52 @@ static struct vm_data *alloc_vm(void)
 	return data;
 }
 
+static bool check_slot_pages(uint32_t host_page_size, uint32_t guest_page_size,
+			     uint64_t pages_per_slot, uint64_t rempages)
+{
+	if (!pages_per_slot)
+		return false;
+
+	if ((pages_per_slot * guest_page_size) % host_page_size)
+		return false;
+
+	if ((rempages * guest_page_size) % host_page_size)
+		return false;
+
+	return true;
+}
+
+
+static uint64_t get_max_slots(struct vm_data *data, uint32_t host_page_size)
+{
+	uint32_t guest_page_size = data->vm->page_size;
+	uint64_t mempages, pages_per_slot, rempages;
+	uint64_t slots;
+
+	mempages = data->npages;
+	slots = data->nslots;
+	while (--slots > 1) {
+		pages_per_slot = mempages / slots;
+		rempages = mempages % pages_per_slot;
+		if (check_slot_pages(host_page_size, guest_page_size,
+				     pages_per_slot, rempages))
+			return slots + 1;	/* slot 0 is reserved */
+	}
+
+	return 0;
+}
+
 static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 		       void *guest_code, uint64_t mem_size,
 		       struct timespec *slot_runtime)
 {
 	uint64_t mempages, rempages;
 	uint64_t guest_addr;
-	uint32_t slot, guest_page_size;
+	uint32_t slot, host_page_size, guest_page_size;
 	struct timespec tstart;
 	struct sync_area *sync;
 
+	host_page_size = getpagesize();
 	guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
 	mempages = mem_size / guest_page_size;
 
@@ -250,12 +286,13 @@ static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
 	TEST_ASSERT(data->npages > 1, "Can't test without any memory");
 	data->nslots = nslots;
 	data->pages_per_slot = data->npages / data->nslots;
-	if (!data->pages_per_slot) {
-		*maxslots = data->npages + 1;
+	rempages = data->npages % data->nslots;
+	if (!check_slot_pages(host_page_size, guest_page_size,
+			      data->pages_per_slot, rempages)) {
+		*maxslots = get_max_slots(data, host_page_size);
 		return false;
 	}
 
-	rempages = data->npages % data->nslots;
 	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
 	TEST_ASSERT(data->hva_slots, "malloc() fail");
 
-- 
2.23.0