Introduce a KVM selftest to check the hypercall interface
for arm64 platforms. The test validates the user-space's
IOCTL interface to read/write the psuedo-firmware registers
as well as its effects on the guest upon certain configurations.
Signed-off-by: Raghavendra Rao Ananta <[email protected]>
---
tools/testing/selftests/kvm/.gitignore | 1 +
tools/testing/selftests/kvm/Makefile | 1 +
.../selftests/kvm/aarch64/hypercalls.c | 344 ++++++++++++++++++
3 files changed, 346 insertions(+)
create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c
diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index e82b816a6608..7ef52b3b1560 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -2,6 +2,7 @@
/aarch64/arch_timer
/aarch64/debug-exceptions
/aarch64/get-reg-list
+/aarch64/hypercalls
/aarch64/psci_test
/aarch64/vgic_init
/aarch64/vgic_irq
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 2f74f502de65..af4cb88bcf83 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -105,6 +105,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
+TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
TEST_GEN_PROGS_aarch64 += aarch64/psci_test
TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
new file mode 100644
index 000000000000..9941fb75772a
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* hypercalls: Check the ARM64's psuedo-firmware bitmap register interface.
+ *
+ * The test validates the basic hypercall functionalities that are exposed
+ * via the psuedo-firmware bitmap register. This includes the registers'
+ * read/write behavior before and after the VM has started, and if the
+ * hypercalls are properly masked or unmasked to the guest when disabled or
+ * enabled from the KVM userspace, respectively.
+ */
+
+#include <errno.h>
+#include <linux/arm-smccc.h>
+#include <asm/kvm.h>
+#include <kvm_util.h>
+
+#include "processor.h"
+
+#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK_ULL(max_feat_bit, 0))
+
+/* Last valid bits of the bitmapped firmware registers */
+#define KVM_REG_ARM_STD_BMAP_BIT_MAX 0
+#define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0
+#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 1
+
+struct kvm_fw_reg_info {
+ uint64_t reg; /* Register definition */
+ uint64_t max_feat_bit; /* Bit that represents the upper limit of the feature-map */
+};
+
+#define FW_REG_INFO(r, bit_max) \
+ { \
+ .reg = r, \
+ .max_feat_bit = bit_max, \
+ }
+
+static const struct kvm_fw_reg_info fw_reg_info[] = {
+ FW_REG_INFO(KVM_REG_ARM_STD_BMAP, KVM_REG_ARM_STD_BMAP_BIT_MAX),
+ FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP, KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX),
+ FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP, KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX),
+};
+
+enum test_stage {
+ TEST_STAGE_REG_IFACE,
+ TEST_STAGE_HVC_IFACE_FEAT_DISABLED,
+ TEST_STAGE_HVC_IFACE_FEAT_ENABLED,
+ TEST_STAGE_HVC_IFACE_FALSE_INFO,
+ TEST_STAGE_END,
+};
+
+static int stage;
+
+struct test_hvc_info {
+ uint32_t func_id;
+ int64_t arg0;
+};
+
+#define TEST_HVC_INFO(f, a0) \
+ { \
+ .func_id = f, \
+ .arg0 = a0, \
+ }
+
+static const struct test_hvc_info hvc_info[] = {
+ /* KVM_REG_ARM_STD_BMAP */
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_VERSION, 0),
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_TRNG_RND64),
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_GET_UUID, 0),
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_RND32, 0),
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_RND64, 0),
+
+ /* KVM_REG_ARM_STD_HYP_BMAP */
+ TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_HV_PV_TIME_FEATURES),
+ TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_HV_PV_TIME_ST),
+ TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_ST, 0),
+
+ /* KVM_REG_ARM_VENDOR_HYP_BMAP */
+ TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
+ ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
+ TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, KVM_PTP_VIRT_COUNTER),
+};
+
+/* Feed false hypercall info to test the KVM behavior */
+static const struct test_hvc_info false_hvc_info[] = {
+ /* Feature support check against a different family of hypercalls */
+ TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
+ TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_TRNG_RND64),
+ TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_TRNG_RND64),
+};
+
+static void guest_test_hvc(const struct test_hvc_info *hc_info)
+{
+ unsigned int i;
+ struct arm_smccc_res res;
+ unsigned int hvc_info_arr_sz;
+
+ hvc_info_arr_sz =
+ hc_info == hvc_info ? ARRAY_SIZE(hvc_info) : ARRAY_SIZE(false_hvc_info);
+
+ for (i = 0; i < hvc_info_arr_sz; i++, hc_info++) {
+
+ memset(&res, 0, sizeof(res));
+ smccc_hvc(hc_info->func_id, hc_info->arg0, 0, 0, 0, 0, 0, 0, &res);
+
+ switch (stage) {
+ case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+ case TEST_STAGE_HVC_IFACE_FALSE_INFO:
+ GUEST_ASSERT_3(res.a0 == SMCCC_RET_NOT_SUPPORTED,
+ res.a0, hc_info->func_id, hc_info->arg0);
+ break;
+ case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+ GUEST_ASSERT_3(res.a0 != SMCCC_RET_NOT_SUPPORTED,
+ res.a0, hc_info->func_id, hc_info->arg0);
+ break;
+ default:
+ GUEST_ASSERT_1(0, stage);
+ }
+ }
+}
+
+static void guest_code(void)
+{
+ while (stage != TEST_STAGE_END) {
+ switch (stage) {
+ case TEST_STAGE_REG_IFACE:
+ break;
+ case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+ case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+ guest_test_hvc(hvc_info);
+ break;
+ case TEST_STAGE_HVC_IFACE_FALSE_INFO:
+ guest_test_hvc(false_hvc_info);
+ break;
+ default:
+ GUEST_ASSERT_1(0, stage);
+ }
+
+ GUEST_SYNC(stage);
+ }
+
+ GUEST_DONE();
+}
+
+static int set_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t val)
+{
+ struct kvm_one_reg reg = {
+ .id = id,
+ .addr = (uint64_t)&val,
+ };
+
+ return _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, ®);
+}
+
+static void get_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t *addr)
+{
+ struct kvm_one_reg reg = {
+ .id = id,
+ .addr = (uint64_t)addr,
+ };
+
+ vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, ®);
+}
+
+struct st_time {
+ uint32_t rev;
+ uint32_t attr;
+ uint64_t st_time;
+};
+
+#define STEAL_TIME_SIZE ((sizeof(struct st_time) + 63) & ~63)
+#define ST_GPA_BASE (1 << 30)
+
+static void steal_time_init(struct kvm_vm *vm)
+{
+ uint64_t st_ipa = (ulong)ST_GPA_BASE;
+ unsigned int gpages;
+ struct kvm_device_attr dev = {
+ .group = KVM_ARM_VCPU_PVTIME_CTRL,
+ .attr = KVM_ARM_VCPU_PVTIME_IPA,
+ .addr = (uint64_t)&st_ipa,
+ };
+
+ gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE);
+ vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
+
+ vcpu_ioctl(vm, 0, KVM_SET_DEVICE_ATTR, &dev);
+}
+
+static void test_fw_regs_before_vm_start(struct kvm_vm *vm)
+{
+ uint64_t val;
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
+ const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
+
+ /* First 'read' should be an upper limit of the features supported */
+ get_fw_reg(vm, reg_info->reg, &val);
+ TEST_ASSERT(val == FW_REG_ULIMIT_VAL(reg_info->max_feat_bit),
+ "Expected all the features to be set for reg: 0x%lx; expected: 0x%llx; read: 0x%lx\n",
+ reg_info->reg, GENMASK_ULL(reg_info->max_feat_bit, 0), val);
+
+ /* Test a 'write' by disabling all the features of the register map */
+ ret = set_fw_reg(vm, reg_info->reg, 0);
+ TEST_ASSERT(ret == 0,
+ "Failed to clear all the features of reg: 0x%lx; ret: %d\n",
+ reg_info->reg, errno);
+
+ get_fw_reg(vm, reg_info->reg, &val);
+ TEST_ASSERT(val == 0,
+ "Expected all the features to be cleared for reg: 0x%lx\n", reg_info->reg);
+
+ /*
+ * Test enabling a feature that's not supported.
+ * Avoid this check if all the bits are occupied.
+ */
+ if (reg_info->max_feat_bit < 63) {
+ ret = set_fw_reg(vm, reg_info->reg, BIT(reg_info->max_feat_bit + 1));
+ TEST_ASSERT(ret != 0 && errno == EINVAL,
+ "Unexpected behavior or return value (%d) while setting an unsupported feature for reg: 0x%lx\n",
+ errno, reg_info->reg);
+ }
+ }
+}
+
+static void test_fw_regs_after_vm_start(struct kvm_vm *vm)
+{
+ uint64_t val;
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
+ const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
+
+ /*
+ * Before starting the VM, the test clears all the bits.
+ * Check if that's still the case.
+ */
+ get_fw_reg(vm, reg_info->reg, &val);
+ TEST_ASSERT(val == 0,
+ "Expected all the features to be cleared for reg: 0x%lx\n",
+ reg_info->reg);
+
+ /*
+ * Test setting the last read value. KVM should allow this
+ * even if VM has started running.
+ */
+ ret = set_fw_reg(vm, reg_info->reg, val);
+ TEST_ASSERT(ret == 0,
+ "Failed to set the register with previously read value after Vm start for reg: 0x%lx; ret: %d\n",
+ reg_info->reg, errno);
+
+ /*
+ * Set all the features for this register again. KVM shouldn't
+ * allow this as the VM is running.
+ */
+ ret = set_fw_reg(vm, reg_info->reg, FW_REG_ULIMIT_VAL(reg_info->max_feat_bit));
+ TEST_ASSERT(ret != 0 && errno == EBUSY,
+ "Unexpected behavior or return value (%d) while setting a feature while VM is running for reg: 0x%lx\n",
+ errno, reg_info->reg);
+ }
+}
+
+static struct kvm_vm *test_vm_create(void)
+{
+ struct kvm_vm *vm;
+
+ vm = vm_create_default(0, 0, guest_code);
+
+ ucall_init(vm, NULL);
+ steal_time_init(vm);
+
+ return vm;
+}
+
+static struct kvm_vm *test_guest_stage(struct kvm_vm *vm)
+{
+ struct kvm_vm *ret_vm = vm;
+
+ pr_debug("Stage: %d\n", stage);
+
+ switch (stage) {
+ case TEST_STAGE_REG_IFACE:
+ test_fw_regs_after_vm_start(vm);
+ break;
+ case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+ /* Start a new VM so that all the features are now enabled by default */
+ kvm_vm_free(vm);
+ ret_vm = test_vm_create();
+ break;
+ case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+ case TEST_STAGE_HVC_IFACE_FALSE_INFO:
+ break;
+ default:
+ TEST_FAIL("Unknown test stage: %d\n", stage);
+ }
+
+ stage++;
+ sync_global_to_guest(vm, stage);
+
+ return ret_vm;
+}
+
+static void test_run(void)
+{
+ struct kvm_vm *vm;
+ struct ucall uc;
+ bool guest_done = false;
+
+ vm = test_vm_create();
+
+ test_fw_regs_before_vm_start(vm);
+
+ while (!guest_done) {
+ vcpu_run(vm, 0);
+
+ switch (get_ucall(vm, 0, &uc)) {
+ case UCALL_SYNC:
+ vm = test_guest_stage(vm);
+ break;
+ case UCALL_DONE:
+ guest_done = true;
+ break;
+ case UCALL_ABORT:
+ TEST_FAIL("%s at %s:%ld\n\tvalues: 0x%lx, 0x%lx; 0x%lx, stage: %u",
+ (const char *)uc.args[0], __FILE__, uc.args[1],
+ uc.args[2], uc.args[3], uc.args[4], stage);
+ break;
+ default:
+ TEST_FAIL("Unexpected guest exit\n");
+ }
+ }
+
+ kvm_vm_free(vm);
+}
+
+int main(void)
+{
+ setbuf(stdout, NULL);
+
+ test_run();
+ return 0;
+}
--
2.35.1.1094.g7c7d902a7c-goog
Hi Raghavendra,
On 4/7/22 9:16 AM, Raghavendra Rao Ananta wrote:
> Introduce a KVM selftest to check the hypercall interface
> for arm64 platforms. The test validates the user-space's
> IOCTL interface to read/write the psuedo-firmware registers
> as well as its effects on the guest upon certain configurations.
>
> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> ---
> tools/testing/selftests/kvm/.gitignore | 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/aarch64/hypercalls.c | 344 ++++++++++++++++++
> 3 files changed, 346 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c
>
To be more precise, s/IOCTL/{GET,SET}_ONE_REG ?
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index e82b816a6608..7ef52b3b1560 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -2,6 +2,7 @@
> /aarch64/arch_timer
> /aarch64/debug-exceptions
> /aarch64/get-reg-list
> +/aarch64/hypercalls
> /aarch64/psci_test
> /aarch64/vgic_init
> /aarch64/vgic_irq
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 2f74f502de65..af4cb88bcf83 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -105,6 +105,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
> TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
> TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> +TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
> TEST_GEN_PROGS_aarch64 += aarch64/psci_test
> TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
> TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
> diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
> new file mode 100644
> index 000000000000..9941fb75772a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* hypercalls: Check the ARM64's psuedo-firmware bitmap register interface.
> + *
> + * The test validates the basic hypercall functionalities that are exposed
> + * via the psuedo-firmware bitmap register. This includes the registers'
> + * read/write behavior before and after the VM has started, and if the
> + * hypercalls are properly masked or unmasked to the guest when disabled or
> + * enabled from the KVM userspace, respectively.
> + */
> +
> +#include <errno.h>
> +#include <linux/arm-smccc.h>
> +#include <asm/kvm.h>
> +#include <kvm_util.h>
> +
> +#include "processor.h"
> +
> +#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK_ULL(max_feat_bit, 0))
> +
> +/* Last valid bits of the bitmapped firmware registers */
> +#define KVM_REG_ARM_STD_BMAP_BIT_MAX 0
> +#define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0
> +#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 1
> +
> +struct kvm_fw_reg_info {
> + uint64_t reg; /* Register definition */
> + uint64_t max_feat_bit; /* Bit that represents the upper limit of the feature-map */
> +};
> +
> +#define FW_REG_INFO(r, bit_max) \
> + { \
> + .reg = r, \
> + .max_feat_bit = bit_max, \
> + }
> +
> +static const struct kvm_fw_reg_info fw_reg_info[] = {
> + FW_REG_INFO(KVM_REG_ARM_STD_BMAP, KVM_REG_ARM_STD_BMAP_BIT_MAX),
> + FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP, KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX),
> + FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP, KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX),
> +};
> +
This can be simplifed by:
#define FW_REG_INFO(r) \
{ .reg = r, \
.max_feat_bit = r_##BIT_MAX, \
}
static const struct kvm_fw_reg_info fw_reg_info[] = {
FW_REG_INFO(KVM_REG_ARM_STD_BMAP),
FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP),
FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP),
};
> +enum test_stage {
> + TEST_STAGE_REG_IFACE,
> + TEST_STAGE_HVC_IFACE_FEAT_DISABLED,
> + TEST_STAGE_HVC_IFACE_FEAT_ENABLED,
> + TEST_STAGE_HVC_IFACE_FALSE_INFO,
> + TEST_STAGE_END,
> +};
> +
> +static int stage;
> +
I think it'd better to initialize @stage to TEST_STAGE_REG_IFACE.
> +struct test_hvc_info {
> + uint32_t func_id;
> + int64_t arg0;
> +};
> +
> +#define TEST_HVC_INFO(f, a0) \
> + { \
> + .func_id = f, \
> + .arg0 = a0, \
> + }
> +
According to those functions (smccc_get_{function, argx}()) defined
in include/kvm/arm_hypercalls.h, @arg0 would have type of "uint64_t"
if I'm correct. Besides, @func_id is arg0 and arg0 should be arg1?
So if I'm correct, this would be:
struct test_hvc_info {
uint32_t func_id;
uint64_t arg1
};
> +static const struct test_hvc_info hvc_info[] = {
> + /* KVM_REG_ARM_STD_BMAP */
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_VERSION, 0),
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_TRNG_RND64),
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_GET_UUID, 0),
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND32, 0),
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND64, 0),
> +
> + /* KVM_REG_ARM_STD_HYP_BMAP */
> + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_HV_PV_TIME_FEATURES),
> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_HV_PV_TIME_ST),
> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_ST, 0),
> +
> + /* KVM_REG_ARM_VENDOR_HYP_BMAP */
> + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
> + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
> + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, KVM_PTP_VIRT_COUNTER),
> +};
> +
> +/* Feed false hypercall info to test the KVM behavior */
> +static const struct test_hvc_info false_hvc_info[] = {
> + /* Feature support check against a different family of hypercalls */
> + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
> + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_TRNG_RND64),
> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_TRNG_RND64),
> +};
> +
> +static void guest_test_hvc(const struct test_hvc_info *hc_info)
> +{
> + unsigned int i;
> + struct arm_smccc_res res;
> + unsigned int hvc_info_arr_sz;
> +
> + hvc_info_arr_sz =
> + hc_info == hvc_info ? ARRAY_SIZE(hvc_info) : ARRAY_SIZE(false_hvc_info);
> +
> + for (i = 0; i < hvc_info_arr_sz; i++, hc_info++) {
> +
> + memset(&res, 0, sizeof(res));
> + smccc_hvc(hc_info->func_id, hc_info->arg0, 0, 0, 0, 0, 0, 0, &res);
> +
Unnecessary empty line before memset(). I don't find where smccc_hvc()
is defined.
> + switch (stage) {
> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> + GUEST_ASSERT_3(res.a0 == SMCCC_RET_NOT_SUPPORTED,
> + res.a0, hc_info->func_id, hc_info->arg0);
^^
It seems the code here isn't properly aligned. Maybe it's your
preference :)
> + break;
> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> + GUEST_ASSERT_3(res.a0 != SMCCC_RET_NOT_SUPPORTED,
> + res.a0, hc_info->func_id, hc_info->arg0);
> + break;
> + default:
> + GUEST_ASSERT_1(0, stage);
> + }
> + }
> +}
> +
> +static void guest_code(void)
> +{
> + while (stage != TEST_STAGE_END) {
> + switch (stage) {
> + case TEST_STAGE_REG_IFACE:
> + break;
> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> + guest_test_hvc(hvc_info);
> + break;
> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> + guest_test_hvc(false_hvc_info);
> + break;
> + default:
> + GUEST_ASSERT_1(0, stage);
> + }
> +
> + GUEST_SYNC(stage);
> + }
> +
> + GUEST_DONE();
> +}
> +
> +static int set_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t val)
> +{
> + struct kvm_one_reg reg = {
> + .id = id,
> + .addr = (uint64_t)&val,
> + };
> +
> + return _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, ®);
> +}
> +
> +static void get_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t *addr)
> +{
> + struct kvm_one_reg reg = {
> + .id = id,
> + .addr = (uint64_t)addr,
> + };
> +
> + vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, ®);
> +}
> +
> +struct st_time {
> + uint32_t rev;
> + uint32_t attr;
> + uint64_t st_time;
> +};
> +
> +#define STEAL_TIME_SIZE ((sizeof(struct st_time) + 63) & ~63)
> +#define ST_GPA_BASE (1 << 30)
> +
> +static void steal_time_init(struct kvm_vm *vm)
> +{
> + uint64_t st_ipa = (ulong)ST_GPA_BASE;
> + unsigned int gpages;
> + struct kvm_device_attr dev = {
> + .group = KVM_ARM_VCPU_PVTIME_CTRL,
> + .attr = KVM_ARM_VCPU_PVTIME_IPA,
> + .addr = (uint64_t)&st_ipa,
> + };
> +
> + gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE);
> + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
> +
> + vcpu_ioctl(vm, 0, KVM_SET_DEVICE_ATTR, &dev);
> +}
> +
It might be helpful to do TEST_FAIL() on error returned from
this vcpu_ioctl(), or skip those PVTIME SMCCC calls accordingly
if the attribute isn't set successfully.
> +static void test_fw_regs_before_vm_start(struct kvm_vm *vm)
> +{
> + uint64_t val;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
> + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
> +
> + /* First 'read' should be an upper limit of the features supported */
> + get_fw_reg(vm, reg_info->reg, &val);
> + TEST_ASSERT(val == FW_REG_ULIMIT_VAL(reg_info->max_feat_bit),
> + "Expected all the features to be set for reg: 0x%lx; expected: 0x%llx; read: 0x%lx\n",
> + reg_info->reg, GENMASK_ULL(reg_info->max_feat_bit, 0), val);
> +
s/GENMASK_ULL(...)/FW_REG_ULIMIT_VAL(...)
> + /* Test a 'write' by disabling all the features of the register map */
> + ret = set_fw_reg(vm, reg_info->reg, 0);
> + TEST_ASSERT(ret == 0,
> + "Failed to clear all the features of reg: 0x%lx; ret: %d\n",
> + reg_info->reg, errno);
> +
> + get_fw_reg(vm, reg_info->reg, &val);
> + TEST_ASSERT(val == 0,
> + "Expected all the features to be cleared for reg: 0x%lx\n", reg_info->reg);
> +
> + /*
> + * Test enabling a feature that's not supported.
> + * Avoid this check if all the bits are occupied.
> + */
> + if (reg_info->max_feat_bit < 63) {
> + ret = set_fw_reg(vm, reg_info->reg, BIT(reg_info->max_feat_bit + 1));
> + TEST_ASSERT(ret != 0 && errno == EINVAL,
> + "Unexpected behavior or return value (%d) while setting an unsupported feature for reg: 0x%lx\n",
> + errno, reg_info->reg);
> + }
> + }
> +}
> +
> +static void test_fw_regs_after_vm_start(struct kvm_vm *vm)
> +{
> + uint64_t val;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
> + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
> +
> + /*
> + * Before starting the VM, the test clears all the bits.
> + * Check if that's still the case.
> + */
> + get_fw_reg(vm, reg_info->reg, &val);
> + TEST_ASSERT(val == 0,
> + "Expected all the features to be cleared for reg: 0x%lx\n",
> + reg_info->reg);
> +
> + /*
> + * Test setting the last read value. KVM should allow this
> + * even if VM has started running.
> + */
> + ret = set_fw_reg(vm, reg_info->reg, val);
> + TEST_ASSERT(ret == 0,
> + "Failed to set the register with previously read value after Vm start for reg: 0x%lx; ret: %d\n",
> + reg_info->reg, errno);
> +
> + /*
> + * Set all the features for this register again. KVM shouldn't
> + * allow this as the VM is running.
> + */
> + ret = set_fw_reg(vm, reg_info->reg, FW_REG_ULIMIT_VAL(reg_info->max_feat_bit));
> + TEST_ASSERT(ret != 0 && errno == EBUSY,
> + "Unexpected behavior or return value (%d) while setting a feature while VM is running for reg: 0x%lx\n",
> + errno, reg_info->reg);
> + }
> +}
> +
> +static struct kvm_vm *test_vm_create(void)
> +{
> + struct kvm_vm *vm;
> +
> + vm = vm_create_default(0, 0, guest_code);
> +
> + ucall_init(vm, NULL);
> + steal_time_init(vm);
> +
> + return vm;
> +}
> +
> +static struct kvm_vm *test_guest_stage(struct kvm_vm *vm)
> +{
> + struct kvm_vm *ret_vm = vm;
> +
> + pr_debug("Stage: %d\n", stage);
> +
> + switch (stage) {
> + case TEST_STAGE_REG_IFACE:
> + test_fw_regs_after_vm_start(vm);
> + break;
> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> + /* Start a new VM so that all the features are now enabled by default */
> + kvm_vm_free(vm);
> + ret_vm = test_vm_create();
> + break;
> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> + break;
> + default:
> + TEST_FAIL("Unknown test stage: %d\n", stage);
> + }
> +
> + stage++;
> + sync_global_to_guest(vm, stage);
> +
> + return ret_vm;
> +}
> +
> +static void test_run(void)
> +{
> + struct kvm_vm *vm;
> + struct ucall uc;
> + bool guest_done = false;
> +
> + vm = test_vm_create();
> +
> + test_fw_regs_before_vm_start(vm);
> +
> + while (!guest_done) {
> + vcpu_run(vm, 0);
> +
> + switch (get_ucall(vm, 0, &uc)) {
> + case UCALL_SYNC:
> + vm = test_guest_stage(vm);
> + break;
> + case UCALL_DONE:
> + guest_done = true;
> + break;
> + case UCALL_ABORT:
> + TEST_FAIL("%s at %s:%ld\n\tvalues: 0x%lx, 0x%lx; 0x%lx, stage: %u",
> + (const char *)uc.args[0], __FILE__, uc.args[1],
> + uc.args[2], uc.args[3], uc.args[4], stage);
> + break;
> + default:
> + TEST_FAIL("Unexpected guest exit\n");
> + }
> + }
> +
> + kvm_vm_free(vm);
> +}
> +
> +int main(void)
> +{
> + setbuf(stdout, NULL);
> +
> + test_run();
> + return 0;
> +}
>
Thanks,
Gavin
On Wed, Apr 13, 2022 at 2:07 AM Gavin Shan <[email protected]> wrote:
>
> Hi Raghavendra,
>
> On 4/7/22 9:16 AM, Raghavendra Rao Ananta wrote:
> > Introduce a KVM selftest to check the hypercall interface
> > for arm64 platforms. The test validates the user-space's
> > IOCTL interface to read/write the psuedo-firmware registers
> > as well as its effects on the guest upon certain configurations.
> >
> > Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> > ---
> > tools/testing/selftests/kvm/.gitignore | 1 +
> > tools/testing/selftests/kvm/Makefile | 1 +
> > .../selftests/kvm/aarch64/hypercalls.c | 344 ++++++++++++++++++
> > 3 files changed, 346 insertions(+)
> > create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c
> >
>
> To be more precise, s/IOCTL/{GET,SET}_ONE_REG ?
>
Sure, I think that'll be better.
> > diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> > index e82b816a6608..7ef52b3b1560 100644
> > --- a/tools/testing/selftests/kvm/.gitignore
> > +++ b/tools/testing/selftests/kvm/.gitignore
> > @@ -2,6 +2,7 @@
> > /aarch64/arch_timer
> > /aarch64/debug-exceptions
> > /aarch64/get-reg-list
> > +/aarch64/hypercalls
> > /aarch64/psci_test
> > /aarch64/vgic_init
> > /aarch64/vgic_irq
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 2f74f502de65..af4cb88bcf83 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -105,6 +105,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
> > TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
> > TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
> > TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> > +TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
> > TEST_GEN_PROGS_aarch64 += aarch64/psci_test
> > TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
> > TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
> > diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
> > new file mode 100644
> > index 000000000000..9941fb75772a
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
> > @@ -0,0 +1,344 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/* hypercalls: Check the ARM64's psuedo-firmware bitmap register interface.
> > + *
> > + * The test validates the basic hypercall functionalities that are exposed
> > + * via the psuedo-firmware bitmap register. This includes the registers'
> > + * read/write behavior before and after the VM has started, and if the
> > + * hypercalls are properly masked or unmasked to the guest when disabled or
> > + * enabled from the KVM userspace, respectively.
> > + */
> > +
> > +#include <errno.h>
> > +#include <linux/arm-smccc.h>
> > +#include <asm/kvm.h>
> > +#include <kvm_util.h>
> > +
> > +#include "processor.h"
> > +
> > +#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK_ULL(max_feat_bit, 0))
> > +
> > +/* Last valid bits of the bitmapped firmware registers */
> > +#define KVM_REG_ARM_STD_BMAP_BIT_MAX 0
> > +#define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0
> > +#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 1
> > +
> > +struct kvm_fw_reg_info {
> > + uint64_t reg; /* Register definition */
> > + uint64_t max_feat_bit; /* Bit that represents the upper limit of the feature-map */
> > +};
> > +
> > +#define FW_REG_INFO(r, bit_max) \
> > + { \
> > + .reg = r, \
> > + .max_feat_bit = bit_max, \
> > + }
> > +
> > +static const struct kvm_fw_reg_info fw_reg_info[] = {
> > + FW_REG_INFO(KVM_REG_ARM_STD_BMAP, KVM_REG_ARM_STD_BMAP_BIT_MAX),
> > + FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP, KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX),
> > + FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP, KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX),
> > +};
> > +
>
> This can be simplifed by:
>
> #define FW_REG_INFO(r) \
> { .reg = r, \
> .max_feat_bit = r_##BIT_MAX, \
> }
>
> static const struct kvm_fw_reg_info fw_reg_info[] = {
> FW_REG_INFO(KVM_REG_ARM_STD_BMAP),
> FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP),
> FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP),
> };
>
Yes, probably that looks better. Thanks for the suggestion.
> > +enum test_stage {
> > + TEST_STAGE_REG_IFACE,
> > + TEST_STAGE_HVC_IFACE_FEAT_DISABLED,
> > + TEST_STAGE_HVC_IFACE_FEAT_ENABLED,
> > + TEST_STAGE_HVC_IFACE_FALSE_INFO,
> > + TEST_STAGE_END,
> > +};
> > +
> > +static int stage;
> > +
>
> I think it'd better to initialize @stage to TEST_STAGE_REG_IFACE.
>
Will do.
> > +struct test_hvc_info {
> > + uint32_t func_id;
> > + int64_t arg0;
> > +};
> > +
> > +#define TEST_HVC_INFO(f, a0) \
> > + { \
> > + .func_id = f, \
> > + .arg0 = a0, \
> > + }
> > +
>
> According to those functions (smccc_get_{function, argx}()) defined
> in include/kvm/arm_hypercalls.h, @arg0 would have type of "uint64_t"
> if I'm correct. Besides, @func_id is arg0 and arg0 should be arg1?
> So if I'm correct, this would be:
>
> struct test_hvc_info {
> uint32_t func_id;
> uint64_t arg1
> };
>
Thanks for noticing this! I'll change it to 'unit64'. Regarding the
argument naming, I understand that it's a little confusing. I went
with 'arg0' to align with the selftest library's convention. But, I
agree that it's not aligned with what the kernel is used to.
Oliver, do you think we can start the argument naming from a1/arg1 in [1]?
> > +static const struct test_hvc_info hvc_info[] = {
> > + /* KVM_REG_ARM_STD_BMAP */
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_VERSION, 0),
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_TRNG_RND64),
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_GET_UUID, 0),
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND32, 0),
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND64, 0),
> > +
> > + /* KVM_REG_ARM_STD_HYP_BMAP */
> > + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_HV_PV_TIME_FEATURES),
> > + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_HV_PV_TIME_ST),
> > + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_ST, 0),
> > +
> > + /* KVM_REG_ARM_VENDOR_HYP_BMAP */
> > + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
> > + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
> > + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, KVM_PTP_VIRT_COUNTER),
> > +};
> > +
> > +/* Feed false hypercall info to test the KVM behavior */
> > +static const struct test_hvc_info false_hvc_info[] = {
> > + /* Feature support check against a different family of hypercalls */
> > + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
> > + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_TRNG_RND64),
> > + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_TRNG_RND64),
> > +};
> > +
> > +static void guest_test_hvc(const struct test_hvc_info *hc_info)
> > +{
> > + unsigned int i;
> > + struct arm_smccc_res res;
> > + unsigned int hvc_info_arr_sz;
> > +
> > + hvc_info_arr_sz =
> > + hc_info == hvc_info ? ARRAY_SIZE(hvc_info) : ARRAY_SIZE(false_hvc_info);
> > +
> > + for (i = 0; i < hvc_info_arr_sz; i++, hc_info++) {
> > +
> > + memset(&res, 0, sizeof(res));
> > + smccc_hvc(hc_info->func_id, hc_info->arg0, 0, 0, 0, 0, 0, 0, &res);
> > +
>
> Unnecessary empty line before memset(). I don't find where smccc_hvc()
> is defined.
>
I can clear the line and for the definition of smccc_hvc(), I applied
Oliver's patch [1].
> > + switch (stage) {
> > + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> > + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> > + GUEST_ASSERT_3(res.a0 == SMCCC_RET_NOT_SUPPORTED,
> > + res.a0, hc_info->func_id, hc_info->arg0);
> ^^
>
> It seems the code here isn't properly aligned. Maybe it's your
> preference :)
>
I think my editor is acting weird. I'll check again. Thanks for catching this!
> > + break;
> > + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> > + GUEST_ASSERT_3(res.a0 != SMCCC_RET_NOT_SUPPORTED,
> > + res.a0, hc_info->func_id, hc_info->arg0);
> > + break;
> > + default:
> > + GUEST_ASSERT_1(0, stage);
> > + }
> > + }
> > +}
> > +
> > +static void guest_code(void)
> > +{
> > + while (stage != TEST_STAGE_END) {
> > + switch (stage) {
> > + case TEST_STAGE_REG_IFACE:
> > + break;
> > + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> > + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> > + guest_test_hvc(hvc_info);
> > + break;
> > + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> > + guest_test_hvc(false_hvc_info);
> > + break;
> > + default:
> > + GUEST_ASSERT_1(0, stage);
> > + }
> > +
> > + GUEST_SYNC(stage);
> > + }
> > +
> > + GUEST_DONE();
> > +}
> > +
> > +static int set_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t val)
> > +{
> > + struct kvm_one_reg reg = {
> > + .id = id,
> > + .addr = (uint64_t)&val,
> > + };
> > +
> > + return _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, ®);
> > +}
> > +
> > +static void get_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t *addr)
> > +{
> > + struct kvm_one_reg reg = {
> > + .id = id,
> > + .addr = (uint64_t)addr,
> > + };
> > +
> > + vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, ®);
> > +}
> > +
> > +struct st_time {
> > + uint32_t rev;
> > + uint32_t attr;
> > + uint64_t st_time;
> > +};
> > +
> > +#define STEAL_TIME_SIZE ((sizeof(struct st_time) + 63) & ~63)
> > +#define ST_GPA_BASE (1 << 30)
> > +
> > +static void steal_time_init(struct kvm_vm *vm)
> > +{
> > + uint64_t st_ipa = (ulong)ST_GPA_BASE;
> > + unsigned int gpages;
> > + struct kvm_device_attr dev = {
> > + .group = KVM_ARM_VCPU_PVTIME_CTRL,
> > + .attr = KVM_ARM_VCPU_PVTIME_IPA,
> > + .addr = (uint64_t)&st_ipa,
> > + };
> > +
> > + gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE);
> > + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
> > +
> > + vcpu_ioctl(vm, 0, KVM_SET_DEVICE_ATTR, &dev);
> > +}
> > +
>
> It might be helpful to do TEST_FAIL() on error returned from
> this vcpu_ioctl(), or skip those PVTIME SMCCC calls accordingly
> if the attribute isn't set successfully.
>
vcpu_ioctl() does a TEST_ASSERT() for us. Of course we can check it
ourselves and skip if needed, but don't you think that may go
unnoticed should any future changes tries to mess with
steal_time_init() incorrectly and we'd end up skipping the test
forever until we really notice skipped test?
> > +static void test_fw_regs_before_vm_start(struct kvm_vm *vm)
> > +{
> > + uint64_t val;
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
> > + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
> > +
> > + /* First 'read' should be an upper limit of the features supported */
> > + get_fw_reg(vm, reg_info->reg, &val);
> > + TEST_ASSERT(val == FW_REG_ULIMIT_VAL(reg_info->max_feat_bit),
> > + "Expected all the features to be set for reg: 0x%lx; expected: 0x%llx; read: 0x%lx\n",
> > + reg_info->reg, GENMASK_ULL(reg_info->max_feat_bit, 0), val);
> > +
>
> s/GENMASK_ULL(...)/FW_REG_ULIMIT_VAL(...)
>
Right, that's better.
> > + /* Test a 'write' by disabling all the features of the register map */
> > + ret = set_fw_reg(vm, reg_info->reg, 0);
> > + TEST_ASSERT(ret == 0,
> > + "Failed to clear all the features of reg: 0x%lx; ret: %d\n",
> > + reg_info->reg, errno);
> > +
> > + get_fw_reg(vm, reg_info->reg, &val);
> > + TEST_ASSERT(val == 0,
> > + "Expected all the features to be cleared for reg: 0x%lx\n", reg_info->reg);
> > +
> > + /*
> > + * Test enabling a feature that's not supported.
> > + * Avoid this check if all the bits are occupied.
> > + */
> > + if (reg_info->max_feat_bit < 63) {
> > + ret = set_fw_reg(vm, reg_info->reg, BIT(reg_info->max_feat_bit + 1));
> > + TEST_ASSERT(ret != 0 && errno == EINVAL,
> > + "Unexpected behavior or return value (%d) while setting an unsupported feature for reg: 0x%lx\n",
> > + errno, reg_info->reg);
> > + }
> > + }
> > +}
> > +
> > +static void test_fw_regs_after_vm_start(struct kvm_vm *vm)
> > +{
> > + uint64_t val;
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
> > + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
> > +
> > + /*
> > + * Before starting the VM, the test clears all the bits.
> > + * Check if that's still the case.
> > + */
> > + get_fw_reg(vm, reg_info->reg, &val);
> > + TEST_ASSERT(val == 0,
> > + "Expected all the features to be cleared for reg: 0x%lx\n",
> > + reg_info->reg);
> > +
> > + /*
> > + * Test setting the last read value. KVM should allow this
> > + * even if VM has started running.
> > + */
> > + ret = set_fw_reg(vm, reg_info->reg, val);
> > + TEST_ASSERT(ret == 0,
> > + "Failed to set the register with previously read value after Vm start for reg: 0x%lx; ret: %d\n",
> > + reg_info->reg, errno);
> > +
> > + /*
> > + * Set all the features for this register again. KVM shouldn't
> > + * allow this as the VM is running.
> > + */
> > + ret = set_fw_reg(vm, reg_info->reg, FW_REG_ULIMIT_VAL(reg_info->max_feat_bit));
> > + TEST_ASSERT(ret != 0 && errno == EBUSY,
> > + "Unexpected behavior or return value (%d) while setting a feature while VM is running for reg: 0x%lx\n",
> > + errno, reg_info->reg);
> > + }
> > +}
> > +
> > +static struct kvm_vm *test_vm_create(void)
> > +{
> > + struct kvm_vm *vm;
> > +
> > + vm = vm_create_default(0, 0, guest_code);
> > +
> > + ucall_init(vm, NULL);
> > + steal_time_init(vm);
> > +
> > + return vm;
> > +}
> > +
> > +static struct kvm_vm *test_guest_stage(struct kvm_vm *vm)
> > +{
> > + struct kvm_vm *ret_vm = vm;
> > +
> > + pr_debug("Stage: %d\n", stage);
> > +
> > + switch (stage) {
> > + case TEST_STAGE_REG_IFACE:
> > + test_fw_regs_after_vm_start(vm);
> > + break;
> > + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
> > + /* Start a new VM so that all the features are now enabled by default */
> > + kvm_vm_free(vm);
> > + ret_vm = test_vm_create();
> > + break;
> > + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
> > + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
> > + break;
> > + default:
> > + TEST_FAIL("Unknown test stage: %d\n", stage);
> > + }
> > +
> > + stage++;
> > + sync_global_to_guest(vm, stage);
> > +
> > + return ret_vm;
> > +}
> > +
> > +static void test_run(void)
> > +{
> > + struct kvm_vm *vm;
> > + struct ucall uc;
> > + bool guest_done = false;
> > +
> > + vm = test_vm_create();
> > +
> > + test_fw_regs_before_vm_start(vm);
> > +
> > + while (!guest_done) {
> > + vcpu_run(vm, 0);
> > +
> > + switch (get_ucall(vm, 0, &uc)) {
> > + case UCALL_SYNC:
> > + vm = test_guest_stage(vm);
> > + break;
> > + case UCALL_DONE:
> > + guest_done = true;
> > + break;
> > + case UCALL_ABORT:
> > + TEST_FAIL("%s at %s:%ld\n\tvalues: 0x%lx, 0x%lx; 0x%lx, stage: %u",
> > + (const char *)uc.args[0], __FILE__, uc.args[1],
> > + uc.args[2], uc.args[3], uc.args[4], stage);
> > + break;
> > + default:
> > + TEST_FAIL("Unexpected guest exit\n");
> > + }
> > + }
> > +
> > + kvm_vm_free(vm);
> > +}
> > +
> > +int main(void)
> > +{
> > + setbuf(stdout, NULL);
> > +
> > + test_run();
> > + return 0;
> > +}
> >
>
> Thanks,
> Gavin
>
Thanks for taking the time to review.
Regards,
Raghavendra
[1]: https://lore.kernel.org/kvmarm/[email protected]/T/#u
Hi Raghavendra,
On 4/14/22 1:32 AM, Raghavendra Rao Ananta wrote:
> On Wed, Apr 13, 2022 at 2:07 AM Gavin Shan <[email protected]> wrote:
>> On 4/7/22 9:16 AM, Raghavendra Rao Ananta wrote:
>>> Introduce a KVM selftest to check the hypercall interface
>>> for arm64 platforms. The test validates the user-space's
>>> IOCTL interface to read/write the psuedo-firmware registers
>>> as well as its effects on the guest upon certain configurations.
>>>
>>> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
>>> ---
>>> tools/testing/selftests/kvm/.gitignore | 1 +
>>> tools/testing/selftests/kvm/Makefile | 1 +
>>> .../selftests/kvm/aarch64/hypercalls.c | 344 ++++++++++++++++++
>>> 3 files changed, 346 insertions(+)
>>> create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c
>>>
>>
>> To be more precise, s/IOCTL/{GET,SET}_ONE_REG ?
>>
> Sure, I think that'll be better.
>
>>> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
>>> index e82b816a6608..7ef52b3b1560 100644
>>> --- a/tools/testing/selftests/kvm/.gitignore
>>> +++ b/tools/testing/selftests/kvm/.gitignore
>>> @@ -2,6 +2,7 @@
>>> /aarch64/arch_timer
>>> /aarch64/debug-exceptions
>>> /aarch64/get-reg-list
>>> +/aarch64/hypercalls
>>> /aarch64/psci_test
>>> /aarch64/vgic_init
>>> /aarch64/vgic_irq
>>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>>> index 2f74f502de65..af4cb88bcf83 100644
>>> --- a/tools/testing/selftests/kvm/Makefile
>>> +++ b/tools/testing/selftests/kvm/Makefile
>>> @@ -105,6 +105,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
>>> TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>>> TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>>> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>>> +TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
>>> TEST_GEN_PROGS_aarch64 += aarch64/psci_test
>>> TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>>> TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
>>> diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
>>> new file mode 100644
>>> index 000000000000..9941fb75772a
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
>>> @@ -0,0 +1,344 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/* hypercalls: Check the ARM64's psuedo-firmware bitmap register interface.
>>> + *
>>> + * The test validates the basic hypercall functionalities that are exposed
>>> + * via the psuedo-firmware bitmap register. This includes the registers'
>>> + * read/write behavior before and after the VM has started, and if the
>>> + * hypercalls are properly masked or unmasked to the guest when disabled or
>>> + * enabled from the KVM userspace, respectively.
>>> + */
>>> +
>>> +#include <errno.h>
>>> +#include <linux/arm-smccc.h>
>>> +#include <asm/kvm.h>
>>> +#include <kvm_util.h>
>>> +
>>> +#include "processor.h"
>>> +
>>> +#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK_ULL(max_feat_bit, 0))
>>> +
>>> +/* Last valid bits of the bitmapped firmware registers */
>>> +#define KVM_REG_ARM_STD_BMAP_BIT_MAX 0
>>> +#define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0
>>> +#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 1
>>> +
>>> +struct kvm_fw_reg_info {
>>> + uint64_t reg; /* Register definition */
>>> + uint64_t max_feat_bit; /* Bit that represents the upper limit of the feature-map */
>>> +};
>>> +
>>> +#define FW_REG_INFO(r, bit_max) \
>>> + { \
>>> + .reg = r, \
>>> + .max_feat_bit = bit_max, \
>>> + }
>>> +
>>> +static const struct kvm_fw_reg_info fw_reg_info[] = {
>>> + FW_REG_INFO(KVM_REG_ARM_STD_BMAP, KVM_REG_ARM_STD_BMAP_BIT_MAX),
>>> + FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP, KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX),
>>> + FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP, KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX),
>>> +};
>>> +
>>
>> This can be simplifed by:
>>
>> #define FW_REG_INFO(r) \
>> { .reg = r, \
>> .max_feat_bit = r_##BIT_MAX, \
>> }
>>
>> static const struct kvm_fw_reg_info fw_reg_info[] = {
>> FW_REG_INFO(KVM_REG_ARM_STD_BMAP),
>> FW_REG_INFO(KVM_REG_ARM_STD_HYP_BMAP),
>> FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP_BMAP),
>> };
>>
> Yes, probably that looks better. Thanks for the suggestion.
>
>>> +enum test_stage {
>>> + TEST_STAGE_REG_IFACE,
>>> + TEST_STAGE_HVC_IFACE_FEAT_DISABLED,
>>> + TEST_STAGE_HVC_IFACE_FEAT_ENABLED,
>>> + TEST_STAGE_HVC_IFACE_FALSE_INFO,
>>> + TEST_STAGE_END,
>>> +};
>>> +
>>> +static int stage;
>>> +
>>
>> I think it'd better to initialize @stage to TEST_STAGE_REG_IFACE.
>>
> Will do.
>>> +struct test_hvc_info {
>>> + uint32_t func_id;
>>> + int64_t arg0;
>>> +};
>>> +
>>> +#define TEST_HVC_INFO(f, a0) \
>>> + { \
>>> + .func_id = f, \
>>> + .arg0 = a0, \
>>> + }
>>> +
>>
>> According to those functions (smccc_get_{function, argx}()) defined
>> in include/kvm/arm_hypercalls.h, @arg0 would have type of "uint64_t"
>> if I'm correct. Besides, @func_id is arg0 and arg0 should be arg1?
>> So if I'm correct, this would be:
>>
>> struct test_hvc_info {
>> uint32_t func_id;
>> uint64_t arg1
>> };
>>
> Thanks for noticing this! I'll change it to 'unit64'. Regarding the
> argument naming, I understand that it's a little confusing. I went
> with 'arg0' to align with the selftest library's convention. But, I
> agree that it's not aligned with what the kernel is used to.
>
> Oliver, do you think we can start the argument naming from a1/arg1 in [1]?
>
>>> +static const struct test_hvc_info hvc_info[] = {
>>> + /* KVM_REG_ARM_STD_BMAP */
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_VERSION, 0),
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_TRNG_RND64),
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_GET_UUID, 0),
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND32, 0),
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_RND64, 0),
>>> +
>>> + /* KVM_REG_ARM_STD_HYP_BMAP */
>>> + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_HV_PV_TIME_FEATURES),
>>> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_HV_PV_TIME_ST),
>>> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_ST, 0),
>>> +
>>> + /* KVM_REG_ARM_VENDOR_HYP_BMAP */
>>> + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
>>> + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
>>> + TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, KVM_PTP_VIRT_COUNTER),
>>> +};
>>> +
>>> +/* Feed false hypercall info to test the KVM behavior */
>>> +static const struct test_hvc_info false_hvc_info[] = {
>>> + /* Feature support check against a different family of hypercalls */
>>> + TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID),
>>> + TEST_HVC_INFO(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_TRNG_RND64),
>>> + TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_TRNG_RND64),
>>> +};
>>> +
>>> +static void guest_test_hvc(const struct test_hvc_info *hc_info)
>>> +{
>>> + unsigned int i;
>>> + struct arm_smccc_res res;
>>> + unsigned int hvc_info_arr_sz;
>>> +
>>> + hvc_info_arr_sz =
>>> + hc_info == hvc_info ? ARRAY_SIZE(hvc_info) : ARRAY_SIZE(false_hvc_info);
>>> +
>>> + for (i = 0; i < hvc_info_arr_sz; i++, hc_info++) {
>>> +
>>> + memset(&res, 0, sizeof(res));
>>> + smccc_hvc(hc_info->func_id, hc_info->arg0, 0, 0, 0, 0, 0, 0, &res);
>>> +
>>
>> Unnecessary empty line before memset(). I don't find where smccc_hvc()
>> is defined.
>>
> I can clear the line and for the definition of smccc_hvc(), I applied
> Oliver's patch [1].
>
>>> + switch (stage) {
>>> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
>>> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
>>> + GUEST_ASSERT_3(res.a0 == SMCCC_RET_NOT_SUPPORTED,
>>> + res.a0, hc_info->func_id, hc_info->arg0);
>> ^^
>>
>> It seems the code here isn't properly aligned. Maybe it's your
>> preference :)
>>
> I think my editor is acting weird. I'll check again. Thanks for catching this!
>
>>> + break;
>>> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
>>> + GUEST_ASSERT_3(res.a0 != SMCCC_RET_NOT_SUPPORTED,
>>> + res.a0, hc_info->func_id, hc_info->arg0);
>>> + break;
>>> + default:
>>> + GUEST_ASSERT_1(0, stage);
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void guest_code(void)
>>> +{
>>> + while (stage != TEST_STAGE_END) {
>>> + switch (stage) {
>>> + case TEST_STAGE_REG_IFACE:
>>> + break;
>>> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
>>> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
>>> + guest_test_hvc(hvc_info);
>>> + break;
>>> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
>>> + guest_test_hvc(false_hvc_info);
>>> + break;
>>> + default:
>>> + GUEST_ASSERT_1(0, stage);
>>> + }
>>> +
>>> + GUEST_SYNC(stage);
>>> + }
>>> +
>>> + GUEST_DONE();
>>> +}
>>> +
>>> +static int set_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t val)
>>> +{
>>> + struct kvm_one_reg reg = {
>>> + .id = id,
>>> + .addr = (uint64_t)&val,
>>> + };
>>> +
>>> + return _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, ®);
>>> +}
>>> +
>>> +static void get_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t *addr)
>>> +{
>>> + struct kvm_one_reg reg = {
>>> + .id = id,
>>> + .addr = (uint64_t)addr,
>>> + };
>>> +
>>> + vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, ®);
>>> +}
>>> +
>>> +struct st_time {
>>> + uint32_t rev;
>>> + uint32_t attr;
>>> + uint64_t st_time;
>>> +};
>>> +
>>> +#define STEAL_TIME_SIZE ((sizeof(struct st_time) + 63) & ~63)
>>> +#define ST_GPA_BASE (1 << 30)
>>> +
>>> +static void steal_time_init(struct kvm_vm *vm)
>>> +{
>>> + uint64_t st_ipa = (ulong)ST_GPA_BASE;
>>> + unsigned int gpages;
>>> + struct kvm_device_attr dev = {
>>> + .group = KVM_ARM_VCPU_PVTIME_CTRL,
>>> + .attr = KVM_ARM_VCPU_PVTIME_IPA,
>>> + .addr = (uint64_t)&st_ipa,
>>> + };
>>> +
>>> + gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE);
>>> + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
>>> +
>>> + vcpu_ioctl(vm, 0, KVM_SET_DEVICE_ATTR, &dev);
>>> +}
>>> +
>>
>> It might be helpful to do TEST_FAIL() on error returned from
>> this vcpu_ioctl(), or skip those PVTIME SMCCC calls accordingly
>> if the attribute isn't set successfully.
>>
> vcpu_ioctl() does a TEST_ASSERT() for us. Of course we can check it
> ourselves and skip if needed, but don't you think that may go
> unnoticed should any future changes tries to mess with
> steal_time_init() incorrectly and we'd end up skipping the test
> forever until we really notice skipped test?
>
Yes, I missed the TEST_ASSERT() in vcpu_ioctl(). In this case,
we're safe and please ignore this comment :)
>>> +static void test_fw_regs_before_vm_start(struct kvm_vm *vm)
>>> +{
>>> + uint64_t val;
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
>>> + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
>>> +
>>> + /* First 'read' should be an upper limit of the features supported */
>>> + get_fw_reg(vm, reg_info->reg, &val);
>>> + TEST_ASSERT(val == FW_REG_ULIMIT_VAL(reg_info->max_feat_bit),
>>> + "Expected all the features to be set for reg: 0x%lx; expected: 0x%llx; read: 0x%lx\n",
>>> + reg_info->reg, GENMASK_ULL(reg_info->max_feat_bit, 0), val);
>>> +
>>
>> s/GENMASK_ULL(...)/FW_REG_ULIMIT_VAL(...)
>>
> Right, that's better.
>
>>> + /* Test a 'write' by disabling all the features of the register map */
>>> + ret = set_fw_reg(vm, reg_info->reg, 0);
>>> + TEST_ASSERT(ret == 0,
>>> + "Failed to clear all the features of reg: 0x%lx; ret: %d\n",
>>> + reg_info->reg, errno);
>>> +
>>> + get_fw_reg(vm, reg_info->reg, &val);
>>> + TEST_ASSERT(val == 0,
>>> + "Expected all the features to be cleared for reg: 0x%lx\n", reg_info->reg);
>>> +
>>> + /*
>>> + * Test enabling a feature that's not supported.
>>> + * Avoid this check if all the bits are occupied.
>>> + */
>>> + if (reg_info->max_feat_bit < 63) {
>>> + ret = set_fw_reg(vm, reg_info->reg, BIT(reg_info->max_feat_bit + 1));
>>> + TEST_ASSERT(ret != 0 && errno == EINVAL,
>>> + "Unexpected behavior or return value (%d) while setting an unsupported feature for reg: 0x%lx\n",
>>> + errno, reg_info->reg);
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void test_fw_regs_after_vm_start(struct kvm_vm *vm)
>>> +{
>>> + uint64_t val;
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
>>> + const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
>>> +
>>> + /*
>>> + * Before starting the VM, the test clears all the bits.
>>> + * Check if that's still the case.
>>> + */
>>> + get_fw_reg(vm, reg_info->reg, &val);
>>> + TEST_ASSERT(val == 0,
>>> + "Expected all the features to be cleared for reg: 0x%lx\n",
>>> + reg_info->reg);
>>> +
>>> + /*
>>> + * Test setting the last read value. KVM should allow this
>>> + * even if VM has started running.
>>> + */
>>> + ret = set_fw_reg(vm, reg_info->reg, val);
>>> + TEST_ASSERT(ret == 0,
>>> + "Failed to set the register with previously read value after Vm start for reg: 0x%lx; ret: %d\n",
>>> + reg_info->reg, errno);
>>> +
>>> + /*
>>> + * Set all the features for this register again. KVM shouldn't
>>> + * allow this as the VM is running.
>>> + */
>>> + ret = set_fw_reg(vm, reg_info->reg, FW_REG_ULIMIT_VAL(reg_info->max_feat_bit));
>>> + TEST_ASSERT(ret != 0 && errno == EBUSY,
>>> + "Unexpected behavior or return value (%d) while setting a feature while VM is running for reg: 0x%lx\n",
>>> + errno, reg_info->reg);
>>> + }
>>> +}
>>> +
>>> +static struct kvm_vm *test_vm_create(void)
>>> +{
>>> + struct kvm_vm *vm;
>>> +
>>> + vm = vm_create_default(0, 0, guest_code);
>>> +
>>> + ucall_init(vm, NULL);
>>> + steal_time_init(vm);
>>> +
>>> + return vm;
>>> +}
>>> +
>>> +static struct kvm_vm *test_guest_stage(struct kvm_vm *vm)
>>> +{
>>> + struct kvm_vm *ret_vm = vm;
>>> +
>>> + pr_debug("Stage: %d\n", stage);
>>> +
>>> + switch (stage) {
>>> + case TEST_STAGE_REG_IFACE:
>>> + test_fw_regs_after_vm_start(vm);
>>> + break;
>>> + case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
>>> + /* Start a new VM so that all the features are now enabled by default */
>>> + kvm_vm_free(vm);
>>> + ret_vm = test_vm_create();
>>> + break;
>>> + case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
>>> + case TEST_STAGE_HVC_IFACE_FALSE_INFO:
>>> + break;
>>> + default:
>>> + TEST_FAIL("Unknown test stage: %d\n", stage);
>>> + }
>>> +
>>> + stage++;
>>> + sync_global_to_guest(vm, stage);
>>> +
>>> + return ret_vm;
>>> +}
>>> +
>>> +static void test_run(void)
>>> +{
>>> + struct kvm_vm *vm;
>>> + struct ucall uc;
>>> + bool guest_done = false;
>>> +
>>> + vm = test_vm_create();
>>> +
>>> + test_fw_regs_before_vm_start(vm);
>>> +
>>> + while (!guest_done) {
>>> + vcpu_run(vm, 0);
>>> +
>>> + switch (get_ucall(vm, 0, &uc)) {
>>> + case UCALL_SYNC:
>>> + vm = test_guest_stage(vm);
>>> + break;
>>> + case UCALL_DONE:
>>> + guest_done = true;
>>> + break;
>>> + case UCALL_ABORT:
>>> + TEST_FAIL("%s at %s:%ld\n\tvalues: 0x%lx, 0x%lx; 0x%lx, stage: %u",
>>> + (const char *)uc.args[0], __FILE__, uc.args[1],
>>> + uc.args[2], uc.args[3], uc.args[4], stage);
>>> + break;
>>> + default:
>>> + TEST_FAIL("Unexpected guest exit\n");
>>> + }
>>> + }
>>> +
>>> + kvm_vm_free(vm);
>>> +}
>>> +
>>> +int main(void)
>>> +{
>>> + setbuf(stdout, NULL);
>>> +
>>> + test_run();
>>> + return 0;
>>> +}
>
> Thanks for taking the time to review.
>
No worries and sorry for late chime-in.>
> [1]: https://lore.kernel.org/kvmarm/[email protected]/T/#u
>
Thanks,
Gavin