On Wed, May 29, 2024 at 01:26:31PM +0100, Pierre-Clément Tosi wrote:
> On Mon, May 13, 2024 at 06:21:21PM +0100, Will Deacon wrote:
> > On Fri, May 10, 2024 at 12:26:38PM +0100, Pierre-Clément Tosi wrote:
> > > diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h
> > > new file mode 100644
> > > index 000000000000..13cc7b19d838
> > > --- /dev/null
> > > +++ b/arch/arm64/include/asm/kvm_cfi.h
> > > @@ -0,0 +1,36 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (C) 2024 - Google Inc
> > > + * Author: Pierre-Clément Tosi <ptosi@xxxxxxxxxx>
> > > + */
> > > +
> > > +#ifndef __ARM64_KVM_CFI_H__
> > > +#define __ARM64_KVM_CFI_H__
> > > +
> > > +#include <asm/kvm_asm.h>
> > > +#include <linux/errno.h>
> > > +
> > > +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
> > > +
> > > +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void));
> > > +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void));
> >
> > Hmm, I tend to think this indirection is a little over the top for a test
> > module that only registers a small handful of handlers:
> >
> > > +static int set_param_mode(const char *val, const struct kernel_param *kp,
> > > + int (*register_cb)(void (*)(void)))
> > > +{
> > > + unsigned int *mode = kp->arg;
> > > + int err;
> > > +
> > > + err = param_set_uint(val, kp);
> > > + if (err)
> > > + return err;
> > > +
> > > + switch (*mode) {
> > > + case 0:
> > > + return register_cb(NULL);
> > > + case 1:
> > > + return register_cb(hyp_trigger_builtin_cfi_fault);
> > > + case 2:
> > > + return register_cb((void *)hyp_cfi_builtin2module_test_target);
> > > + case 3:
> > > + return register_cb(trigger_module2builtin_cfi_fault);
> > > + case 4:
> > > + return register_cb(trigger_module2module_cfi_fault);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> >
> > Why not just have a hyp selftest that runs through all of this behind a
> > static key? I think it would simplify the code quite a bit, and you could
> > move the registration and indirection logic.
>
> I agree that the code would be simpler but note that the resulting tests would
> have a more limited coverage compared to what this currently implements. In
> particular, they would likely miss issues with the failure path itself (e.g.
> [1]) as the synchronous exception would need to be "handled" to let the selftest
> complete. OTOH, that would have the benefit of not triggering a kernel panic,
> making the test easier to integrate into existing CI suites.
>
> However, as the original request for those tests [2] was specifically about
> testing the failure path, I've held off from modifying the test module (in v4)
> until I get confirmation that Marc would be happy with your suggestion.
>
> [1]: https://lore.kernel.org/kvmarm/20240529121251.1993135-2-ptosi@xxxxxxxxxx/
> [2]: https://lore.kernel.org/kvmarm/867ci10zv6.wl-maz@xxxxxxxxxx/
In which case, I think I'd drop the tests for now because I think the cure
is worse than the disease with the current implementation.
Will
