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

[Ricardo Koller <ricarkol@google.com>]

On Fri, Apr 23, 2021 at 01:05:29PM +0200, Andrew Jones wrote:
> 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
> 

Thank you Andrew and Marc for the reviews. Will send v2 with all the
feedback.