On Thu, Jun 11, 2026, Ritesh Harjani wrote:
> Sean Christopherson <[email protected]> writes:
>
> > On Wed, Jun 10, 2026, Ritesh Harjani (IBM) wrote:
> >> 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(-)
> >>
> >> 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.
