Re: [PATCH v4 06/14] virt: gunyah: Add sysfs nodes

[Elliot Berman <quic_eberman@quicinc.com>]

On 9/30/2022 5:09 AM, Greg Kroah-Hartman wrote:
> On Wed, Sep 28, 2022 at 12:56:25PM -0700, Elliot Berman wrote:
>> Add /sys/hypervisor support when detecting that Linux is running in a
>> Gunyah environment. Export the version of Gunyah which is reported via
>> the hyp_identify hypercall.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-hypervisor-gunyah       | 15 ++++
>>   MAINTAINERS                                   |  1 +
>>   drivers/virt/Makefile                         |  1 +
>>   drivers/virt/gunyah/Makefile                  |  2 +
>>   drivers/virt/gunyah/sysfs.c                   | 71 +++++++++++++++++++
>>   5 files changed, 90 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-hypervisor-gunyah
>>   create mode 100644 drivers/virt/gunyah/Makefile
>>   create mode 100644 drivers/virt/gunyah/sysfs.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor-gunyah b/Documentation/ABI/testing/sysfs-hypervisor-gunyah
>> new file mode 100644
>> index 000000000000..7d74e74e9edd
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor-gunyah
>> @@ -0,0 +1,15 @@
>> +What:		/sys/hypervisor/gunyah/api
>> +Date:		October 2022
>> +KernelVersion:	6.1
>> +Contact:	linux-arm-msm@vger.kernel.org
>> +Description:	If running under Gunyah:
>> +		The Gunyah API version.
> 
> What does this version mean?  What format is it in?
> 

The version is incremented on backwards-incompatible API changes. It's 
an integer: I've updated the description to mention it's an integer. FYI 
-- we are still currently at "1" and not aiming to increment this 
number. I'd like to get it reported in sysfs in case the version is 
incremented later.

>> +
>> +What:		/sys/hypervisor/gunyah/variant
>> +Date:		October 2022
>> +KernelVersion:	6.1
>> +Contact:	linux-arm-msm@vger.kernel.org
>> +Description:	If running under Gunyah:
>> +		Reports the build variant of Gunyah:
>> +		The open source build of Gunyah will report "81".
>> +		The Qualcomm build of Gunyah will report "72".
> 
> So there are only 2 versions variants?  What happens when you get a
> third?  And why the odd numbers?
> 

The kernel isn't parsing the reported build variant and is passing the 
reported value up to the sysfs node. If a new third variant comes along, 
its build variant number would be reported. Would it be preferred to 
instead link to Gunyah's definitions for the build variant?

> What will userspace do with this information and what tool will parse
> it? >

The usecase I'm envisioning is to help user check what build of Gunyah 
is present on the host. We don't have any intention to require userspace 
(or kernel) to behave differently whether they are on Qualcomm-built 
Gunyah or the open-source Gunyah.

>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index feafac12db35..a26e67ef36b4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8885,6 +8885,7 @@ M:	Elliot Berman <quic_eberman@quicinc.com>
>>   M:	Murali Nalajala <quic_mnalajal@quicinc.com>
>>   L:	linux-arm-msm@vger.kernel.org
>>   S:	Supported
>> +F:	Documentation/ABI/testing/sysfs-hypervisor-gunyah
>>   F:	Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml
>>   F:	Documentation/virt/gunyah/
>>   F:	arch/arm64/gunyah/
>> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
>> index 093674e05c40..10b87f934730 100644
>> --- a/drivers/virt/Makefile
>> +++ b/drivers/virt/Makefile
>> @@ -11,3 +11,4 @@ obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
>>   obj-$(CONFIG_ACRN_HSM)		+= acrn/
>>   obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
>>   obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
>> +obj-$(CONFIG_GUNYAH)		+= gunyah/
>> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
>> new file mode 100644
>> index 000000000000..e15f16c17142
>> --- /dev/null
>> +++ b/drivers/virt/gunyah/Makefile
>> @@ -0,0 +1,2 @@
>> +gunyah-y += sysfs.o
>> +obj-$(CONFIG_GUNYAH) += gunyah.o
>> diff --git a/drivers/virt/gunyah/sysfs.c b/drivers/virt/gunyah/sysfs.c
>> new file mode 100644
>> index 000000000000..ec11510cbece
>> --- /dev/null
>> +++ b/drivers/virt/gunyah/sysfs.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "gunyah: " fmt
>> +
>> +#include <linux/kobject.h>
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/init.h>
>> +#include <asm-generic/gunyah.h>
>> +
>> +static struct gh_hypercall_hyp_identify_resp gunyah_api;
>> +
>> +static ssize_t api_show(struct kobject *kobj, struct kobj_attribute *attr, char *buffer)
>> +{
>> +	return sysfs_emit(buffer, "%d\n", (int)GH_API_INFO_API_VERSION(gunyah_api.api_info));
>> +}
>> +static struct kobj_attribute api_attr = __ATTR_RO(api);
>> +
>> +static ssize_t variant_show(struct kobject *kobj, struct kobj_attribute *attr, char *buffer)
>> +{
>> +	return sysfs_emit(buffer, "%d\n", (int)GH_API_INFO_VARIANT(gunyah_api.api_info));
>> +}
>> +static struct kobj_attribute variant_attr = __ATTR_RO(variant);
>> +
>> +static struct attribute *gunyah_attrs[] = {
>> +	&api_attr.attr,
>> +	&variant_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group gunyah_group = {
>> +	.name = "gunyah",
>> +	.attrs = gunyah_attrs,
>> +};
>> +
>> +static int __init gunyah_init(void)
>> +{
>> +	u32 uid[4];
>> +
>> +	gh_hypercall_get_uid(uid);
> 
> No error checking?
> 

The UID is filled by the first 4 return registers of the hypercall. If 
running under some other hypervisor or no hypervisor is present, then 
our expectation is that the relevant handler would fill some error value 
that isn't the UUID.

KVM uses similar approach to allow guests to identify they are a KVM guest:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/smccc/kvm_guest.c?h=v6.0-rc7#n23

>> +
>> +	if (!(gh_uid_matches(GUNYAH, uid) || gh_uid_matches(QC_HYP, uid)))
>> +		return 0;
>> +
>> +	gh_hypercall_hyp_identify(&gunyah_api);
>> +
>> +	if (GH_API_INFO_API_VERSION(gunyah_api.api_info) != 1) {
>> +		pr_warn("Unrecognized gunyah version: %llu. Currently supported: 1\n",
>> +			GH_API_INFO_API_VERSION(gunyah_api.api_info));
> 
> Shouldn't the "1" be defined somewhere?
> 
>> +		return 0;
>> +	}
>> +
>> +	pr_notice("Running under Gunyah hypervisor %llx/v%lld\n",
>> +		  GH_API_INFO_VARIANT(gunyah_api.api_info),
>> +		  GH_API_INFO_API_VERSION(gunyah_api.api_info));
> 
> When kernel code is working properly, it is quiet.  What is this going
> to be used for?
> 

Xen and KVM guest drivers also report when they detect that they are 
running under a hypervisor. I'll drop this down to pr_info to match.

> thanks,
> 
> greg k-h

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