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.
