* [RFC PATCH] Export Runtime Configuration Interface table to sysfs
@ 2019-06-17 10:11 Narendra.K
2019-06-21 16:35 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Narendra.K @ 2019-06-17 10:11 UTC (permalink / raw)
To: linux-efi; +Cc: Narendra.K
From: Narendra K <Narendra.K@dell.com>
System firmware advertises the address of the 'Runtime
Configuration Interface table version 2 (RCI2)' via
an EFI Configuration Table entry. This code retrieves the RCI2
table from the address and exports it to sysfs as a binary
attribute 'rci2' under /sys/firmware/efi/tables directory.
The approach adopted is similar to the attribute 'DMI' under
/sys/firmware/dmi/tables.
RCI2 table contains BIOS HII in XML format and is used to populate
BIOS setup page in Dell EMC OpenManage Server Administrator tool.
The BIOS setup page contains BIOS tokens which can be configured.
Signed-off-by: Narendra K <Narendra.K@dell.com>
---
Hi, the patch is created on kernel version 5.2-rc4. It applies to
5.2-rc5 also. If the approach looks correct, I will resubmit with RFC
tag removed.
Documentation/ABI/testing/sysfs-firmware-efi | 9 ++
drivers/firmware/efi/Makefile | 2 +-
drivers/firmware/efi/efi.c | 20 ++-
drivers/firmware/efi/rci2_table.c | 148 +++++++++++++++++++
include/linux/efi.h | 7 +
5 files changed, 184 insertions(+), 2 deletions(-)
create mode 100644 drivers/firmware/efi/rci2_table.c
diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
index e794eac32a90..cb887b5e10cb 100644
--- a/Documentation/ABI/testing/sysfs-firmware-efi
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -28,3 +28,12 @@ Description: Displays the physical addresses of all EFI Configuration
versions are always printed first, i.e. ACPI20 comes
before ACPI.
Users: dmidecode
+
+What: /sys/firmware/efi/tables/rci2
+Date: June 2019
+Contact: Narendra K <Narendra.K@dell.com>, linux-bugs@dell.com
+Description: Displays the content of the Runtime Configuration Interface
+ Table version 2 on Dell EMC PowerEdge systems in binary format
+Users: It is used by Dell EMC OpenManage Server Administrator tool to
+ populate BIOS setup page.
+
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index d2d0d2030620..db07828ca1ed 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -11,7 +11,7 @@
KASAN_SANITIZE_runtime-wrappers.o := n
obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
-obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
+obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o rci2_table.o
obj-$(CONFIG_EFI) += capsule.o memmap.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_ESRT) += esrt.o
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 16b2137d117c..2fe114ff8149 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
.rng_seed = EFI_INVALID_TABLE_ADDR,
.tpm_log = EFI_INVALID_TABLE_ADDR,
.mem_reserve = EFI_INVALID_TABLE_ADDR,
+ .rci2 = EFI_INVALID_TABLE_ADDR,
};
EXPORT_SYMBOL(efi);
@@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
&efi.esrt,
&efi.properties_table,
&efi.mem_attr_table,
+ &efi.rci2,
};
struct mm_struct efi_mm = {
@@ -384,6 +386,9 @@ static int __init efisubsys_init(void)
goto err_remove_group;
}
+ if (efi_rci2_sysfs_init() != 0)
+ pr_debug("efi rci2: sysfs attribute creation under /sys/firmware/efi/ failed");
+
return 0;
err_remove_group:
@@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
{NULL_GUID, NULL, NULL},
};
+/* OEM Tables */
+static __initdata efi_config_table_type_t oem_tables[] = {
+ {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
+ {NULL_GUID, NULL, NULL},
+};
+
static __init int match_config_table(efi_guid_t *guid,
unsigned long table,
efi_config_table_type_t *table_types)
@@ -538,8 +549,10 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
table = ((efi_config_table_32_t *)tablep)->table;
}
- if (!match_config_table(&guid, table, common_tables))
+ if (!match_config_table(&guid, table, common_tables)) {
match_config_table(&guid, table, arch_tables);
+ match_config_table(&guid, table, oem_tables);
+ }
tablep += sz;
}
@@ -627,6 +640,11 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
}
}
+ if (efi.rci2 != EFI_INVALID_TABLE_ADDR)
+ efi_rci2_table_init();
+ else
+ pr_debug("EFI RCI2 table address not found\n");
+
return 0;
}
diff --git a/drivers/firmware/efi/rci2_table.c b/drivers/firmware/efi/rci2_table.c
new file mode 100644
index 000000000000..b18354d5b81e
--- /dev/null
+++ b/drivers/firmware/efi/rci2_table.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Export Runtime Configuration Interface Table Version 2 (RCI2)
+ * to sysfs
+ *
+ * Copyright (C) 2019 Dell Inc
+ * by Narendra K <Narendra.K@dell.com>
+ *
+ * System firmware advertises the address of the RCI2 Table via
+ * an EFI Configuration Table entry. This code retrieves the RCI2
+ * table from the address and exports it to sysfs as a binary
+ * attribute 'rci2' under /sys/firmware/efi/tables directory.
+ */
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/sysfs.h>
+#include <linux/efi.h>
+#include <linux/types.h>
+#include <linux/io.h>
+
+#define RCI_SIGNATURE "_RC_"
+
+struct rci2_table_global_hdr {
+ u16 type;
+ u16 resvd0;
+ u16 hdr_len;
+ u8 rci2_sig[4];
+ u16 resvd1;
+ u32 resvd2;
+ u32 resvd3;
+ u8 major_rev;
+ u8 minor_rev;
+ u16 num_of_structs;
+ u32 rci2_len;
+ u16 rci2_chksum;
+} __packed;
+
+static u8 *rci2_base;
+static u32 rci2_table_len;
+
+int __init efi_rci2_table_init(void)
+{
+ rci2_base = early_memremap(efi.rci2,
+ sizeof(struct rci2_table_global_hdr));
+ if (!rci2_base) {
+ pr_debug("RCI2 table init failed - could not map RCI2 table\n");
+ return -ENOMEM;
+ }
+
+ if (strncmp(rci2_base +
+ offsetof(struct rci2_table_global_hdr, rci2_sig),
+ RCI_SIGNATURE, 4)) {
+ memunmap(rci2_base);
+ pr_debug("RCI2 table init failed - incorrect signature\n");
+ return -ENODEV;
+ }
+
+ rci2_table_len = *(u32 *)(rci2_base +
+ offsetof(struct rci2_table_global_hdr,
+ rci2_len));
+
+ early_memunmap(rci2_base, sizeof(struct rci2_table_global_hdr));
+
+ return 0;
+}
+
+static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t count)
+{
+ memcpy(buf, attr->private + pos, count);
+ return count;
+}
+
+static BIN_ATTR(rci2, S_IRUSR, raw_table_read, NULL, 0);
+
+static u16 checksum(void)
+{
+ u8 len_is_odd = rci2_table_len % 2;
+ u32 chksum_len = rci2_table_len;
+ u16 *base = (u16 *)rci2_base;
+ u8 buf[2] = {0};
+ u32 offset = 0;
+ u16 chksum = 0;
+
+ if (len_is_odd)
+ chksum_len -= 1;
+
+ while (offset < chksum_len) {
+ chksum += *base;
+ offset += 2;
+ base++;
+ }
+
+ if (len_is_odd) {
+ buf[0] = *(u8 *)base;
+ chksum += *(u16 *)(buf);
+ }
+
+ return chksum;
+}
+
+int __init efi_rci2_sysfs_init(void)
+{
+
+ struct kobject *tables_kobj;
+ int ret = -ENOMEM;
+
+ if (!rci2_table_len)
+ goto err;
+
+ rci2_base = memremap(efi.rci2, rci2_table_len, MEMREMAP_WB);
+ if (!rci2_base) {
+ pr_debug("RCI2 table - could not map RCI2 table\n");
+ return -ENOMEM;
+ }
+
+ if (checksum() != 0) {
+ pr_debug("RCI2 table - incorrect checksum\n");
+ ret = -ENODEV;
+ goto err_unmap;
+ }
+
+ tables_kobj = kobject_create_and_add("tables", efi_kobj);
+ if (!tables_kobj) {
+ pr_debug("RCI2 table - tables_kobj creation failed\n");
+ goto err_unmap;
+ }
+
+ bin_attr_rci2.size = rci2_table_len;
+ bin_attr_rci2.private = rci2_base;
+ ret = sysfs_create_bin_file(tables_kobj, &bin_attr_rci2);
+ if (ret != 0) {
+ pr_debug("RCI2 table - rci2 sysfs bin file creation failed\n");
+ kobject_del(tables_kobj);
+ kobject_put(tables_kobj);
+ goto err_unmap;
+ }
+
+ return 0;
+
+ err_unmap:
+ memunmap(rci2_base);
+ err:
+ pr_debug("RCI2 table - sysfs initialization failed\n");
+ return ret;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6ebc2098cfe1..3a3f37ee5c48 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -691,6 +691,9 @@ void efi_native_runtime_setup(void);
#define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
#define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
+/* OEM GUIDs */
+#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
+
typedef struct {
efi_guid_t guid;
u64 table;
@@ -997,6 +1000,7 @@ extern struct efi {
unsigned long rng_seed; /* UEFI firmware random seed */
unsigned long tpm_log; /* TPM2 Event Log table */
unsigned long mem_reserve; /* Linux EFI memreserve table */
+ unsigned long rci2; /* Dell EMC EFI RCI2 Table */
efi_get_time_t *get_time;
efi_set_time_t *set_time;
efi_get_wakeup_time_t *get_wakeup_time;
@@ -1712,6 +1716,9 @@ struct linux_efi_tpm_eventlog {
extern int efi_tpm_eventlog_init(void);
+extern int efi_rci2_table_init(void);
+extern int efi_rci2_sysfs_init(void);
+
/*
* efi_runtime_service() function identifiers.
* "NONE" is used by efi_recover_from_page_fault() to check if the page
--
2.18.1
With regards,
Narendra K
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
2019-06-17 10:11 [RFC PATCH] Export Runtime Configuration Interface table to sysfs Narendra.K
@ 2019-06-21 16:35 ` Ard Biesheuvel
2019-06-25 12:10 ` Narendra.K
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2019-06-21 16:35 UTC (permalink / raw)
To: Narendra.K, Peter Jones; +Cc: linux-efi
(+ Peter)
On Mon, 17 Jun 2019 at 12:11, <Narendra.K@dell.com> wrote:
>
> From: Narendra K <Narendra.K@dell.com>
>
> System firmware advertises the address of the 'Runtime
> Configuration Interface table version 2 (RCI2)' via
> an EFI Configuration Table entry. This code retrieves the RCI2
> table from the address and exports it to sysfs as a binary
> attribute 'rci2' under /sys/firmware/efi/tables directory.
> The approach adopted is similar to the attribute 'DMI' under
> /sys/firmware/dmi/tables.
>
> RCI2 table contains BIOS HII in XML format and is used to populate
> BIOS setup page in Dell EMC OpenManage Server Administrator tool.
> The BIOS setup page contains BIOS tokens which can be configured.
>
> Signed-off-by: Narendra K <Narendra.K@dell.com>
> ---
> Hi, the patch is created on kernel version 5.2-rc4. It applies to
> 5.2-rc5 also. If the approach looks correct, I will resubmit with RFC
> tag removed.
>
Unfortunately, we cannot implement a generic interface for dumping
config tables, since there is no generic method to record the length.
So I don't have any problems with doing it this way.
I do have some comments, though.
First of all, do you know which memory type is used for this table? (more below)
> Documentation/ABI/testing/sysfs-firmware-efi | 9 ++
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/efi.c | 20 ++-
> drivers/firmware/efi/rci2_table.c | 148 +++++++++++++++++++
> include/linux/efi.h | 7 +
> 5 files changed, 184 insertions(+), 2 deletions(-)
> create mode 100644 drivers/firmware/efi/rci2_table.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> index e794eac32a90..cb887b5e10cb 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -28,3 +28,12 @@ Description: Displays the physical addresses of all EFI Configuration
> versions are always printed first, i.e. ACPI20 comes
> before ACPI.
> Users: dmidecode
> +
> +What: /sys/firmware/efi/tables/rci2
> +Date: June 2019
> +Contact: Narendra K <Narendra.K@dell.com>, linux-bugs@dell.com
> +Description: Displays the content of the Runtime Configuration Interface
> + Table version 2 on Dell EMC PowerEdge systems in binary format
> +Users: It is used by Dell EMC OpenManage Server Administrator tool to
> + populate BIOS setup page.
> +
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index d2d0d2030620..db07828ca1ed 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -11,7 +11,7 @@
> KASAN_SANITIZE_runtime-wrappers.o := n
>
> obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
> -obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
> +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o rci2_table.o
> obj-$(CONFIG_EFI) += capsule.o memmap.o
> obj-$(CONFIG_EFI_VARS) += efivars.o
> obj-$(CONFIG_EFI_ESRT) += esrt.o
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 16b2137d117c..2fe114ff8149 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> .rng_seed = EFI_INVALID_TABLE_ADDR,
> .tpm_log = EFI_INVALID_TABLE_ADDR,
> .mem_reserve = EFI_INVALID_TABLE_ADDR,
> + .rci2 = EFI_INVALID_TABLE_ADDR,
Does this really need to live in the efi struct?
> };
> EXPORT_SYMBOL(efi);
>
> @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> &efi.esrt,
> &efi.properties_table,
> &efi.mem_attr_table,
> + &efi.rci2,
> };
>
AFAICT, this table is only used by memremap_is_efi_data() to decide
whether a page should be map as unencrypted, and if the address is in
boot services data or runtime services data, the test will already
success, regardless of whether it appears in this enumeration.
> struct mm_struct efi_mm = {
> @@ -384,6 +386,9 @@ static int __init efisubsys_init(void)
> goto err_remove_group;
> }
>
> + if (efi_rci2_sysfs_init() != 0)
> + pr_debug("efi rci2: sysfs attribute creation under /sys/firmware/efi/ failed");
> +
> return 0;
>
> err_remove_group:
> @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
> {NULL_GUID, NULL, NULL},
> };
>
> +/* OEM Tables */
> +static __initdata efi_config_table_type_t oem_tables[] = {
> + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
Please drop the string. We don't have to print the presence of this
table in the bootlog since it has no significance to the OS itself.
> + {NULL_GUID, NULL, NULL},
> +};
> +
Do we really need a separate oem_tables[] array?
> static __init int match_config_table(efi_guid_t *guid,
> unsigned long table,
> efi_config_table_type_t *table_types)
> @@ -538,8 +549,10 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> table = ((efi_config_table_32_t *)tablep)->table;
> }
>
> - if (!match_config_table(&guid, table, common_tables))
> + if (!match_config_table(&guid, table, common_tables)) {
> match_config_table(&guid, table, arch_tables);
> + match_config_table(&guid, table, oem_tables);
> + }
>
> tablep += sz;
> }
> @@ -627,6 +640,11 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> }
> }
>
> + if (efi.rci2 != EFI_INVALID_TABLE_ADDR)
> + efi_rci2_table_init();
> + else
> + pr_debug("EFI RCI2 table address not found\n");
> +
> return 0;
> }
>
> diff --git a/drivers/firmware/efi/rci2_table.c b/drivers/firmware/efi/rci2_table.c
> new file mode 100644
> index 000000000000..b18354d5b81e
> --- /dev/null
> +++ b/drivers/firmware/efi/rci2_table.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Export Runtime Configuration Interface Table Version 2 (RCI2)
> + * to sysfs
> + *
> + * Copyright (C) 2019 Dell Inc
> + * by Narendra K <Narendra.K@dell.com>
> + *
> + * System firmware advertises the address of the RCI2 Table via
> + * an EFI Configuration Table entry. This code retrieves the RCI2
> + * table from the address and exports it to sysfs as a binary
> + * attribute 'rci2' under /sys/firmware/efi/tables directory.
> + */
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +#include <linux/sysfs.h>
> +#include <linux/efi.h>
> +#include <linux/types.h>
> +#include <linux/io.h>
> +
> +#define RCI_SIGNATURE "_RC_"
> +
> +struct rci2_table_global_hdr {
> + u16 type;
> + u16 resvd0;
> + u16 hdr_len;
> + u8 rci2_sig[4];
> + u16 resvd1;
> + u32 resvd2;
> + u32 resvd3;
> + u8 major_rev;
> + u8 minor_rev;
> + u16 num_of_structs;
> + u32 rci2_len;
> + u16 rci2_chksum;
> +} __packed;
> +
> +static u8 *rci2_base;
> +static u32 rci2_table_len;
> +
> +int __init efi_rci2_table_init(void)
> +{
> + rci2_base = early_memremap(efi.rci2,
> + sizeof(struct rci2_table_global_hdr));
> + if (!rci2_base) {
> + pr_debug("RCI2 table init failed - could not map RCI2 table\n");
> + return -ENOMEM;
> + }
> +
Do we really need to do this early? And is it guaranteed that the
memory will not be given to the OS for general allocation?
> + if (strncmp(rci2_base +
> + offsetof(struct rci2_table_global_hdr, rci2_sig),
> + RCI_SIGNATURE, 4)) {
> + memunmap(rci2_base);
> + pr_debug("RCI2 table init failed - incorrect signature\n");
> + return -ENODEV;
> + }
> +
> + rci2_table_len = *(u32 *)(rci2_base +
> + offsetof(struct rci2_table_global_hdr,
> + rci2_len));
> +
> + early_memunmap(rci2_base, sizeof(struct rci2_table_global_hdr));
> +
> + return 0;
> +}
> +
> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t count)
> +{
> + memcpy(buf, attr->private + pos, count);
> + return count;
> +}
> +
> +static BIN_ATTR(rci2, S_IRUSR, raw_table_read, NULL, 0);
> +
> +static u16 checksum(void)
> +{
> + u8 len_is_odd = rci2_table_len % 2;
> + u32 chksum_len = rci2_table_len;
> + u16 *base = (u16 *)rci2_base;
> + u8 buf[2] = {0};
> + u32 offset = 0;
> + u16 chksum = 0;
> +
> + if (len_is_odd)
> + chksum_len -= 1;
> +
> + while (offset < chksum_len) {
> + chksum += *base;
> + offset += 2;
> + base++;
> + }
> +
> + if (len_is_odd) {
> + buf[0] = *(u8 *)base;
> + chksum += *(u16 *)(buf);
> + }
> +
> + return chksum;
> +}
> +
What is this random checksum function? Which algorithm does it
implement, and did you check whether we already have a library
function for it?
> +int __init efi_rci2_sysfs_init(void)
> +{
> +
> + struct kobject *tables_kobj;
> + int ret = -ENOMEM;
> +
> + if (!rci2_table_len)
> + goto err;
> +
> + rci2_base = memremap(efi.rci2, rci2_table_len, MEMREMAP_WB);
> + if (!rci2_base) {
> + pr_debug("RCI2 table - could not map RCI2 table\n");
> + return -ENOMEM;
> + }
> +
> + if (checksum() != 0) {
> + pr_debug("RCI2 table - incorrect checksum\n");
> + ret = -ENODEV;
> + goto err_unmap;
> + }
> +
> + tables_kobj = kobject_create_and_add("tables", efi_kobj);
> + if (!tables_kobj) {
> + pr_debug("RCI2 table - tables_kobj creation failed\n");
> + goto err_unmap;
> + }
> +
> + bin_attr_rci2.size = rci2_table_len;
> + bin_attr_rci2.private = rci2_base;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_rci2);
> + if (ret != 0) {
> + pr_debug("RCI2 table - rci2 sysfs bin file creation failed\n");
> + kobject_del(tables_kobj);
> + kobject_put(tables_kobj);
> + goto err_unmap;
> + }
> +
> + return 0;
> +
> + err_unmap:
> + memunmap(rci2_base);
> + err:
> + pr_debug("RCI2 table - sysfs initialization failed\n");
> + return ret;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 6ebc2098cfe1..3a3f37ee5c48 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -691,6 +691,9 @@ void efi_native_runtime_setup(void);
> #define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> #define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
>
> +/* OEM GUIDs */
> +#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
> +
> typedef struct {
> efi_guid_t guid;
> u64 table;
> @@ -997,6 +1000,7 @@ extern struct efi {
> unsigned long rng_seed; /* UEFI firmware random seed */
> unsigned long tpm_log; /* TPM2 Event Log table */
> unsigned long mem_reserve; /* Linux EFI memreserve table */
> + unsigned long rci2; /* Dell EMC EFI RCI2 Table */
> efi_get_time_t *get_time;
> efi_set_time_t *set_time;
> efi_get_wakeup_time_t *get_wakeup_time;
> @@ -1712,6 +1716,9 @@ struct linux_efi_tpm_eventlog {
>
> extern int efi_tpm_eventlog_init(void);
>
> +extern int efi_rci2_table_init(void);
> +extern int efi_rci2_sysfs_init(void);
> +
> /*
> * efi_runtime_service() function identifiers.
> * "NONE" is used by efi_recover_from_page_fault() to check if the page
> --
> 2.18.1
>
> With regards,
> Narendra K
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
2019-06-21 16:35 ` Ard Biesheuvel
@ 2019-06-25 12:10 ` Narendra.K
2019-06-25 14:21 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Narendra.K @ 2019-06-25 12:10 UTC (permalink / raw)
To: ard.biesheuvel; +Cc: pjones, linux-efi
On Fri, Jun 21, 2019 at 06:35:42PM +0200, Ard Biesheuvel wrote:
> (+ Peter)
>
> On Mon, 17 Jun 2019 at 12:11, <Narendra.K@dell.com> wrote:
> >
> > From: Narendra K <Narendra.K@dell.com>
> >
> > System firmware advertises the address of the 'Runtime
> > Configuration Interface table version 2 (RCI2)' via
> > an EFI Configuration Table entry. This code retrieves the RCI2
> > table from the address and exports it to sysfs as a binary
> > attribute 'rci2' under /sys/firmware/efi/tables directory.
> > The approach adopted is similar to the attribute 'DMI' under
> > /sys/firmware/dmi/tables.
[...]
> > ---
> > Hi, the patch is created on kernel version 5.2-rc4. It applies to
> > 5.2-rc5 also. If the approach looks correct, I will resubmit with RFC
> > tag removed.
> >
Hi Ard,
Thank you for the review comments.
> Unfortunately, we cannot implement a generic interface for dumping
> config tables, since there is no generic method to record the length.
> So I don't have any problems with doing it this way.
>
> I do have some comments, though.
>
> First of all, do you know which memory type is used for this table? (more below)
The memory type used for the table is EfiReservedMemoryType.
[...]
> > +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o rci2_table.o
> > obj-$(CONFIG_EFI) += capsule.o memmap.o
> > obj-$(CONFIG_EFI_VARS) += efivars.o
> > obj-$(CONFIG_EFI_ESRT) += esrt.o
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 16b2137d117c..2fe114ff8149 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> > .rng_seed = EFI_INVALID_TABLE_ADDR,
> > .tpm_log = EFI_INVALID_TABLE_ADDR,
> > .mem_reserve = EFI_INVALID_TABLE_ADDR,
> > + .rci2 = EFI_INVALID_TABLE_ADDR,
>
> Does this really need to live in the efi struct?
It probably need not be part of struct efi. We could define a struct of
type 'efi_config_table_type_t' in the rci2_table.c. Did you have a
similar idea in mind ? If yes, I will modify and test this idea.
>
> > };
> > EXPORT_SYMBOL(efi);
> >
> > @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> > &efi.esrt,
> > &efi.properties_table,
> > &efi.mem_attr_table,
> > + &efi.rci2,
> > };
> >
>
> AFAICT, this table is only used by memremap_is_efi_data() to decide
> whether a page should be map as unencrypted, and if the address is in
> boot services data or runtime services data, the test will already
> success, regardless of whether it appears in this enumeration.
Yes. Before 'memremap_is_efi_data()' checks if the memory type is boot
services data for runtime services data, it checks if the address is a
'table' address in 'efi_is_table_address'. I added it because of this
check. Since the memory type used for the table is efi reserved type, we
need to add the table address to 'efi_tables' array so that it could be
checked in 'efi_is_table_address'. Please share your thought on this.
>
> > struct mm_struct efi_mm = {
> > @@ -384,6 +386,9 @@ static int __init efisubsys_init(void)
> > goto err_remove_group;
> > }
> >
> > + if (efi_rci2_sysfs_init() != 0)
> > + pr_debug("efi rci2: sysfs attribute creation under /sys/firmware/efi/ failed");
> > +
> > return 0;
> >
> > err_remove_group:
> > @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
> > {NULL_GUID, NULL, NULL},
> > };
> >
> > +/* OEM Tables */
> > +static __initdata efi_config_table_type_t oem_tables[] = {
> > + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
>
> Please drop the string. We don't have to print the presence of this
> table in the bootlog since it has no significance to the OS itself.
Okay. I will drop this in the next version of the patch.
>
> > + {NULL_GUID, NULL, NULL},
> > +};
> > +
>
> Do we really need a separate oem_tables[] array?
The RCI2 table did not seem to be part of the group of common tables
such as SMBIOS and ACPI. To indicate this, I created a separate array.
It seems like it is not required. Having the array allows to leverage
the table matching code in 'match_config_table' function. Would you prefer
to have this entry added to the 'common_tables' array ?
[...]
> > +
> > +int __init efi_rci2_table_init(void)
> > +{
> > + rci2_base = early_memremap(efi.rci2,
> > + sizeof(struct rci2_table_global_hdr));
> > + if (!rci2_base) {
> > + pr_debug("RCI2 table init failed - could not map RCI2 table\n");
> > + return -ENOMEM;
> > + }
> > +
>
> Do we really need to do this early? And is it guaranteed that the
> memory will not be given to the OS for general allocation?
As the memory type is efi reserved type, it is guaranteed that the
memory will not be give to OS for general allocation. It is mapped here
to determine the size of the table and unmapped in the same function.
Would you prefer to merge this function into 'efi_rci2_sysfs_init' function ?
>
>
> > + if (strncmp(rci2_base +
> > + offsetof(struct rci2_table_global_hdr, rci2_sig),
> > + RCI_SIGNATURE, 4)) {
> > + memunmap(rci2_base);
> > + pr_debug("RCI2 table init failed - incorrect signature\n");
> > + return -ENODEV;
> > + }
> > +
> > + rci2_table_len = *(u32 *)(rci2_base +
> > + offsetof(struct rci2_table_global_hdr,
> > + rci2_len));
> > +
> > + early_memunmap(rci2_base, sizeof(struct rci2_table_global_hdr));
> > +
> > + return 0;
> > +}
[...]
> > +
> > +static u16 checksum(void)
> > +{
> > + u8 len_is_odd = rci2_table_len % 2;
> > + u32 chksum_len = rci2_table_len;
> > + u16 *base = (u16 *)rci2_base;
> > + u8 buf[2] = {0};
> > + u32 offset = 0;
> > + u16 chksum = 0;
> > +
> > + if (len_is_odd)
> > + chksum_len -= 1;
> > +
> > + while (offset < chksum_len) {
> > + chksum += *base;
> > + offset += 2;
> > + base++;
> > + }
> > +
> > + if (len_is_odd) {
> > + buf[0] = *(u8 *)base;
> > + chksum += *(u16 *)(buf);
> > + }
> > +
> > + return chksum;
> > +}
> > +
>
> What is this random checksum function? Which algorithm does it
> implement, and did you check whether we already have a library
> function for it?
It does not implement any algorithm. The 16 bit sum of rci2_chksum field in the
struct rci2_table_global_hdr and rest of bytes of the table needs to be zero.
If length of the table is odd, a single byte with value zero needs to be added
at the end for the purpose of checksum calculation.
I checked in the library and did not find a function implementing
similar functionality. Please let me know if I missed any detail here.
>
>
> > +int __init efi_rci2_sysfs_init(void)
> > +{
> > +
> > + struct kobject *tables_kobj;
> > + int ret = -ENOMEM;
> > +
> > + if (!rci2_table_len)
> > + goto err;
> > +
> > + rci2_base = memremap(efi.rci2, rci2_table_len, MEMREMAP_WB);
> > + if (!rci2_base) {
> > + pr_debug("RCI2 table - could not map RCI2 table\n");
> > + return -ENOMEM;
> > + }
> > +
> > + if (checksum() != 0) {
> > + pr_debug("RCI2 table - incorrect checksum\n");
> > + ret = -ENODEV;
> > + goto err_unmap;
> > + }
> > +
> > + tables_kobj = kobject_create_and_add("tables", efi_kobj);
> > + if (!tables_kobj) {
> > + pr_debug("RCI2 table - tables_kobj creation failed\n");
> > + goto err_unmap;
> > + }
> > +
> > + bin_attr_rci2.size = rci2_table_len;
> > + bin_attr_rci2.private = rci2_base;
> > + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_rci2);
> > + if (ret != 0) {
> > + pr_debug("RCI2 table - rci2 sysfs bin file creation failed\n");
> > + kobject_del(tables_kobj);
> > + kobject_put(tables_kobj);
> > + goto err_unmap;
> > + }
> > +
> > + return 0;
> > +
> > + err_unmap:
> > + memunmap(rci2_base);
> > + err:
> > + pr_debug("RCI2 table - sysfs initialization failed\n");
> > + return ret;
> > +}
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 6ebc2098cfe1..3a3f37ee5c48 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -691,6 +691,9 @@ void efi_native_runtime_setup(void);
> > #define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> > #define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
> >
> > +/* OEM GUIDs */
> > +#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
> > +
> > typedef struct {
> > efi_guid_t guid;
> > u64 table;
> > @@ -997,6 +1000,7 @@ extern struct efi {
> > unsigned long rng_seed; /* UEFI firmware random seed */
> > unsigned long tpm_log; /* TPM2 Event Log table */
> > unsigned long mem_reserve; /* Linux EFI memreserve table */
> > + unsigned long rci2; /* Dell EMC EFI RCI2 Table */
> > efi_get_time_t *get_time;
> > efi_set_time_t *set_time;
> > efi_get_wakeup_time_t *get_wakeup_time;
> > @@ -1712,6 +1716,9 @@ struct linux_efi_tpm_eventlog {
> >
> > extern int efi_tpm_eventlog_init(void);
> >
> > +extern int efi_rci2_table_init(void);
> > +extern int efi_rci2_sysfs_init(void);
> > +
> > /*
> > * efi_runtime_service() function identifiers.
> > * "NONE" is used by efi_recover_from_page_fault() to check if the page
> > --
> > 2.18.1
> >
> > With regards,
> > Narendra K
--
With regards,
Narendra K
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
2019-06-25 12:10 ` Narendra.K
@ 2019-06-25 14:21 ` Ard Biesheuvel
2019-06-26 12:17 ` Narendra.K
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2019-06-25 14:21 UTC (permalink / raw)
To: Narendra.K; +Cc: Peter Jones, linux-efi
On Tue, 25 Jun 2019 at 14:10, <Narendra.K@dell.com> wrote:
>
> On Fri, Jun 21, 2019 at 06:35:42PM +0200, Ard Biesheuvel wrote:
> > (+ Peter)
> >
> > On Mon, 17 Jun 2019 at 12:11, <Narendra.K@dell.com> wrote:
> > >
> > > From: Narendra K <Narendra.K@dell.com>
> > >
> > > System firmware advertises the address of the 'Runtime
> > > Configuration Interface table version 2 (RCI2)' via
> > > an EFI Configuration Table entry. This code retrieves the RCI2
> > > table from the address and exports it to sysfs as a binary
> > > attribute 'rci2' under /sys/firmware/efi/tables directory.
> > > The approach adopted is similar to the attribute 'DMI' under
> > > /sys/firmware/dmi/tables.
> [...]
> > > ---
> > > Hi, the patch is created on kernel version 5.2-rc4. It applies to
> > > 5.2-rc5 also. If the approach looks correct, I will resubmit with RFC
> > > tag removed.
> > >
>
> Hi Ard,
>
> Thank you for the review comments.
>
> > Unfortunately, we cannot implement a generic interface for dumping
> > config tables, since there is no generic method to record the length.
> > So I don't have any problems with doing it this way.
> >
> > I do have some comments, though.
> >
> > First of all, do you know which memory type is used for this table? (more below)
>
> The memory type used for the table is EfiReservedMemoryType.
>
OK
> [...]
> > > +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o rci2_table.o
> > > obj-$(CONFIG_EFI) += capsule.o memmap.o
> > > obj-$(CONFIG_EFI_VARS) += efivars.o
> > > obj-$(CONFIG_EFI_ESRT) += esrt.o
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 16b2137d117c..2fe114ff8149 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> > > .rng_seed = EFI_INVALID_TABLE_ADDR,
> > > .tpm_log = EFI_INVALID_TABLE_ADDR,
> > > .mem_reserve = EFI_INVALID_TABLE_ADDR,
> > > + .rci2 = EFI_INVALID_TABLE_ADDR,
> >
> > Does this really need to live in the efi struct?
>
> It probably need not be part of struct efi. We could define a struct of
> type 'efi_config_table_type_t' in the rci2_table.c. Did you have a
> similar idea in mind ? If yes, I will modify and test this idea.
>
Yes, I'd like to start keeping these things separate.
I pushed a branch here
https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
that changes the way uv_systab is handled, and moves it into arch/x86.
Please follow that pattern instead.
> >
> > > };
> > > EXPORT_SYMBOL(efi);
> > >
> > > @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> > > &efi.esrt,
> > > &efi.properties_table,
> > > &efi.mem_attr_table,
> > > + &efi.rci2,
> > > };
> > >
> >
> > AFAICT, this table is only used by memremap_is_efi_data() to decide
> > whether a page should be map as unencrypted, and if the address is in
> > boot services data or runtime services data, the test will already
> > success, regardless of whether it appears in this enumeration.
>
> Yes. Before 'memremap_is_efi_data()' checks if the memory type is boot
> services data for runtime services data, it checks if the address is a
> 'table' address in 'efi_is_table_address'. I added it because of this
> check. Since the memory type used for the table is efi reserved type, we
> need to add the table address to 'efi_tables' array so that it could be
> checked in 'efi_is_table_address'. Please share your thought on this.
>
OK. My branch ^^^ moves this into arch/x86 as well, please add it there
> >
> > > struct mm_struct efi_mm = {
> > > @@ -384,6 +386,9 @@ static int __init efisubsys_init(void)
> > > goto err_remove_group;
> > > }
> > >
> > > + if (efi_rci2_sysfs_init() != 0)
> > > + pr_debug("efi rci2: sysfs attribute creation under /sys/firmware/efi/ failed");
> > > +
> > > return 0;
> > >
> > > err_remove_group:
> > > @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
> > > {NULL_GUID, NULL, NULL},
> > > };
> > >
> > > +/* OEM Tables */
> > > +static __initdata efi_config_table_type_t oem_tables[] = {
> > > + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
> >
> > Please drop the string. We don't have to print the presence of this
> > table in the bootlog since it has no significance to the OS itself.
>
> Okay. I will drop this in the next version of the patch.
>
> >
> > > + {NULL_GUID, NULL, NULL},
> > > +};
> > > +
> >
> > Do we really need a separate oem_tables[] array?
>
> The RCI2 table did not seem to be part of the group of common tables
> such as SMBIOS and ACPI. To indicate this, I created a separate array.
> It seems like it is not required. Having the array allows to leverage
> the table matching code in 'match_config_table' function. Would you prefer
> to have this entry added to the 'common_tables' array ?
>
Please add it to the arch_tables array in arch/x86 (if my assumption
is correct that this is x86-only)
> [...]
> > > +
> > > +int __init efi_rci2_table_init(void)
> > > +{
> > > + rci2_base = early_memremap(efi.rci2,
> > > + sizeof(struct rci2_table_global_hdr));
> > > + if (!rci2_base) {
> > > + pr_debug("RCI2 table init failed - could not map RCI2 table\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> >
> > Do we really need to do this early? And is it guaranteed that the
> > memory will not be given to the OS for general allocation?
>
> As the memory type is efi reserved type, it is guaranteed that the
> memory will not be give to OS for general allocation. It is mapped here
> to determine the size of the table and unmapped in the same function.
>
> Would you prefer to merge this function into 'efi_rci2_sysfs_init' function ?
>
Yes. Only user space needs to access this, so we can defer this to
later, when the normal memremap() functions are available.
> >
> >
> > > + if (strncmp(rci2_base +
> > > + offsetof(struct rci2_table_global_hdr, rci2_sig),
> > > + RCI_SIGNATURE, 4)) {
> > > + memunmap(rci2_base);
> > > + pr_debug("RCI2 table init failed - incorrect signature\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + rci2_table_len = *(u32 *)(rci2_base +
> > > + offsetof(struct rci2_table_global_hdr,
> > > + rci2_len));
> > > +
> > > + early_memunmap(rci2_base, sizeof(struct rci2_table_global_hdr));
> > > +
> > > + return 0;
> > > +}
>
> [...]
>
> > > +
> > > +static u16 checksum(void)
> > > +{
> > > + u8 len_is_odd = rci2_table_len % 2;
> > > + u32 chksum_len = rci2_table_len;
> > > + u16 *base = (u16 *)rci2_base;
> > > + u8 buf[2] = {0};
> > > + u32 offset = 0;
> > > + u16 chksum = 0;
> > > +
> > > + if (len_is_odd)
> > > + chksum_len -= 1;
> > > +
> > > + while (offset < chksum_len) {
> > > + chksum += *base;
> > > + offset += 2;
> > > + base++;
> > > + }
> > > +
> > > + if (len_is_odd) {
> > > + buf[0] = *(u8 *)base;
> > > + chksum += *(u16 *)(buf);
> > > + }
> > > +
> > > + return chksum;
> > > +}
> > > +
> >
> > What is this random checksum function? Which algorithm does it
> > implement, and did you check whether we already have a library
> > function for it?
>
> It does not implement any algorithm. The 16 bit sum of rci2_chksum field in the
> struct rci2_table_global_hdr and rest of bytes of the table needs to be zero.
> If length of the table is odd, a single byte with value zero needs to be added
> at the end for the purpose of checksum calculation.
>
> I checked in the library and did not find a function implementing
> similar functionality. Please let me know if I missed any detail here.
>
OK.
> >
> >
> > > +int __init efi_rci2_sysfs_init(void)
> > > +{
> > > +
> > > + struct kobject *tables_kobj;
> > > + int ret = -ENOMEM;
> > > +
> > > + if (!rci2_table_len)
> > > + goto err;
> > > +
> > > + rci2_base = memremap(efi.rci2, rci2_table_len, MEMREMAP_WB);
> > > + if (!rci2_base) {
> > > + pr_debug("RCI2 table - could not map RCI2 table\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + if (checksum() != 0) {
> > > + pr_debug("RCI2 table - incorrect checksum\n");
> > > + ret = -ENODEV;
> > > + goto err_unmap;
> > > + }
> > > +
> > > + tables_kobj = kobject_create_and_add("tables", efi_kobj);
> > > + if (!tables_kobj) {
> > > + pr_debug("RCI2 table - tables_kobj creation failed\n");
> > > + goto err_unmap;
> > > + }
> > > +
> > > + bin_attr_rci2.size = rci2_table_len;
> > > + bin_attr_rci2.private = rci2_base;
> > > + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_rci2);
> > > + if (ret != 0) {
> > > + pr_debug("RCI2 table - rci2 sysfs bin file creation failed\n");
> > > + kobject_del(tables_kobj);
> > > + kobject_put(tables_kobj);
> > > + goto err_unmap;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > + err_unmap:
> > > + memunmap(rci2_base);
> > > + err:
> > > + pr_debug("RCI2 table - sysfs initialization failed\n");
> > > + return ret;
> > > +}
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 6ebc2098cfe1..3a3f37ee5c48 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -691,6 +691,9 @@ void efi_native_runtime_setup(void);
> > > #define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> > > #define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
> > >
> > > +/* OEM GUIDs */
> > > +#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
> > > +
> > > typedef struct {
> > > efi_guid_t guid;
> > > u64 table;
> > > @@ -997,6 +1000,7 @@ extern struct efi {
> > > unsigned long rng_seed; /* UEFI firmware random seed */
> > > unsigned long tpm_log; /* TPM2 Event Log table */
> > > unsigned long mem_reserve; /* Linux EFI memreserve table */
> > > + unsigned long rci2; /* Dell EMC EFI RCI2 Table */
> > > efi_get_time_t *get_time;
> > > efi_set_time_t *set_time;
> > > efi_get_wakeup_time_t *get_wakeup_time;
> > > @@ -1712,6 +1716,9 @@ struct linux_efi_tpm_eventlog {
> > >
> > > extern int efi_tpm_eventlog_init(void);
> > >
> > > +extern int efi_rci2_table_init(void);
> > > +extern int efi_rci2_sysfs_init(void);
> > > +
> > > /*
> > > * efi_runtime_service() function identifiers.
> > > * "NONE" is used by efi_recover_from_page_fault() to check if the page
> > > --
> > > 2.18.1
> > >
> > > With regards,
> > > Narendra K
> --
> With regards,
> Narendra K
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
2019-06-25 14:21 ` Ard Biesheuvel
@ 2019-06-26 12:17 ` Narendra.K
2019-06-26 12:22 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Narendra.K @ 2019-06-26 12:17 UTC (permalink / raw)
To: ard.biesheuvel; +Cc: pjones, linux-efi
On Tue, Jun 25, 2019 at 04:21:33PM +0200, Ard Biesheuvel wrote:
[...]
> > > > --- a/drivers/firmware/efi/efi.c
> > > > +++ b/drivers/firmware/efi/efi.c
> > > > @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> > > > .rng_seed = EFI_INVALID_TABLE_ADDR,
> > > > .tpm_log = EFI_INVALID_TABLE_ADDR,
> > > > .mem_reserve = EFI_INVALID_TABLE_ADDR,
> > > > + .rci2 = EFI_INVALID_TABLE_ADDR,
> > >
> > > Does this really need to live in the efi struct?
> >
> > It probably need not be part of struct efi. We could define a struct of
> > type 'efi_config_table_type_t' in the rci2_table.c. Did you have a
> > similar idea in mind ? If yes, I will modify and test this idea.
> >
>
> Yes, I'd like to start keeping these things separate.
>
> I pushed a branch here
>
> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
>
> that changes the way uv_systab is handled, and moves it into arch/x86.
> Please follow that pattern instead.
Okay. Thank you for the guidance. I will make this change in the next version.
>
> > >
> > > > };
> > > > EXPORT_SYMBOL(efi);
> > > >
> > > > @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> > > > &efi.esrt,
> > > > &efi.properties_table,
> > > > &efi.mem_attr_table,
> > > > + &efi.rci2,
> > > > };
> > > >
> > >
> > > AFAICT, this table is only used by memremap_is_efi_data() to decide
> > > whether a page should be map as unencrypted, and if the address is in
> > > boot services data or runtime services data, the test will already
> > > success, regardless of whether it appears in this enumeration.
> >
> > Yes. Before 'memremap_is_efi_data()' checks if the memory type is boot
> > services data for runtime services data, it checks if the address is a
> > 'table' address in 'efi_is_table_address'. I added it because of this
> > check. Since the memory type used for the table is efi reserved type, we
> > need to add the table address to 'efi_tables' array so that it could be
> > checked in 'efi_is_table_address'. Please share your thought on this.
> >
>
> OK. My branch ^^^ moves this into arch/x86 as well, please add it there
I have a query related to this change. I will discuss it in next section
below as it helps provide complete context.
> > > > @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
> > > > {NULL_GUID, NULL, NULL},
> > > > };
> > > >
> > > > +/* OEM Tables */
> > > > +static __initdata efi_config_table_type_t oem_tables[] = {
> > > > + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
> > >
> > > Please drop the string. We don't have to print the presence of this
> > > table in the bootlog since it has no significance to the OS itself.
> >
> > Okay. I will drop this in the next version of the patch.
> >
> > >
> > > > + {NULL_GUID, NULL, NULL},
> > > > +};
> > > > +
> > >
> > > Do we really need a separate oem_tables[] array?
> >
> > The RCI2 table did not seem to be part of the group of common tables
> > such as SMBIOS and ACPI. To indicate this, I created a separate array.
> > It seems like it is not required. Having the array allows to leverage
> > the table matching code in 'match_config_table' function. Would you prefer
> > to have this entry added to the 'common_tables' array ?
> >
>
> Please add it to the arch_tables array in arch/x86 (if my assumption
> is correct that this is x86-only)
The table is used on x86. But it is not specific to x86 and is
independent of the architecture. Because of this detail, my thinking is
to keep the rci2_table.c and related changes in generic efi layer
drivers/firmware/efi/. If we keep the changes in drivers/firmware/efi/,
then two options are
1. Retain the oem_tables array and add rci2 table address to this array
2. Add rci2 table address to common_tables array
Does this detail sound correct ?
Also, since the 'efi_is_table_address' function and efi_tables array are moved
to arch/x86, for rci2 table address to be detected as a table address, it needs to be
added to 'efi_tables' array. Would it be correct to add rci2 table
address to this array with rest of the changes residing in the generic efi
layer ?
Please share your thoughts on the two details.
[...]
> > Would you prefer to merge this function into 'efi_rci2_sysfs_init' function ?
> >
>
> Yes. Only user space needs to access this, so we can defer this to
> later, when the normal memremap() functions are available.
>
Okay, I will make this change in the next version.
--
With regards,
Narendra K
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
2019-06-26 12:17 ` Narendra.K
@ 2019-06-26 12:22 ` Ard Biesheuvel
0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2019-06-26 12:22 UTC (permalink / raw)
To: Narendra.K; +Cc: Peter Jones, linux-efi
On Wed, 26 Jun 2019 at 14:17, <Narendra.K@dell.com> wrote:
>
> On Tue, Jun 25, 2019 at 04:21:33PM +0200, Ard Biesheuvel wrote:
> [...]
> > > > > --- a/drivers/firmware/efi/efi.c
> > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> > > > > .rng_seed = EFI_INVALID_TABLE_ADDR,
> > > > > .tpm_log = EFI_INVALID_TABLE_ADDR,
> > > > > .mem_reserve = EFI_INVALID_TABLE_ADDR,
> > > > > + .rci2 = EFI_INVALID_TABLE_ADDR,
> > > >
> > > > Does this really need to live in the efi struct?
> > >
> > > It probably need not be part of struct efi. We could define a struct of
> > > type 'efi_config_table_type_t' in the rci2_table.c. Did you have a
> > > similar idea in mind ? If yes, I will modify and test this idea.
> > >
> >
> > Yes, I'd like to start keeping these things separate.
> >
> > I pushed a branch here
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
> >
> > that changes the way uv_systab is handled, and moves it into arch/x86.
> > Please follow that pattern instead.
>
> Okay. Thank you for the guidance. I will make this change in the next version.
>
> >
> > > >
> > > > > };
> > > > > EXPORT_SYMBOL(efi);
> > > > >
> > > > > @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> > > > > &efi.esrt,
> > > > > &efi.properties_table,
> > > > > &efi.mem_attr_table,
> > > > > + &efi.rci2,
> > > > > };
> > > > >
> > > >
> > > > AFAICT, this table is only used by memremap_is_efi_data() to decide
> > > > whether a page should be map as unencrypted, and if the address is in
> > > > boot services data or runtime services data, the test will already
> > > > success, regardless of whether it appears in this enumeration.
> > >
> > > Yes. Before 'memremap_is_efi_data()' checks if the memory type is boot
> > > services data for runtime services data, it checks if the address is a
> > > 'table' address in 'efi_is_table_address'. I added it because of this
> > > check. Since the memory type used for the table is efi reserved type, we
> > > need to add the table address to 'efi_tables' array so that it could be
> > > checked in 'efi_is_table_address'. Please share your thought on this.
> > >
> >
> > OK. My branch ^^^ moves this into arch/x86 as well, please add it there
>
> I have a query related to this change. I will discuss it in next section
> below as it helps provide complete context.
>
> > > > > @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t common_tables[] = {
> > > > > {NULL_GUID, NULL, NULL},
> > > > > };
> > > > >
> > > > > +/* OEM Tables */
> > > > > +static __initdata efi_config_table_type_t oem_tables[] = {
> > > > > + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
> > > >
> > > > Please drop the string. We don't have to print the presence of this
> > > > table in the bootlog since it has no significance to the OS itself.
> > >
> > > Okay. I will drop this in the next version of the patch.
> > >
> > > >
> > > > > + {NULL_GUID, NULL, NULL},
> > > > > +};
> > > > > +
> > > >
> > > > Do we really need a separate oem_tables[] array?
> > >
> > > The RCI2 table did not seem to be part of the group of common tables
> > > such as SMBIOS and ACPI. To indicate this, I created a separate array.
> > > It seems like it is not required. Having the array allows to leverage
> > > the table matching code in 'match_config_table' function. Would you prefer
> > > to have this entry added to the 'common_tables' array ?
> > >
> >
> > Please add it to the arch_tables array in arch/x86 (if my assumption
> > is correct that this is x86-only)
>
> The table is used on x86. But it is not specific to x86 and is
> independent of the architecture. Because of this detail, my thinking is
> to keep the rci2_table.c and related changes in generic efi layer
> drivers/firmware/efi/. If we keep the changes in drivers/firmware/efi/,
> then two options are
>
> 1. Retain the oem_tables array and add rci2 table address to this array
> 2. Add rci2 table address to common_tables array
>
> Does this detail sound correct ?
>
Yes, and I'd prefer the latter.
> Also, since the 'efi_is_table_address' function and efi_tables array are moved
> to arch/x86, for rci2 table address to be detected as a table address, it needs to be
> added to 'efi_tables' array. Would it be correct to add rci2 table
> address to this array with rest of the changes residing in the generic efi
> layer ?
>
Yes.
> Please share your thoughts on the two details.
>
> [...]
> > > Would you prefer to merge this function into 'efi_rci2_sysfs_init' function ?
> > >
> >
> > Yes. Only user space needs to access this, so we can defer this to
> > later, when the normal memremap() functions are available.
> >
>
> Okay, I will make this change in the next version.
>
> --
> With regards,
> Narendra K
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-26 12:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 10:11 [RFC PATCH] Export Runtime Configuration Interface table to sysfs Narendra.K
2019-06-21 16:35 ` Ard Biesheuvel
2019-06-25 12:10 ` Narendra.K
2019-06-25 14:21 ` Ard Biesheuvel
2019-06-26 12:17 ` Narendra.K
2019-06-26 12:22 ` Ard Biesheuvel
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).