Re: [PATCH v3 RESEND 02/10] KVM: selftests: Add aligned guest physical page allocator

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Sean Christopherson <seanjc@google.com> writes:

> On Thu, Jun 11, 2026, Ritesh Harjani wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > On Wed, Jun 10, 2026, Ritesh Harjani (IBM) wrote:
>> >> From: Nicholas Piggin <npiggin@gmail.com>
>> >> 
>> >> powerpc will require this to allocate MMU tables in guest memory that
>> >> are larger than guest base page size.
>> >> 
>> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> >> [Rebased to latest mainline tree]
>> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> >> ---
>> >>  .../testing/selftests/kvm/include/kvm_util.h  | 20 +++++++++--
>> >>  tools/testing/selftests/kvm/lib/kvm_util.c    | 33 +++++++++----------
>> >>  2 files changed, 33 insertions(+), 20 deletions(-)
>> >> 
>> >> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
>> >> index 3666a8530f31..c515c918c2c9 100644
>> >> --- a/tools/testing/selftests/kvm/include/kvm_util.h
>> >> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>> >> @@ -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));
>> >>  }
>> >>  
>> >> +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.
>
> 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.
>

I agree, that's what I mentioned too, so far all cases we care about,
they require natual alignment. So it should be fine.

> 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.
>
>> 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.
>
> ..
>
>> > 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?
>
> 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)
>

Right, Thanks!

>> > 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.
>
> 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.
>


That should be ok Sean. I can rebase my changes on top of yours once
your series is ready then. I will be on travel next week (OSS Mumbai)
and I don't want to stop your patch series due to my late replies for
any of the powerpc part.

So once your series is ready - I can rebase this series on top of it and
it can go as we discussed it earlier - so that we can try and land these
for 7.3, if possible.

Thanks again for your help :)

>> >> @@ -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",
>> >> -			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.
>
> 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. 
>
>> 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.

Sure then, Thanks!


-ritesh