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

[Sean Christopherson <seanjc@google.com>]

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.

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)

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

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