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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
