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:

quoted

On Fri, May 10, 2024 at 12:26:38PM +0100, Pierre-Clément Tosi wrote:

quoted

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@google.com>
+ */
+
+#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:

quoted

+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@google.com/ (local)
[2]: https://lore.kernel.org/kvmarm/867ci10zv6.wl-maz@kernel.org/ (local)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel