diff mbox series

[1/3] regmap: add regmap using ARM SMCCC

Message ID 20210723135239.388325-2-clement.leger@bootlin.com
State New
Headers show
Series add SMC based regmap driver for secure syscon access | expand

Commit Message

Clément Léger July 23, 2021, 1:52 p.m. UTC
When running under secure monitor control, some controllers can be placed in
secure world and their access is thus not possible from normal world. However,
these controllers frequently contain registers than are needed by the normal
world for a few specific operations.

This patch adds a regmap where registers are accessed using SMCs. The secure
monitor is then responsible to allow or deny access to the requested registers.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/regmap/Kconfig        |   7 +-
 drivers/base/regmap/Makefile       |   1 +
 drivers/base/regmap/regmap-smccc.c | 131 +++++++++++++++++++++++++++++
 include/linux/regmap.h             |  38 +++++++++
 4 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-smccc.c

Comments

Clément Léger July 23, 2021, 3:53 p.m. UTC | #1
Hi Mark,

Le Fri, 23 Jul 2021 15:43:18 +0100,
Mark Brown <broonie@kernel.org> a écrit :

> On Fri, Jul 23, 2021 at 03:52:37PM +0200, Clément Léger wrote:
> 
> > When running under secure monitor control, some controllers can be
> > placed in secure world and their access is thus not possible from
> > normal world. However, these controllers frequently contain
> > registers than are needed by the normal world for a few specific
> > operations.  
> 
> > This patch adds a regmap where registers are accessed using SMCs.
> > The secure monitor is then responsible to allow or deny access to
> > the requested registers.  
> 
> I can't see any SMC specification for this interface?  Frankly I have
> some very substantial concerns about the use case for this over
> exposing the functionality of whatever device the SMC is gating
> access to through SMC interfaces specific to that functionality.

This would require to modify drivers to check if the access should be
done using SMCs, parse the device tree to find appropriate SMC ids for
each functionality, add dependencies in KConfig on
HAVE_ARM_SMCCC_DISCOVERY, and do SMC calls instead of regmap access.
I'm not saying this is not the way to go but this is clearly more
intrusive than keeping the existing syscon support.

> Exposing raw access to a (presumed?) subset of whatever device
> functionality feels like the wrong abstraction level to be working at
> and like an invitation to system integrators to do things that are
> going to get them into trouble down the line.

Indeed, access is reduced to a subset of registers offset which are
checked by the TEE.

> 
> If the end user really is just twiddling a few bits here and there I'd
> expect those functionality specific services to be pretty simple to
> do, slightly more effort on the secure monitor side but a lot safer.

The SMC id is supposed to be unique for a given device. The TEE check is
merely a register offset check and a value check. But I agree that the
attack surface is larger than with a SMC targeted for a single
functionality though.

> If there is a use case for passing through an entire device for some
> reason (ran out of controllers or something?) then I think we
> probably want an abstraction at the bus level so we don't need to add
> custom support to every device that we want to pass through and it's
> clear what's going on.

In our use case, only a few registers located in a secure controller
is needed to be done. We don't have a use case for an entire device
access.

Clément
Mark Brown July 23, 2021, 4:37 p.m. UTC | #2
On Fri, Jul 23, 2021 at 05:53:15PM +0200, Clément Léger wrote:
> Mark Brown <broonie@kernel.org> a écrit :

> > I can't see any SMC specification for this interface?  Frankly I have
> > some very substantial concerns about the use case for this over
> > exposing the functionality of whatever device the SMC is gating
> > access to through SMC interfaces specific to that functionality.

> This would require to modify drivers to check if the access should be
> done using SMCs, parse the device tree to find appropriate SMC ids for
> each functionality, add dependencies in KConfig on
> HAVE_ARM_SMCCC_DISCOVERY, and do SMC calls instead of regmap access.
> I'm not saying this is not the way to go but this is clearly more
> intrusive than keeping the existing syscon support.

You're not doing this at the syscon level, you're doing this at the
regmap level.  Any user of this code is going to have to be modified to
use the SMCCC regmap and discover the relevant SMCCC interfaces no
matter what, but by having it we're saying that that's a sensible and
reasonable thing to do and encouraging implementations as a result.

Device specific regmap interfaces do not require adding anything to the
core, there's the reg_read() and reg_write() callbacks for this, if
there is a sensible use case for this at the syscon level and only the
syscon level (but I really do strongly question if it's a good idea at
all) then you can use those without adding a generic interface for
defining SMCCC conduits as regmaps.  TBH what's being added to the
regmap core is so trival that I don't see what we'd be gaining anyway
even if this was widely used, it's not helping with the SMCCC
enumeration side at all.

> > Exposing raw access to a (presumed?) subset of whatever device
> > functionality feels like the wrong abstraction level to be working at
> > and like an invitation to system integrators to do things that are
> > going to get them into trouble down the line.

> Indeed, access is reduced to a subset of registers offset which are
> checked by the TEE.

I really think it would be clearer and safer to have the TEE expose
specific operations that encode the intent of whatever it is trying to
accomplish rather than just expose the register map and then audit the
operations that are going on in the register map after the fact.  It
seems like it's going to be more error prone to do things this way,
especially as this starts getting used as a generic pipe for exposing
things and things get built up - as well as auditing concerns if any
problems are identified it's going to be harder to track the intent of
what the non-secure world is doing.
diff mbox series

Patch

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 159bac6c5046..6957a6b21ad9 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,7 +4,7 @@ 
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM || REGMAP_MDIO)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM || REGMAP_MDIO || REGMAP_SMCCC)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	select MDIO_BUS if REGMAP_MDIO
 	bool
@@ -65,3 +65,8 @@  config REGMAP_I3C
 config REGMAP_SPI_AVMM
 	tristate
 	depends on SPI
+
+config REGMAP_SMCCC
+	default y if HAVE_ARM_SMCCC_DISCOVERY
+	tristate
+	depends on HAVE_ARM_SMCCC_DISCOVERY
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 11facb32a027..3d92503a3b4e 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -20,3 +20,4 @@  obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
 obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
 obj-$(CONFIG_REGMAP_MDIO) += regmap-mdio.o
+obj-$(CONFIG_REGMAP_SMCCC) += regmap-smccc.o
diff --git a/drivers/base/regmap/regmap-smccc.c b/drivers/base/regmap/regmap-smccc.c
new file mode 100644
index 000000000000..a64d58f97d21
--- /dev/null
+++ b/drivers/base/regmap/regmap-smccc.c
@@ -0,0 +1,131 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2021 Bootlin
+
+#include <linux/arm-smccc.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define REGMAP_SMC_READ		0
+#define REGMAP_SMC_WRITE	1
+
+struct regmap_smccc_ctx {
+	u32 regmap_smc_id;
+};
+
+static int regmap_smccc_reg_write(void *context, unsigned int reg,
+				  unsigned int val)
+{
+	struct regmap_smccc_ctx *ctx = context;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ctx->regmap_smc_id, REGMAP_SMC_WRITE, reg, val,
+			     &res);
+
+	if (res.a0)
+		return -EACCES;
+
+	return 0;
+}
+
+static int regmap_smccc_reg_read(void *context, unsigned int reg,
+				 unsigned int *val)
+{
+	struct regmap_smccc_ctx *ctx = context;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ctx->regmap_smc_id, REGMAP_SMC_READ, reg, &res);
+
+	if (res.a0)
+		return -EACCES;
+
+	*val = res.a1;
+
+	return 0;
+}
+
+static struct regmap_bus regmap_smccc = {
+	.reg_write = regmap_smccc_reg_write,
+	.reg_read = regmap_smccc_reg_read,
+};
+
+static int regmap_smccc_bits_is_supported(int val_bits)
+{
+	switch (val_bits) {
+	case 8:
+	case 16:
+	case 32:
+		return 0;
+	case 64:
+	/*
+	 * SMCs are using registers to pass information so if architecture is
+	 * not using 64 bits registers, we won't be able to pass information
+	 * transparently.
+	 */
+#if !defined(CONFIG_64BIT)
+		return -EINVAL;
+#else
+		return 0;
+#endif
+	default:
+		return -EINVAL;
+	}
+}
+
+static struct regmap_smccc_ctx *smccc_regmap_init_ctx(
+					const struct regmap_config *config,
+					u32 regmap_smc_id)
+{
+	int ret;
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+
+	ret = regmap_smccc_bits_is_supported(config->val_bits);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ctx->regmap_smc_id = regmap_smc_id;
+
+	return ctx;
+}
+
+struct regmap *__regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+				   const struct regmap_config *config,
+				   struct lock_class_key *lock_key,
+				   const char *lock_name)
+{
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = smccc_regmap_init_ctx(config, regmap_smc_id);
+	if (IS_ERR(ctx))
+		return ERR_CAST(ctx);
+
+	return __regmap_init(dev, &regmap_smccc, ctx, config, lock_key,
+			     lock_name);
+}
+EXPORT_SYMBOL_GPL(__regmap_init_smccc);
+
+struct regmap *__devm_regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+					const struct regmap_config *config,
+					struct lock_class_key *lock_key,
+					const char *lock_name)
+{
+	struct regmap_smccc_ctx *ctx;
+
+	ctx = smccc_regmap_init_ctx(config, regmap_smc_id);
+	if (IS_ERR(ctx))
+		return ERR_CAST(ctx);
+
+	return __devm_regmap_init(dev, &regmap_smccc, ctx, config, lock_key,
+				  lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_smccc);
+
+MODULE_AUTHOR("Clément Léger <clement.leger@bootlin.com>");
+MODULE_DESCRIPTION("Regmap SMCCC Module");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f5f08dd0a116..08aa523403e8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -591,6 +591,11 @@  struct regmap *__regmap_init_spi_avmm(struct spi_device *spi,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name);
 
+struct regmap *__regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+				   const struct regmap_config *config,
+				   struct lock_class_key *lock_key,
+				   const char *lock_name);
+
 struct regmap *__devm_regmap_init(struct device *dev,
 				  const struct regmap_bus *bus,
 				  void *bus_context,
@@ -655,6 +660,10 @@  struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
 					   const struct regmap_config *config,
 					   struct lock_class_key *lock_key,
 					   const char *lock_name);
+struct regmap *__devm_regmap_init_smccc(struct device *dev, u32 regmap_smc_id,
+					const struct regmap_config *config,
+					struct lock_class_key *lock_key,
+					const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.
@@ -881,6 +890,20 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__regmap_init_spi_avmm, #config,		\
 				 spi, config)
 
+/**
+ * regmap_init_smccc() - Initialize register map for ARM SMCCC
+ *
+ * @dev: Device that will be interacted with
+ * @smc_id: SMC id to used for calls
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.
+ */
+#define regmap_init_smccc(dev, smc_id, config)			\
+	__regmap_lockdep_wrapper(__regmap_init_smccc, #config,	\
+				 dev, smc_id, config)
+
 /**
  * devm_regmap_init() - Initialise managed register map
  *
@@ -1110,6 +1133,21 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_spi_avmm, #config,	\
 				 spi, config)
 
+/**
+ * devm_regmap_init_smccc() - Initialize register map for ARM SMCCC
+ *
+ * @dev: Device that will be interacted with
+ * @smc_id: SMC id to used for calls
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The map will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_smccc(dev, smc_id, config)			\
+	__regmap_lockdep_wrapper(__devm_regmap_init_smccc, #config,	\
+				 dev, smc_id, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);