LinuxLists.cc - [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

2021-02-02 22:25:56

Subject: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

From: Will Deacon <[email protected]>

Although the SMCCC specification provides some limited functionality for
describing the presence of hypervisor and firmware services, this is
generally applicable only to functions designated as "Arm Architecture
Service Functions" and no portable discovery mechanism is provided for
standard hypervisor services, despite having a designated range of
function identifiers reserved by the specification.

In an attempt to avoid the need for additional firmware changes every
time a new function is added, introduce a UID to identify the service
provider as being compatible with KVM. Once this has been established,
additional services can be discovered via a feature bitmap.

Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Jianyong Wu <[email protected]>
[maz: move code to its own file, plug it into PSCI]
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/arm/include/asm/hypervisor.h | 3 ++
arch/arm64/include/asm/hypervisor.h | 3 ++
drivers/firmware/psci/psci.c | 2 ++
drivers/firmware/smccc/Makefile | 2 +-
drivers/firmware/smccc/kvm_guest.c | 51 +++++++++++++++++++++++++++++
drivers/firmware/smccc/smccc.c | 1 +
include/linux/arm-smccc.h | 25 ++++++++++++++
7 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/smccc/kvm_guest.c

diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
index df8524365637..bd61502b9715 100644
--- a/arch/arm/include/asm/hypervisor.h
+++ b/arch/arm/include/asm/hypervisor.h
@@ -4,4 +4,7 @@

#include <asm/xen/hypervisor.h>

+void kvm_init_hyp_services(void);
+bool kvm_arm_hyp_service_available(u32 func_id);
+
#endif
diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
index f9cc1d021791..0ae427f352c8 100644
--- a/arch/arm64/include/asm/hypervisor.h
+++ b/arch/arm64/include/asm/hypervisor.h
@@ -4,4 +4,7 @@

#include <asm/xen/hypervisor.h>

+void kvm_init_hyp_services(void);
+bool kvm_arm_hyp_service_available(u32 func_id);
+
#endif
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f5fc429cae3f..69e296f02902 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -23,6 +23,7 @@

#include <asm/cpuidle.h>
#include <asm/cputype.h>
+#include <asm/hypervisor.h>
#include <asm/system_misc.h>
#include <asm/smp_plat.h>
#include <asm/suspend.h>
@@ -498,6 +499,7 @@ static int __init psci_probe(void)
psci_init_cpu_suspend();
psci_init_system_suspend();
psci_init_system_reset2();
+ kvm_init_hyp_services();
}

return 0;
diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
index 72ab84042832..40d19144a860 100644
--- a/drivers/firmware/smccc/Makefile
+++ b/drivers/firmware/smccc/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
#
-obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o
+obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o kvm_guest.o
obj-$(CONFIG_ARM_SMCCC_SOC_ID) += soc_id.o
diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
new file mode 100644
index 000000000000..23ce1ded88b4
--- /dev/null
+++ b/drivers/firmware/smccc/kvm_guest.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) "smccc: KVM: " fmt
+
+#include <linux/init.h>
+#include <linux/arm-smccc.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+
+static DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS) __ro_after_init = { };
+
+void __init kvm_init_hyp_services(void)
+{
+ int i;
+ struct arm_smccc_res res;
+
+ if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
+ return;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
+ if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
+ res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
+ res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
+ res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
+ return;
+
+ memset(&res, 0, sizeof(res));
+ arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
+ for (i = 0; i < 32; ++i) {
+ if (res.a0 & (i))
+ set_bit(i + (32 * 0), __kvm_arm_hyp_services);
+ if (res.a1 & (i))
+ set_bit(i + (32 * 1), __kvm_arm_hyp_services);
+ if (res.a2 & (i))
+ set_bit(i + (32 * 2), __kvm_arm_hyp_services);
+ if (res.a3 & (i))
+ set_bit(i + (32 * 3), __kvm_arm_hyp_services);
+ }
+
+ pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
+ res.a3, res.a2, res.a1, res.a0);
+}
+
+bool kvm_arm_hyp_service_available(u32 func_id)
+{
+ if (func_id >= ARM_SMCCC_KVM_NUM_FUNCS)
+ return -EINVAL;
+
+ return test_bit(func_id, __kvm_arm_hyp_services);
+}
+EXPORT_SYMBOL_GPL(kvm_arm_hyp_service_available);
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index 00c88b809c0c..94eca6ffda05 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -7,6 +7,7 @@

#include <linux/init.h>
#include <linux/arm-smccc.h>
+#include <linux/kernel.h>

static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index f860645f6512..74e90b65b489 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -55,6 +55,8 @@
#define ARM_SMCCC_OWNER_TRUSTED_OS 50
#define ARM_SMCCC_OWNER_TRUSTED_OS_END 63

+#define ARM_SMCCC_FUNC_QUERY_CALL_UID 0xff01
+
#define ARM_SMCCC_QUIRK_NONE 0
#define ARM_SMCCC_QUIRK_QCOM_A6 1 /* Save/restore register a6 */

@@ -87,6 +89,29 @@
ARM_SMCCC_SMC_32, \
0, 0x7fff)

+#define ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_VENDOR_HYP, \
+ ARM_SMCCC_FUNC_QUERY_CALL_UID)
+
+/* KVM UID value: 28b46fb6-2ec5-11e9-a9ca-4b564d003a74 */
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 0xb66fb428U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 0xe911c52eU
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 0x564bcaa9U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3 0x743a004dU
+
+/* KVM "vendor specific" services */
+#define ARM_SMCCC_KVM_FUNC_FEATURES 0
+#define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
+#define ARM_SMCCC_KVM_NUM_FUNCS 128
+
+#define ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_VENDOR_HYP, \
+ ARM_SMCCC_KVM_FUNC_FEATURES)
+
#define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED 1

/* Paravirtualised time calls (defined by ARM DEN0057A) */
--
2.29.2

2021-02-05 09:17:43

by Steven Price

[permalink] [raw]

Subject: Re: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

On 02/02/2021 14:11, Marc Zyngier wrote:
> From: Will Deacon <[email protected]>
>
> Although the SMCCC specification provides some limited functionality for
> describing the presence of hypervisor and firmware services, this is
> generally applicable only to functions designated as "Arm Architecture
> Service Functions" and no portable discovery mechanism is provided for
> standard hypervisor services, despite having a designated range of
> function identifiers reserved by the specification.
>
> In an attempt to avoid the need for additional firmware changes every
> time a new function is added, introduce a UID to identify the service
> provider as being compatible with KVM. Once this has been established,
> additional services can be discovered via a feature bitmap.
>
> Signed-off-by: Will Deacon <[email protected]>
> Signed-off-by: Jianyong Wu <[email protected]>
> [maz: move code to its own file, plug it into PSCI]
> Signed-off-by: Marc Zyngier <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> arch/arm/include/asm/hypervisor.h | 3 ++
> arch/arm64/include/asm/hypervisor.h | 3 ++
> drivers/firmware/psci/psci.c | 2 ++
> drivers/firmware/smccc/Makefile | 2 +-
> drivers/firmware/smccc/kvm_guest.c | 51 +++++++++++++++++++++++++++++
> drivers/firmware/smccc/smccc.c | 1 +
> include/linux/arm-smccc.h | 25 ++++++++++++++
> 7 files changed, 86 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/smccc/kvm_guest.c
>
> diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
> index df8524365637..bd61502b9715 100644
> --- a/arch/arm/include/asm/hypervisor.h
> +++ b/arch/arm/include/asm/hypervisor.h
> @@ -4,4 +4,7 @@
>
> #include <asm/xen/hypervisor.h>
>
> +void kvm_init_hyp_services(void);
> +bool kvm_arm_hyp_service_available(u32 func_id);
> +
> #endif
> diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
> index f9cc1d021791..0ae427f352c8 100644
> --- a/arch/arm64/include/asm/hypervisor.h
> +++ b/arch/arm64/include/asm/hypervisor.h
> @@ -4,4 +4,7 @@
>
> #include <asm/xen/hypervisor.h>
>
> +void kvm_init_hyp_services(void);
> +bool kvm_arm_hyp_service_available(u32 func_id);
> +
> #endif
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index f5fc429cae3f..69e296f02902 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -23,6 +23,7 @@
>
> #include <asm/cpuidle.h>
> #include <asm/cputype.h>
> +#include <asm/hypervisor.h>
> #include <asm/system_misc.h>
> #include <asm/smp_plat.h>
> #include <asm/suspend.h>
> @@ -498,6 +499,7 @@ static int __init psci_probe(void)
> psci_init_cpu_suspend();
> psci_init_system_suspend();
> psci_init_system_reset2();
> + kvm_init_hyp_services();
> }
>
> return 0;
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 72ab84042832..40d19144a860 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> #
> -obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o
> +obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o kvm_guest.o
> obj-$(CONFIG_ARM_SMCCC_SOC_ID) += soc_id.o
> diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> new file mode 100644
> index 000000000000..23ce1ded88b4
> --- /dev/null
> +++ b/drivers/firmware/smccc/kvm_guest.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt) "smccc: KVM: " fmt
> +
> +#include <linux/init.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +static DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS) __ro_after_init = { };
> +
> +void __init kvm_init_hyp_services(void)
> +{
> + int i;
> + struct arm_smccc_res res;
> +
> + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> + return;
> +
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
> + if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
> + res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
> + res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
> + res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
> + return;
> +
> + memset(&res, 0, sizeof(res));
> + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
> + for (i = 0; i < 32; ++i) {
> + if (res.a0 & (i))
> + set_bit(i + (32 * 0), __kvm_arm_hyp_services);
> + if (res.a1 & (i))
> + set_bit(i + (32 * 1), __kvm_arm_hyp_services);
> + if (res.a2 & (i))
> + set_bit(i + (32 * 2), __kvm_arm_hyp_services);
> + if (res.a3 & (i))
> + set_bit(i + (32 * 3), __kvm_arm_hyp_services);

The bit shifts are missing, the tests should be of the form:

if (res.a0 & (1 << i))

Or indeed using a BIT() macro.

Steve

> + }
> +
> + pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
> + res.a3, res.a2, res.a1, res.a0);
> +}
> +
> +bool kvm_arm_hyp_service_available(u32 func_id)
> +{
> + if (func_id >= ARM_SMCCC_KVM_NUM_FUNCS)
> + return -EINVAL;
> +
> + return test_bit(func_id, __kvm_arm_hyp_services);
> +}
> +EXPORT_SYMBOL_GPL(kvm_arm_hyp_service_available);
> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index 00c88b809c0c..94eca6ffda05 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -7,6 +7,7 @@
>
> #include <linux/init.h>
> #include <linux/arm-smccc.h>
> +#include <linux/kernel.h>
>
> static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
> static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index f860645f6512..74e90b65b489 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -55,6 +55,8 @@
> #define ARM_SMCCC_OWNER_TRUSTED_OS 50
> #define ARM_SMCCC_OWNER_TRUSTED_OS_END 63
>
> +#define ARM_SMCCC_FUNC_QUERY_CALL_UID 0xff01
> +
> #define ARM_SMCCC_QUIRK_NONE 0
> #define ARM_SMCCC_QUIRK_QCOM_A6 1 /* Save/restore register a6 */
>
> @@ -87,6 +89,29 @@
> ARM_SMCCC_SMC_32, \
> 0, 0x7fff)
>
> +#define ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_VENDOR_HYP, \
> + ARM_SMCCC_FUNC_QUERY_CALL_UID)
> +
> +/* KVM UID value: 28b46fb6-2ec5-11e9-a9ca-4b564d003a74 */
> +#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 0xb66fb428U
> +#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 0xe911c52eU
> +#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 0x564bcaa9U
> +#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3 0x743a004dU
> +
> +/* KVM "vendor specific" services */
> +#define ARM_SMCCC_KVM_FUNC_FEATURES 0
> +#define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
> +#define ARM_SMCCC_KVM_NUM_FUNCS 128
> +
> +#define ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_VENDOR_HYP, \
> + ARM_SMCCC_KVM_FUNC_FEATURES)
> +
> #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED 1
>
> /* Paravirtualised time calls (defined by ARM DEN0057A) */
>

2021-02-05 11:29:09

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

2021-02-05 11:29:36

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

On Fri, Feb 05, 2021 at 09:11:00AM +0000, Steven Price wrote:
> On 02/02/2021 14:11, Marc Zyngier wrote:
> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> > new file mode 100644
> > index 000000000000..23ce1ded88b4
> > --- /dev/null
> > +++ b/drivers/firmware/smccc/kvm_guest.c
> > @@ -0,0 +1,51 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define pr_fmt(fmt) "smccc: KVM: " fmt
> > +
> > +#include <linux/init.h>
> > +#include <linux/arm-smccc.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +static DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS) __ro_after_init = { };
> > +
> > +void __init kvm_init_hyp_services(void)
> > +{
> > + int i;
> > + struct arm_smccc_res res;
> > +
> > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> > + return;
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
> > + if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
> > + res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
> > + res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
> > + res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
> > + return;
> > +
> > + memset(&res, 0, sizeof(res));
> > + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
> > + for (i = 0; i < 32; ++i) {
> > + if (res.a0 & (i))
> > + set_bit(i + (32 * 0), __kvm_arm_hyp_services);
> > + if (res.a1 & (i))
> > + set_bit(i + (32 * 1), __kvm_arm_hyp_services);
> > + if (res.a2 & (i))
> > + set_bit(i + (32 * 2), __kvm_arm_hyp_services);
> > + if (res.a3 & (i))
> > + set_bit(i + (32 * 3), __kvm_arm_hyp_services);
>
> The bit shifts are missing, the tests should be of the form:
>
> if (res.a0 & (1 << i))
>
> Or indeed using a BIT() macro.

Maybe even test_bit()?

Will

2021-02-05 20:07:00

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

On Fri, Feb 05, 2021 at 04:50:27PM +0000, Marc Zyngier wrote:
> On 2021-02-05 11:19, Will Deacon wrote:
> > On Fri, Feb 05, 2021 at 09:11:00AM +0000, Steven Price wrote:
> > > On 02/02/2021 14:11, Marc Zyngier wrote:
> > > > + for (i = 0; i < 32; ++i) {
> > > > + if (res.a0 & (i))
> > > > + set_bit(i + (32 * 0), __kvm_arm_hyp_services);
> > > > + if (res.a1 & (i))
> > > > + set_bit(i + (32 * 1), __kvm_arm_hyp_services);
> > > > + if (res.a2 & (i))
> > > > + set_bit(i + (32 * 2), __kvm_arm_hyp_services);
> > > > + if (res.a3 & (i))
> > > > + set_bit(i + (32 * 3), __kvm_arm_hyp_services);
> > >
> > > The bit shifts are missing, the tests should be of the form:
> > >
> > > if (res.a0 & (1 << i))
> > >
> > > Or indeed using a BIT() macro.
> >
> > Maybe even test_bit()?
>
> Actually, maybe not doing things a-bit-at-a-time is less error prone.
> See below what I intend to fold in.
>
> Thanks,
>
> M.
>
> diff --git a/drivers/firmware/smccc/kvm_guest.c
> b/drivers/firmware/smccc/kvm_guest.c
> index 00bf3c7969fc..08836f2f39ee 100644
> --- a/drivers/firmware/smccc/kvm_guest.c
> +++ b/drivers/firmware/smccc/kvm_guest.c
> @@ -2,8 +2,8 @@
>
> #define pr_fmt(fmt) "smccc: KVM: " fmt
>
> -#include <linux/init.h>
> #include <linux/arm-smccc.h>
> +#include <linux/bitmap.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
>
> @@ -13,8 +13,8 @@ static DECLARE_BITMAP(__kvm_arm_hyp_services,
> ARM_SMCCC_KVM_NUM_FUNCS) __ro_afte
>
> void __init kvm_init_hyp_services(void)
> {
> - int i;
> struct arm_smccc_res res;
> + u32 val[4];
>
> if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> return;
> @@ -28,16 +28,13 @@ void __init kvm_init_hyp_services(void)
>
> memset(&res, 0, sizeof(res));
> arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
> - for (i = 0; i < 32; ++i) {
> - if (res.a0 & (i))
> - set_bit(i + (32 * 0), __kvm_arm_hyp_services);
> - if (res.a1 & (i))
> - set_bit(i + (32 * 1), __kvm_arm_hyp_services);
> - if (res.a2 & (i))
> - set_bit(i + (32 * 2), __kvm_arm_hyp_services);
> - if (res.a3 & (i))
> - set_bit(i + (32 * 3), __kvm_arm_hyp_services);
> - }
> +
> + val[0] = lower_32_bits(res.a0);
> + val[1] = lower_32_bits(res.a1);
> + val[2] = lower_32_bits(res.a2);
> + val[3] = lower_32_bits(res.a3);
> +
> + bitmap_from_arr32(__kvm_arm_hyp_services, val, ARM_SMCCC_KVM_NUM_FUNCS);

Nice! That's loads better.

Will

2021-02-05 20:13:21

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH v17 1/7] arm/arm64: Probe for the presence of KVM hypervisor

On 2021-02-05 11:19, Will Deacon wrote:
> On Fri, Feb 05, 2021 at 09:11:00AM +0000, Steven Price wrote:
>> On 02/02/2021 14:11, Marc Zyngier wrote:
>> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
>> > new file mode 100644
>> > index 000000000000..23ce1ded88b4
>> > --- /dev/null
>> > +++ b/drivers/firmware/smccc/kvm_guest.c
>> > @@ -0,0 +1,51 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +#define pr_fmt(fmt) "smccc: KVM: " fmt
>> > +
>> > +#include <linux/init.h>
>> > +#include <linux/arm-smccc.h>
>> > +#include <linux/kernel.h>
>> > +#include <linux/string.h>
>> > +
>> > +static DECLARE_BITMAP(__kvm_arm_hyp_services, ARM_SMCCC_KVM_NUM_FUNCS) __ro_after_init = { };
>> > +
>> > +void __init kvm_init_hyp_services(void)
>> > +{
>> > + int i;
>> > + struct arm_smccc_res res;
>> > +
>> > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
>> > + return;
>> > +
>> > + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
>> > + if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
>> > + res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
>> > + res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
>> > + res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
>> > + return;
>> > +
>> > + memset(&res, 0, sizeof(res));
>> > + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
>> > + for (i = 0; i < 32; ++i) {
>> > + if (res.a0 & (i))
>> > + set_bit(i + (32 * 0), __kvm_arm_hyp_services);
>> > + if (res.a1 & (i))
>> > + set_bit(i + (32 * 1), __kvm_arm_hyp_services);
>> > + if (res.a2 & (i))
>> > + set_bit(i + (32 * 2), __kvm_arm_hyp_services);
>> > + if (res.a3 & (i))
>> > + set_bit(i + (32 * 3), __kvm_arm_hyp_services);
>>
>> The bit shifts are missing, the tests should be of the form:
>>
>> if (res.a0 & (1 << i))
>>
>> Or indeed using a BIT() macro.
>
> Maybe even test_bit()?

Actually, maybe not doing things a-bit-at-a-time is less error prone.
See below what I intend to fold in.

Thanks,

M.

diff --git a/drivers/firmware/smccc/kvm_guest.c
b/drivers/firmware/smccc/kvm_guest.c
index 00bf3c7969fc..08836f2f39ee 100644
--- a/drivers/firmware/smccc/kvm_guest.c
+++ b/drivers/firmware/smccc/kvm_guest.c
@@ -2,8 +2,8 @@

#define pr_fmt(fmt) "smccc: KVM: " fmt

-#include <linux/init.h>
#include <linux/arm-smccc.h>
+#include <linux/bitmap.h>
#include <linux/kernel.h>
#include <linux/string.h>

@@ -13,8 +13,8 @@ static DECLARE_BITMAP(__kvm_arm_hyp_services,
ARM_SMCCC_KVM_NUM_FUNCS) __ro_afte

void __init kvm_init_hyp_services(void)
{
- int i;
struct arm_smccc_res res;
+ u32 val[4];

if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
return;
@@ -28,16 +28,13 @@ void __init kvm_init_hyp_services(void)

memset(&res, 0, sizeof(res));
arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID, &res);
- for (i = 0; i < 32; ++i) {
- if (res.a0 & (i))
- set_bit(i + (32 * 0), __kvm_arm_hyp_services);
- if (res.a1 & (i))
- set_bit(i + (32 * 1), __kvm_arm_hyp_services);
- if (res.a2 & (i))
- set_bit(i + (32 * 2), __kvm_arm_hyp_services);
- if (res.a3 & (i))
- set_bit(i + (32 * 3), __kvm_arm_hyp_services);
- }
+
+ val[0] = lower_32_bits(res.a0);
+ val[1] = lower_32_bits(res.a1);
+ val[2] = lower_32_bits(res.a2);
+ val[3] = lower_32_bits(res.a3);
+
+ bitmap_from_arr32(__kvm_arm_hyp_services, val,
ARM_SMCCC_KVM_NUM_FUNCS);

pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx
0x%08lx)\n",
res.a3, res.a2, res.a1, res.a0);

--
Jazz is not dead. It just smells funny...