Re: [PATCH v5 1/9] KVM: PPC: selftests: add support for powerpc

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] kvm: selftests: missing save/restore of %r1 and %r2 in ex_regs
> --
>
> commit 2e259a0445aae14d6a7262b100f4677735b2217f
> Author: Nicholas Piggin <npiggin@gmail.com>
>
> KVM: PPC: selftests: add support for powerpc
>
> This commit implements KVM selftests support for powerpc (Book3S-64). It
> provides virtual memory implementation for the radix MMU, ucalls using an
> unsupported PAPR hcall number, and real-mode guest interrupts.
>
> Link: https://lore.kernel.org/linuxppc-dev/cover.1781093720.git.ritesh.list@gmail.com/
>
>> diff --git a/tools/testing/selftests/kvm/lib/powerpc/handlers.S b/tools/testing/selftests/kvm/lib/powerpc/handlers.S
>> new file mode 100644
>> index 0000000000000..b860f6a520a13
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/lib/powerpc/handlers.S
>
> [ ... ]
>
>> +virt_handle_interrupt:
>> +	stdu	%r1,-INT_FRAME_SIZE(%r1)
>> +	mr	%r3,%r31
>
> [ ... ]
>
>> +	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]
> Does this restore loop in virt_handle_interrupt intentionally skip restoring
> %r1 (stack pointer) and %r2 (TOC base)?
>
> Because the loop starts at reg=4 and ends at %r31, any changes made to the
> stack pointer or TOC base by the C exception handler in the ex_regs
> structure will be silently ignored when returning to the guest.
>

That is intentional since it is unnecessary.


>> +	addi	%r1,%r1,INT_FRAME_SIZE
>> +	rfid
>> +
>> +virt_handle_interrupt_p:
>> +	.llong virt_handle_interrupt
>> +
>> +handle_interrupt:
>> +reg=4
>> +.rept 28
>> +	std	reg,(reg*8)(%r13)
>> +	reg=reg+1
>> +.endr
>
> [Severity: Medium]
> Similarly, does handle_interrupt skip saving %r1 and %r2 into the ex_regs
> structure before routing to the C handler? 
>
> If a selftest takes an exception and attempts to inspect the interrupted
> stack or TOC by reading regs->gprs[1] or regs->gprs[2], won't it receive
> uninitialized values instead of the actual register states?
>

ditto. None of the tests depend on it.

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