diff mbox series

ARM: soc: aspeed: Add secure boot controller support

Message ID 20220304030336.1017197-1-joel@jms.id.au (mailing list archive)
State New, archived
Headers show
Series ARM: soc: aspeed: Add secure boot controller support | expand

Commit Message

Joel Stanley March 4, 2022, 3:03 a.m. UTC 
This reads out the status of the secure boot controller and exposes it
in debugfs.

An example on a AST2600A3 QEMU model:

 # grep -r . /sys/kernel/debug/aspeed/*
 /sys/kernel/debug/aspeed/abr_image:0
 /sys/kernel/debug/aspeed/low_security_key:0
 /sys/kernel/debug/aspeed/otp_protected:0
 /sys/kernel/debug/aspeed/secure_boot:1
 /sys/kernel/debug/aspeed/uart_boot:0

On boot the state of the system according to the secure boot controller
will be printed:

 [    0.037634] AST2600 secure boot enabled

or

 [    0.037935] AST2600 secure boot disabled

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
We're creating a common API for a subset of this information in sysfs:

 https://lore.kernel.org/all/20220204072234.304543-1-joel@jms.id.au/

However, machines with an ASPEED soc need the detailed information from
the SBE that is not relevant for other systems, so expose it all in
debugfs.

 drivers/soc/aspeed/aspeed-sbc.c | 71 +++++++++++++++++++++++++++++++++
 drivers/soc/aspeed/Kconfig      |  7 ++++
 drivers/soc/aspeed/Makefile     |  1 +
 3 files changed, 79 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-sbc.c

Comments

Andrew Jeffery March 7, 2022, 1:06 a.m. UTC | #1 
On Fri, 4 Mar 2022, at 13:33, Joel Stanley wrote:
> This reads out the status of the secure boot controller and exposes it
> in debugfs.
>
> An example on a AST2600A3 QEMU model:
>
>  # grep -r . /sys/kernel/debug/aspeed/*
>  /sys/kernel/debug/aspeed/abr_image:0
>  /sys/kernel/debug/aspeed/low_security_key:0
>  /sys/kernel/debug/aspeed/otp_protected:0
>  /sys/kernel/debug/aspeed/secure_boot:1
>  /sys/kernel/debug/aspeed/uart_boot:0
>
> On boot the state of the system according to the secure boot controller
> will be printed:
>
>  [    0.037634] AST2600 secure boot enabled
>
> or
>
>  [    0.037935] AST2600 secure boot disabled
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> We're creating a common API for a subset of this information in sysfs:
>
>  https://lore.kernel.org/all/20220204072234.304543-1-joel@jms.id.au/
>
> However, machines with an ASPEED soc need the detailed information from
> the SBE that is not relevant for other systems, so expose it all in
> debugfs.
>
>  drivers/soc/aspeed/aspeed-sbc.c | 71 +++++++++++++++++++++++++++++++++
>  drivers/soc/aspeed/Kconfig      |  7 ++++
>  drivers/soc/aspeed/Makefile     |  1 +
>  3 files changed, 79 insertions(+)
>  create mode 100644 drivers/soc/aspeed/aspeed-sbc.c
>
> diff --git a/drivers/soc/aspeed/aspeed-sbc.c 
> b/drivers/soc/aspeed/aspeed-sbc.c
> new file mode 100644
> index 000000000000..ee466f02ae4c
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-sbc.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright 2022 IBM Corp. */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/debugfs.h>
> +
> +#define SEC_STATUS		0x14
> +#define ABR_IMAGE_SOURCE	BIT(13)
> +#define OTP_PROTECTED		BIT(8)
> +#define LOW_SEC_KEY		BIT(7)
> +#define SECURE_BOOT		BIT(6)
> +#define UART_BOOT		BIT(5)
> +
> +struct sbe {
> +	u8 abr_image;
> +	u8 low_security_key;
> +	u8 otp_protected;
> +	u8 secure_boot;
> +	u8 invert;
> +	u8 uart_boot;
> +};
> +
> +static struct sbe sbe;
> +
> +static int __init aspeed_sbc_init(void)
> +{
> +	struct device_node *np;
> +	void __iomem *base;
> +	struct dentry *debugfs_root;
> +	u32 security_status;

If you change anything else, maybe reverse-christmas-tree this too?

> +
> +	/* AST2600 only */
> +	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
> +	if (!of_device_is_available(np))
> +		return -ENODEV;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		of_node_put(np);
> +		return -ENODEV;
> +	}
> +
> +	security_status = readl(base + SEC_STATUS);
> +
> +	iounmap(base);
> +	of_node_put(np);

The cleanup looks right to me. I half wonder if it would be better with 
a single-exit and gotos, but that's just an idle thought.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> +
> +	sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
> +	sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
> +	sbe.otp_protected = !!(security_status & OTP_PROTECTED);
> +	sbe.secure_boot = !!(security_status & SECURE_BOOT);
> +	/* Invert the bit, as 1 is boot from SPI/eMMC */
> +	sbe.uart_boot =  !(security_status & UART_BOOT);
> +
> +	debugfs_root = debugfs_create_dir("aspeed", NULL);
> +	debugfs_create_u8("abr_image", 0444, debugfs_root, &sbe.abr_image);
> +	debugfs_create_u8("low_security_key", 0444, debugfs_root, 
> &sbe.low_security_key);
> +	debugfs_create_u8("otp_protected", 0444, debugfs_root, 
> &sbe.otp_protected);
> +	debugfs_create_u8("uart_boot", 0444, debugfs_root, &sbe.uart_boot);
> +	debugfs_create_u8("secure_boot", 0444, debugfs_root, 
> &sbe.secure_boot);
> +
> +	pr_info("AST2600 secure boot %s\n", sbe.secure_boot ? "enabled" : 
> "disabled");
> +
> +	return 0;
> +}
> +
> +
> +subsys_initcall(aspeed_sbc_init);
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index f579ee0b5afa..7a2a5bed8bc5 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -52,6 +52,13 @@ config ASPEED_SOCINFO
>  	help
>  	  Say yes to support decoding of ASPEED BMC information.
> 
> +config ASPEED_SBC
> +	bool "ASPEED Secure Boot Controller driver"
> +	default MACH_ASPEED_G6
> +	help
> +	  Say yes to provide information about the secure boot controller in
> +	  debugfs.
> +
>  endmenu
> 
>  endif
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index b35d74592964..042235ffa05b 100644
> --- a/drivers/soc/aspeed/Makefile
> +++ b/drivers/soc/aspeed/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
>  obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
>  obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
>  obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
> +obj-$(CONFIG_ASPEED_SBC)		+= aspeed-sbc.o
> -- 
> 2.34.1
Cédric Le Goater March 9, 2022, 3:53 p.m. UTC | #2 
Hello Joel,

On 3/4/22 04:03, Joel Stanley wrote:
> This reads out the status of the secure boot controller and exposes it
> in debugfs.
> 
> An example on a AST2600A3 QEMU model:
> 
>   # grep -r . /sys/kernel/debug/aspeed/*
>   /sys/kernel/debug/aspeed/abr_image:0
>   /sys/kernel/debug/aspeed/low_security_key:0
>   /sys/kernel/debug/aspeed/otp_protected:0
>   /sys/kernel/debug/aspeed/secure_boot:1
>   /sys/kernel/debug/aspeed/uart_boot:0
> 
> On boot the state of the system according to the secure boot controller
> will be printed:
> 
>   [    0.037634] AST2600 secure boot enabled
> 
> or
> 
>   [    0.037935] AST2600 secure boot disabled
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> We're creating a common API for a subset of this information in sysfs:
> 
>   https://lore.kernel.org/all/20220204072234.304543-1-joel@jms.id.au/
> 
> However, machines with an ASPEED soc need the detailed information from
> the SBE that is not relevant for other systems, so expose it all in
> debugfs.
> 
>   drivers/soc/aspeed/aspeed-sbc.c | 71 +++++++++++++++++++++++++++++++++
>   drivers/soc/aspeed/Kconfig      |  7 ++++
>   drivers/soc/aspeed/Makefile     |  1 +
>   3 files changed, 79 insertions(+)
>   create mode 100644 drivers/soc/aspeed/aspeed-sbc.c
> 
> diff --git a/drivers/soc/aspeed/aspeed-sbc.c b/drivers/soc/aspeed/aspeed-sbc.c
> new file mode 100644
> index 000000000000..ee466f02ae4c
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-sbc.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright 2022 IBM Corp. */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/debugfs.h>
> +
> +#define SEC_STATUS		0x14
> +#define ABR_IMAGE_SOURCE	BIT(13)
> +#define OTP_PROTECTED		BIT(8)
> +#define LOW_SEC_KEY		BIT(7)
> +#define SECURE_BOOT		BIT(6)
> +#define UART_BOOT		BIT(5)

Why not put these definitions under a common header file ?

> +struct sbe {> +	u8 abr_image;
> +	u8 low_security_key;
> +	u8 otp_protected;
> +	u8 secure_boot;
> +	u8 invert;
> +	u8 uart_boot;

 From what the code does below, these could be of type 'bool'

> +};
> +
> +static struct sbe sbe;
> +
> +static int __init aspeed_sbc_init(void)
> +{
> +	struct device_node *np;
> +	void __iomem *base;
> +	struct dentry *debugfs_root;
> +	u32 security_status;
> +
> +	/* AST2600 only */
> +	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
> +	if (!of_device_is_available(np))
> +		return -ENODEV;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		of_node_put(np);
> +		return -ENODEV;
> +	}
> +
> +	security_status = readl(base + SEC_STATUS);
> +
> +	iounmap(base);
> +	of_node_put(np);
> +
> +	sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
> +	sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
> +	sbe.otp_protected = !!(security_status & OTP_PROTECTED);
> +	sbe.secure_boot = !!(security_status & SECURE_BOOT);
> +	/* Invert the bit, as 1 is boot from SPI/eMMC */
> +	sbe.uart_boot =  !(security_status & UART_BOOT);
> +
> +	debugfs_root = debugfs_create_dir("aspeed", NULL);

may be use 'arch_debugfs_dir' or is that the same ? and test the returned
value as it can fail.

Also, instead of 'debugfs_root', we could introduce an extern
'aspeed_debugfs_dir' for other aspeed drivers. For instance, the spi-mem
driver for flash devices could expose some internal settings like the
direct mapping ranges for each flash device. I am sure other drivers
would use it.

> +	debugfs_create_u8("abr_image", 0444, debugfs_root, &sbe.abr_image);
> +	debugfs_create_u8("low_security_key", 0444, debugfs_root, &sbe.low_security_key);
> +	debugfs_create_u8("otp_protected", 0444, debugfs_root, &sbe.otp_protected);
> +	debugfs_create_u8("uart_boot", 0444, debugfs_root, &sbe.uart_boot);
> +	debugfs_create_u8("secure_boot", 0444, debugfs_root, &sbe.secure_boot);

It would be cleaner if these files were under a 'sbe' or 'sbc' directory.

Thanks,

C.

> +
> +	pr_info("AST2600 secure boot %s\n", sbe.secure_boot ? "enabled" : "disabled");
> +
> +	return 0;
> +}
> +
> +
> +subsys_initcall(aspeed_sbc_init);
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index f579ee0b5afa..7a2a5bed8bc5 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -52,6 +52,13 @@ config ASPEED_SOCINFO
>   	help
>   	  Say yes to support decoding of ASPEED BMC information.
>   
> +config ASPEED_SBC
> +	bool "ASPEED Secure Boot Controller driver"
> +	default MACH_ASPEED_G6> +	help
> +	  Say yes to provide information about the secure boot controller in
> +	  debugfs.
> +
>   endmenu
>   
>   endif
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index b35d74592964..042235ffa05b 100644
> --- a/drivers/soc/aspeed/Makefile
> +++ b/drivers/soc/aspeed/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
>   obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
>   obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
>   obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
> +obj-$(CONFIG_ASPEED_SBC)		+= aspeed-sbc.o
Cédric Le Goater March 9, 2022, 3:59 p.m. UTC | #3 
[ ... ]

>> +    sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
>> +    sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
>> +    sbe.otp_protected = !!(security_status & OTP_PROTECTED);
>> +    sbe.secure_boot = !!(security_status & SECURE_BOOT);
>> +    /* Invert the bit, as 1 is boot from SPI/eMMC */
>> +    sbe.uart_boot =  !(security_status & UART_BOOT);
>> +
>> +    debugfs_root = debugfs_create_dir("aspeed", NULL);
> 
> may be use 'arch_debugfs_dir' or is that the same ? and test the returned
> value as it can fail.

Also, CONFIG_DEBUG_FS is not always selected.

Thanks,

C.
Joel Stanley March 9, 2022, 10:40 p.m. UTC | #4 
On Wed, 9 Mar 2022 at 15:53, Cédric Le Goater <clg@kaod.org> wrote:
> > +#define SEC_STATUS           0x14
> > +#define ABR_IMAGE_SOURCE     BIT(13)
> > +#define OTP_PROTECTED                BIT(8)
> > +#define LOW_SEC_KEY          BIT(7)
> > +#define SECURE_BOOT          BIT(6)
> > +#define UART_BOOT            BIT(5)
>
> Why not put these definitions under a common header file ?

They are the register definitions. I don't think there will be any
users outside of this driver.

>
> > +struct sbe {> +      u8 abr_image;
> > +     u8 low_security_key;
> > +     u8 otp_protected;
> > +     u8 secure_boot;
> > +     u8 invert;
> > +     u8 uart_boot;
>
>  From what the code does below, these could be of type 'bool'

I made them boot initially, but debugfs_create_u8 gets unhappy about
taking a bool.

We could use debugfs_create_bool, but then the files return Y/N which
I didn't like.

> > +     sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
> > +     sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
> > +     sbe.otp_protected = !!(security_status & OTP_PROTECTED);
> > +     sbe.secure_boot = !!(security_status & SECURE_BOOT);
> > +     /* Invert the bit, as 1 is boot from SPI/eMMC */
> > +     sbe.uart_boot =  !(security_status & UART_BOOT);
> > +
> > +     debugfs_root = debugfs_create_dir("aspeed", NULL);
>
> may be use 'arch_debugfs_dir' or is that the same ? and test the returned
> value as it can fail.
>
> Also, instead of 'debugfs_root', we could introduce an extern
> 'aspeed_debugfs_dir' for other aspeed drivers. For instance, the spi-mem
> driver for flash devices could expose some internal settings like the
> direct mapping ranges for each flash device. I am sure other drivers
> would use it.

Okay. I was hesitant to export it before we had other users, but given
you already have some in mind I'll do that.

The hard bit is where to put it.

We have arch_debugfs_dir in include/linux/debugfs.h. This is used by
ppc, x86, s390 and sh, but arm doesn't populate it. Neither do any of
the arm socs.

We could initalise that to the machine name. This means we don't need
to add the soc-specific names into the driver. OTOH, "arch" is "arm"
not "aspeed".

I like the idea.

>
> > +     debugfs_create_u8("abr_image", 0444, debugfs_root, &sbe.abr_image);
> > +     debugfs_create_u8("low_security_key", 0444, debugfs_root, &sbe.low_security_key);
> > +     debugfs_create_u8("otp_protected", 0444, debugfs_root, &sbe.otp_protected);
> > +     debugfs_create_u8("uart_boot", 0444, debugfs_root, &sbe.uart_boot);
> > +     debugfs_create_u8("secure_boot", 0444, debugfs_root, &sbe.secure_boot);
>
> It would be cleaner if these files were under a 'sbe' or 'sbc' directory.

Ok.

Thanks for the review.
diff mbox series

Patch

diff --git a/drivers/soc/aspeed/aspeed-sbc.c b/drivers/soc/aspeed/aspeed-sbc.c
new file mode 100644
index 000000000000..ee466f02ae4c
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-sbc.c
@@ -0,0 +1,71 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright 2022 IBM Corp. */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/debugfs.h>
+
+#define SEC_STATUS		0x14
+#define ABR_IMAGE_SOURCE	BIT(13)
+#define OTP_PROTECTED		BIT(8)
+#define LOW_SEC_KEY		BIT(7)
+#define SECURE_BOOT		BIT(6)
+#define UART_BOOT		BIT(5)
+
+struct sbe {
+	u8 abr_image;
+	u8 low_security_key;
+	u8 otp_protected;
+	u8 secure_boot;
+	u8 invert;
+	u8 uart_boot;
+};
+
+static struct sbe sbe;
+
+static int __init aspeed_sbc_init(void)
+{
+	struct device_node *np;
+	void __iomem *base;
+	struct dentry *debugfs_root;
+	u32 security_status;
+
+	/* AST2600 only */
+	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
+	if (!of_device_is_available(np))
+		return -ENODEV;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	security_status = readl(base + SEC_STATUS);
+
+	iounmap(base);
+	of_node_put(np);
+
+	sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
+	sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
+	sbe.otp_protected = !!(security_status & OTP_PROTECTED);
+	sbe.secure_boot = !!(security_status & SECURE_BOOT);
+	/* Invert the bit, as 1 is boot from SPI/eMMC */
+	sbe.uart_boot =  !(security_status & UART_BOOT);
+
+	debugfs_root = debugfs_create_dir("aspeed", NULL);
+	debugfs_create_u8("abr_image", 0444, debugfs_root, &sbe.abr_image);
+	debugfs_create_u8("low_security_key", 0444, debugfs_root, &sbe.low_security_key);
+	debugfs_create_u8("otp_protected", 0444, debugfs_root, &sbe.otp_protected);
+	debugfs_create_u8("uart_boot", 0444, debugfs_root, &sbe.uart_boot);
+	debugfs_create_u8("secure_boot", 0444, debugfs_root, &sbe.secure_boot);
+
+	pr_info("AST2600 secure boot %s\n", sbe.secure_boot ? "enabled" : "disabled");
+
+	return 0;
+}
+
+
+subsys_initcall(aspeed_sbc_init);
diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..7a2a5bed8bc5 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -52,6 +52,13 @@  config ASPEED_SOCINFO
 	help
 	  Say yes to support decoding of ASPEED BMC information.
 
+config ASPEED_SBC
+	bool "ASPEED Secure Boot Controller driver"
+	default MACH_ASPEED_G6
+	help
+	  Say yes to provide information about the secure boot controller in
+	  debugfs.
+
 endmenu
 
 endif
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..042235ffa05b 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,4 @@  obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
 obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
 obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
+obj-$(CONFIG_ASPEED_SBC)		+= aspeed-sbc.o