Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Punit Agrawal <punit.agrawal@arm.com>]

"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