Hey thanks for the review. Points about formatting and style all
valid, I'll tidy those up. For the others,
On Thu Mar 30, 2023 at 6:19 AM AEST, Sean Christopherson wrote:
> On Thu, Mar 16, 2023, Nicholas Piggin wrote:
> > +#ifdef __powerpc__
> > + TEST_ASSERT(getpagesize() == 64*1024,
>
> This can use SZ_64K (we really need to convert a bunch of open coded stuff...)
>
> > + "KVM selftests requires 64K host page size\n");
>
> What is the actual requirement? E.g. is it that the host and guest page sizes
> must match, or is that the selftest setup itself only supports 64KiB pages? If
> it's the former, would it make sense to assert outside of the switch statement, e.g.
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 298c4372fb1a..920813a71be0 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -291,6 +291,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
> #ifdef __aarch64__
> if (vm->pa_bits != 40)
> vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
> +#endif
> +#ifdef __powerpc__
> + TEST_ASSERT(getpagesize() == vm->page_size, "blah blah blah");
> +
> #endif
>
> vm_open(vm);
>
> If it's the latter (selftests limitation), can you add a comment explaining the
> limitation?
It's the selftests setup, requires both host and guest to be 64k page
size. I think it shouldn't be *too* hard to add any mix of 64k/4k, but
there are a few quirks like requiring pgd to have 64k size allocation.
64/64 is the most important for us, but it would be nice to get other
combos working soon if nothing else than because they don't get as much
testing in other ways.
I can add a comment.
> > +
> > + /* If needed, create page table */
> > + if (vm->pgd_created)
> > + return;
>
> Heh, every arch has this. Any objection to moving the check to virt_pgd_alloc()
> as a prep patch?
I have no objection, I can do that for the next spin.
> > + "PTE not valid at level: %u gva: 0x%lx pte:0x%lx\n",
> > + level, gva, pte);
> > +
> > + return (pte & PTE_PAGE_MASK) + (gva & (vm->page_size - 1));
> > +}
> > +
> > +static void virt_arch_dump_pt(FILE *stream, struct kvm_vm *vm, vm_paddr_t pt, vm_vaddr_t va, int level, uint8_t indent)
>
> And here. Actually, why bother with the helper? There's one caller, and that
> callers checks pgd_created, i.e. is already assuming its dumping only page tables.
> Ooh, nevermind, it's recursive.
>
> Can you drop "arch" from the name? Selftests uses "arch" to tag functions that
> are provided by arch code for use in generic code.
Yeah agree, I'll drop that.
> > + } else {
> > + virt_arch_dump_pt(stream, vm, pte & PDE_PT_MASK, va, level + 1, indent);
> > + }
> > + }
> > + va += pt_entry_coverage(vm, level);
>
> The shift is constant for vm+level, correct? In that case, can't this be written
> as
>
> for (idx = 0; idx < size; idx++, va += va_coverage) {
>
> or even without a snapshot
>
> for (idx = 0; idx < size; idx++, va += pt_entry_coverage(vm, level)) {
>
> That would allow
>
> if (!(pte & PTE_VALID)
> continue
>
> to reduce the indentation of the printing.
It is constant for a given (vm, level). Good thinking, that should work.
> > + stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
> > + DEFAULT_GUEST_STACK_VADDR_MIN,
> > + MEM_REGION_DATA);
> > +
> > + vcpu = __vm_vcpu_add(vm, vcpu_id);
> > +
> > + vcpu_enable_cap(vcpu, KVM_CAP_PPC_PAPR, 1);
> > +
> > + /* Setup guest registers */
> > + vcpu_regs_get(vcpu, ®s);
> > + vcpu_get_reg(vcpu, KVM_REG_PPC_LPCR_64, &lpcr);
> > +
> > + regs.pc = (uintptr_t)guest_code;
> > + regs.gpr[12] = (uintptr_t)guest_code;
> > + regs.msr = 0x8000000002103032ull;
> > + regs.gpr[1] = stack_vaddr + stack_size - 256;
> > +
> > + if (BYTE_ORDER == LITTLE_ENDIAN) {
> > + regs.msr |= 0x1; // LE
> > + lpcr |= 0x0000000002000000; // ILE
>
> Would it be appropriate to add #defines to processor.h instead of open coding the
> magic numbers?
Yes it would. I should have not been lazy about it from the start, will
fix.
(Other comments snipped but agreed for all)
Thanks,
Nick
