Re: [PATCH v9 04/27] virt: gunyah: Add hypercalls to identify Gunyah

From: Elliot Berman
Date: Mon Jan 30 2023 - 14:05:52 EST




On 1/30/2023 2:01 AM, Greg Kroah-Hartman wrote:
On Fri, Jan 20, 2023 at 02:46:03PM -0800, Elliot Berman wrote:
Add hypercalls to identify when Linux is running a virtual machine under
Gunyah.

There are two calls to help identify Gunyah:

1. gh_hypercall_get_uid() returns a UID when running under a Gunyah
hypervisor.
2. gh_hypercall_hyp_identify() returns build information and a set of
feature flags that are supported by Gunyah.

Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
---
MAINTAINERS | 2 +
arch/arm64/Kbuild | 1 +
arch/arm64/gunyah/Makefile | 3 ++
arch/arm64/gunyah/gunyah_hypercall.c | 75 ++++++++++++++++++++++++++++
drivers/virt/Kconfig | 2 +
drivers/virt/gunyah/Kconfig | 14 ++++++
include/linux/gunyah.h | 33 ++++++++++++
7 files changed, 130 insertions(+)
create mode 100644 arch/arm64/gunyah/Makefile
create mode 100644 arch/arm64/gunyah/gunyah_hypercall.c
create mode 100644 drivers/virt/gunyah/Kconfig

diff --git a/MAINTAINERS b/MAINTAINERS
index f1e07e39b2f5..fe19e71efc6d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9048,6 +9048,8 @@ L: linux-arm-msm@xxxxxxxxxxxxxxx
S: Supported
F: Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml
F: Documentation/virt/gunyah/
+F: arch/arm64/gunyah/
+F: drivers/virt/gunyah/
F: include/linux/gunyah.h
HABANALABS PCI DRIVER
diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
index 5bfbf7d79c99..e4847ba0e3c9 100644
--- a/arch/arm64/Kbuild
+++ b/arch/arm64/Kbuild
@@ -3,6 +3,7 @@ obj-y += kernel/ mm/ net/
obj-$(CONFIG_KVM) += kvm/
obj-$(CONFIG_XEN) += xen/
obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/
+obj-$(CONFIG_GUNYAH) += gunyah/
obj-$(CONFIG_CRYPTO) += crypto/
# for cleaning
diff --git a/arch/arm64/gunyah/Makefile b/arch/arm64/gunyah/Makefile
new file mode 100644
index 000000000000..84f1e38cafb1
--- /dev/null
+++ b/arch/arm64/gunyah/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_GUNYAH) += gunyah_hypercall.o
diff --git a/arch/arm64/gunyah/gunyah_hypercall.c b/arch/arm64/gunyah/gunyah_hypercall.c
new file mode 100644
index 000000000000..ffed4b71641f
--- /dev/null
+++ b/arch/arm64/gunyah/gunyah_hypercall.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/module.h>
+#include <linux/gunyah.h>
+
+static const uint32_t gunyah_known_uuids[][4] = {

uint32_t is not a kernel type, please use sane ones, like u32.

+ {0x19bd54bd, 0x0b37571b, 0x946f609b, 0x54539de6}, /* QC_HYP (Qualcomm's build) */
+ {0x673d5f14, 0x9265ce36, 0xa4535fdb, 0xc1d58fcd}, /* GUNYAH (open source build) */

And why not use the kernel uuid type here? Why create your own?

+};
+
+#define GH_HYPERCALL_HYP_IDENTIFY GH_HYPERCALL(0x0000)
+
+/**
+ * gh_hypercall_get_uid() - Returns a UID when running under a Gunyah hypervisor
+ * @uid: An array of 4 u32's (u32 uid[4];)
+ *
+ * Caller should compare the resulting UID to a list of known Gunyah UIDs to
+ * confirm that Linux is running as a guest of Gunyah.
+ */
+void gh_hypercall_get_uid(u32 uid[4])
+{
+ struct arm_smccc_res res;
+ u32 uid[4];

How does this work when you have a local variable the same name as the
parameter to the function? What shadows what and how? The compiler
didn't complain about this?

+ int i;
+
+ arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
+
+ uid[0] = lower_32_bits(res.a0);
+ uid[1] = lower_32_bits(res.a1);
+ uid[2] = lower_32_bits(res.a2);
+ uid[3] = lower_32_bits(res.a3);
+
+ for (i = 0; i < ARRAY_SIZE(gunyah_known_uuids); i++)
+ if (!memcmp(uid, gunyah_known_uuids[i], sizeof(uid)))
+ break;
+
+ return i != ARRAY_SIZE(gunyah_known_uuids);

How can a function that returns void actually return anything?

This obviously was never compiled. I'm stopping here in my review.


Fell victim to trusting automatic git commit --fixup tools here. I'll send out v10 with proper bisectability.