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

[Greg Kroah-Hartman]

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.

greg k-h