Re: [RFC v2 3/5] KVM: PPC: selftests: add support for powerpc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Guest registers r1 (stack pointer) and r2 (TOC pointer) are not saved to or restored from ex_regs during exception handling.
- [Medium] Overly restrictive PTE assertions in addr_arch_gva2gpa() prevent address translation for non-RWX pages.
--

commit b672003494da996fee181637da999ccf8c28f3e8
Author: Nicholas Piggin <npiggin@gmail.com>
Subject: KVM: PPC: selftests: add support for powerpc

This commit implements KVM selftests support for powerpc (Book3S-64).
It includes virtual memory support for the radix MMU, ucall implementation
via PAPR hcalls, and real-mode guest interrupt handling.

> diff --git a/tools/testing/selftests/kvm/lib/powerpc/handlers.S b/tools/testing/selftests/kvm/lib/powerpc/handlers.S
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/powerpc/handlers.S
[ ... ]
> +virt_handle_interrupt:
> +	stdu	%r1,-INT_FRAME_SIZE(%r1)
> +	mr	%r3,%r31
> +	bl	route_interrupt
> +	ld	%r4,(32*8)(%r31) /* NIA */
> +	ld	%r5,(33*8)(%r31) /* MSR */
> +	ld	%r6,(35*8)(%r31) /* LR */
> +	ld	%r7,(36*8)(%r31) /* CTR */
> +	ld	%r8,(37*8)(%r31) /* XER */
> +	lwz	%r9,(38*8)(%r31) /* CR */
> +	mtspr	SPR_SRR0,%r4
> +	mtspr	SPR_SRR1,%r5
> +	mtlr	%r6
> +	mtctr	%r7
> +	mtxer	%r8
> +	mtcr	%r9
> +reg=4
> +	ld	%r0,(0*8)(%r31)
> +	ld	%r3,(3*8)(%r31)
> +.rept 28
> +	ld	reg,(reg*8)(%r31)
> +	reg=reg+1
> +.endr

[Severity: Medium]
Are r1 (stack pointer) and r2 (TOC pointer) missing from the restore sequence?

Since they aren't restored here, tests cannot modify the stack pointer during
exception handling.

[ ... ]
> +handle_interrupt:
> +reg=4
> +.rept 28
> +	std	reg,(reg*8)(%r13)
> +	reg=reg+1
> +.endr

[Severity: Medium]
Does this sequence skip saving r1 and r2 into ex_regs?

If they are skipped, the C handlers receive uninitialized values for these
registers in ex_regs->gprs[1] and gprs[2], meaning they cannot inspect the
stack pointer or TOC pointer at the time of the exception.

> diff --git a/tools/testing/selftests/kvm/lib/powerpc/processor.c b/tools/testing/selftests/kvm/lib/powerpc/processor.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/powerpc/processor.c
[ ... ]
> +gpa_t addr_arch_gva2gpa(struct kvm_vm *vm, gva_t gva)
> +{
[ ... ]
> +	ptep = virt_get_pte(vm, pt, gva, level, NULL);
> +	pte = be64_to_cpu(*ptep);
> +
> +	TEST_ASSERT(pte,
> +		"PTE not present at level: %u gva: 0x%lx pte:0x%lx\n",
> +		level, gva, pte);
> +
> +	TEST_ASSERT((pte & PTE_VALID) && (pte & PTE_LEAF) &&
> +		    (pte & PTE_READ) && (pte & PTE_RW) && (pte & PTE_EXEC),
> +		    "PTE not valid at level: %u gva: 0x%lx pte:0x%lx\n",
> +		    level, gva, pte);

[Severity: Medium]
Is it possible for tests to map memory without write or execute permissions
(e.g., read-only or non-executable pages)?

If so, this assertion will falsely trigger and abort the test during virtual
to physical address translation. Should this check only verify PTE_VALID and
PTE_LEAF to support testing diverse page permissions?

> +
> +	return (pte & PTE_PAGE_MASK) + (gva & (vm->page_size - 1));
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779524962.git.ritesh.list@gmail.com?part=3