* [PATCH] ACPI/APEI: Add BERT data driver @ 2017-08-14 16:56 Luck, Tony 2017-08-15 10:22 ` Punit Agrawal 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-14 16:56 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tony Luck, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel From: Tony Luck <tony.luck@intel.com> The ACPI Boot Error Record Table provides a method for platform firmware to give information to the operating system about error that occurred prior to boot (of particular interest are problems that caused the previous OS instance to crash). The BERT table simply provides the size and address of the error record in BIOS reserved memory. In an earlier age we might have used /dev/mem to retrieve this error record, but many systems disable /dev/mem for security reasons. This driver provides read-only access to the data via a character special device "/dev/bert-data". Cc: Len Brown <lenb@kernel.org> Cc: Boris Petkov <bp@suse.de> Cc: Tyler Baicar <tbaicar@codeaurora.org> Cc: linux-acpi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Tony Luck <tony.luck@intel.com> --- drivers/acpi/apei/Kconfig | 7 +++ drivers/acpi/apei/Makefile | 1 + drivers/acpi/apei/bert-data.c | 107 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 drivers/acpi/apei/bert-data.c diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig index de14d49a5c90..a785bfbf7e8c 100644 --- a/drivers/acpi/apei/Kconfig +++ b/drivers/acpi/apei/Kconfig @@ -77,3 +77,10 @@ config ACPI_APEI_ERST_DEBUG error information to and from a persistent store. Enable this if you want to debugging and testing the ERST kernel support and firmware implementation. + +config ACPI_APEI_BERT_DATA + tristate "APEI BERT data driver" + depends on ACPI_APEI + help + This driver provides read access to the error record that + the ACPI/APEI/BERT table points at. diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile index e50573de25f1..f092c1bc60b8 100644 --- a/drivers/acpi/apei/Makefile +++ b/drivers/acpi/apei/Makefile @@ -2,5 +2,6 @@ obj-$(CONFIG_ACPI_APEI) += apei.o obj-$(CONFIG_ACPI_APEI_GHES) += ghes.o obj-$(CONFIG_ACPI_APEI_EINJ) += einj.o obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o +obj-$(CONFIG_ACPI_APEI_BERT_DATA) += bert-data.o apei-y := apei-base.o hest.o erst.o bert.o diff --git a/drivers/acpi/apei/bert-data.c b/drivers/acpi/apei/bert-data.c new file mode 100644 index 000000000000..1590bd82ef63 --- /dev/null +++ b/drivers/acpi/apei/bert-data.c @@ -0,0 +1,107 @@ +/* + * bert-data: driver to provide read access to the error record(s) + * provided by the ACPI BERT table. + * See ACPI specification section 18.3.1 "Boot Error Source" + * + * Copyright (C) 2017 Intel Corporation + * + * Author: + * Tony Luck <tony.luck@intel.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +#define pr_fmt(fmt) "bert-data: " fmt + +#include <linux/module.h> +#include <linux/acpi.h> +#include <acpi/acpiosxf.h> +#include <linux/fs.h> +#include <linux/miscdevice.h> +#include <linux/mm.h> +#include <linux/uaccess.h> + +static u32 bert_size; +static __iomem void *bert_data; + +static int bert_chrdev_open(struct inode *inode, struct file *file) +{ + if (file->f_flags & (O_WRONLY | O_RDWR)) + return -EPERM; + inode->i_size = bert_size; + return 0; +} + +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf, + size_t usize, loff_t *off) +{ + if (*off > bert_size) + return -EINVAL; + if (*off + usize > bert_size) + usize = bert_size - *off; + if (copy_to_user(ubuf, bert_data + *off, usize)) + return -EFAULT; + *off += usize; + return usize; +} + +static const struct file_operations bert_chrdev_ops = { + .open = bert_chrdev_open, + .read = bert_chrdev_read, + .llseek = default_llseek, +}; + +static struct miscdevice bert_chrdev_device = { + .minor = MISC_DYNAMIC_MINOR, + .name = "bert-data", + .fops = &bert_chrdev_ops, + .mode = 0444, +}; + +static __init int bert_init(void) +{ + struct acpi_table_bert *bert; + acpi_status stat; + int err; + + if (acpi_disabled) + return -ENODEV; + + stat = acpi_get_table(ACPI_SIG_BERT, 0, + (struct acpi_table_header **)&bert); + if (stat == AE_NOT_FOUND) + return -ENODEV; + if (ACPI_FAILURE(stat)) { + pr_err("get table failed, %s.\n", acpi_format_exception(stat)); + return -EINVAL; + } + + bert_size = bert->region_length; + bert_data = acpi_os_map_memory(bert->address, bert->region_length); + acpi_put_table((struct acpi_table_header *)bert); + if (!bert_data) + return -ENOMEM; + err = misc_register(&bert_chrdev_device); + if (err) + acpi_os_unmap_memory(bert_data, bert_size); + + return err; +} +module_init(bert_init); + +static __exit void bert_exit(void) +{ + acpi_os_unmap_memory(bert_data, bert_size); + misc_deregister(&bert_chrdev_device); +} +module_exit(bert_exit); + +MODULE_DESCRIPTION("ACPI Boot Error Data"); +MODULE_LICENSE("GPL v2"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-14 16:56 [PATCH] ACPI/APEI: Add BERT data driver Luck, Tony @ 2017-08-15 10:22 ` Punit Agrawal 2017-08-15 21:15 ` Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Punit Agrawal @ 2017-08-15 10:22 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel Hi Tony, "Luck, Tony" <tony.luck@intel.com> writes: > From: Tony Luck <tony.luck@intel.com> > > The ACPI Boot Error Record Table provides a method for platform > firmware to give information to the operating system about error > that occurred prior to boot (of particular interest are problems > that caused the previous OS instance to crash). > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory. In an earlier age we might have > used /dev/mem to retrieve this error record, but many systems > disable /dev/mem for security reasons. > > This driver provides read-only access to the data via a character > special device "/dev/bert-data". There is already a bert driver which prints the error record. Would it make sense to integrate the character device there instead of creating a new driver? [...] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-15 10:22 ` Punit Agrawal @ 2017-08-15 21:15 ` Luck, Tony 2017-08-16 13:14 ` Punit Agrawal 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-15 21:15 UTC (permalink / raw) To: Punit Agrawal Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel On Tue, Aug 15, 2017 at 11:22:06AM +0100, Punit Agrawal wrote: > There is already a bert driver which prints the error record. Would it > make sense to integrate the character device there instead of creating a > new driver? Like this? The source code is smaller. But it doesn't offer the option to unload the driver and unmap the BERT data region after you have retrieved the error record. Either looks plausible to me (but I'm hardly a disinterested party). Votes? -Tony >From cbeabf2d83fe91eebd960cd5cc1b61faaeed1441 Mon Sep 17 00:00:00 2001 From: Tony Luck <tony.luck@intel.com> Date: Tue, 15 Aug 2017 13:48:28 -0700 Subject: [PATCH] ACPI: APEI: Extend BERT driver to provide a character device to access data The BERT table simply provides the size and address of the error record in BIOS reserved memory. Currently this driver decodes the record to the console log. But other users of BERT may want to access the full binary record. In an earlier age we might have used /dev/mem to retrieve this error record, but many systems disable /dev/mem for security reasons. Extend this driver to provide read-only access to the data via a character special device "/dev/bert-data". Cc: Len Brown <lenb@kernel.org> Cc: Boris Petkov <bp@suse.de> Cc: Tyler Baicar <tbaicar@codeaurora.org> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: linux-acpi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Tony Luck <tony.luck@intel.com> v2: (suggested by Punit Agrawal) don't make a whole new driver, merge this functionality into the existing BERT driver. --- drivers/acpi/apei/bert.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c index 12771fcf0417..9bc39b1bffde 100644 --- a/drivers/acpi/apei/bert.c +++ b/drivers/acpi/apei/bert.c @@ -26,12 +26,16 @@ #include <linux/init.h> #include <linux/acpi.h> #include <linux/io.h> +#include <linux/miscdevice.h> +#include <linux/uaccess.h> #include "apei-internal.h" #undef pr_fmt #define pr_fmt(fmt) "BERT: " fmt +static __iomem void *bert_data; +static unsigned int region_len; static int bert_disable; static void __init bert_print_all(struct acpi_bert_region *region, @@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab) return 0; } +static int bert_chrdev_open(struct inode *inode, struct file *file) +{ + if (file->f_flags & (O_WRONLY | O_RDWR)) + return -EPERM; + inode->i_size = region_len; + return 0; +} + +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf, + size_t usize, loff_t *off) +{ + if (*off > region_len) + return -EINVAL; + if (*off + usize > region_len) + usize = region_len - *off; + if (copy_to_user(ubuf, bert_data + *off, usize)) + return -EFAULT; + *off += usize; + return usize; +} + +static const struct file_operations bert_chrdev_ops = { + .open = bert_chrdev_open, + .read = bert_chrdev_read, + .llseek = default_llseek, +}; + +static struct miscdevice bert_chrdev_device = { + .minor = MISC_DYNAMIC_MINOR, + .name = "bert-data", + .fops = &bert_chrdev_ops, + .mode = 0444, +}; + static int __init bert_init(void) { - struct apei_resources bert_resources; struct acpi_bert_region *boot_error_region; + struct apei_resources bert_resources; struct acpi_table_bert *bert_tab; - unsigned int region_len; acpi_status status; int rc = 0; @@ -139,7 +176,9 @@ static int __init bert_init(void) boot_error_region = ioremap_cache(bert_tab->address, region_len); if (boot_error_region) { bert_print_all(boot_error_region, region_len); - iounmap(boot_error_region); + bert_data = boot_error_region; + if (misc_register(&bert_chrdev_device)) + iounmap(boot_error_region); } else { rc = -ENOMEM; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-15 21:15 ` Luck, Tony @ 2017-08-16 13:14 ` Punit Agrawal 2017-08-16 15:22 ` Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Punit Agrawal @ 2017-08-16 13:14 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel "Luck, Tony" <tony.luck@intel.com> writes: > On Tue, Aug 15, 2017 at 11:22:06AM +0100, Punit Agrawal wrote: >> There is already a bert driver which prints the error record. Would it >> make sense to integrate the character device there instead of creating a >> new driver? > > Like this? The source code is smaller. But it doesn't offer the option to unload > the driver and unmap the BERT data region after you have retrieved the error record. Is there any benefit in being able to unload the driver in real world usage? It should be possible to convert the existing driver into a loadable module - thought that means re-printing the error records to the kernel log if the module is re-loaded. Not sure if that breaks any existing usecases. One thing I missed commenting on in the previous version - Have you thought of exposing the error records via /sys/firmware/acpi? The tables are already exposed there and as BERT is part of ACPI logically that's a better fit compared to a misc device. > > Either looks plausible to me (but I'm hardly a disinterested party). > > Votes? > > -Tony > > From cbeabf2d83fe91eebd960cd5cc1b61faaeed1441 Mon Sep 17 00:00:00 2001 > From: Tony Luck <tony.luck@intel.com> > Date: Tue, 15 Aug 2017 13:48:28 -0700 > Subject: [PATCH] ACPI: APEI: Extend BERT driver to provide a character device > to access data > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory. Currently this driver decodes the > record to the console log. But other users of BERT may want to access > the full binary record. > > In an earlier age we might have used /dev/mem to retrieve this error > record, but many systems disable /dev/mem for security reasons. > > Extend this driver to provide read-only access to the data via a character > special device "/dev/bert-data". > > Cc: Len Brown <lenb@kernel.org> > Cc: Boris Petkov <bp@suse.de> > Cc: Tyler Baicar <tbaicar@codeaurora.org> > Cc: Punit Agrawal <punit.agrawal@arm.com> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Tony Luck <tony.luck@intel.com> > > v2: (suggested by Punit Agrawal) don't make a whole new driver, merge > this functionality into the existing BERT driver. > --- > drivers/acpi/apei/bert.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c > index 12771fcf0417..9bc39b1bffde 100644 > --- a/drivers/acpi/apei/bert.c > +++ b/drivers/acpi/apei/bert.c > @@ -26,12 +26,16 @@ > #include <linux/init.h> > #include <linux/acpi.h> > #include <linux/io.h> > +#include <linux/miscdevice.h> > +#include <linux/uaccess.h> > > #include "apei-internal.h" > > #undef pr_fmt > #define pr_fmt(fmt) "BERT: " fmt > > +static __iomem void *bert_data; > +static unsigned int region_len; > static int bert_disable; > > static void __init bert_print_all(struct acpi_bert_region *region, > @@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab) > return 0; > } > > +static int bert_chrdev_open(struct inode *inode, struct file *file) > +{ > + if (file->f_flags & (O_WRONLY | O_RDWR)) > + return -EPERM; > + inode->i_size = region_len; > + return 0; > +} > + > +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf, > + size_t usize, loff_t *off) > +{ > + if (*off > region_len) > + return -EINVAL; > + if (*off + usize > region_len) > + usize = region_len - *off; > + if (copy_to_user(ubuf, bert_data + *off, usize)) > + return -EFAULT; > + *off += usize; > + return usize; > +} > + > +static const struct file_operations bert_chrdev_ops = { > + .open = bert_chrdev_open, > + .read = bert_chrdev_read, > + .llseek = default_llseek, > +}; > + > +static struct miscdevice bert_chrdev_device = { > + .minor = MISC_DYNAMIC_MINOR, > + .name = "bert-data", > + .fops = &bert_chrdev_ops, > + .mode = 0444, > +}; > + > static int __init bert_init(void) > { > - struct apei_resources bert_resources; > struct acpi_bert_region *boot_error_region; > + struct apei_resources bert_resources; > struct acpi_table_bert *bert_tab; > - unsigned int region_len; > acpi_status status; > int rc = 0; > > @@ -139,7 +176,9 @@ static int __init bert_init(void) > boot_error_region = ioremap_cache(bert_tab->address, region_len); > if (boot_error_region) { > bert_print_all(boot_error_region, region_len); > - iounmap(boot_error_region); > + bert_data = boot_error_region; > + if (misc_register(&bert_chrdev_device)) > + iounmap(boot_error_region); > } else { > rc = -ENOMEM; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-16 13:14 ` Punit Agrawal @ 2017-08-16 15:22 ` Luck, Tony 2017-08-17 10:25 ` Punit Agrawal 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-16 15:22 UTC (permalink / raw) To: Punit Agrawal Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel > One thing I missed commenting on in the previous version - > > Have you thought of exposing the error records via /sys/firmware/acpi? > The tables are already exposed there and as BERT is part of ACPI > logically that's a better fit compared to a misc device. That was my first thought :-) But I got stuck on how to name things. The BERT entry appears in /sys/firmware/acpi/tables/ ... but the code doesn't know anything special about "BERT", it just iterates over all the tables and makes them all appear. I thought about making it /sys/firmware/acpi/tables/BERT.data, but that seemed very ugly (this file isn't a "table", so why does it appear in the "tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now the driver has to make another directory. Thoughts? -Tony ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-16 15:22 ` Luck, Tony @ 2017-08-17 10:25 ` Punit Agrawal 2017-08-17 17:49 ` Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Punit Agrawal @ 2017-08-17 10:25 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel "Luck, Tony" <tony.luck@intel.com> writes: >> One thing I missed commenting on in the previous version - >> >> Have you thought of exposing the error records via /sys/firmware/acpi? >> The tables are already exposed there and as BERT is part of ACPI >> logically that's a better fit compared to a misc device. > > That was my first thought :-) > > But I got stuck on how to name things. The BERT entry appears in > /sys/firmware/acpi/tables/ ... but the code doesn't know anything special > about "BERT", it just iterates over all the tables and makes them all > appear. > > I thought about making it /sys/firmware/acpi/tables/BERT.data, but that > seemed very ugly (this file isn't a "table", so why does it appear in the > "tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now > the driver has to make another directory. > > Thoughts? I agree that keeping the error record out of the tables directory makes sense as it's not an ACPI table. The best I could come up with was bert-data or bert-region in /sys/firmware/acpi/apei/. Though, I am OK with "table-data/BERT" as well. > > -Tony > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-17 10:25 ` Punit Agrawal @ 2017-08-17 17:49 ` Luck, Tony 2017-08-17 19:28 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-17 17:49 UTC (permalink / raw) To: Punit Agrawal Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel On Thu, Aug 17, 2017 at 11:25:23AM +0100, Punit Agrawal wrote: > Though, I am OK with "table-data/BERT" as well. So I coded this up ... and it looks much better than I thought it might. A bit larger than the previous version that modified drivers/acpi/apei/bert.c. But on the plus side easy to extend if there are other ACPI tables that point at interesting data. It doesn't handle dynamic tables. That would add a lot of complexity and BERT isn't dynamic. Could be a later patch if someone has a need for it. All names negotiable. -Tony >From 595562691d2747459235ddbc396949337605ac2a Mon Sep 17 00:00:00 2001 From: Tony Luck <tony.luck@intel.com> Date: Wed, 16 Aug 2017 15:36:28 -0700 Subject: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region The ACPI sysfs interface provides a way to read each ACPI table from userspace via entries in /sys/firmware/acpi/tables/ The BERT table simply provides the size and address of the error record in BIOS reserved memory and users may want access to this record. In an earlier age we might have used /dev/mem to retrieve this error record, but many systems disable /dev/mem for security reasons. Extend this driver to provide read-only access to the data via a file in a new directory /sys/firmware/acpi/tables-data/ Cc: Len Brown <lenb@kernel.org> Cc: Boris Petkov <bp@suse.de> Cc: Tyler Baicar <tbaicar@codeaurora.org> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: linux-acpi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Tony Luck <tony.luck@intel.com> v3: (suggested by Punit Agrawal) extend the /sys/firmware/acpi code to provide this functionality. --- drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c index e414fabf7315..b24549a012f4 100644 --- a/drivers/acpi/sysfs.c +++ b/drivers/acpi/sysfs.c @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444); /* * ACPI table sysfs I/F: * /sys/firmware/acpi/tables/ + * /sys/firmware/acpi/tables-data/ * /sys/firmware/acpi/tables/dynamic/ */ static LIST_HEAD(acpi_table_attr_list); static struct kobject *tables_kobj; +static struct kobject *tables_data_kobj; static struct kobject *dynamic_tables_kobj; static struct kobject *hotplug_kobj; @@ -325,6 +327,11 @@ struct acpi_table_attr { struct list_head node; }; +struct acpi_data_attr { + struct bin_attribute attr; + u64 addr; +}; + static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t offset, size_t count) @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context) return AE_OK; } +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t offset, size_t count) +{ + struct acpi_data_attr *data_attr; + void __iomem *base; + ssize_t rc; + + data_attr = container_of(bin_attr, struct acpi_data_attr, attr); + + base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size); + if (!base) + return -ENOMEM; + rc = memory_read_from_buffer(buf, count, &offset, base, + data_attr->attr.size); + acpi_os_unmap_memory(base, data_attr->attr.size); + + return rc; +} + +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr) +{ + struct acpi_table_bert *bert = th; + + if (bert->header.length < sizeof(struct acpi_table_bert) || + bert->region_length < sizeof(struct acpi_hest_generic_status)) { + kfree(data_attr); + return -EINVAL; + } + data_attr->addr = bert->address; + data_attr->attr.size = bert->region_length; + data_attr->attr.attr.name = "BERT-DATA"; + + return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr); +} + +static struct acpi_data_obj { + char *name; + int (*fn)(void *, struct acpi_data_attr *); +} acpi_data_objs[] = { + { ACPI_SIG_BERT, acpi_bert_data_init }, +}; + +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs) + +static int acpi_table_data_init(struct acpi_table_header *th) +{ + struct acpi_data_attr *data_attr; + int i; + + for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) { + if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) { + data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL); + if (!data_attr) + return -ENOMEM; + sysfs_attr_init(&data_attr->attr.attr); + data_attr->attr.read = acpi_data_show; + data_attr->attr.attr.mode = 0400; + return acpi_data_objs[i].fn(th, data_attr); + } + } + return 0; +} + static int acpi_tables_sysfs_init(void) { struct acpi_table_attr *table_attr; @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void) if (!tables_kobj) goto err; + tables_data_kobj = kobject_create_and_add("tables-data", acpi_kobj); + if (!tables_data_kobj) + goto err_tables_data; + dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj); if (!dynamic_tables_kobj) goto err_dynamic_tables; @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void) return ret; } list_add_tail(&table_attr->node, &acpi_table_attr_list); + acpi_table_data_init(table_header); } kobject_uevent(tables_kobj, KOBJ_ADD); + kobject_uevent(tables_data_kobj, KOBJ_ADD); kobject_uevent(dynamic_tables_kobj, KOBJ_ADD); return 0; err_dynamic_tables: + kobject_put(tables_data_kobj); +err_tables_data: kobject_put(tables_kobj); err: return -ENOMEM; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-17 17:49 ` Luck, Tony @ 2017-08-17 19:28 ` Rafael J. Wysocki 2017-08-17 20:29 ` Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2017-08-17 19:28 UTC (permalink / raw) To: Luck, Tony Cc: Punit Agrawal, Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel On Thu, Aug 17, 2017 at 7:49 PM, Luck, Tony <tony.luck@intel.com> wrote: > On Thu, Aug 17, 2017 at 11:25:23AM +0100, Punit Agrawal wrote: >> Though, I am OK with "table-data/BERT" as well. > > So I coded this up ... and it looks much better than I thought > it might. A bit larger than the previous version that modified > drivers/acpi/apei/bert.c. But on the plus side easy to extend if > there are other ACPI tables that point at interesting data. > > It doesn't handle dynamic tables. That would add a lot of complexity > and BERT isn't dynamic. Could be a later patch if someone has a > need for it. > > All names negotiable. OK What about /sys/firmware/acpi/tables/data/ and then one file per table under it with the same name as the table file under tables/ ? So in particular for BERT the data would be in /sys/firmware/acpi/tables/data/BERT ? Thanks, Rafael ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-17 19:28 ` Rafael J. Wysocki @ 2017-08-17 20:29 ` Luck, Tony 2017-08-17 20:41 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-17 20:29 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Punit Agrawal, Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel On Thu, Aug 17, 2017 at 09:28:53PM +0200, Rafael J. Wysocki wrote: > What about /sys/firmware/acpi/tables/data/ and then one file per table > under it with the same name as the table file under tables/ ? "data" works. ACPI table names are all upper-case, so it can't conflict. Any programs that scan the whole directory have to already skip "dynamic", so they likely would not be confused by the appearance of a second directory. For BERT there is only one blob of data. So naming it the same as the table is fine. Maybe other tables might have more than one thing? If so, they could use the table name as a prefix, or make a subdirectory. We can cross that bridge if anyone finds additional useful things to add. > So in particular for BERT the data would be in > /sys/firmware/acpi/tables/data/BERT ? Ok. Will re-spin the patch with these names. -Tony ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI/APEI: Add BERT data driver 2017-08-17 20:29 ` Luck, Tony @ 2017-08-17 20:41 ` Rafael J. Wysocki 2017-08-17 21:39 ` [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2017-08-17 20:41 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Punit Agrawal, Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel On Thu, Aug 17, 2017 at 10:29 PM, Luck, Tony <tony.luck@intel.com> wrote: > On Thu, Aug 17, 2017 at 09:28:53PM +0200, Rafael J. Wysocki wrote: >> What about /sys/firmware/acpi/tables/data/ and then one file per table >> under it with the same name as the table file under tables/ ? > > "data" works. ACPI table names are all upper-case, so it > can't conflict. Any programs that scan the whole directory > have to already skip "dynamic", so they likely would not > be confused by the appearance of a second directory. > > For BERT there is only one blob of data. So naming it the > same as the table is fine. Maybe other tables might have > more than one thing? If so, they could use the table name > as a prefix, or make a subdirectory. We can cross that > bridge if anyone finds additional useful things to add. Actually, there already are multiple SSDTs on the majority of systems and the corresponding files under tables/ are called SSDT1, SSDT2 etc., so we just need to follow the existing convention. Thanks, Rafael ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-17 20:41 ` Rafael J. Wysocki @ 2017-08-17 21:39 ` Luck, Tony 2017-08-18 0:30 ` Alan Cox 2017-08-18 17:38 ` Punit Agrawal 0 siblings, 2 replies; 19+ messages in thread From: Luck, Tony @ 2017-08-17 21:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tony Luck, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel From: Tony Luck <tony.luck@intel.com> The ACPI sysfs interface provides a way to read each ACPI table from userspace via entries in /sys/firmware/acpi/tables/ The BERT table simply provides the size and address of the error record in BIOS reserved memory and users may want access to this record. In an earlier age we might have used /dev/mem to retrieve this error record, but many systems disable /dev/mem for security reasons. Extend this driver to provide read-only access to the data via a file in a new directory /sys/firmware/acpi/tables/data/BERT Cc: Len Brown <lenb@kernel.org> Cc: Boris Petkov <bp@suse.de> Cc: Tyler Baicar <tbaicar@codeaurora.org> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: linux-acpi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Tony Luck <tony.luck@intel.com> v4: (suggested by Punit Agrawal) extend the /sys/firmware/acpi code to provide this functionality. File/directory names changed to Rafael's suggestions --- drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c index e414fabf7315..7c4eed0594d0 100644 --- a/drivers/acpi/sysfs.c +++ b/drivers/acpi/sysfs.c @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444); /* * ACPI table sysfs I/F: * /sys/firmware/acpi/tables/ + * /sys/firmware/acpi/tables-data/ * /sys/firmware/acpi/tables/dynamic/ */ static LIST_HEAD(acpi_table_attr_list); static struct kobject *tables_kobj; +static struct kobject *tables_data_kobj; static struct kobject *dynamic_tables_kobj; static struct kobject *hotplug_kobj; @@ -325,6 +327,11 @@ struct acpi_table_attr { struct list_head node; }; +struct acpi_data_attr { + struct bin_attribute attr; + u64 addr; +}; + static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t offset, size_t count) @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context) return AE_OK; } +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t offset, size_t count) +{ + struct acpi_data_attr *data_attr; + void __iomem *base; + ssize_t rc; + + data_attr = container_of(bin_attr, struct acpi_data_attr, attr); + + base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size); + if (!base) + return -ENOMEM; + rc = memory_read_from_buffer(buf, count, &offset, base, + data_attr->attr.size); + acpi_os_unmap_memory(base, data_attr->attr.size); + + return rc; +} + +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr) +{ + struct acpi_table_bert *bert = th; + + if (bert->header.length < sizeof(struct acpi_table_bert) || + bert->region_length < sizeof(struct acpi_hest_generic_status)) { + kfree(data_attr); + return -EINVAL; + } + data_attr->addr = bert->address; + data_attr->attr.size = bert->region_length; + data_attr->attr.attr.name = "BERT"; + + return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr); +} + +static struct acpi_data_obj { + char *name; + int (*fn)(void *, struct acpi_data_attr *); +} acpi_data_objs[] = { + { ACPI_SIG_BERT, acpi_bert_data_init }, +}; + +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs) + +static int acpi_table_data_init(struct acpi_table_header *th) +{ + struct acpi_data_attr *data_attr; + int i; + + for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) { + if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) { + data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL); + if (!data_attr) + return -ENOMEM; + sysfs_attr_init(&data_attr->attr.attr); + data_attr->attr.read = acpi_data_show; + data_attr->attr.attr.mode = 0400; + return acpi_data_objs[i].fn(th, data_attr); + } + } + return 0; +} + static int acpi_tables_sysfs_init(void) { struct acpi_table_attr *table_attr; @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void) if (!tables_kobj) goto err; + tables_data_kobj = kobject_create_and_add("data", tables_kobj); + if (!tables_data_kobj) + goto err_tables_data; + dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj); if (!dynamic_tables_kobj) goto err_dynamic_tables; @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void) return ret; } list_add_tail(&table_attr->node, &acpi_table_attr_list); + acpi_table_data_init(table_header); } kobject_uevent(tables_kobj, KOBJ_ADD); + kobject_uevent(tables_data_kobj, KOBJ_ADD); kobject_uevent(dynamic_tables_kobj, KOBJ_ADD); return 0; err_dynamic_tables: + kobject_put(tables_data_kobj); +err_tables_data: kobject_put(tables_kobj); err: return -ENOMEM; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-17 21:39 ` [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region Luck, Tony @ 2017-08-18 0:30 ` Alan Cox 2017-08-18 2:12 ` Luck, Tony 2017-08-23 14:56 ` Luck, Tony 2017-08-18 17:38 ` Punit Agrawal 1 sibling, 2 replies; 19+ messages in thread From: Alan Cox @ 2017-08-18 0:30 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel On Thu, 17 Aug 2017 14:39:46 -0700 "Luck, Tony" <tony.luck@intel.com> wrote: > From: Tony Luck <tony.luck@intel.com> > > The ACPI sysfs interface provides a way to read each ACPI table from > userspace via entries in /sys/firmware/acpi/tables/ > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory and users may want access to this > record. > > In an earlier age we might have used /dev/mem to retrieve this error > record, but many systems disable /dev/mem for security reasons. > > Extend this driver to provide read-only access to the data via a > file in a new directory /sys/firmware/acpi/tables/data/BERT Should this not also have a capability check. Assuming file permissions are sufficient for grabbing a chunk of system memory holding error info doesn't seem too scary but it's at odds with a lot of other cases ? Alan ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-18 0:30 ` Alan Cox @ 2017-08-18 2:12 ` Luck, Tony 2017-08-23 14:56 ` Luck, Tony 1 sibling, 0 replies; 19+ messages in thread From: Luck, Tony @ 2017-08-18 2:12 UTC (permalink / raw) To: Alan Cox Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel > Should this not also have a capability check. Assuming file permissions > are sufficient for grabbing a chunk of system memory holding error > info doesn't seem too scary but it's at odds with a lot of other cases ? At least one of those other cases (pstore) added a capability check and now regret it. There's a thread on reverting it. Look for: Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" -Tony ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-18 0:30 ` Alan Cox 2017-08-18 2:12 ` Luck, Tony @ 2017-08-23 14:56 ` Luck, Tony 2017-08-29 0:10 ` Kees Cook 1 sibling, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-23 14:56 UTC (permalink / raw) To: Alan Cox, Kees Cook Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel >> Should this not also have a capability check. Assuming file permissions >> are sufficient for grabbing a chunk of system memory holding error >> info doesn't seem too scary but it's at odds with a lot of other cases ? > > At least one of those other cases (pstore) added a capability check and now regret > it. There's a thread on reverting it. Look for: > > Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" Here's at least part of that thread: https://marc.info/?l=linux-kernel&m=150301241114262&w=2 Kees: you were OK with removing the capability check from pstore, right? I'm now adding another place to access a blob of memory containing crash information from the previous kernel (pointed at by the ACPI BERT record). -Tony ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-23 14:56 ` Luck, Tony @ 2017-08-29 0:10 ` Kees Cook 2017-08-29 15:55 ` Luck, Tony 0 siblings, 1 reply; 19+ messages in thread From: Kees Cook @ 2017-08-29 0:10 UTC (permalink / raw) To: Luck, Tony Cc: Alan Cox, Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel On Wed, Aug 23, 2017 at 7:56 AM, Luck, Tony <tony.luck@intel.com> wrote: >>> Should this not also have a capability check. Assuming file permissions >>> are sufficient for grabbing a chunk of system memory holding error >>> info doesn't seem too scary but it's at odds with a lot of other cases ? >> >> At least one of those other cases (pstore) added a capability check and now regret >> it. There's a thread on reverting it. Look for: >> >> Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" > > Here's at least part of that thread: > > https://marc.info/?l=linux-kernel&m=150301241114262&w=2 > > Kees: you were OK with removing the capability check from pstore, right? Yeah, as long as there is comparable protections. -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-29 0:10 ` Kees Cook @ 2017-08-29 15:55 ` Luck, Tony 0 siblings, 0 replies; 19+ messages in thread From: Luck, Tony @ 2017-08-29 15:55 UTC (permalink / raw) To: Kees Cook Cc: Alan Cox, Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, Punit Agrawal, linux-acpi, linux-kernel On Mon, Aug 28, 2017 at 05:10:33PM -0700, Kees Cook wrote: > On Wed, Aug 23, 2017 at 7:56 AM, Luck, Tony <tony.luck@intel.com> wrote: > >>> Should this not also have a capability check. Assuming file permissions > >>> are sufficient for grabbing a chunk of system memory holding error > >>> info doesn't seem too scary but it's at odds with a lot of other cases ? > >> > >> At least one of those other cases (pstore) added a capability check and now regret > >> it. There's a thread on reverting it. Look for: > >> > >> Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps" > > > > Here's at least part of that thread: > > > > https://marc.info/?l=linux-kernel&m=150301241114262&w=2 > > > > Kees: you were OK with removing the capability check from pstore, right? > > Yeah, as long as there is comparable protections. File system permission protection is "0400": # ls -al /sys/firmware/acpi/tables/data total 0 drwxr-xr-x. 2 root root 0 Aug 28 14:13 . drwxr-xr-x. 4 root root 0 Aug 28 14:10 .. -r--------. 1 root root 32768 Aug 28 14:13 BERT -Tony ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-17 21:39 ` [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region Luck, Tony 2017-08-18 0:30 ` Alan Cox @ 2017-08-18 17:38 ` Punit Agrawal 2017-08-18 23:19 ` [PATCH v4] " Luck, Tony 1 sibling, 1 reply; 19+ messages in thread From: Punit Agrawal @ 2017-08-18 17:38 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel "Luck, Tony" <tony.luck@intel.com> writes: > From: Tony Luck <tony.luck@intel.com> > > The ACPI sysfs interface provides a way to read each ACPI table from > userspace via entries in /sys/firmware/acpi/tables/ > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory and users may want access to this > record. > > In an earlier age we might have used /dev/mem to retrieve this error > record, but many systems disable /dev/mem for security reasons. > > Extend this driver to provide read-only access to the data via a > file in a new directory /sys/firmware/acpi/tables/data/BERT > > Cc: Len Brown <lenb@kernel.org> > Cc: Boris Petkov <bp@suse.de> > Cc: Tyler Baicar <tbaicar@codeaurora.org> > Cc: Punit Agrawal <punit.agrawal@arm.com> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Tony Luck <tony.luck@intel.com> > > v4: (suggested by Punit Agrawal) extend the /sys/firmware/acpi > code to provide this functionality. File/directory names changed > to Rafael's suggestions > --- > drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c > index e414fabf7315..7c4eed0594d0 100644 > --- a/drivers/acpi/sysfs.c > +++ b/drivers/acpi/sysfs.c > @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444); > /* > * ACPI table sysfs I/F: > * /sys/firmware/acpi/tables/ > + * /sys/firmware/acpi/tables-data/ Typo: Should be "/sys/firmware/acpi/tables/data/" > * /sys/firmware/acpi/tables/dynamic/ > */ > > static LIST_HEAD(acpi_table_attr_list); > static struct kobject *tables_kobj; > +static struct kobject *tables_data_kobj; > static struct kobject *dynamic_tables_kobj; > static struct kobject *hotplug_kobj; > > @@ -325,6 +327,11 @@ struct acpi_table_attr { > struct list_head node; > }; > > +struct acpi_data_attr { > + struct bin_attribute attr; > + u64 addr; > +}; If all we are going to need is an address, it could also be stored in the "private" member of attr (similar to how it's done in pci-sysfs.c). But it goes against usage in the rest of the file so I am fine either ways. > + > static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj, > struct bin_attribute *bin_attr, char *buf, > loff_t offset, size_t count) > @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context) > return AE_OK; > } > > +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t offset, size_t count) > +{ > + struct acpi_data_attr *data_attr; > + void __iomem *base; > + ssize_t rc; > + > + data_attr = container_of(bin_attr, struct acpi_data_attr, attr); > + > + base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size); > + if (!base) > + return -ENOMEM; > + rc = memory_read_from_buffer(buf, count, &offset, base, > + data_attr->attr.size); > + acpi_os_unmap_memory(base, data_attr->attr.size); > + > + return rc; > +} > + > +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr) > +{ > + struct acpi_table_bert *bert = th; > + > + if (bert->header.length < sizeof(struct acpi_table_bert) || > + bert->region_length < sizeof(struct acpi_hest_generic_status)) { > + kfree(data_attr); > + return -EINVAL; > + } > + data_attr->addr = bert->address; > + data_attr->attr.size = bert->region_length; > + data_attr->attr.attr.name = "BERT"; > + > + return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr); > +} > + > +static struct acpi_data_obj { > + char *name; > + int (*fn)(void *, struct acpi_data_attr *); > +} acpi_data_objs[] = { > + { ACPI_SIG_BERT, acpi_bert_data_init }, > +}; > + > +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs) > + > +static int acpi_table_data_init(struct acpi_table_header *th) > +{ > + struct acpi_data_attr *data_attr; > + int i; > + > + for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) { > + if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) { > + data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL); > + if (!data_attr) > + return -ENOMEM; > + sysfs_attr_init(&data_attr->attr.attr); > + data_attr->attr.read = acpi_data_show; > + data_attr->attr.attr.mode = 0400; > + return acpi_data_objs[i].fn(th, data_attr); > + } > + } > + return 0; > +} > + > static int acpi_tables_sysfs_init(void) > { > struct acpi_table_attr *table_attr; > @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void) > if (!tables_kobj) > goto err; > > + tables_data_kobj = kobject_create_and_add("data", tables_kobj); > + if (!tables_data_kobj) > + goto err_tables_data; > + > dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj); > if (!dynamic_tables_kobj) > goto err_dynamic_tables; > @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void) > return ret; > } > list_add_tail(&table_attr->node, &acpi_table_attr_list); > + acpi_table_data_init(table_header); > } > > kobject_uevent(tables_kobj, KOBJ_ADD); > + kobject_uevent(tables_data_kobj, KOBJ_ADD); > kobject_uevent(dynamic_tables_kobj, KOBJ_ADD); > > return 0; > err_dynamic_tables: > + kobject_put(tables_data_kobj); > +err_tables_data: > kobject_put(tables_kobj); > err: > return -ENOMEM; I like how this fits into existing infrastructure. With the typo addressed - Acked-by: Punit Agrawal <punit.agrawal@arm.com> Thanks, Punit ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-18 17:38 ` Punit Agrawal @ 2017-08-18 23:19 ` Luck, Tony 2017-08-28 20:55 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Luck, Tony @ 2017-08-18 23:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tony Luck, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel, Punit Agrawal From: Tony Luck <tony.luck@intel.com> The ACPI sysfs interface provides a way to read each ACPI table from userspace via entries in /sys/firmware/acpi/tables/ The BERT table simply provides the size and address of the error record in BIOS reserved memory and users may want access to this record. In an earlier age we might have used /dev/mem to retrieve this error record, but many systems disable /dev/mem for security reasons. Extend this driver to provide read-only access to the data via a file in a new directory /sys/firmware/acpi/tables/data/BERT Cc: Len Brown <lenb@kernel.org> Cc: Boris Petkov <bp@suse.de> Cc: Tyler Baicar <tbaicar@codeaurora.org> Cc: linux-acpi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Acked-by: Punit Agrawal <punit.agrawal@arm.com> Signed-off-by: Tony Luck <tony.luck@intel.com> v4: fix typo reported by Punit --- drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c index e414fabf7315..faa1aa3ed0e1 100644 --- a/drivers/acpi/sysfs.c +++ b/drivers/acpi/sysfs.c @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444); /* * ACPI table sysfs I/F: * /sys/firmware/acpi/tables/ + * /sys/firmware/acpi/tables/data/ * /sys/firmware/acpi/tables/dynamic/ */ static LIST_HEAD(acpi_table_attr_list); static struct kobject *tables_kobj; +static struct kobject *tables_data_kobj; static struct kobject *dynamic_tables_kobj; static struct kobject *hotplug_kobj; @@ -325,6 +327,11 @@ struct acpi_table_attr { struct list_head node; }; +struct acpi_data_attr { + struct bin_attribute attr; + u64 addr; +}; + static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t offset, size_t count) @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context) return AE_OK; } +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t offset, size_t count) +{ + struct acpi_data_attr *data_attr; + void __iomem *base; + ssize_t rc; + + data_attr = container_of(bin_attr, struct acpi_data_attr, attr); + + base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size); + if (!base) + return -ENOMEM; + rc = memory_read_from_buffer(buf, count, &offset, base, + data_attr->attr.size); + acpi_os_unmap_memory(base, data_attr->attr.size); + + return rc; +} + +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr) +{ + struct acpi_table_bert *bert = th; + + if (bert->header.length < sizeof(struct acpi_table_bert) || + bert->region_length < sizeof(struct acpi_hest_generic_status)) { + kfree(data_attr); + return -EINVAL; + } + data_attr->addr = bert->address; + data_attr->attr.size = bert->region_length; + data_attr->attr.attr.name = "BERT"; + + return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr); +} + +static struct acpi_data_obj { + char *name; + int (*fn)(void *, struct acpi_data_attr *); +} acpi_data_objs[] = { + { ACPI_SIG_BERT, acpi_bert_data_init }, +}; + +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs) + +static int acpi_table_data_init(struct acpi_table_header *th) +{ + struct acpi_data_attr *data_attr; + int i; + + for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) { + if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) { + data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL); + if (!data_attr) + return -ENOMEM; + sysfs_attr_init(&data_attr->attr.attr); + data_attr->attr.read = acpi_data_show; + data_attr->attr.attr.mode = 0400; + return acpi_data_objs[i].fn(th, data_attr); + } + } + return 0; +} + static int acpi_tables_sysfs_init(void) { struct acpi_table_attr *table_attr; @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void) if (!tables_kobj) goto err; + tables_data_kobj = kobject_create_and_add("data", tables_kobj); + if (!tables_data_kobj) + goto err_tables_data; + dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj); if (!dynamic_tables_kobj) goto err_dynamic_tables; @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void) return ret; } list_add_tail(&table_attr->node, &acpi_table_attr_list); + acpi_table_data_init(table_header); } kobject_uevent(tables_kobj, KOBJ_ADD); + kobject_uevent(tables_data_kobj, KOBJ_ADD); kobject_uevent(dynamic_tables_kobj, KOBJ_ADD); return 0; err_dynamic_tables: + kobject_put(tables_data_kobj); +err_tables_data: kobject_put(tables_kobj); err: return -ENOMEM; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v4] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region 2017-08-18 23:19 ` [PATCH v4] " Luck, Tony @ 2017-08-28 20:55 ` Rafael J. Wysocki 0 siblings, 0 replies; 19+ messages in thread From: Rafael J. Wysocki @ 2017-08-28 20:55 UTC (permalink / raw) To: Luck, Tony Cc: Rafael J. Wysocki, Len Brown, Boris Petkov, Tyler Baicar, linux-acpi, linux-kernel, Punit Agrawal On Saturday, August 19, 2017 1:19:00 AM CEST Luck, Tony wrote: > From: Tony Luck <tony.luck@intel.com> > > The ACPI sysfs interface provides a way to read each ACPI table from > userspace via entries in /sys/firmware/acpi/tables/ > > The BERT table simply provides the size and address of the error > record in BIOS reserved memory and users may want access to this > record. > > In an earlier age we might have used /dev/mem to retrieve this error > record, but many systems disable /dev/mem for security reasons. > > Extend this driver to provide read-only access to the data via a > file in a new directory /sys/firmware/acpi/tables/data/BERT > > Cc: Len Brown <lenb@kernel.org> > Cc: Boris Petkov <bp@suse.de> > Cc: Tyler Baicar <tbaicar@codeaurora.org> > Cc: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Acked-by: Punit Agrawal <punit.agrawal@arm.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > > v4: fix typo reported by Punit > --- > drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c > index e414fabf7315..faa1aa3ed0e1 100644 > --- a/drivers/acpi/sysfs.c > +++ b/drivers/acpi/sysfs.c > @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444); > /* > * ACPI table sysfs I/F: > * /sys/firmware/acpi/tables/ > + * /sys/firmware/acpi/tables/data/ > * /sys/firmware/acpi/tables/dynamic/ > */ > > static LIST_HEAD(acpi_table_attr_list); > static struct kobject *tables_kobj; > +static struct kobject *tables_data_kobj; > static struct kobject *dynamic_tables_kobj; > static struct kobject *hotplug_kobj; > > @@ -325,6 +327,11 @@ struct acpi_table_attr { > struct list_head node; > }; > > +struct acpi_data_attr { > + struct bin_attribute attr; > + u64 addr; > +}; > + > static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj, > struct bin_attribute *bin_attr, char *buf, > loff_t offset, size_t count) > @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context) > return AE_OK; > } > > +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t offset, size_t count) > +{ > + struct acpi_data_attr *data_attr; > + void __iomem *base; > + ssize_t rc; > + > + data_attr = container_of(bin_attr, struct acpi_data_attr, attr); > + > + base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size); > + if (!base) > + return -ENOMEM; > + rc = memory_read_from_buffer(buf, count, &offset, base, > + data_attr->attr.size); > + acpi_os_unmap_memory(base, data_attr->attr.size); > + > + return rc; > +} > + > +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr) > +{ > + struct acpi_table_bert *bert = th; > + > + if (bert->header.length < sizeof(struct acpi_table_bert) || > + bert->region_length < sizeof(struct acpi_hest_generic_status)) { > + kfree(data_attr); > + return -EINVAL; > + } > + data_attr->addr = bert->address; > + data_attr->attr.size = bert->region_length; > + data_attr->attr.attr.name = "BERT"; > + > + return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr); > +} > + > +static struct acpi_data_obj { > + char *name; > + int (*fn)(void *, struct acpi_data_attr *); > +} acpi_data_objs[] = { > + { ACPI_SIG_BERT, acpi_bert_data_init }, > +}; > + > +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs) > + > +static int acpi_table_data_init(struct acpi_table_header *th) > +{ > + struct acpi_data_attr *data_attr; > + int i; > + > + for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) { > + if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) { > + data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL); > + if (!data_attr) > + return -ENOMEM; > + sysfs_attr_init(&data_attr->attr.attr); > + data_attr->attr.read = acpi_data_show; > + data_attr->attr.attr.mode = 0400; > + return acpi_data_objs[i].fn(th, data_attr); > + } > + } > + return 0; > +} > + > static int acpi_tables_sysfs_init(void) > { > struct acpi_table_attr *table_attr; > @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void) > if (!tables_kobj) > goto err; > > + tables_data_kobj = kobject_create_and_add("data", tables_kobj); > + if (!tables_data_kobj) > + goto err_tables_data; > + > dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj); > if (!dynamic_tables_kobj) > goto err_dynamic_tables; > @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void) > return ret; > } > list_add_tail(&table_attr->node, &acpi_table_attr_list); > + acpi_table_data_init(table_header); > } > > kobject_uevent(tables_kobj, KOBJ_ADD); > + kobject_uevent(tables_data_kobj, KOBJ_ADD); > kobject_uevent(dynamic_tables_kobj, KOBJ_ADD); > > return 0; > err_dynamic_tables: > + kobject_put(tables_data_kobj); > +err_tables_data: > kobject_put(tables_kobj); > err: > return -ENOMEM; > Applied, thanks! ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-08-29 15:55 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-14 16:56 [PATCH] ACPI/APEI: Add BERT data driver Luck, Tony 2017-08-15 10:22 ` Punit Agrawal 2017-08-15 21:15 ` Luck, Tony 2017-08-16 13:14 ` Punit Agrawal 2017-08-16 15:22 ` Luck, Tony 2017-08-17 10:25 ` Punit Agrawal 2017-08-17 17:49 ` Luck, Tony 2017-08-17 19:28 ` Rafael J. Wysocki 2017-08-17 20:29 ` Luck, Tony 2017-08-17 20:41 ` Rafael J. Wysocki 2017-08-17 21:39 ` [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region Luck, Tony 2017-08-18 0:30 ` Alan Cox 2017-08-18 2:12 ` Luck, Tony 2017-08-23 14:56 ` Luck, Tony 2017-08-29 0:10 ` Kees Cook 2017-08-29 15:55 ` Luck, Tony 2017-08-18 17:38 ` Punit Agrawal 2017-08-18 23:19 ` [PATCH v4] " Luck, Tony 2017-08-28 20:55 ` Rafael J. Wysocki
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).