Skip to contentSkip to conversation

arm64: SMCCC conduit cleanup

PATCH Superseded Mark Rutland 6 participants 13 messages May 3, 2018 – Feb 13, 2019

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Mark Rutland

author maintainer
#0 lore raw

cover arm64: SMCCC conduit cleanup

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

Mark.

Mark Rutland (4):
arm/arm64: smccc/psci: add arm_smccc_get_conduit()
arm64: errata: use arm_smccc_get_conduit() firmware/psci: use common SMCCC_CONDUIT_*
firmware: arm_sdei: use common SMCCC_CONDUIT_*

 arch/arm64/kernel/cpu_errata.c | 11 +++--------
 arch/arm64/kernel/sdei.c       |  3 ++-
 drivers/firmware/arm_sdei.c    | 12 ++++++------
 drivers/firmware/psci.c        | 24 ++++++++++++++++--------
 include/linux/arm-smccc.h      | 16 ++++++++++++++++
 include/linux/arm_sdei.h       |  6 ------
 include/linux/psci.h           |  9 ++-------
 7 files changed, 45 insertions(+), 36 deletions(-)
Signature · 1 line
2.11.0
author maintainer
#1 lore raw

PATCH 1/4 arm/arm64: smccc/psci: add arm_smccc_get_conduit()

SMCCC callers are currently amassing a collection of enums for the SMCCC conduit, and are having to dig into the PSCI driver's internals in order to figure out what to do.

Let's clean this up, with common SMCCC_CONDUIT_* definitions, and an arm_smccc_get_conduit() helper that abstracts the PSCI driver's internal state.

We can kill off the PSCI_CONDUIT_* definitions once we've migrated users over to the nerw interface.

Cc Lorenzo Pieralisi <[email protected]>
Cc Will Deacon <[email protected]>
Cc Catalin Marinas <[email protected]>
---
 drivers/firmware/psci.c   | 15 +++++++++++++++
 include/linux/arm-smccc.h | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)
drivers/firmware/psci.c +15 -0
index c80ec1d03274..310882185ca4 100644
@@ -64,6 +64,21 @@ struct psci_operations psci_ops = {
 	.smccc_version = SMCCC_VERSION_1_0,
 };
 
+enum arm_smccc_conduit arm_smccc_get_conduit(void)
+{
+	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+		return SMCCC_CONDUIT_NONE;
+
+	switch (psci_ops.conduit) {
+	case PSCI_CONDUIT_SMC:
+		return SMCCC_CONDUIT_SMC;
+	case PSCI_CONDUIT_HVC:
+		return SMCCC_CONDUIT_HVC;
+	default:
+		return SMCCC_CONDUIT_NONE;
+	}
+}
+
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
 				unsigned long, unsigned long);
 static psci_fn *invoke_psci_fn;
include/linux/arm-smccc.h +16 -0
index a031897fca76..193e9d8a1ac2 100644
@@ -84,6 +84,22 @@
 
 #include <linux/linkage.h>
 #include <linux/types.h>
+
+enum arm_smccc_conduit {
+	SMCCC_CONDUIT_NONE,
+	SMCCC_CONDUIT_SMC,
+	SMCCC_CONDUIT_HVC,
+};
+
+/**
+ * arm_smccc_get_conduit()
+ *
+ * Returns the conduit to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 is not present, returns SMCCC_CONDUIT_NONE.
+ */
+enum arm_smccc_conduit arm_smccc_get_conduit(void);
+
 /**
  * struct arm_smccc_res - Result from SMC/HVC call
  * @a0-a3 result values from registers 0 to 3
Signature · 1 line
2.11.0
#2 lore raw

PATCH 1/4 arm/arm64: smccc/psci: add arm_smccc_get_conduit()

Hi Mark,

Quoted 10 lines · Mark Rutland

SMCCC callers are currently amassing a collection of enums for the SMCCC conduit, and are having to dig into the PSCI driver's internals in order to figure out what to do.

Let's clean this up, with common SMCCC_CONDUIT_* definitions, and an arm_smccc_get_conduit() helper that abstracts the PSCI driver's internal state.

We can kill off the PSCI_CONDUIT_* definitions once we've migrated users over to the nerw interface.

new

Quoted 48 lines
Cc Lorenzo Pieralisi <[email protected]>
Cc Will Deacon <[email protected]>
Cc Catalin Marinas <[email protected]>
---
  drivers/firmware/psci.c   | 15 +++++++++++++++
  include/linux/arm-smccc.h | 16 ++++++++++++++++
  2 files changed, 31 insertions(+)
drivers/firmware/psci.c +15 -0
index c80ec1d03274..310882185ca4 100644
@@ -64,6 +64,21 @@ struct psci_operations psci_ops = {
  	.smccc_version = SMCCC_VERSION_1_0,
  };
 
+enum arm_smccc_conduit arm_smccc_get_conduit(void)
+{
+	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+		return SMCCC_CONDUIT_NONE;
+
+	switch (psci_ops.conduit) {
+	case PSCI_CONDUIT_SMC:
+		return SMCCC_CONDUIT_SMC;
+	case PSCI_CONDUIT_HVC:
+		return SMCCC_CONDUIT_HVC;
+	default:
+		return SMCCC_CONDUIT_NONE;
+	}
+}
+
  typedef unsigned long (psci_fn)(unsigned long, unsigned long,
  				unsigned long, unsigned long);
  static psci_fn *invoke_psci_fn;
include/linux/arm-smccc.h +5 -0
index a031897fca76..193e9d8a1ac2 100644
@@ -84,6 +84,22 @@
 
  #include <linux/linkage.h>
  #include <linux/types.h>
+
+enum arm_smccc_conduit {
+	SMCCC_CONDUIT_NONE,
+	SMCCC_CONDUIT_SMC,
+	SMCCC_CONDUIT_HVC,

include/linux/arm-smccc.h line 101

Quoted 10 lines
include/linux/arm-smccc.h +10 -0
+};
+
+/**
+ * arm_smccc_get_conduit()
+ *
+ * Returns the conduit to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 is not present, returns SMCCC_CONDUIT_NONE.
+ */
+enum arm_smccc_conduit arm_smccc_get_conduit(void);

Given that implicit condition, can we save some confusion by naming this arm_smccc_1_1_conduit(), in line with the actual SMCCCv1.1 calls?

Robin.

Quoted 5 lines
include/linux/arm-smccc.h +1 -0
+
  /**
   * struct arm_smccc_res - Result from SMC/HVC call
   * @a0-a3 result values from registers 0 to 3
author maintainer
#3 lore raw

PATCH 1/4 arm/arm64: smccc/psci: add arm_smccc_get_conduit()

Quoted 6 lines · Robin Murphy
Quoted 2 lines · Mark Rutland

We can kill off the PSCI_CONDUIT_* definitions once we've migrated users over to the nerw interface.

new

Whoops; fixed locally.

[...]

Quoted 11 lines

include/linux/arm-smccc.h line 101

Quoted 8 lines
include/linux/arm-smccc.h +8 -0
+/**
+ * arm_smccc_get_conduit()
+ *
+ * Returns the conduit to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 is not present, returns SMCCC_CONDUIT_NONE.
+ */
+enum arm_smccc_conduit arm_smccc_get_conduit(void);

Given that implicit condition, can we save some confusion by naming this arm_smccc_1_1_conduit(), in line with the actual SMCCCv1.1 calls?

Sure; done.

Thanks,
Mark.

author maintainer
#4 lore raw

PATCH 2/4 arm64: errata: use arm_smccc_get_conduit()

In reply to Mark Rutland

Now that we have arm_smccc_get_conduit(), we can hide the PSCI implementation details from the arm64 cpu errata code, so let's do so.

As arm_smccc_get_conduit() implicitly checks that the SMCCC version is at least SMCCC_VERSION_1_1, we no longer need to check this explicitly, as the switch statement's default case is a return.

Cc Catalin Marinas <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Suzuki K Poulose <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/cpu_errata.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
arch/arm64/kernel/cpu_errata.c +3 -8
index a900befadfe8..afd5e66f52f1 100644
@@ -156,9 +156,7 @@ static void  install_bp_hardening_cb(const struct arm64_cpu_capabilities *entry,
 	__install_bp_hardening_cb(fn, hyp_vecs_start, hyp_vecs_end);
 }
 
-#include <uapi/linux/psci.h>
 #include <linux/arm-smccc.h>
-#include <linux/psci.h>
 
 static void call_smc_arch_workaround_1(void)
 {
@@ -193,11 +191,8 @@ enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
 	if (!entry->matches(entry, SCOPE_LOCAL_CPU))
 		return;
 
-	if (psci_ops.smccc_version == SMCCC_VERSION_1_0)
-		return;
-
-	switch (psci_ops.conduit) {
-	case PSCI_CONDUIT_HVC:
+	switch (arm_smccc_get_conduit()) {
+	case SMCCC_CONDUIT_HVC:
 		arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
 				  ARM_SMCCC_ARCH_WORKAROUND_1, &res);
 		if ((int)res.a0 < 0)
@@ -208,7 +203,7 @@ enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
 		smccc_end = NULL;
 		break;
 
-	case PSCI_CONDUIT_SMC:
+	case SMCCC_CONDUIT_SMC:
 		arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
 				  ARM_SMCCC_ARCH_WORKAROUND_1, &res);
 		if ((int)res.a0 < 0)
Signature · 1 line
2.11.0
author maintainer
#5 lore raw

PATCH 3/4 firmware/psci: use common SMCCC_CONDUIT_*

In reply to Mark Rutland

Now that we have common SMCCC_CONDUIT_* definitions, migrate the PSCI code over to them, and kill off the old PSCI_CONDUIT_* definitions.

Cc Lorenzo Pieralisi <[email protected]>
---
 drivers/firmware/psci.c | 25 +++++++++----------------
 include/linux/psci.h    |  9 ++-------
 2 files changed, 11 insertions(+), 23 deletions(-)
drivers/firmware/psci.c +9 -16
index 310882185ca4..05792699e6b5 100644
@@ -60,7 +60,7 @@ bool psci_tos_resident_on(int cpu)
 }
 
 struct psci_operations psci_ops = {
-	.conduit = PSCI_CONDUIT_NONE,
+	.conduit = SMCCC_CONDUIT_NONE,
 	.smccc_version = SMCCC_VERSION_1_0,
 };
 
@@ -69,14 +69,7 @@ enum arm_smccc_conduit arm_smccc_get_conduit(void)
 	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
 		return SMCCC_CONDUIT_NONE;
 
-	switch (psci_ops.conduit) {
-	case PSCI_CONDUIT_SMC:
-		return SMCCC_CONDUIT_SMC;
-	case PSCI_CONDUIT_HVC:
-		return SMCCC_CONDUIT_HVC;
-	default:
-		return SMCCC_CONDUIT_NONE;
-	}
+	return psci_ops.conduit;
 }
 
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
@@ -228,13 +221,13 @@ static unsigned long psci_migrate_info_up_cpu(void)
 			      0, 0, 0);
 }
 
-static void set_conduit(enum psci_conduit conduit)
+static void set_conduit(enum arm_smccc_conduit conduit)
 {
 	switch (conduit) {
-	case PSCI_CONDUIT_HVC:
+	case SMCCC_CONDUIT_HVC:
 		invoke_psci_fn = __invoke_psci_fn_hvc;
 		break;
-	case PSCI_CONDUIT_SMC:
+	case SMCCC_CONDUIT_SMC:
 		invoke_psci_fn = __invoke_psci_fn_smc;
 		break;
 	default:
@@ -256,9 +249,9 @@ static int get_set_conduit_method(struct device_node *np)
 	}
 
 	if (!strcmp("hvc", method)) {
-		set_conduit(PSCI_CONDUIT_HVC);
+		set_conduit(SMCCC_CONDUIT_HVC);
 	} else if (!strcmp("smc", method)) {
-		set_conduit(PSCI_CONDUIT_SMC);
+		set_conduit(SMCCC_CONDUIT_SMC);
 	} else {
 		pr_warn("invalid \"method\" property: %s\n", method);
 		return -EINVAL;
@@ -714,9 +707,9 @@ int __init psci_acpi_init(void)
 	pr_info("probing for conduit method from ACPI.\n");
 
 	if (acpi_psci_use_hvc())
-		set_conduit(PSCI_CONDUIT_HVC);
+		set_conduit(SMCCC_CONDUIT_HVC);
 	else
-		set_conduit(PSCI_CONDUIT_SMC);
+		set_conduit(SMCCC_CONDUIT_SMC);
 
 	return psci_probe();
 }
include/linux/psci.h +2 -7
index 8b1b3b5935ab..affcd2128df8 100644
@@ -14,6 +14,7 @@
 #ifndef __LINUX_PSCI_H
 #define __LINUX_PSCI_H
 
+#include <linux/arm-smccc.h>
 #include <linux/init.h>
 #include <linux/types.h>
 
@@ -25,12 +26,6 @@ bool psci_tos_resident_on(int cpu);
 int psci_cpu_init_idle(unsigned int cpu);
 int psci_cpu_suspend_enter(unsigned long index);
 
-enum psci_conduit {
-	PSCI_CONDUIT_NONE,
-	PSCI_CONDUIT_SMC,
-	PSCI_CONDUIT_HVC,
-};
-
 enum smccc_version {
 	SMCCC_VERSION_1_0,
 	SMCCC_VERSION_1_1,
@@ -45,7 +40,7 @@ struct psci_operations {
 	int (*affinity_info)(unsigned long target_affinity,
 			unsigned long lowest_affinity_level);
 	int (*migrate_info_type)(void);
-	enum psci_conduit conduit;
+	enum arm_smccc_conduit conduit;
 	enum smccc_version smccc_version;
 };
 
Signature · 1 line
2.11.0
author maintainer
#6 lore raw

PATCH 4/4 firmware: arm_sdei: use common SMCCC_CONDUIT_*

In reply to Mark Rutland

Now that we have common definitions for SMCCC conduits, move the SDEI code over to them, and remove the SDEI-specific definitions.

There should be no functional change as a result of this patch.

---
 arch/arm64/kernel/sdei.c    |  3 ++-
 drivers/firmware/arm_sdei.c | 12 ++++++------
 include/linux/arm_sdei.h    |  6 ------
 3 files changed, 8 insertions(+), 13 deletions(-)
arch/arm64/kernel/sdei.c +2 -1
index 6b8d90d5ceae..a3be67ae8a88 100644
@@ -2,6 +2,7 @@
 // Copyright (C) 2017 Arm Ltd.
 #define pr_fmt(fmt) "sdei: " fmt
 
+#include <linux/arm-smccc.h>
 #include <linux/arm_sdei.h>
 #include <linux/hardirq.h>
 #include <linux/irqflags.h>
@@ -125,7 +126,7 @@ unsigned long sdei_arch_get_entry_point(int conduit)
 			return 0;
 	}
 
-	sdei_exit_mode = (conduit == CONDUIT_HVC) ? SDEI_EXIT_HVC : SDEI_EXIT_SMC;
+	sdei_exit_mode = (conduit == SMCCC_CONDUIT_HVC) ? SDEI_EXIT_HVC : SDEI_EXIT_SMC;
 
 #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
 	if (arm64_kernel_unmapped_at_el0()) {
drivers/firmware/arm_sdei.c +6 -6
index 1ea71640fdc2..1357dbd7c7fc 100644
@@ -896,29 +896,29 @@ static int sdei_get_conduit(struct platform_device *pdev)
 	if (np) {
 		if (of_property_read_string(np, "method", &method)) {
 			pr_warn("missing \"method\" property\n");
-			return CONDUIT_INVALID;
+			return SMCCC_CONDUIT_NONE;
 		}
 
 		if (!strcmp("hvc", method)) {
 			sdei_firmware_call = &sdei_smccc_hvc;
-			return CONDUIT_HVC;
+			return SMCCC_CONDUIT_HVC;
 		} else if (!strcmp("smc", method)) {
 			sdei_firmware_call = &sdei_smccc_smc;
-			return CONDUIT_SMC;
+			return SMCCC_CONDUIT_SMC;
 		}
 
 		pr_warn("invalid \"method\" property: %s\n", method);
 	} else if (IS_ENABLED(CONFIG_ACPI) && !acpi_disabled) {
 		if (acpi_psci_use_hvc()) {
 			sdei_firmware_call = &sdei_smccc_hvc;
-			return CONDUIT_HVC;
+			return SMCCC_CONDUIT_HVC;
 		} else {
 			sdei_firmware_call = &sdei_smccc_smc;
-			return CONDUIT_SMC;
+			return SMCCC_CONDUIT_SMC;
 		}
 	}
 
-	return CONDUIT_INVALID;
+	return SMCCC_CONDUIT_NONE;
 }
 
 static int sdei_probe(struct platform_device *pdev)
include/linux/arm_sdei.h +0 -6
index 942afbd544b7..21e337d99fa6 100644
@@ -5,12 +5,6 @@
 
 #include <uapi/linux/arm_sdei.h>
 
-enum sdei_conduit_types {
-	CONDUIT_INVALID = 0,
-	CONDUIT_SMC,
-	CONDUIT_HVC,
-};
-
 #include <asm/sdei.h>
 
 /* Arch code should override this to set the entry point from firmware... */
Signature · 1 line
2.11.0
maintainer
#7 lore raw

PATCH 4/4 firmware: arm_sdei: use common SMCCC_CONDUIT_*

Hi Mark,

Quoted 4 lines · Mark Rutland

Now that we have common definitions for SMCCC conduits, move the SDEI code over to them, and remove the SDEI-specific definitions.

There should be no functional change as a result of this patch.

Thanks for clearing this up!

James

#8 lore raw

cover arm64: SMCCC conduit cleanup

In reply to Mark Rutland
Quoted 19 lines · Mark Rutland

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

Mark.

Mark Rutland (4):
arm/arm64: smccc/psci: add arm_smccc_get_conduit()
arm64: errata: use arm_smccc_get_conduit() firmware/psci: use common SMCCC_CONDUIT_*
firmware: arm_sdei: use common SMCCC_CONDUIT_*

Quoted 8 lines
 arch/arm64/kernel/cpu_errata.c | 11 +++--------
 arch/arm64/kernel/sdei.c       |  3 ++-
 drivers/firmware/arm_sdei.c    | 12 ++++++------
 drivers/firmware/psci.c        | 24 ++++++++++++++++--------
 include/linux/arm-smccc.h      | 16 ++++++++++++++++
 include/linux/arm_sdei.h       |  6 ------
 include/linux/psci.h           |  9 ++-------
 7 files changed, 45 insertions(+), 36 deletions(-)

For the series:

#9 lore raw

cover arm64: SMCCC conduit cleanup

In reply to Mark Rutland
Quoted 10 lines · Mark Rutland

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

For the series:

Will

#10 lore raw
In reply to Mark Rutland
Quoted 10 lines · Mark Rutland

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

I just noticed that this didn't get merged, for some reason. I acked it at the time, so what's left to do?

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

maintainer
#11 lore raw
Quoted 8 lines · Will Deacon
Quoted 7 lines · Mark Rutland

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Quoted 6 lines
Quoted 3 lines

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

I just noticed that this didn't get merged, for some reason. I acked it at the time, so what's left to do?

Apparently I queued it briefly and then dropped it:

https://lore.kernel.org/linux-arm-kernel/[email protected]/

Signature · 6 lines
Catalin

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author maintainer
#12 lore raw
Quoted 13 lines · Catalin Marinas
Quoted 12 lines · Will Deacon
Quoted 10 lines · Mark Rutland

Currently, the cpu errata code goes digging into PSCI internals to discover the SMCCC conduit, using the (arguably misnamed) PSCI_CONDUIT_* definitions. This lack of abstraction is somewhat unfortunate.

Further, the SDEI code has an almost identical set of CONDUIT_* definitions, and the duplication is rather unfortunate.

Let's unify things behind a common set of SMCCC_CONDUIT_* definitions, and expose the SMCCCv1.1 conduit via a new helper that hides the PSCI driver internals.

Quoted 6 lines
Quoted 2 lines

I just noticed that this didn't get merged, for some reason. I acked it at the time, so what's left to do?

Apparently I queued it briefly and then dropped it:

https://lore.kernel.org/linux-arm-kernel/[email protected]/

IIRC there were conflicts with some of the FW mitigations code.

I'll try to rebase and resend this shortly.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel