* [PATCH 0/3] ARM: aspeed: Secure Boot Controller support
@ 2021-11-17 3:51 Joel Stanley
2021-11-17 3:51 ` [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings Joel Stanley
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Joel Stanley @ 2021-11-17 3:51 UTC (permalink / raw)
To: Andrew Jeffery, Rob Herring
Cc: Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed
This adds the reporting of the secure boot state to the ASPEED socinfo
driver.
Joel Stanley (3):
dt-bindings: aspeed: Add Secure Boot Controller bindings
ARM: dts: aspeed: Add secure boot controller node
ARM: aspeed: Add secure boot controller support
.../bindings/arm/aspeed/aspeed,sbc.yaml | 37 ++++++++++
arch/arm/boot/dts/aspeed-g6.dtsi | 5 ++
drivers/soc/aspeed/aspeed-socinfo.c | 73 +++++++++++++++++++
3 files changed, 115 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml
--
2.33.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings 2021-11-17 3:51 [PATCH 0/3] ARM: aspeed: Secure Boot Controller support Joel Stanley @ 2021-11-17 3:51 ` Joel Stanley 2021-11-29 22:41 ` Rob Herring 2021-11-17 3:51 ` [PATCH 2/3] ARM: dts: aspeed: Add secure boot controller node Joel Stanley 2021-11-17 3:51 ` [PATCH 3/3] ARM: aspeed: Add secure boot controller support Joel Stanley 2 siblings, 1 reply; 8+ messages in thread From: Joel Stanley @ 2021-11-17 3:51 UTC (permalink / raw) To: Andrew Jeffery, Rob Herring Cc: Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed The secure boot controller was first introduced in the AST2600. Signed-off-by: Joel Stanley <joel@jms.id.au> --- .../bindings/arm/aspeed/aspeed,sbc.yaml | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml new file mode 100644 index 000000000000..c72aab706484 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml @@ -0,0 +1,37 @@ +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) +# Copyright 2021 Joel Stanley, IBM Corp. +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/arm/aspeed/aspeed,sbc.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: ASPEED Secure Boot Controller + +maintainers: + - Joel Stanley <joel@jms.id.au> + - Andrew Jeffery <andrew@aj.id.au> + +description: | + The ASPEED SoCs have a register bank for interacting with the secure boot + controller. + +properties: + compatible: + items: + - const: aspeed,ast2600-sbc + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + sbc: secure-boot-controller@1e6f2000 { + compatible = "aspeed,ast2600-sbc"; + reg = <0x1e6f2000 0x1000>; + }; -- 2.33.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings 2021-11-17 3:51 ` [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings Joel Stanley @ 2021-11-29 22:41 ` Rob Herring 0 siblings, 0 replies; 8+ messages in thread From: Rob Herring @ 2021-11-29 22:41 UTC (permalink / raw) To: Joel Stanley Cc: Andrew Jeffery, Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed On Wed, Nov 17, 2021 at 11:51:04AM +0800, Joel Stanley wrote: > The secure boot controller was first introduced in the AST2600. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > .../bindings/arm/aspeed/aspeed,sbc.yaml | 37 +++++++++++++++++++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml > > diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml > new file mode 100644 > index 000000000000..c72aab706484 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed,sbc.yaml > @@ -0,0 +1,37 @@ > +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) GPL-2.0-only OR BSD-2-Clause > +# Copyright 2021 Joel Stanley, IBM Corp. > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/arm/aspeed/aspeed,sbc.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: ASPEED Secure Boot Controller > + > +maintainers: > + - Joel Stanley <joel@jms.id.au> > + - Andrew Jeffery <andrew@aj.id.au> > + > +description: | Only need '|' to preserve formatting which you don't have. With those addressed, Reviewed-by: Rob Herring <robh@kernel.org> > + The ASPEED SoCs have a register bank for interacting with the secure boot > + controller. > + > +properties: > + compatible: > + items: > + - const: aspeed,ast2600-sbc > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + sbc: secure-boot-controller@1e6f2000 { > + compatible = "aspeed,ast2600-sbc"; > + reg = <0x1e6f2000 0x1000>; > + }; > -- > 2.33.0 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] ARM: dts: aspeed: Add secure boot controller node 2021-11-17 3:51 [PATCH 0/3] ARM: aspeed: Secure Boot Controller support Joel Stanley 2021-11-17 3:51 ` [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings Joel Stanley @ 2021-11-17 3:51 ` Joel Stanley 2021-11-17 3:51 ` [PATCH 3/3] ARM: aspeed: Add secure boot controller support Joel Stanley 2 siblings, 0 replies; 8+ messages in thread From: Joel Stanley @ 2021-11-17 3:51 UTC (permalink / raw) To: Andrew Jeffery, Rob Herring Cc: Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed The ast2600 has a secure boot controller. Signed-off-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Ryan Chen <ryan_chen@aspeedtech.com> --- arch/arm/boot/dts/aspeed-g6.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi index 5106a424f1ce..16b36c13695a 100644 --- a/arch/arm/boot/dts/aspeed-g6.dtsi +++ b/arch/arm/boot/dts/aspeed-g6.dtsi @@ -384,6 +384,11 @@ adc1: adc@1e6e9100 { status = "disabled"; }; + sbc: secure-boot-controller@1e6f2000 { + compatible = "aspeed,ast2600-sbc"; + reg = <0x1e6f2000 0x1000>; + }; + gpio0: gpio@1e780000 { #gpio-cells = <2>; gpio-controller; -- 2.33.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] ARM: aspeed: Add secure boot controller support 2021-11-17 3:51 [PATCH 0/3] ARM: aspeed: Secure Boot Controller support Joel Stanley 2021-11-17 3:51 ` [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings Joel Stanley 2021-11-17 3:51 ` [PATCH 2/3] ARM: dts: aspeed: Add secure boot controller node Joel Stanley @ 2021-11-17 3:51 ` Joel Stanley 2021-11-25 8:38 ` kernel test robot 2022-01-10 23:29 ` Joel Stanley 2 siblings, 2 replies; 8+ messages in thread From: Joel Stanley @ 2021-11-17 3:51 UTC (permalink / raw) To: Andrew Jeffery, Rob Herring Cc: Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed This reads out the status of the secure boot controller and exposes it in sysfs. An example on a AST2600A3 QEMU model: # grep . /sys/bus/soc/devices/soc0/* /sys/bus/soc/devices/soc0/abr_image:0 /sys/bus/soc/devices/soc0/family:AST2600 /sys/bus/soc/devices/soc0/low_security_key:0 /sys/bus/soc/devices/soc0/machine:Rainier 2U /sys/bus/soc/devices/soc0/otp_protected:0 /sys/bus/soc/devices/soc0/revision:A3 /sys/bus/soc/devices/soc0/secure_boot:1 /sys/bus/soc/devices/soc0/serial_number:888844441234abcd /sys/bus/soc/devices/soc0/soc_id:05030303 /sys/bus/soc/devices/soc0/uart_boot:1 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> Reviewed-by: Ryan Chen <ryan_chen@aspeedtech.com> --- drivers/soc/aspeed/aspeed-socinfo.c | 73 +++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/drivers/soc/aspeed/aspeed-socinfo.c b/drivers/soc/aspeed/aspeed-socinfo.c index 1ca140356a08..6faf2c199c90 100644 --- a/drivers/soc/aspeed/aspeed-socinfo.c +++ b/drivers/soc/aspeed/aspeed-socinfo.c @@ -9,6 +9,8 @@ #include <linux/slab.h> #include <linux/sys_soc.h> +static u32 security_status; + static struct { const char *name; const u32 id; @@ -74,6 +76,54 @@ static const char *siliconid_to_rev(u32 siliconid) return "??"; } +#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) + +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & ABR_IMAGE_SOURCE)); +} +static DEVICE_ATTR_RO(abr_image); + +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & LOW_SEC_KEY)); +} +static DEVICE_ATTR_RO(low_security_key); + +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & OTP_PROTECTED)); +} +static DEVICE_ATTR_RO(otp_protected); + +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & SECURE_BOOT)); +} +static DEVICE_ATTR_RO(secure_boot); + +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + /* Invert the bit, as 1 is boot from SPI/eMMC */ + return sprintf(buf, "%d\n", !(security_status & UART_BOOT)); +} +static DEVICE_ATTR_RO(uart_boot); + +static struct attribute *aspeed_attrs[] = { + &dev_attr_abr_image.attr, + &dev_attr_low_security_key.attr, + &dev_attr_otp_protected.attr, + &dev_attr_secure_boot.attr, + &dev_attr_uart_boot.attr, + NULL, +}; +ATTRIBUTE_GROUPS(aspeed); + static int __init aspeed_socinfo_init(void) { struct soc_device_attribute *attrs; @@ -81,6 +131,7 @@ static int __init aspeed_socinfo_init(void) struct device_node *np; void __iomem *reg; bool has_chipid = false; + bool has_sbe = false; u32 siliconid; u32 chipid[2]; const char *machine = NULL; @@ -109,6 +160,20 @@ static int __init aspeed_socinfo_init(void) } of_node_put(np); + /* AST2600 only */ + np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc"); + if (of_device_is_available(np)) { + void *base = of_iomap(np, 0); + if (!base) { + of_node_put(np); + return -ENODEV; + } + security_status = readl(base + SEC_STATUS); + has_sbe = true; + iounmap(base); + of_node_put(np); + } + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL); if (!attrs) return -ENODEV; @@ -135,6 +200,9 @@ static int __init aspeed_socinfo_init(void) attrs->serial_number = kasprintf(GFP_KERNEL, "%08x%08x", chipid[1], chipid[0]); + if (has_sbe) + attrs->custom_attr_group = aspeed_groups[0]; + soc_dev = soc_device_register(attrs); if (IS_ERR(soc_dev)) { kfree(attrs->soc_id); @@ -148,6 +216,11 @@ static int __init aspeed_socinfo_init(void) attrs->revision, attrs->soc_id); + if (has_sbe) { + pr_info("AST2600 secure boot %s\n", + (security_status & SECURE_BOOT) ? "enabled" : "disabled"); + } + return 0; } early_initcall(aspeed_socinfo_init); -- 2.33.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ARM: aspeed: Add secure boot controller support 2021-11-17 3:51 ` [PATCH 3/3] ARM: aspeed: Add secure boot controller support Joel Stanley @ 2021-11-25 8:38 ` kernel test robot 2022-01-10 23:29 ` Joel Stanley 1 sibling, 0 replies; 8+ messages in thread From: kernel test robot @ 2021-11-25 8:38 UTC (permalink / raw) To: Joel Stanley, Andrew Jeffery, Rob Herring Cc: kbuild-all, Ryan Chen, Paul Menzel, devicetree, linux-arm-kernel, linux-aspeed Hi Joel, I love your patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on soc/for-next rockchip/for-next shawnguo/for-next v5.16-rc2 next-20211125] [cannot apply to arm/for-next arm64/for-next/core] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Joel-Stanley/ARM-aspeed-Secure-Boot-Controller-support/20211117-115258 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: sparc64-randconfig-s032-20211118 (https://download.01.org/0day-ci/archive/20211125/202111251600.hBmyvSD0-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 11.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-dirty # https://github.com/0day-ci/linux/commit/5057997868102d26f0af9c8d769a4809a3bd60be git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Joel-Stanley/ARM-aspeed-Secure-Boot-Controller-support/20211117-115258 git checkout 5057997868102d26f0af9c8d769a4809a3bd60be # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=sparc64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> drivers/soc/aspeed/aspeed-socinfo.c:166:38: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void *base @@ got void [noderef] __iomem * @@ drivers/soc/aspeed/aspeed-socinfo.c:166:38: sparse: expected void *base drivers/soc/aspeed/aspeed-socinfo.c:166:38: sparse: got void [noderef] __iomem * >> drivers/soc/aspeed/aspeed-socinfo.c:171:46: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got void * @@ drivers/soc/aspeed/aspeed-socinfo.c:171:46: sparse: expected void const volatile [noderef] __iomem *addr drivers/soc/aspeed/aspeed-socinfo.c:171:46: sparse: got void * >> drivers/soc/aspeed/aspeed-socinfo.c:173:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got void *base @@ drivers/soc/aspeed/aspeed-socinfo.c:173:25: sparse: expected void volatile [noderef] __iomem *addr drivers/soc/aspeed/aspeed-socinfo.c:173:25: sparse: got void *base vim +166 drivers/soc/aspeed/aspeed-socinfo.c 126 127 static int __init aspeed_socinfo_init(void) 128 { 129 struct soc_device_attribute *attrs; 130 struct soc_device *soc_dev; 131 struct device_node *np; 132 void __iomem *reg; 133 bool has_chipid = false; 134 bool has_sbe = false; 135 u32 siliconid; 136 u32 chipid[2]; 137 const char *machine = NULL; 138 139 np = of_find_compatible_node(NULL, NULL, "aspeed,silicon-id"); 140 if (!of_device_is_available(np)) { 141 of_node_put(np); 142 return -ENODEV; 143 } 144 145 reg = of_iomap(np, 0); 146 if (!reg) { 147 of_node_put(np); 148 return -ENODEV; 149 } 150 siliconid = readl(reg); 151 iounmap(reg); 152 153 /* This is optional, the ast2400 does not have it */ 154 reg = of_iomap(np, 1); 155 if (reg) { 156 has_chipid = true; 157 chipid[0] = readl(reg); 158 chipid[1] = readl(reg + 4); 159 iounmap(reg); 160 } 161 of_node_put(np); 162 163 /* AST2600 only */ 164 np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc"); 165 if (of_device_is_available(np)) { > 166 void *base = of_iomap(np, 0); 167 if (!base) { 168 of_node_put(np); 169 return -ENODEV; 170 } > 171 security_status = readl(base + SEC_STATUS); 172 has_sbe = true; > 173 iounmap(base); --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ARM: aspeed: Add secure boot controller support 2021-11-17 3:51 ` [PATCH 3/3] ARM: aspeed: Add secure boot controller support Joel Stanley 2021-11-25 8:38 ` kernel test robot @ 2022-01-10 23:29 ` Joel Stanley 2022-01-17 14:51 ` Arnd Bergmann 1 sibling, 1 reply; 8+ messages in thread From: Joel Stanley @ 2022-01-10 23:29 UTC (permalink / raw) To: Rob Herring, Andrew Jeffery, Arnd Bergmann Cc: Ryan Chen, Paul Menzel, devicetree, Linux ARM, linux-aspeed Hi Arnd, On Wed, 17 Nov 2021 at 03:51, Joel Stanley <joel@jms.id.au> wrote: > > This reads out the status of the secure boot controller and exposes it > in sysfs. > > An example on a AST2600A3 QEMU model: > > # grep . /sys/bus/soc/devices/soc0/* > /sys/bus/soc/devices/soc0/abr_image:0 > /sys/bus/soc/devices/soc0/family:AST2600 > /sys/bus/soc/devices/soc0/low_security_key:0 > /sys/bus/soc/devices/soc0/machine:Rainier 2U > /sys/bus/soc/devices/soc0/otp_protected:0 > /sys/bus/soc/devices/soc0/revision:A3 > /sys/bus/soc/devices/soc0/secure_boot:1 > /sys/bus/soc/devices/soc0/serial_number:888844441234abcd > /sys/bus/soc/devices/soc0/soc_id:05030303 > /sys/bus/soc/devices/soc0/uart_boot:1 Quoting from your response to my pull request: > - I actually want to avoid custom attributes on soc device instances as much > as possible. I have not looked in detail at what you add here, but the > number of custom attributes means we should discuss this properly. Can you explain the reasoning here? I am a bit surprised given we have this nice feature in struct soc_device_attribute: struct soc_device_attribute { ... const struct attribute_group *custom_attr_group; }; Cheers, Joel > > 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> > Reviewed-by: Ryan Chen <ryan_chen@aspeedtech.com> > --- > drivers/soc/aspeed/aspeed-socinfo.c | 73 +++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/soc/aspeed/aspeed-socinfo.c b/drivers/soc/aspeed/aspeed-socinfo.c > index 1ca140356a08..6faf2c199c90 100644 > --- a/drivers/soc/aspeed/aspeed-socinfo.c > +++ b/drivers/soc/aspeed/aspeed-socinfo.c > @@ -9,6 +9,8 @@ > #include <linux/slab.h> > #include <linux/sys_soc.h> > > +static u32 security_status; > + > static struct { > const char *name; > const u32 id; > @@ -74,6 +76,54 @@ static const char *siliconid_to_rev(u32 siliconid) > return "??"; > } > > +#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) > + > +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", !!(security_status & ABR_IMAGE_SOURCE)); > +} > +static DEVICE_ATTR_RO(abr_image); > + > +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", !!(security_status & LOW_SEC_KEY)); > +} > +static DEVICE_ATTR_RO(low_security_key); > + > +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", !!(security_status & OTP_PROTECTED)); > +} > +static DEVICE_ATTR_RO(otp_protected); > + > +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d\n", !!(security_status & SECURE_BOOT)); > +} > +static DEVICE_ATTR_RO(secure_boot); > + > +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + /* Invert the bit, as 1 is boot from SPI/eMMC */ > + return sprintf(buf, "%d\n", !(security_status & UART_BOOT)); > +} > +static DEVICE_ATTR_RO(uart_boot); > + > +static struct attribute *aspeed_attrs[] = { > + &dev_attr_abr_image.attr, > + &dev_attr_low_security_key.attr, > + &dev_attr_otp_protected.attr, > + &dev_attr_secure_boot.attr, > + &dev_attr_uart_boot.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(aspeed); > + > static int __init aspeed_socinfo_init(void) > { > struct soc_device_attribute *attrs; > @@ -81,6 +131,7 @@ static int __init aspeed_socinfo_init(void) > struct device_node *np; > void __iomem *reg; > bool has_chipid = false; > + bool has_sbe = false; > u32 siliconid; > u32 chipid[2]; > const char *machine = NULL; > @@ -109,6 +160,20 @@ static int __init aspeed_socinfo_init(void) > } > of_node_put(np); > > + /* AST2600 only */ > + np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc"); > + if (of_device_is_available(np)) { > + void *base = of_iomap(np, 0); > + if (!base) { > + of_node_put(np); > + return -ENODEV; > + } > + security_status = readl(base + SEC_STATUS); > + has_sbe = true; > + iounmap(base); > + of_node_put(np); > + } > + > attrs = kzalloc(sizeof(*attrs), GFP_KERNEL); > if (!attrs) > return -ENODEV; > @@ -135,6 +200,9 @@ static int __init aspeed_socinfo_init(void) > attrs->serial_number = kasprintf(GFP_KERNEL, "%08x%08x", > chipid[1], chipid[0]); > > + if (has_sbe) > + attrs->custom_attr_group = aspeed_groups[0]; > + > soc_dev = soc_device_register(attrs); > if (IS_ERR(soc_dev)) { > kfree(attrs->soc_id); > @@ -148,6 +216,11 @@ static int __init aspeed_socinfo_init(void) > attrs->revision, > attrs->soc_id); > > + if (has_sbe) { > + pr_info("AST2600 secure boot %s\n", > + (security_status & SECURE_BOOT) ? "enabled" : "disabled"); > + } > + > return 0; > } > early_initcall(aspeed_socinfo_init); > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ARM: aspeed: Add secure boot controller support 2022-01-10 23:29 ` Joel Stanley @ 2022-01-17 14:51 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2022-01-17 14:51 UTC (permalink / raw) To: Joel Stanley Cc: Rob Herring, Andrew Jeffery, Arnd Bergmann, Ryan Chen, Paul Menzel, devicetree, Linux ARM, linux-aspeed On Tue, Jan 11, 2022 at 12:29 AM Joel Stanley <joel@jms.id.au> wrote: > On Wed, 17 Nov 2021 at 03:51, Joel Stanley <joel@jms.id.au> wrote: > > > > This reads out the status of the secure boot controller and exposes it > > in sysfs. > > > > An example on a AST2600A3 QEMU model: > > > > # grep . /sys/bus/soc/devices/soc0/* > > /sys/bus/soc/devices/soc0/abr_image:0 > > /sys/bus/soc/devices/soc0/family:AST2600 > > /sys/bus/soc/devices/soc0/low_security_key:0 > > /sys/bus/soc/devices/soc0/machine:Rainier 2U > > /sys/bus/soc/devices/soc0/otp_protected:0 > > /sys/bus/soc/devices/soc0/revision:A3 > > /sys/bus/soc/devices/soc0/secure_boot:1 > > /sys/bus/soc/devices/soc0/serial_number:888844441234abcd > > /sys/bus/soc/devices/soc0/soc_id:05030303 > > /sys/bus/soc/devices/soc0/uart_boot:1 > > Quoting from your response to my pull request: > > > - I actually want to avoid custom attributes on soc device instances as much > > as possible. I have not looked in detail at what you add here, but the > > number of custom attributes means we should discuss this properly. > > Can you explain the reasoning here? > > I am a bit surprised given we have this nice feature in struct > soc_device_attribute: > > struct soc_device_attribute { > ... > const struct attribute_group *custom_attr_group; > }; I have two main concerns: - any attribute that makes sense across multiple SoC families should probably be part of the standard set of attributes. Ideally this could fit within the existing attributes, but if you can make a reasonable case for adding further ones, that is fine as well. The standard attributes can then also be accessed from within the kernel with soc_device_match(), rather than just being available to user space. - The attributes should all be used to identify the SoC, not a particular part of the SoC that is better represented as a separate device. If you are adding five attributes at once, it's likely that these don't all fit the constraints, though I had not yet looked at the implementation. From what I see in +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & ABR_IMAGE_SOURCE)); +} + +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & LOW_SEC_KEY)); +} + +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & OTP_PROTECTED)); +} + +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !!(security_status & SECURE_BOOT)); +} + +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", !(security_status & UART_BOOT)); +} it appears that these are related to how the system was started, which doesn't fit either of the requirements: the same information may be useful for non-aspeed systems, so it would be good to have it in a standardized interface rather than vendor extensions, and it doesn't really identify the SoC but instead provides information from a device that is inside of the SoC. Maybe this could be turned into a generalized interface similar to soc_device that exposes the boot status in sysfs? We have a couple of files that determine e.g. whether the kernel was booted securely, and those could all hook up here. It doesn't have to be anything complex, just a node under /sys/firmware or /sys/power that has a couple of documented attributes that can be filled by drivers. Arnd ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-17 14:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-17 3:51 [PATCH 0/3] ARM: aspeed: Secure Boot Controller support Joel Stanley 2021-11-17 3:51 ` [PATCH 1/3] dt-bindings: aspeed: Add Secure Boot Controller bindings Joel Stanley 2021-11-29 22:41 ` Rob Herring 2021-11-17 3:51 ` [PATCH 2/3] ARM: dts: aspeed: Add secure boot controller node Joel Stanley 2021-11-17 3:51 ` [PATCH 3/3] ARM: aspeed: Add secure boot controller support Joel Stanley 2021-11-25 8:38 ` kernel test robot 2022-01-10 23:29 ` Joel Stanley 2022-01-17 14:51 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).