Re: [PATCH 1/2] KVM: PPC: Add kvm selftests support for powerpc

["Nicholas Piggin" <npiggin@gmail.com>]

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, &regs);
> > +	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