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) continueto 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; // LElpcr |= 0x0000000002000000; // ILEWould 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