Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

From: Andrew Jones <drjones@redhat.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64
Date: Fri, 23 Apr 2021 13:05:29 +0200	[thread overview]
Message-ID: <20210423110529.vivemdwnznhblhyf@gator> (raw)
In-Reply-To: <87sg3hnzrj.wl-maz@kernel.org>

On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> Thanks for starting this.

Indeed! Thank you for contributing to AArch64 kvm selftests!

> > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > +			void (*handler)(struct ex_regs *));
> > +
> > +#define SPSR_D          (1 << 9)
> > +#define SPSR_SS         (1 << 21)
> > +
> > +#define write_sysreg(reg, val)						  \
> > +({									  \
> > +	asm volatile("msr "__stringify(reg)", %0" : : "r"(val));	  \
> > +})

Linux does fancy stuff with the Z constraint to allow xzr. We might as
well copy that.

> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > new file mode 100644
> > index 000000000000..c920679b87c0
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > @@ -0,0 +1,104 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +.macro save_registers, el
> > +	stp	x28, x29, [sp, #-16]!
> > +	stp	x26, x27, [sp, #-16]!
> > +	stp	x24, x25, [sp, #-16]!
> > +	stp	x22, x23, [sp, #-16]!
> > +	stp	x20, x21, [sp, #-16]!
> > +	stp	x18, x19, [sp, #-16]!
> > +	stp	x16, x17, [sp, #-16]!
> > +	stp	x14, x15, [sp, #-16]!
> > +	stp	x12, x13, [sp, #-16]!
> > +	stp	x10, x11, [sp, #-16]!
> > +	stp	x8, x9, [sp, #-16]!
> > +	stp	x6, x7, [sp, #-16]!
> > +	stp	x4, x5, [sp, #-16]!
> > +	stp	x2, x3, [sp, #-16]!
> > +	stp	x0, x1, [sp, #-16]!
> > +
> > +	.if \el == 0
> > +	mrs	x1, sp_el0
> > +	.else
> > +	mov	x1, sp
> > +	.endif
> 
> It there any point in saving SP_EL1, given that you already have
> altered it significantly and will not be restoring it? I don't care
> much, and maybe it is useful as debug information, but a comment would
> certainly make the intent clearer.

kvm-unit-tests takes some pains to save the original sp. We may be able to
take some inspiration from there for this save and restore.

> > +void kvm_exit_unexpected_vector(int vector, uint64_t ec)
> > +{
> > +	ucall(UCALL_UNHANDLED, 2, vector, ec);
> > +}
> > +
> > +#define HANDLERS_IDX(_vector, _ec)	((_vector * ESR_EC_NUM) + _ec)
> 
> This is definitely odd. Not all the ECs are valid for all vector entry
> points. Actually, ECs only make sense for synchronous exceptions, and
> asynchronous events (IRQ, FIQ, SError) cannot populate ESR_ELx.

For this, kvm-unit-tests provides a separate API for interrupt handler
installation, which ensures ec is not used. Also, kvm-unit-tests uses
a 2-D array [vector][ec] for the synchronous exceptions. I think we
should be able to use a 2-D array here too, instead of the IDX macro.

> > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > +			 void (*handler)(struct ex_regs *))
> 
> The name seems to be slightly ill defined. To me "handle exception" is
> the action of handling the exception. Here, you are merely installing
> an exception handler.
>

I agree. Please rename this for all of kvm selftests to something with
'install' in the name with the first patch of this series.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm