| Message ID | 20221011000840.289033-7-quic_eberman@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
On Mon, Oct 10, 2022 at 05:08:33PM -0700, Elliot Berman wrote: > Export the version of Gunyah which is reported via the hyp_identify > hypercall. Increments of the major API version indicate possibly > backwards incompatible changes. Export the hypervisor identity so that > Gunyah drivers can act according to the major API version. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > MAINTAINERS | 1 + > drivers/virt/Makefile | 1 + > drivers/virt/gunyah/Makefile | 2 ++ > drivers/virt/gunyah/gunyah.c | 41 ++++++++++++++++++++++++++++++++++++ > include/asm-generic/gunyah.h | 3 +++ > 5 files changed, 48 insertions(+) > create mode 100644 drivers/virt/gunyah/Makefile > create mode 100644 drivers/virt/gunyah/gunyah.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ed2bc98c3818..c5458aeec023 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8884,6 +8884,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 That file is not in this patch :( > 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..dc081e2dc02b > --- /dev/null > +++ b/drivers/virt/gunyah/Makefile > @@ -0,0 +1,2 @@ > +gunyah-y += gunyah.o > +obj-$(CONFIG_GUNYAH) += gunyah.o > diff --git a/drivers/virt/gunyah/gunyah.c b/drivers/virt/gunyah/gunyah.c > new file mode 100644 > index 000000000000..2893a56f3dfc > --- /dev/null > +++ b/drivers/virt/gunyah/gunyah.c > @@ -0,0 +1,41 @@ > +// 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/module.h> > +#include <linux/printk.h> > +#include <linux/init.h> > +#include <asm-generic/gunyah.h> > + > +struct gh_hypercall_hyp_identify_resp gunyah_api; > +EXPORT_SYMBOL(gunyah_api); EXPORT_SYMBOL_GPL()? I have to ask. But why is it exported at all? No one is using it in this patch. > + > +static int __init gunyah_init(void) > +{ > + u32 uid[4]; > + > + gh_hypercall_get_uid(uid); > + > + if (!(gh_uid_matches(GUNYAH, uid) || gh_uid_matches(QC_HYP, uid))) > + return 0; Why return success if this is not true? Shouldn't you return an error and fail to load? > + > + gh_hypercall_hyp_identify(&gunyah_api); > + > + pr_info("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)); > + > + return 0; > +} > +arch_initcall(gunyah_init); > + > +static void __exit gunyah_exit(void) > +{ > +} > +module_exit(gunyah_exit); Why do you need a module_exit() call? > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Gunyah Hypervisor Driver"); What will cause this module to be properly automatically loaded? I do not see that happening here at all. > diff --git a/include/asm-generic/gunyah.h b/include/asm-generic/gunyah.h > index 86eb59e203ef..8f9d4c649ba8 100644 > --- a/include/asm-generic/gunyah.h > +++ b/include/asm-generic/gunyah.h > @@ -85,6 +85,8 @@ static inline int gh_remap_error(int gh_error) > ((uid)[0] == prefix ## _UID0 && (uid)[1] == prefix ## _UID1 && \ > (uid)[2] == prefix ## _UID2 && (uid)[3] == prefix ## _UID3) > > +#define GUNYAH_API_V1 1 You do not use this define anywhere in this patch. > + > #define GH_API_INFO_API_VERSION(x) (((x) >> 0) & 0x3fff) > #define GH_API_INFO_BIG_ENDIAN(x) (((x) >> 14) & 1) > #define GH_API_INFO_IS_64BIT(x) (((x) >> 15) & 1) > @@ -103,6 +105,7 @@ struct gh_hypercall_hyp_identify_resp { > u64 api_info; > u64 flags[3]; > }; > +extern struct gh_hypercall_hyp_identify_resp gunyah_api; Again, not used. thanks, greg k-h
Hi Elliot,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 4fe89d07dcc2804c8b562f6c7896a45643d34b2f]
url: https://github.com/intel-lab-lkp/linux/commits/Elliot-Berman/Drivers-for-gunyah-hypervisor/20221011-081934
base: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
reproduce:
# https://github.com/intel-lab-lkp/linux/commit/8abb083e1c217e016adc6b955d8311c5f2bbcbbd
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Elliot-Berman/Drivers-for-gunyah-hypervisor/20221011-081934
git checkout 8abb083e1c217e016adc6b955d8311c5f2bbcbbd
make menuconfig
# enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
make htmldocs
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/ABI/testing/sysfs-hypervisor-gunyah
On 10/10/2022 11:13 PM, Greg Kroah-Hartman wrote: > On Mon, Oct 10, 2022 at 05:08:33PM -0700, Elliot Berman wrote: > > EXPORT_SYMBOL_GPL()? I have to ask. typo only :) > > But why is it exported at all? No one is using it in this patch. > It's used later in the series by the message queue driver. The idea here now is that gunyah.ko is capable of identifying Gunyah and other drivers can check if they are individually compatible with the reported version of Gunyah. From Patch 9: +static int __init gh_msgq_init(void) +{ + if (GH_API_INFO_API_VERSION(gunyah_api.api_info) != GUNYAH_API_V1) { + pr_warn("Unrecognized gunyah version: %llu. Currently supported: %d\n", + GH_API_INFO_API_VERSION(gunyah_api.api_info), GUNYAH_API_V1); + return -ENODEV; + } + + return 0; +} >> + >> +static int __init gunyah_init(void) >> +{ >> + u32 uid[4]; >> + >> + gh_hypercall_get_uid(uid); >> + >> + if (!(gh_uid_matches(GUNYAH, uid) || gh_uid_matches(QC_HYP, uid))) >> + return 0; > > Why return success if this is not true? Shouldn't you return an error > and fail to load? > That's fair -- easy to fix. >> + >> + gh_hypercall_hyp_identify(&gunyah_api); >> + >> + pr_info("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)); >> + >> + return 0; >> +} >> +arch_initcall(gunyah_init); >> + >> +static void __exit gunyah_exit(void) >> +{ >> +} >> +module_exit(gunyah_exit); > > Why do you need a module_exit() call? > I can remove. >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("Gunyah Hypervisor Driver"); > > What will cause this module to be properly automatically loaded? I do > not see that happening here at all. > >> diff --git a/include/asm-generic/gunyah.h b/include/asm-generic/gunyah.h >> index 86eb59e203ef..8f9d4c649ba8 100644 >> --- a/include/asm-generic/gunyah.h >> +++ b/include/asm-generic/gunyah.h >> @@ -85,6 +85,8 @@ static inline int gh_remap_error(int gh_error) >> ((uid)[0] == prefix ## _UID0 && (uid)[1] == prefix ## _UID1 && \ >> (uid)[2] == prefix ## _UID2 && (uid)[3] == prefix ## _UID3) >> >> +#define GUNYAH_API_V1 1 > > You do not use this define anywhere in this patch. > > >> + >> #define GH_API_INFO_API_VERSION(x) (((x) >> 0) & 0x3fff) >> #define GH_API_INFO_BIG_ENDIAN(x) (((x) >> 14) & 1) >> #define GH_API_INFO_IS_64BIT(x) (((x) >> 15) & 1) >> @@ -103,6 +105,7 @@ struct gh_hypercall_hyp_identify_resp { >> u64 api_info; >> u64 flags[3]; >> }; >> +extern struct gh_hypercall_hyp_identify_resp gunyah_api; > > Again, not used. > > thanks, > > greg k-h
On Thu, Oct 13, 2022 at 04:00:10PM -0700, Elliot Berman wrote: > On 10/10/2022 11:13 PM, Greg Kroah-Hartman wrote: > > On Mon, Oct 10, 2022 at 05:08:33PM -0700, Elliot Berman wrote: > > > > EXPORT_SYMBOL_GPL()? I have to ask. > > typo only :) > > > > > But why is it exported at all? No one is using it in this patch. > > > It's used later in the series by the message queue driver. The idea here now > is that gunyah.ko is capable of identifying Gunyah and other drivers can > check if they are individually compatible with the reported version of > Gunyah. Then export it when you use it. If we had taken just half of the series, then this would be an export that no one used, which is not ok. thanks, greg k-h
diff --git a/MAINTAINERS b/MAINTAINERS index ed2bc98c3818..c5458aeec023 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8884,6 +8884,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..dc081e2dc02b --- /dev/null +++ b/drivers/virt/gunyah/Makefile @@ -0,0 +1,2 @@ +gunyah-y += gunyah.o +obj-$(CONFIG_GUNYAH) += gunyah.o diff --git a/drivers/virt/gunyah/gunyah.c b/drivers/virt/gunyah/gunyah.c new file mode 100644 index 000000000000..2893a56f3dfc --- /dev/null +++ b/drivers/virt/gunyah/gunyah.c @@ -0,0 +1,41 @@ +// 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/module.h> +#include <linux/printk.h> +#include <linux/init.h> +#include <asm-generic/gunyah.h> + +struct gh_hypercall_hyp_identify_resp gunyah_api; +EXPORT_SYMBOL(gunyah_api); + +static int __init gunyah_init(void) +{ + u32 uid[4]; + + gh_hypercall_get_uid(uid); + + if (!(gh_uid_matches(GUNYAH, uid) || gh_uid_matches(QC_HYP, uid))) + return 0; + + gh_hypercall_hyp_identify(&gunyah_api); + + pr_info("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)); + + return 0; +} +arch_initcall(gunyah_init); + +static void __exit gunyah_exit(void) +{ +} +module_exit(gunyah_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Gunyah Hypervisor Driver"); diff --git a/include/asm-generic/gunyah.h b/include/asm-generic/gunyah.h index 86eb59e203ef..8f9d4c649ba8 100644 --- a/include/asm-generic/gunyah.h +++ b/include/asm-generic/gunyah.h @@ -85,6 +85,8 @@ static inline int gh_remap_error(int gh_error) ((uid)[0] == prefix ## _UID0 && (uid)[1] == prefix ## _UID1 && \ (uid)[2] == prefix ## _UID2 && (uid)[3] == prefix ## _UID3) +#define GUNYAH_API_V1 1 + #define GH_API_INFO_API_VERSION(x) (((x) >> 0) & 0x3fff) #define GH_API_INFO_BIG_ENDIAN(x) (((x) >> 14) & 1) #define GH_API_INFO_IS_64BIT(x) (((x) >> 15) & 1) @@ -103,6 +105,7 @@ struct gh_hypercall_hyp_identify_resp { u64 api_info; u64 flags[3]; }; +extern struct gh_hypercall_hyp_identify_resp gunyah_api; void gh_hypercall_get_uid(u32 *uid); void gh_hypercall_hyp_identify(struct gh_hypercall_hyp_identify_resp *hyp_identity);
Export the version of Gunyah which is reported via the hyp_identify hypercall. Increments of the major API version indicate possibly backwards incompatible changes. Export the hypervisor identity so that Gunyah drivers can act according to the major API version. Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> --- MAINTAINERS | 1 + drivers/virt/Makefile | 1 + drivers/virt/gunyah/Makefile | 2 ++ drivers/virt/gunyah/gunyah.c | 41 ++++++++++++++++++++++++++++++++++++ include/asm-generic/gunyah.h | 3 +++ 5 files changed, 48 insertions(+) create mode 100644 drivers/virt/gunyah/Makefile create mode 100644 drivers/virt/gunyah/gunyah.c