| Message ID | 20211117035106.321454-4-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: aspeed: Secure Boot Controller support | expand |
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
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 >
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
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);