KVM: selftests: add powerpc support

From: Nicholas Piggin <[email protected]>

powerpc will require this to allocate MMU tables in guest memory that are larger than guest base page size.

Signed-off-by Nicholas Piggin <[email protected]>

[Rebased to latest mainline tree]

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

---
 .../testing/selftests/kvm/include/kvm_util.h  | 20 +++++++++--
 tools/testing/selftests/kvm/lib/kvm_util.c    | 33 +++++++++----------
 2 files changed, 33 insertions(+), 20 deletions(-)

tools/testing/selftests/kvm/include/kvm_util.h +2 -2

index 3666a8530f31..c515c918c2c9 100644
@@ -991,8 +991,8 @@ void kvm_gsi_routing_write(struct kvm_vm *vm, struct kvm_irq_routing *routing);
 const char *exit_reason_str(unsigned int exit_reason);
 
 gpa_t vm_phy_page_alloc(struct kvm_vm *vm, gpa_t min_gpa, u32 memslot);
-gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, gpa_t min_gpa,
-			   u32 memslot, bool protected);
+gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, size_t align,
+			   gpa_t min_gpa, u32 memslot, bool protected);
 gpa_t vm_alloc_page_table(struct kvm_vm *vm);
 
 static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,

@@ -1003,10 +1003,24 @@ static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	 * protected memory, as the majority of memory for such VMs is

tools/testing/selftests/kvm/include/kvm_util.h +14 -6

@@ -2039,23 +2039,22 @@ gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	TEST_ASSERT(!protected || region->protected_phy_pages,
 		    "Region doesn't support protected memory");
 
-	base = pg = min_gpa >> vm->page_shift;
-	do {
-		for (; pg < base + num; ++pg) {
-			if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
-				base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
-				break;
+	base = min_gpa >> vm->page_shift;
+again:
+	base = (base + align - 1) & ~(align - 1);
+	for (pg = base; pg < base + num; ++pg) {
+		if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
+			base = sparsebit_next_set(region->unused_phy_pages, pg);
+			if (!base) {
+				fprintf(stderr, "No guest physical page available, "
+					"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",
+					min_gpa, vm->page_size, memslot);
+				fputs("---- vm dump ----\n", stderr);
+				vm_dump(stderr, vm, 2);
+				abort();
 			}
+			goto again;
 		}

tools/testing/selftests/kvm/include/kvm_util.h +0 -1

 	for (pg = base; pg < base + num; ++pg) {
-- 

2.50.1 (Apple Git-155)

Quoted 37 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc will require this to allocate MMU tables in guest memory that are larger than guest base page size.

Signed-off-by Nicholas Piggin <[email protected]>

[Rebased to latest mainline tree]

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

---
 .../testing/selftests/kvm/include/kvm_util.h  | 20 +++++++++--
 tools/testing/selftests/kvm/lib/kvm_util.c    | 33 +++++++++----------
 2 files changed, 33 insertions(+), 20 deletions(-)

tools/testing/selftests/kvm/include/kvm_util.h +3 -3

index 3666a8530f31..c515c918c2c9 100644
@@ -991,8 +991,8 @@ void kvm_gsi_routing_write(struct kvm_vm *vm, struct kvm_irq_routing *routing);
 const char *exit_reason_str(unsigned int exit_reason);
 
 gpa_t vm_phy_page_alloc(struct kvm_vm *vm, gpa_t min_gpa, u32 memslot);
-gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, gpa_t min_gpa,
-			   u32 memslot, bool protected);
+gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, size_t align,
+			   gpa_t min_gpa, u32 memslot, bool protected);
 gpa_t vm_alloc_page_table(struct kvm_vm *vm);
 
 static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,

@@ -1003,10 +1003,24 @@ static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	 * protected memory, as the majority of memory for such VMs is
 	 * protected, i.e. using shared memory is effectively opt-in.
 	 */
-	return __vm_phy_pages_alloc(vm, num, min_gpa, memslot,
+	return __vm_phy_pages_alloc(vm, num, 1, min_gpa, memslot,
 				    vm_arch_has_protected_memory(vm));
 }
 

tools/testing/selftests/kvm/include/kvm_util.h static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 1012

Quoted 3 lines

tools/testing/selftests/kvm/include/kvm_util.h +3 -0

+static inline gpa_t vm_phy_pages_alloc_align(struct kvm_vm *vm, size_t num,
+					     size_t align, gpa_t min_gpa,
+					     u32 memslot)

Given that the PPC usage is all for naturally aligned allocations, I think it makes sense for that to be the API, i.e. have "bool naturally_aligned" instead of an arbitrary alignment.

tools/testing/selftests/kvm/include/kvm_util.h static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 1018

Quoted 6 lines

tools/testing/selftests/kvm/include/kvm_util.h +6 -0

+{
+	/*
+	 * By default, allocate memory as protected for VMs that support
+	 * protected memory, as the majority of memory for such VMs is
+	 * protected, i.e. using shared memory is effectively opt-in.
+	 */

Duplicating this big comment is very ugly.

In general, these APIs could use
some love. E.g. taking in @memslot is essentially a historical wart that isn't necessary except for literally just memslot_perf_test.c, which allocates memory in a huge number of memslots.

If we rework the APIs to take the memory region type instead of the memslot, then we can kill many birds with one stone. It takes quite a bit of cleanup to throw that one stone, but I think the end result can be quite nice.

Compile tested only at this point, but I now have a series of ~17 patches to yield:

__weak bool kvm_arch_needs_naturally_aligned_page_tables(void)

  {
	return false;

}

gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t nr_pages,
enum kvm_mem_region_type type, bool protected)

  {
	struct userspace_mem_region *region = vm_get_mem_region(vm, type);
	bool naturally_aligned = false;
	gpa_t min_gpa;

	TEST_ASSERT(region, "No region for type '%u', memslot '%u'",
		    type, vm->memslots[type]);

	switch (type) {
	case MEM_REGION_CODE:
	case MEM_REGION_DATA:
	case MEM_REGION_TEST_DATA:
		/*
		 * If the region is backed by the default memslot (id=0), use
		 * selftests' hardcoded minimum PFN, otherwise use the base of
		 * the custom memory slot that backs the region.
		 */
		if (!vm->memslots[type])
			min_gpa = KVM_UTIL_MIN_PFN * vm->page_size;
		else
			min_gpa = region->region.guest_phys_addr;
		break;
	case MEM_REGION_PT:
		min_gpa = KVM_GUEST_PAGE_TABLE_MIN_PADDR;
		naturally_aligned = kvm_arch_needs_naturally_aligned_page_tables();
		break;
	case MEM_REGION_TEST_EXTRA:
		min_gpa = region->region.guest_phys_addr;
		break;
	default:
		TEST_FAIL("Invalid memory region type '%u'", type);
		break;
	}

	return ____vm_phy_pages_alloc(vm, nr_pages, min_gpa, vm->memslots[type],
				      protected, naturally_aligned);

}

with convenience wrappers:

static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t nr_pages,
enum kvm_mem_region_type type)
{
/*
* By default, allocate memory as protected for VMs that support
* protected memory, as the majority of memory for such VMs is
* protected, i.e. using shared memory is effectively opt-in. */
return __vm_phy_pages_alloc(vm, nr_pages, type,
vm_arch_has_protected_memory(vm));
}

static inline gpa_t vm_phy_page_alloc(struct kvm_vm *vm,
enum kvm_mem_region_type type)

  {
	return vm_phy_pages_alloc(vm, 1, type);

}

static inline gpa_t vm_alloc_page_table_pages(struct kvm_vm *vm, size_t nr_pages)

  {
	return vm_phy_page_alloc(vm, MEM_REGION_PT);

}

static inline gpa_t vm_alloc_page_table(struct kvm_vm *vm)

  {
	return vm_alloc_page_table_pages(vm, 1);

}

That way we don't need to add yet another rarely used param to the APIs, and PPC just needs to define kvm_arch_needs_naturally_aligned_page_tables().

The bonus is that @min_gpa goes away too.

It'll probably take me a few days/weeks, but I'll try get a series posted before the 7.2 merge window closes, so that you can build on top to get the PPC selftests support landed in 7.3.

Quoted 31 lines

tools/testing/selftests/kvm/include/kvm_util.h +14 -11

@@ -2039,23 +2039,22 @@ gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	TEST_ASSERT(!protected || region->protected_phy_pages,
 		    "Region doesn't support protected memory");
 
-	base = pg = min_gpa >> vm->page_shift;
-	do {
-		for (; pg < base + num; ++pg) {
-			if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
-				base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
-				break;
+	base = min_gpa >> vm->page_shift;
+again:
+	base = (base + align - 1) & ~(align - 1);
+	for (pg = base; pg < base + num; ++pg) {
+		if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
+			base = sparsebit_next_set(region->unused_phy_pages, pg);
+			if (!base) {
+				fprintf(stderr, "No guest physical page available, "
+					"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",
+					min_gpa, vm->page_size, memslot);
+				fputs("---- vm dump ----\n", stderr);
+				vm_dump(stderr, vm, 2);
+				abort();
 			}
+			goto again;
 		}
-	} while (pg && pg != base + num);
-
-	if (pg == 0) {
-		fprintf(stderr, "No guest physical page available, "
-			"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",

tools/testing/selftests/kvm/include/kvm_util.h gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 2058

Quoted 5 lines

tools/testing/selftests/kvm/include/kvm_util.h +0 -4

-			min_gpa, vm->page_size, memslot);
-		fputs("---- vm dump ----\n", stderr);
-		vm_dump(stderr, vm, 2);
-		abort();
 	}

This is unnecessary churn. I'm not saying the current code is pretty or anything, but unless I'm missing something, this can simply be:

tools/testing/selftests/kvm/lib/kvm_util.c +1 -1

@@ -2025,7 +2027,7 @@ gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t nr_pages, gpa_t min_gpa,
        TEST_ASSERT(!protected || region->protected_phy_pages,
                    "Region doesn't support protected memory");
 
-       base = pg = min_gpa >> vm->page_shift;
+       base = pg = ALIGN(min_gpa >> vm->page_shift, alignment);
        do {
                for (; pg < base + nr_pages; ++pg) {
                        if (!sparsebit_is_set(region->unused_phy_pages, pg)) {

Quoted 2 lines

Quoted 4 lines

for (pg = base; pg < base + num; ++pg) {

Signature · 1 line

2.50.1 (Apple Git-155)

Quoted 38 lines · Sean Christopherson

Quoted 37 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc will require this to allocate MMU tables in guest memory that are larger than guest base page size.

Signed-off-by Nicholas Piggin <[email protected]>

[Rebased to latest mainline tree]

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

---
 .../testing/selftests/kvm/include/kvm_util.h  | 20 +++++++++--
 tools/testing/selftests/kvm/lib/kvm_util.c    | 33 +++++++++----------
 2 files changed, 33 insertions(+), 20 deletions(-)

tools/testing/selftests/kvm/include/kvm_util.h +3 -3

index 3666a8530f31..c515c918c2c9 100644
@@ -991,8 +991,8 @@ void kvm_gsi_routing_write(struct kvm_vm *vm, struct kvm_irq_routing *routing);
 const char *exit_reason_str(unsigned int exit_reason);
 
 gpa_t vm_phy_page_alloc(struct kvm_vm *vm, gpa_t min_gpa, u32 memslot);
-gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, gpa_t min_gpa,
-			   u32 memslot, bool protected);
+gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, size_t align,
+			   gpa_t min_gpa, u32 memslot, bool protected);
 gpa_t vm_alloc_page_table(struct kvm_vm *vm);
 
 static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,

@@ -1003,10 +1003,24 @@ static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	 * protected memory, as the majority of memory for such VMs is
 	 * protected, i.e. using shared memory is effectively opt-in.
 	 */
-	return __vm_phy_pages_alloc(vm, num, min_gpa, memslot,
+	return __vm_phy_pages_alloc(vm, num, 1, min_gpa, memslot,
 				    vm_arch_has_protected_memory(vm));
 }
 

Quoted 8 lines

tools/testing/selftests/kvm/include/kvm_util.h static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 1012

Quoted 3 lines

tools/testing/selftests/kvm/include/kvm_util.h +3 -0

+static inline gpa_t vm_phy_pages_alloc_align(struct kvm_vm *vm, size_t num,
+					     size_t align, gpa_t min_gpa,
+					     u32 memslot)

Given that the PPC usage is all for naturally aligned allocations, I think it makes sense for that to be the API, i.e. have "bool naturally_aligned" instead of an arbitrary alignment.

I would still prefer passing an explicit align value to the allocator, because IMHO that's a useful allocator interface to have.

However, if we do want to go down this road than I don't have any strong objection either, since as you mentioned powerpc mostly just needs natual alignment for it's page table region. So alignment can be extracted from the region type as you described below, so no need to pass it all the time.

Quoted 1 line

The bonus is that @min_gpa goes away too.

powerpc needs min_gpa for it's exception handling pages. see.

	excp_paddr = vm_phy_pages_alloc(vm, excp_pages, 0,
					vm->memslots[MEM_REGION_DATA]);

	TEST_ASSERT(excp_paddr == 0,
		    "Interrupt vectors not allocated at gPA address 0: (0x%lx)",
		    excp_paddr);

So, will arch still have an access to the API for passing min_gpa = 0 for cases like above?

Quoted 4 lines

It'll probably take me a few days/weeks, but I'll try get a series posted before the 7.2 merge window closes, so that you can build on top to get the PPC selftests support landed in 7.3.

Thanks Sean for your help! Yes, landing kvm selftests for powerpc will be definitely helpful to verify against any kvm regressions.

BTW, I was thinking whether landing powerpc first will be easier for you to consider all the API requirements from all users / usecases? But either way is fine please. I can work on top of your changes too and if something is missing for powerpc, we can add / modify on top, once your series is finished.

Quoted 31 lines

Quoted 31 lines

tools/testing/selftests/kvm/include/kvm_util.h +14 -11

@@ -2039,23 +2039,22 @@ gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	TEST_ASSERT(!protected || region->protected_phy_pages,
 		    "Region doesn't support protected memory");
 
-	base = pg = min_gpa >> vm->page_shift;
-	do {
-		for (; pg < base + num; ++pg) {
-			if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
-				base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
-				break;
+	base = min_gpa >> vm->page_shift;
+again:
+	base = (base + align - 1) & ~(align - 1);
+	for (pg = base; pg < base + num; ++pg) {
+		if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
+			base = sparsebit_next_set(region->unused_phy_pages, pg);
+			if (!base) {
+				fprintf(stderr, "No guest physical page available, "
+					"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",
+					min_gpa, vm->page_size, memslot);
+				fputs("---- vm dump ----\n", stderr);
+				vm_dump(stderr, vm, 2);
+				abort();
 			}
+			goto again;
 		}
-	} while (pg && pg != base + num);
-
-	if (pg == 0) {
-		fprintf(stderr, "No guest physical page available, "
-			"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",

Quoted 8 lines

tools/testing/selftests/kvm/include/kvm_util.h gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 2058

Quoted 5 lines

tools/testing/selftests/kvm/include/kvm_util.h +0 -4

-			min_gpa, vm->page_size, memslot);
-		fputs("---- vm dump ----\n", stderr);
-		vm_dump(stderr, vm, 2);
-		abort();
 	}

This is unnecessary churn. I'm not saying the current code is pretty or anything, but unless I'm missing something, this can simply be:

Not really, we need the base to be aligned everytime, that's why the goto again loop aligns the base in the new code.

Plus I feel the above refactoring also simplifies the special handling of pg == 0 case, which earlier was being handled separately after the loop ends.

Quoted 49 lines · Ritesh Harjani

Quoted 46 lines · Sean Christopherson

Quoted 30 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc will require this to allocate MMU tables in guest memory that are larger than guest base page size.

Signed-off-by Nicholas Piggin <[email protected]>

[Rebased to latest mainline tree]

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

---
 .../testing/selftests/kvm/include/kvm_util.h  | 20 +++++++++--
 tools/testing/selftests/kvm/lib/kvm_util.c    | 33 +++++++++----------
 2 files changed, 33 insertions(+), 20 deletions(-)

tools/testing/selftests/kvm/include/kvm_util.h +2 -2

index 3666a8530f31..c515c918c2c9 100644
@@ -991,8 +991,8 @@ void kvm_gsi_routing_write(struct kvm_vm *vm, struct kvm_irq_routing *routing);
 const char *exit_reason_str(unsigned int exit_reason);
 
 gpa_t vm_phy_page_alloc(struct kvm_vm *vm, gpa_t min_gpa, u32 memslot);
-gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, gpa_t min_gpa,
-			   u32 memslot, bool protected);
+gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, size_t align,
+			   gpa_t min_gpa, u32 memslot, bool protected);
 gpa_t vm_alloc_page_table(struct kvm_vm *vm);
 
 static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,

@@ -1003,10 +1003,24 @@ static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	 * protected memory, as the majority of memory for such VMs is

tools/testing/selftests/kvm/include/kvm_util.h static inline gpa_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 1012

Quoted 10 lines

tools/testing/selftests/kvm/include/kvm_util.h +4 -1

 	 * protected, i.e. using shared memory is effectively opt-in.
 	 */
-	return __vm_phy_pages_alloc(vm, num, min_gpa, memslot,
+	return __vm_phy_pages_alloc(vm, num, 1, min_gpa, memslot,
 				    vm_arch_has_protected_memory(vm));
 }
 
+static inline gpa_t vm_phy_pages_alloc_align(struct kvm_vm *vm, size_t num,
+					     size_t align, gpa_t min_gpa,
+					     u32 memslot)

Given that the PPC usage is all for naturally aligned allocations, I think it makes sense for that to be the API, i.e. have "bool naturally_aligned" instead of an arbitrary alignment.

Quoted 2 lines

I would still prefer passing an explicit align value to the allocator, because IMHO that's a useful allocator interface to have.

Why? What are the use cases? I can't think of anything that requires multi-page allocations to have specific alignment, all of the cases I can think of require natural alignment.

For sub-page allocations, supporting semi-arbitrary alignments makes sense, but I'm not convinced we should try and support that for page-granularity allocations.

Quoted 5 lines

However, if we do want to go down this road than I don't have any strong objection either, since as you mentioned powerpc mostly just needs natual alignment for it's page table region. So alignment can be extracted from the region type as you described below, so no need to pass it all the time.

..

Quoted 11 lines

Quoted 1 line

The bonus is that @min_gpa goes away too.

powerpc needs min_gpa for it's exception handling pages. see.

	excp_paddr = vm_phy_pages_alloc(vm, excp_pages, 0,
					vm->memslots[MEM_REGION_DATA]);

	TEST_ASSERT(excp_paddr == 0,
		    "Interrupt vectors not allocated at gPA address 0: (0x%lx)",
		    excp_paddr);

Quoted 2 lines

So, will arch still have an access to the API for passing min_gpa = 0 for cases like above?

Yep, ____vm_phy_pages_alloc() will be globally visible, and I think is quite appropriate in this case since it's more than just specifying a minimum GPA, it's really specifying an _exact_ GPA for the allocation.

gpa_t ____vm_phy_pages_alloc(struct kvm_vm *vm, size_t nr_pages, gpa_t min_gpa,
u32 memslot, bool protected, bool naturally_aligned)

Quoted 8 lines

Quoted 4 lines

It'll probably take me a few days/weeks, but I'll try get a series posted before the 7.2 merge window closes, so that you can build on top to get the PPC selftests support landed in 7.3.

Thanks Sean for your help! Yes, landing kvm selftests for powerpc will be definitely helpful to verify against any kvm regressions.

Quoted 5 lines

BTW, I was thinking whether landing powerpc first will be easier for you to consider all the API requirements from all users / usecases? But either way is fine please. I can work on top of your changes too and if something is missing for powerpc, we can add / modify on top, once your series is finished.

One idea would be for me to include the PPC support in the series; it's "just" a patch or two on top, albeit one pretty big patch.

Quoted 40 lines

Quoted 31 lines

Quoted 31 lines

tools/testing/selftests/kvm/include/kvm_util.h +14 -11

@@ -2039,23 +2039,22 @@ gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	TEST_ASSERT(!protected || region->protected_phy_pages,
 		    "Region doesn't support protected memory");
 
-	base = pg = min_gpa >> vm->page_shift;
-	do {
-		for (; pg < base + num; ++pg) {
-			if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
-				base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
-				break;
+	base = min_gpa >> vm->page_shift;
+again:
+	base = (base + align - 1) & ~(align - 1);
+	for (pg = base; pg < base + num; ++pg) {
+		if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
+			base = sparsebit_next_set(region->unused_phy_pages, pg);
+			if (!base) {
+				fprintf(stderr, "No guest physical page available, "
+					"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",
+					min_gpa, vm->page_size, memslot);
+				fputs("---- vm dump ----\n", stderr);
+				vm_dump(stderr, vm, 2);
+				abort();
 			}
+			goto again;
 		}
-	} while (pg && pg != base + num);
-
-	if (pg == 0) {
-		fprintf(stderr, "No guest physical page available, "
-			"min_gpa: 0x%lx page_size: 0x%x memslot: %u\n",

Quoted 8 lines

tools/testing/selftests/kvm/include/kvm_util.h gpa_t __vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, · line 2058

Quoted 5 lines

tools/testing/selftests/kvm/include/kvm_util.h +0 -4

-			min_gpa, vm->page_size, memslot);
-		fputs("---- vm dump ----\n", stderr);
-		vm_dump(stderr, vm, 2);
-		abort();
 	}

This is unnecessary churn. I'm not saying the current code is pretty or anything, but unless I'm missing something, this can simply be:

Quoted 2 lines

Not really, we need the base to be aligned everytime, that's why the goto again loop aligns the base in the new code.

Ooh, right, pg needs to be advanced by the aligned number of pages. Ugh, the whole do-while part is bizarre, and AFAICT only "works" by dumb luck.

Quoted 2 lines

Plus I feel the above refactoring also simplifies the special handling of pg == 0 case, which earlier was being handled separately after the loop ends.

Yeah, I fiddled with a few other options, but I think I like your approach the most.

Compile tested only at this point, but I now have a series of ~17 patches to yield:
<..>
It'll probably take me a few days/weeks, but I'll try get a series posted before the 7.2 merge window closes, so that you can build on top to get the PPC selftests support landed in 7.3.

From: Nicholas Piggin <[email protected]>

powerpc's maximum permitted vCPU ID depends on the VM's SMT mode, and the maximum reported by KVM_CAP_MAX_VCPU_ID exceeds a simple non-SMT VM's limit.

The powerpc KVM selftest port uses non-SMT VMs, so add a workaround to the kvm_create_max_vcpus test case to limit vCPU IDs to KVM_CAP_MAX_VCPUS on powerpc.

Quoted 6 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc's maximum permitted vCPU ID depends on the VM's SMT mode, and the maximum reported by KVM_CAP_MAX_VCPU_ID exceeds a simple non-SMT VM's limit.

Quoted 3 lines

The powerpc KVM selftest port uses non-SMT VMs, so add a workaround to the kvm_create_max_vcpus test case to limit vCPU IDs to KVM_CAP_MAX_VCPUS on powerpc.

How is this not a KVM bug? Literally the reason this test exists is to validate KVM's advertised KVM_CAP_MAX_VCPU_ID and KVM_CAP_MAX_VCPUS.

Quoted 7 lines · Sean Christopherson

Quoted 6 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc's maximum permitted vCPU ID depends on the VM's SMT mode, and the maximum reported by KVM_CAP_MAX_VCPU_ID exceeds a simple non-SMT VM's limit.

Quoted 6 lines

Quoted 3 lines

The powerpc KVM selftest port uses non-SMT VMs, so add a workaround to the kvm_create_max_vcpus test case to limit vCPU IDs to KVM_CAP_MAX_VCPUS on powerpc.

How is this not a KVM bug? Literally the reason this test exists is to validate KVM's advertised KVM_CAP_MAX_VCPU_ID and KVM_CAP_MAX_VCPUS.

It's not a KVM bug, it's expected on PowerPC. On PowerPC, vCPU ID encodes SMT topology, e.g. on P9, vcpu id = core * stride + thread,
.. where the stride is same as kvm->arch.emul_smt_mode (VM's emulated SMT mode)

So the vcpu ID space can be sparse, however KVM_CAP_MAX_VCPU_ID is the absolute ceil value (MAX_SMT_THREADS * KVM_MAX_VCORES) i.e. the value with the maximum stride / SMT value.

Since default selftest VM uses stride 1, so it rejects IDS >= max_vcpus.

e.g.
static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
...

		if (id >= (KVM_MAX_VCPUS * kvm->arch.emul_smt_mode)) {
			pr_devel("KVM: VCPU ID too high\n");
			core = KVM_MAX_VCORES;              /* rejected case */
		} else {

So, it's expected on PowerPC. vcpus with higher IDs can be created but for that we need to set KVM_CAP_PPC_SMT and use strided (sparse) IDs. But since the test as of now is not doing that - that's the reason why the patch only allows to test max vcpu IDs upto max vcpus.

But I guess you must be hating the #ifdef __powerpc__ there. I agree I don't like it either.. maybe we can do it this way?

(quoted diff) +4 -6

-#ifdef __powerpc64__
        /*
-        * powerpc has a particular format for the vcpu ID that depends on
-        * the guest SMT mode, and the max ID cap is too large for non-SMT
-        * modes, where the maximum ID is the same as the maximum vCPUs.
+        * Some architectures (e.g. powerpc) encode topology into the vCPU ID,
+        * so a default VM can't necessarily use the full advertised ID range.
+        * Let the arch limit the highest ID this test will create.
         */
-       kvm_max_vcpu_id = kvm_max_vcpus;
-#endif
+       kvm_max_vcpu_id = kvm_arch_vcpu_id_limit(kvm_max_vcpus, kvm_max_vcpu_id);

And then in kvm_util.c -

Quoted 34 lines · Ritesh Harjani

Quoted 7 lines · Sean Christopherson

Quoted 6 lines · Ritesh Harjani (IBM)

From: Nicholas Piggin <[email protected]>

powerpc's maximum permitted vCPU ID depends on the VM's SMT mode, and the maximum reported by KVM_CAP_MAX_VCPU_ID exceeds a simple non-SMT VM's limit.

Quoted 6 lines

Quoted 3 lines

The powerpc KVM selftest port uses non-SMT VMs, so add a workaround to the kvm_create_max_vcpus test case to limit vCPU IDs to KVM_CAP_MAX_VCPUS on powerpc.

How is this not a KVM bug? Literally the reason this test exists is to validate KVM's advertised KVM_CAP_MAX_VCPU_ID and KVM_CAP_MAX_VCPUS.

It's not a KVM bug, it's expected on PowerPC. On PowerPC, vCPU ID encodes SMT topology, e.g. on P9, vcpu id = core * stride + thread,
.. where the stride is same as kvm->arch.emul_smt_mode (VM's emulated SMT mode)

So the vcpu ID space can be sparse, however KVM_CAP_MAX_VCPU_ID is the absolute ceil value (MAX_SMT_THREADS * KVM_MAX_VCORES) i.e. the value with the maximum stride / SMT value.

Since default selftest VM uses stride 1, so it rejects IDS >= max_vcpus.

e.g.
static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
...

		if (id >= (KVM_MAX_VCPUS * kvm->arch.emul_smt_mode)) {
			pr_devel("KVM: VCPU ID too high\n");
			core = KVM_MAX_VCORES;              /* rejected case */
		} else {

Quoted 7 lines

So, it's expected on PowerPC. vcpus with higher IDs can be created but for that we need to set KVM_CAP_PPC_SMT and use strided (sparse) IDs. But since the test as of now is not doing that - that's the reason why the patch only allows to test max vcpu IDs upto max vcpus.

But I guess you must be hating the #ifdef __powerpc__ there. I agree I don't like it either.. maybe we can do it this way?

I don't love the #ifdef, but it's more that I didn't want to effectively skip a test because KVM was reporting bad information. But after peeking at KVM_CAP_PPC_SMT, I 100% agree we don't want to deal with that here.

Quoted 15 lines

(quoted diff) +4 -6

-#ifdef __powerpc64__
        /*
-        * powerpc has a particular format for the vcpu ID that depends on
-        * the guest SMT mode, and the max ID cap is too large for non-SMT
-        * modes, where the maximum ID is the same as the maximum vCPUs.
+        * Some architectures (e.g. powerpc) encode topology into the vCPU ID,
+        * so a default VM can't necessarily use the full advertised ID range.
+        * Let the arch limit the highest ID this test will create.
         */
-       kvm_max_vcpu_id = kvm_max_vcpus;
-#endif
+       kvm_max_vcpu_id = kvm_arch_vcpu_id_limit(kvm_max_vcpus, kvm_max_vcpu_id);

And then in kvm_util.c -

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+__weak int kvm_arch_vcpu_id_limit(int nr_vcpus, int vcpu_id_max)
+{
+       return vcpu_id_max;
+}

What if we do this? We're going to be bleeding PPC details into the test no matter what, adding an arch hooks just seems like extra cruft and an unnecessary layer of indirection.

	/*
	 * Skip the vCPU ID test when running on PowerPC with SMT support, in
	 * which case system topology is encoded into the vCPU ID, and so a VM
	 * can't use the full advertised vCPU ID range without crafting a valid
	 * platform specific topology.
	 */
	if (kvm_max_vcpu_id > kvm_max_vcpus && !kvm_check_cap(KVM_CAP_PPC_SMT))
		test_vcpu_creation(
			kvm_max_vcpu_id - kvm_max_vcpus, kvm_max_vcpus);

Print the vcpu_id and errno when the KVM_CREATE_VCPU ioctl fails, for debug purposes.
This helped in debugging an issue with KVM on PowerVM, where KVM_CREATE_VCPU only supports max 2048 vcpus, because the PAPR_HCALL H_GUEST_CREATE_VCPU ("Documentation/arch/powerpc/kvm-nested.rst") supports only up to 0-2047 vcpu_id. However KVM_CAP_MAX_VCPUS capability extension always reports max_vcpus as NR_CPUS of the host.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

---
 tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++++++
 1 file changed, 7 insertions(+)

tools/testing/selftests/kvm/lib/kvm_util.c +0 -0

index e00a25f59416..6319e2037882 100644

Nicholas Piggin (4):
KVM: selftests: Move pgd_created check into virt_pgd_alloc
KVM: selftests: Add aligned guest physical page allocator
KVM: PPC: selftests: add support for powerpc
KVM: PPC: selftests: powerpc enable kvm_create_max_vcpus test

Ritesh Harjani (IBM) (6):
KVM: selftests: Print the vcpu_id when KVM_CREATE_VCPU ioctl fails
KVM: PPC: selftests: Use u64 instead of uint64_t
KVM: PPC: selftests: Use s64 instead of int64_t
KVM: PPC: selftests: Use u32 instead of uint32_t
KVM: PPC: selftests: Use u8 instead of uint8_t
KVM: PPC: selftests: Replace u64 gpa, u64 gva|vaddr with gpa_t and
gva_t

Quoted 6 lines · Ritesh Harjani (IBM)

Nicholas Piggin (4):
KVM: selftests: Move pgd_created check into virt_pgd_alloc
KVM: selftests: Add aligned guest physical page allocator
KVM: PPC: selftests: add support for powerpc
KVM: PPC: selftests: powerpc enable kvm_create_max_vcpus test

Quoted 8 lines

Ritesh Harjani (IBM) (6):
KVM: selftests: Print the vcpu_id when KVM_CREATE_VCPU ioctl fails
KVM: PPC: selftests: Use u64 instead of uint64_t
KVM: PPC: selftests: Use s64 instead of int64_t
KVM: PPC: selftests: Use u32 instead of uint32_t
KVM: PPC: selftests: Use u8 instead of uint8_t
KVM: PPC: selftests: Replace u64 gpa, u64 gva|vaddr with gpa_t and
gva_t

Squash these typedef changes into the patch where the "bad" typedefs are used. Give yourself attribution for Co-developed-by if desired, but don't introduce code only to immediately fix it.

One other question - Should this go via powerpc tree or via kvm tree?