Sean Christopherson <[email protected]> writes:
> 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.
>
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
