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

From: Elliot Berman <quic_eberman@quicinc.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Bjorn Andersson <quic_bjorande@quicinc.com>,
	Alex Elder <elder@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Murali Nalajala <quic_mnalajal@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Arnd Bergmann <arnd@arndb.de>, Rob Herring <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Marc Zyngier <maz@kernel.org>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v9 04/27] virt: gunyah: Add hypercalls to identify Gunyah
Date: Mon, 30 Jan 2023 11:05:25 -0800	[thread overview]
Message-ID: <5dbce62b-5c51-6989-d08d-16955d97b4e6@quicinc.com> (raw)
In-Reply-To: <Y9eVcHM9nZZ/4ZSh@kroah.com>

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@quicinc.com>
>> ---
>>   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@vger.kernel.org
>>   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.

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