[PATCH 1/2] efi: don't call the system table version the runtime services version

[PATCH 1/2] efi: don't call the system table version the runtime services version

[Peter Jones]

The system table's hdr.revision is not the runtime services revision,
it's the EFI Spec revision.  The runtime services revision is the one on
systab->runtime->hdr.revision.

So we shouldn't call it runtime_version throughtout the code, as that's
misleading if you're *actually* looking for the runtime services
revision.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c             | 6 ++++--
 arch/x86/platform/efi/efi_64.c          | 2 +-
 arch/x86/xen/efi.c                      | 4 ++--
 drivers/firmware/efi/arm-init.c         | 4 ++--
 drivers/firmware/efi/runtime-wrappers.c | 8 ++++----
 drivers/xen/efi.c                       | 6 +++---
 include/linux/efi.h                     | 2 +-
 7 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 1fbb408..e36290e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -468,6 +468,8 @@ void __init efi_init(void)
 	efi.fw_vendor	 = (unsigned long)efi.systab->fw_vendor;
 	efi.runtime	 = (unsigned long)efi.systab->runtime;
 
+	efi.spec_version = efi.systab->hdr.revision;
+
 	/*
 	 * Show what we know for posterity
 	 */
@@ -843,7 +845,7 @@ static void __init kexec_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
+	efi.spec_version = efi_systab.hdr.revision;
 
 	efi_native_runtime_setup();
 
@@ -939,7 +941,7 @@ static void __init __efi_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
+	efi.spec_version = efi_systab.hdr.revision;
 
 	if (efi_is_native())
 		efi_native_runtime_setup();
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 677e29e..2366cf2 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -677,7 +677,7 @@ efi_thunk_query_variable_info(u32 attr, u64 *storage_space,
 	efi_status_t status;
 	u32 phys_storage, phys_remaining, phys_max;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	phys_storage = virt_to_phys(storage_space);
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 3be0121..e7c8357 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -56,7 +56,7 @@ static efi_system_table_t efi_systab_xen __initdata = {
 
 static const struct efi efi_xen __initconst = {
 	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
+	.spec_version		  = 0,    /* Initialized_later. */
 	.mps                      = EFI_INVALID_TABLE_ADDR,
 	.acpi                     = EFI_INVALID_TABLE_ADDR,
 	.acpi20                   = EFI_INVALID_TABLE_ADDR,
@@ -131,7 +131,7 @@ static efi_system_table_t __init *xen_efi_probe(void)
 	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
 
 	if (HYPERVISOR_platform_op(&op) == 0)
-		efi.runtime_version = info->version;
+		efi.spec_version = info->version;
 
 	return &efi_systab_xen;
 }
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index c49d50e..b89fc23 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -117,8 +117,6 @@ static int __init uefi_init(void)
 			efi.systab->hdr.revision >> 16,
 			efi.systab->hdr.revision & 0xffff);
 
-	efi.runtime_version = efi.systab->hdr.revision;
-
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
 				sizeof(vendor) * sizeof(efi_char16_t));
@@ -133,6 +131,8 @@ static int __init uefi_init(void)
 		efi.systab->hdr.revision >> 16,
 		efi.systab->hdr.revision & 0xffff, vendor);
 
+	efi.spec_version = (u32)efi.systab->hdr.revision;
+
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
 	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
 					  table_size);
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4195877..753df73 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -196,7 +196,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -214,7 +214,7 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	if (!spin_trylock(&efi_runtime_lock))
@@ -252,7 +252,7 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -268,7 +268,7 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 22f71ff..44294b6 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -192,7 +192,7 @@ efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	efi_data(op).u.query_variable_info.attr = attr;
@@ -226,7 +226,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(update_capsule);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
@@ -247,7 +247,7 @@ efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a582..c6a3126 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -826,7 +826,7 @@ typedef struct {
  */
 extern struct efi {
 	efi_system_table_t *systab;	/* EFI system table */
-	unsigned int runtime_version;	/* Runtime services version */
+	u32 spec_version;		/* EFI Spec revision of our firmware */
 	unsigned long mps;		/* MPS table */
 	unsigned long acpi;		/* ACPI table  (IA64 ext 0.71) */
 	unsigned long acpi20;		/* ACPI table  (ACPI 2.0) */
-- 
2.7.4

[PATCH 2/2] efi: add firmware version information to sysfs

[Peter Jones]

This adds the EFI Spec version from the system table to sysfs as
/sys/firmware/efi/spec_version .

Userland tools need this information in order to work around
specification deficiencies in 2.4 and earlier firmwares.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
 arch/ia64/kernel/efi.c                       |  3 ++
 drivers/firmware/efi/arm-init.c              |  2 +
 drivers/firmware/efi/efi.c                   | 71 ++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
index e794eac..4eec7c2 100644
--- a/Documentation/ABI/testing/sysfs-firmware-efi
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -28,3 +28,9 @@ 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/spec_version
+Date:		August 2016
+Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Displays the UEFI Specification revision the firmware claims to
+		be based upon.
diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 1212956..a48377f 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -475,6 +475,7 @@ efi_init (void)
 	u64 efi_desc_size;
 	char *cp, vendor[100] = "unknown";
 	int i;
+	efi_table_hdr_t *hdr;
 
 	set_bit(EFI_BOOT, &efi.flags);
 	set_bit(EFI_64BIT, &efi.flags);
@@ -531,6 +532,8 @@ efi_init (void)
 	       efi.systab->hdr.revision >> 16,
 	       efi.systab->hdr.revision & 0xffff, vendor);
 
+	efi.spec_version = efi.systab->hdr.version;
+
 	palo_phys      = EFI_INVALID_TABLE_ADDR;
 
 	if (efi_config_init(arch_tables) != 0)
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index b89fc23..8e8cb8c 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -133,6 +133,8 @@ static int __init uefi_init(void)
 
 	efi.spec_version = (u32)efi.systab->hdr.revision;
 
+	early_memunmap(tmp, sizeof(efi_table_hdr_t));
+
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
 	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
 					  table_size);
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631a..5947d19 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/ucs2_string.h>
+#include <linux/bcd.h>
 
 #include <asm/early_ioremap.h>
 
@@ -124,6 +125,74 @@ static struct kobj_attribute efi_attr_systab =
 
 #define EFI_FIELD(var) efi.var
 
+static ssize_t efi_version_show(struct kobject *kobj,
+				struct kobj_attribute *attr,
+				char *buf,
+				u32 revision)
+{
+	char *str = buf;
+	u16 major;
+	u8 minor, tiny;
+
+	if (!kobj || !buf)
+		return -EINVAL;
+
+	/* The spec says:
+	 *  The revision of the EFI Specification to which this table
+	 *  conforms. The upper 16 bits of this field contain the major
+	 *  revision value, and the lower 16 bits contain the minor revision
+	 *  value. The minor revision values are binary coded decimals and are
+	 *  limited to the range of 00..99.
+	 *
+	 *  When printed or displayed UEFI spec revision is referred as (Major
+	 *  revision).(Minor revision upper decimal).(Minor revision lower
+	 *  decimal) or (Major revision).(Minor revision upper decimal) in
+	 *  case Minor revision lower decimal is set to 0. For example:
+	 *
+	 *  A specification with the revision value ((2<<16) | (30)) would be
+	 *  referred as 2.3;
+	 *  A specification with the revision value ((2<<16) | (31)) would be
+	 *  referred as 2.3.1
+	 *
+	 * Then later it says:
+	 *  Related Definitions
+	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
+	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
+	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
+	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
+	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
+	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
+	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
+	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
+	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
+	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
+	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
+	 *
+	 * (Apparently this bit of the spec failed to get updated for 2.5
+	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
+	 */
+
+	major = (revision & 0xffff0000) >> 16;
+	minor = bcd2bin((revision & 0x0000ff00) >> 8);
+	tiny = bcd2bin(revision & 0x000000ff);
+
+	if (tiny == 0)
+		str += sprintf(str, "%d.%d\n", major, minor);
+	else
+		str += sprintf(str, "%d.%d.%d\n", major, minor, tiny);
+
+	return str - buf;
+}
+
+#define EFI_VERSION_ATTR_SHOW(name) \
+static ssize_t name##_show(struct kobject *kobj, \
+				struct kobj_attribute *attr, char *buf) \
+{ \
+	return efi_version_show(kobj, attr, buf, EFI_FIELD(name)); \
+}
+
+EFI_VERSION_ATTR_SHOW(spec_version);
+
 #define EFI_ATTR_SHOW(name) \
 static ssize_t name##_show(struct kobject *kobj, \
 				struct kobj_attribute *attr, char *buf) \
@@ -146,6 +215,7 @@ static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
 static struct kobj_attribute efi_attr_fw_platform_size =
 	__ATTR_RO(fw_platform_size);
+static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
 
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
@@ -153,6 +223,7 @@ static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_runtime.attr,
 	&efi_attr_config_table.attr,
 	&efi_attr_fw_platform_size.attr,
+	&efi_attr_spec_version.attr,
 	NULL,
 };
 
-- 
2.7.4

Re: [PATCH 2/2] efi: add firmware version information to sysfs

[Lukas Wunner]

On Tue, Aug 23, 2016 at 12:13:52PM -0400, Peter Jones wrote:
> This adds the EFI Spec version from the system table to sysfs as
> /sys/firmware/efi/spec_version .
> 
> Userland tools need this information in order to work around
> specification deficiencies in 2.4 and earlier firmwares.

Some details or examples on the nature of those specification
deficiencies would help an uninitiated reader understand the
necessity of this patch.

> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
>  arch/ia64/kernel/efi.c                       |  3 ++
>  drivers/firmware/efi/arm-init.c              |  2 +
>  drivers/firmware/efi/efi.c                   | 71 ++++++++++++++++++++++++++++
>  4 files changed, 82 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> index e794eac..4eec7c2 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -28,3 +28,9 @@ 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/spec_version
> +Date:		August 2016
> +Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:	Displays the UEFI Specification revision the firmware claims to
> +		be based upon.
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 1212956..a48377f 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -475,6 +475,7 @@ efi_init (void)
>  	u64 efi_desc_size;
>  	char *cp, vendor[100] = "unknown";
>  	int i;
> +	efi_table_hdr_t *hdr;

Unused new variable?

>  
>  	set_bit(EFI_BOOT, &efi.flags);
>  	set_bit(EFI_64BIT, &efi.flags);
> @@ -531,6 +532,8 @@ efi_init (void)
>  	       efi.systab->hdr.revision >> 16,
>  	       efi.systab->hdr.revision & 0xffff, vendor);
>  
> +	efi.spec_version = efi.systab->hdr.version;
> +

I think this deserves a mention in the commit message that the
spec_version struct field was previously not filled for ia64 arch
and that you're fixing that while you're at it. Alternatively,
move this to a separate commit.

>  	palo_phys      = EFI_INVALID_TABLE_ADDR;
>  
>  	if (efi_config_init(arch_tables) != 0)
> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> index b89fc23..8e8cb8c 100644
> --- a/drivers/firmware/efi/arm-init.c
> +++ b/drivers/firmware/efi/arm-init.c
> @@ -133,6 +133,8 @@ static int __init uefi_init(void)
>  
>  	efi.spec_version = (u32)efi.systab->hdr.revision;
>  
> +	early_memunmap(tmp, sizeof(efi_table_hdr_t));
> +

What is the connection of this change to exposing the spec_version
in sysfs? Seems like a fix that should be moved to a separate commit.

>  	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
>  	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
>  					  table_size);
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..5947d19 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -27,6 +27,7 @@
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>  #include <linux/ucs2_string.h>
> +#include <linux/bcd.h>
>  
>  #include <asm/early_ioremap.h>
>  
> @@ -124,6 +125,74 @@ static struct kobj_attribute efi_attr_systab =
>  
>  #define EFI_FIELD(var) efi.var
>  
> +static ssize_t efi_version_show(struct kobject *kobj,
> +				struct kobj_attribute *attr,
> +				char *buf,
> +				u32 revision)
> +{
> +	char *str = buf;
> +	u16 major;
> +	u8 minor, tiny;
> +
> +	if (!kobj || !buf)
> +		return -EINVAL;
> +
> +	/* The spec says:
> +	 *  The revision of the EFI Specification to which this table
> +	 *  conforms. The upper 16 bits of this field contain the major
> +	 *  revision value, and the lower 16 bits contain the minor revision
> +	 *  value. The minor revision values are binary coded decimals and are
> +	 *  limited to the range of 00..99.
> +	 *
> +	 *  When printed or displayed UEFI spec revision is referred as (Major
> +	 *  revision).(Minor revision upper decimal).(Minor revision lower
> +	 *  decimal) or (Major revision).(Minor revision upper decimal) in
> +	 *  case Minor revision lower decimal is set to 0. For example:
> +	 *
> +	 *  A specification with the revision value ((2<<16) | (30)) would be
> +	 *  referred as 2.3;
> +	 *  A specification with the revision value ((2<<16) | (31)) would be
> +	 *  referred as 2.3.1
> +	 *
> +	 * Then later it says:
> +	 *  Related Definitions
> +	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
> +	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
> +	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
> +	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
> +	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
> +	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
> +	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
> +	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
> +	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
> +	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> +	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
> +	 *
> +	 * (Apparently this bit of the spec failed to get updated for 2.5
> +	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
> +	 */

I'd only include a 2-3 line summary of the spec and move this documentation
to the commit message in order to keep the code concise.

> +
> +	major = (revision & 0xffff0000) >> 16;
> +	minor = bcd2bin((revision & 0x0000ff00) >> 8);
> +	tiny = bcd2bin(revision & 0x000000ff);

You can improve readability by inserting a few blanks like this:

	major =         (revision & 0xffff0000) >> 16;
	minor = bcd2bin((revision & 0x0000ff00) >> 8);
	tiny  = bcd2bin( revision & 0x000000ff);

I'd also use minor_u and minor_l or somesuch instead of "tiny"
but that's just my preferred bikeshed color.

> +
> +	if (tiny == 0)
> +		str += sprintf(str, "%d.%d\n", major, minor);
> +	else
> +		str += sprintf(str, "%d.%d.%d\n", major, minor, tiny);
> +
> +	return str - buf;

Hm, why not return the result of sprintf directly (cast to ssize_t),
or if you want to handle negative return values properly, store the
result in an int?

> +}
> +
> +#define EFI_VERSION_ATTR_SHOW(name) \
> +static ssize_t name##_show(struct kobject *kobj, \
> +				struct kobj_attribute *attr, char *buf) \
> +{ \
> +	return efi_version_show(kobj, attr, buf, EFI_FIELD(name)); \
> +}
> +
> +EFI_VERSION_ATTR_SHOW(spec_version);
> +

Unless you plan to expose other versions like this, you can save on
code by not using a macro here.

Best regards,

Lukas

>  #define EFI_ATTR_SHOW(name) \
>  static ssize_t name##_show(struct kobject *kobj, \
>  				struct kobj_attribute *attr, char *buf) \
> @@ -146,6 +215,7 @@ static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>  static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>  static struct kobj_attribute efi_attr_fw_platform_size =
>  	__ATTR_RO(fw_platform_size);
> +static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
>  
>  static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_systab.attr,
> @@ -153,6 +223,7 @@ static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_runtime.attr,
>  	&efi_attr_config_table.attr,
>  	&efi_attr_fw_platform_size.attr,
> +	&efi_attr_spec_version.attr,
>  	NULL,
>  };
>  
> -- 
> 2.7.4

Re: [PATCH 2/2] efi: add firmware version information to sysfs

[Peter Jones]

On Wed, Aug 24, 2016 at 12:30:21PM +0200, Lukas Wunner wrote:

(sorry for the delay, it turned into a busy pair of weeks)

> On Tue, Aug 23, 2016 at 12:13:52PM -0400, Peter Jones wrote:
> > This adds the EFI Spec version from the system table to sysfs as
> > /sys/firmware/efi/spec_version .
> > 
> > Userland tools need this information in order to work around
> > specification deficiencies in 2.4 and earlier firmwares.
> 
> Some details or examples on the nature of those specification
> deficiencies would help an uninitiated reader understand the
> necessity of this patch.
> 
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
> >  arch/ia64/kernel/efi.c                       |  3 ++
> >  drivers/firmware/efi/arm-init.c              |  2 +
> >  drivers/firmware/efi/efi.c                   | 71 ++++++++++++++++++++++++++++
> >  4 files changed, 82 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > index e794eac..4eec7c2 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-efi
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > @@ -28,3 +28,9 @@ 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/spec_version
> > +Date:		August 2016
> > +Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > +Description:	Displays the UEFI Specification revision the firmware claims to
> > +		be based upon.
> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> > index 1212956..a48377f 100644
> > --- a/arch/ia64/kernel/efi.c
> > +++ b/arch/ia64/kernel/efi.c
> > @@ -475,6 +475,7 @@ efi_init (void)
> >  	u64 efi_desc_size;
> >  	char *cp, vendor[100] = "unknown";
> >  	int i;
> > +	efi_table_hdr_t *hdr;
> 
> Unused new variable?

A remnant I apparently have failed to remove from an old version of the
patch.

> 
> >  
> >  	set_bit(EFI_BOOT, &efi.flags);
> >  	set_bit(EFI_64BIT, &efi.flags);
> > @@ -531,6 +532,8 @@ efi_init (void)
> >  	       efi.systab->hdr.revision >> 16,
> >  	       efi.systab->hdr.revision & 0xffff, vendor);
> >  
> > +	efi.spec_version = efi.systab->hdr.version;
> > +
> 
> I think this deserves a mention in the commit message that the
> spec_version struct field was previously not filled for ia64 arch
> and that you're fixing that while you're at it. Alternatively,
> move this to a separate commit.

Sure.

> 
> >  	palo_phys      = EFI_INVALID_TABLE_ADDR;
> >  
> >  	if (efi_config_init(arch_tables) != 0)
> > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> > index b89fc23..8e8cb8c 100644
> > --- a/drivers/firmware/efi/arm-init.c
> > +++ b/drivers/firmware/efi/arm-init.c
> > @@ -133,6 +133,8 @@ static int __init uefi_init(void)
> >  
> >  	efi.spec_version = (u32)efi.systab->hdr.revision;
> >  
> > +	early_memunmap(tmp, sizeof(efi_table_hdr_t));
> > +
> 
> What is the connection of this change to exposing the spec_version
> in sysfs? Seems like a fix that should be moved to a separate commit.

This is another remnant from that same previous version; I'll fix it
before resending as well.

> 
> >  	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
> >  	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
> >  					  table_size);
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 5a2631a..5947d19 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> >  #include <linux/ucs2_string.h>
> > +#include <linux/bcd.h>
> >  
> >  #include <asm/early_ioremap.h>
> >  
> > @@ -124,6 +125,74 @@ static struct kobj_attribute efi_attr_systab =
> >  
> >  #define EFI_FIELD(var) efi.var
> >  
> > +static ssize_t efi_version_show(struct kobject *kobj,
> > +				struct kobj_attribute *attr,
> > +				char *buf,
> > +				u32 revision)
> > +{
> > +	char *str = buf;
> > +	u16 major;
> > +	u8 minor, tiny;
> > +
> > +	if (!kobj || !buf)
> > +		return -EINVAL;
> > +
> > +	/* The spec says:
> > +	 *  The revision of the EFI Specification to which this table
> > +	 *  conforms. The upper 16 bits of this field contain the major
> > +	 *  revision value, and the lower 16 bits contain the minor revision
> > +	 *  value. The minor revision values are binary coded decimals and are
> > +	 *  limited to the range of 00..99.
> > +	 *
> > +	 *  When printed or displayed UEFI spec revision is referred as (Major
> > +	 *  revision).(Minor revision upper decimal).(Minor revision lower
> > +	 *  decimal) or (Major revision).(Minor revision upper decimal) in
> > +	 *  case Minor revision lower decimal is set to 0. For example:
> > +	 *
> > +	 *  A specification with the revision value ((2<<16) | (30)) would be
> > +	 *  referred as 2.3;
> > +	 *  A specification with the revision value ((2<<16) | (31)) would be
> > +	 *  referred as 2.3.1
> > +	 *
> > +	 * Then later it says:
> > +	 *  Related Definitions
> > +	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
> > +	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
> > +	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
> > +	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
> > +	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
> > +	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
> > +	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
> > +	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
> > +	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
> > +	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> > +	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
> > +	 *
> > +	 * (Apparently this bit of the spec failed to get updated for 2.5
> > +	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
> > +	 */
> 
> I'd only include a 2-3 line summary of the spec and move this documentation
> to the commit message in order to keep the code concise.

This is the first time I think I've ever been told by anybody to comment
less :)  But I think this comment explains why it's formatting it this
way pretty well, without really breaking up the code meaningfully at
all.  I'd rather leave it.

> 
> > +
> > +	major = (revision & 0xffff0000) >> 16;
> > +	minor = bcd2bin((revision & 0x0000ff00) >> 8);
> > +	tiny = bcd2bin(revision & 0x000000ff);
> 
> You can improve readability by inserting a few blanks like this:
> 
> 	major =         (revision & 0xffff0000) >> 16;
> 	minor = bcd2bin((revision & 0x0000ff00) >> 8);
> 	tiny  = bcd2bin( revision & 0x000000ff);
> 
> I'd also use minor_u and minor_l or somesuch instead of "tiny"
> but that's just my preferred bikeshed color.

I've never understood why people think that makes things more readable,
but I can read it fine either way, so sure, I'll take these suggestions
for the next version of the patch.

> > +
> > +	if (tiny == 0)
> > +		str += sprintf(str, "%d.%d\n", major, minor);
> > +	else
> > +		str += sprintf(str, "%d.%d.%d\n", major, minor, tiny);
> > +
> > +	return str - buf;
> 
> Hm, why not return the result of sprintf directly (cast to ssize_t),
> or if you want to handle negative return values properly, store the
> result in an int?

Fair enough.

> > +}
> > +
> > +#define EFI_VERSION_ATTR_SHOW(name) \
> > +static ssize_t name##_show(struct kobject *kobj, \
> > +				struct kobj_attribute *attr, char *buf) \
> > +{ \
> > +	return efi_version_show(kobj, attr, buf, EFI_FIELD(name)); \
> > +}
> > +
> > +EFI_VERSION_ATTR_SHOW(spec_version);
> > +
> 
> Unless you plan to expose other versions like this, you can save on
> code by not using a macro here.

Yep; originally I'd had 3, but it turned out not to be worthwhile.

I'll send an updated version.

-- 
        Peter

[PATCH 1/3] efi: don't call the system table version the runtime services version

[Peter Jones]

The system table's hdr.revision is not the runtime services revision,
it's the EFI Spec revision.  The runtime services revision is the one on
systab->runtime->hdr.revision.

So we shouldn't call it runtime_version throughout the code, as that's
misleading if you're *actually* looking for the runtime services
revision.

We also move some of the assignments around just a bit, in support of
making a future patch more readable.

This also fixes a minor bug where the version field was not set in the
efi structure on ia64.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/ia64/kernel/efi.c                  | 3 +++
 arch/x86/platform/efi/efi.c             | 7 ++++---
 arch/x86/platform/efi/efi_64.c          | 2 +-
 arch/x86/xen/efi.c                      | 4 ++--
 drivers/firmware/efi/arm-init.c         | 5 +++--
 drivers/firmware/efi/runtime-wrappers.c | 8 ++++----
 drivers/xen/efi.c                       | 6 +++---
 include/linux/efi.h                     | 2 +-
 8 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 1212956..2af99a8 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -513,6 +513,9 @@ efi_init (void)
 		panic("Whoa! Can't find EFI system table.\n");
 	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		panic("Whoa! EFI system table signature incorrect\n");
+
+	efi.spec_version = efi.systab->hdr.version;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		printk(KERN_WARNING "Warning: EFI system table version "
 		       "%d.%02d, expected 1.00 or greater\n",
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 1fbb408..51c091e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -325,6 +325,9 @@ static int __init efi_systab_init(void *phys)
 		pr_err("System table signature incorrect!\n");
 		return -EINVAL;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
 		       efi.systab->hdr.revision >> 16,
@@ -843,7 +846,7 @@ static void __init kexec_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
+	efi.spec_version = efi_systab.hdr.revision;
 
 	efi_native_runtime_setup();
 
@@ -939,8 +942,6 @@ static void __init __efi_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
-
 	if (efi_is_native())
 		efi_native_runtime_setup();
 	else
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 677e29e..2366cf2 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -677,7 +677,7 @@ efi_thunk_query_variable_info(u32 attr, u64 *storage_space,
 	efi_status_t status;
 	u32 phys_storage, phys_remaining, phys_max;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	phys_storage = virt_to_phys(storage_space);
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 3be0121..e7c8357 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -56,7 +56,7 @@ static efi_system_table_t efi_systab_xen __initdata = {
 
 static const struct efi efi_xen __initconst = {
 	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
+	.spec_version		  = 0,    /* Initialized_later. */
 	.mps                      = EFI_INVALID_TABLE_ADDR,
 	.acpi                     = EFI_INVALID_TABLE_ADDR,
 	.acpi20                   = EFI_INVALID_TABLE_ADDR,
@@ -131,7 +131,7 @@ static efi_system_table_t __init *xen_efi_probe(void)
 	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
 
 	if (HYPERVISOR_platform_op(&op) == 0)
-		efi.runtime_version = info->version;
+		efi.spec_version = info->version;
 
 	return &efi_systab_xen;
 }
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index c49d50e..8f4aeba 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -112,13 +112,14 @@ static int __init uefi_init(void)
 		retval = -EINVAL;
 		goto out;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
 		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
 			efi.systab->hdr.revision >> 16,
 			efi.systab->hdr.revision & 0xffff);
 
-	efi.runtime_version = efi.systab->hdr.revision;
-
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
 				sizeof(vendor) * sizeof(efi_char16_t));
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4195877..753df73 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -196,7 +196,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -214,7 +214,7 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	if (!spin_trylock(&efi_runtime_lock))
@@ -252,7 +252,7 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -268,7 +268,7 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 22f71ff..44294b6 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -192,7 +192,7 @@ efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	efi_data(op).u.query_variable_info.attr = attr;
@@ -226,7 +226,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(update_capsule);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
@@ -247,7 +247,7 @@ efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a582..c6a3126 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -826,7 +826,7 @@ typedef struct {
  */
 extern struct efi {
 	efi_system_table_t *systab;	/* EFI system table */
-	unsigned int runtime_version;	/* Runtime services version */
+	u32 spec_version;		/* EFI Spec revision of our firmware */
 	unsigned long mps;		/* MPS table */
 	unsigned long acpi;		/* ACPI table  (IA64 ext 0.71) */
 	unsigned long acpi20;		/* ACPI table  (ACPI 2.0) */
-- 
2.7.4

[PATCH 2/3] efi: add firmware version information to sysfs

[Peter Jones]

This adds the EFI Spec version from the system table to sysfs as
/sys/firmware/efi/spec_version .

Userland tools need this information in order to work around
specification deficiencies in 2.4 and earlier firmwares.  Specifically,
UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
firmware we'll want to work around BDS's boot order management by adding
fwupdate entries to BootOrder.  We'd prefer not to do this on newer
firmware, as it is both non-sensical in terms of what it expresses, and
it results in more relatively failure prone flash writes.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
 drivers/firmware/efi/efi.c                   | 55 ++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
index e794eac..4eec7c2 100644
--- a/Documentation/ABI/testing/sysfs-firmware-efi
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -28,3 +28,9 @@ 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/spec_version
+Date:		August 2016
+Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Displays the UEFI Specification revision the firmware claims to
+		be based upon.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631a..3ef3a2a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,11 +141,65 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	/* The spec says:
+	 *  The revision of the EFI Specification to which this table
+	 *  conforms. The upper 16 bits of this field contain the major
+	 *  revision value, and the lower 16 bits contain the minor revision
+	 *  value. The minor revision values are binary coded decimals and are
+	 *  limited to the range of 00..99.
+	 *
+	 *  When printed or displayed UEFI spec revision is referred as (Major
+	 *  revision).(Minor revision upper decimal).(Minor revision lower
+	 *  decimal) or (Major revision).(Minor revision upper decimal) in
+	 *  case Minor revision lower decimal is set to 0. For example:
+	 *
+	 *  A specification with the revision value ((2<<16) | (30)) would be
+	 *  referred as 2.3;
+	 *  A specification with the revision value ((2<<16) | (31)) would be
+	 *  referred as 2.3.1
+	 *
+	 * Then later it says:
+	 *  Related Definitions
+	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
+	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
+	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
+	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
+	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
+	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
+	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
+	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
+	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
+	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
+	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
+	 *
+	 * (Apparently this bit of the spec failed to get updated for 2.5
+	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
+	 *
+	 * This of course does not match the description above at all, but it
+	 * does match the code in Tiano.  So we decode it Tiano's way.
+	 */
+	u16 major = (efi.spec_version & 0xffff0000) >> 16;
+	u16 minor = (efi.spec_version & 0x0000ffff);
+
+	if (!buf)
+		return -EINVAL;
+
+	if ((minor % 10) == 0)
+		return sprintf(buf, "%u.%u", major, minor / 10);
+	else
+		return sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
 static struct kobj_attribute efi_attr_fw_platform_size =
 	__ATTR_RO(fw_platform_size);
+static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
 
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
@@ -153,6 +207,7 @@ static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_runtime.attr,
 	&efi_attr_config_table.attr,
 	&efi_attr_fw_platform_size.attr,
+	&efi_attr_spec_version.attr,
 	NULL,
 };
 
-- 
2.7.4

Re: [PATCH 2/3] efi: add firmware version information to sysfs

[Lukas Wunner]

On Fri, Sep 02, 2016 at 04:57:52PM -0400, Peter Jones wrote:
> This adds the EFI Spec version from the system table to sysfs as
> /sys/firmware/efi/spec_version .
> 
> Userland tools need this information in order to work around
> specification deficiencies in 2.4 and earlier firmwares.  Specifically,
> UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
> UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
> firmware we'll want to work around BDS's boot order management by adding
> fwupdate entries to BootOrder.  We'd prefer not to do this on newer
> firmware, as it is both non-sensical in terms of what it expresses, and
> it results in more relatively failure prone flash writes.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
>  drivers/firmware/efi/efi.c                   | 55 ++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> index e794eac..4eec7c2 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -28,3 +28,9 @@ 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/spec_version
> +Date:		August 2016
> +Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:	Displays the UEFI Specification revision the firmware claims to
> +		be based upon.
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..3ef3a2a 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -141,11 +141,65 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
>  	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
>  }
>  
> +static ssize_t spec_version_show(struct kobject *kobj,
> +				 struct kobj_attribute *attr,
> +				 char *buf)

The function is renamed efi_spec_version_format() in the next patch.
Why not call it that right away in this patch?

I'd change the return type to void and make this function copy
"(unknown)" to buf on error. That way you don't have to repeat
that over and over again in the next patch.

> +{
> +	/* The spec says:
> +	 *  The revision of the EFI Specification to which this table
> +	 *  conforms. The upper 16 bits of this field contain the major
> +	 *  revision value, and the lower 16 bits contain the minor revision
> +	 *  value. The minor revision values are binary coded decimals and are
> +	 *  limited to the range of 00..99.
> +	 *
> +	 *  When printed or displayed UEFI spec revision is referred as (Major
> +	 *  revision).(Minor revision upper decimal).(Minor revision lower
> +	 *  decimal) or (Major revision).(Minor revision upper decimal) in
> +	 *  case Minor revision lower decimal is set to 0. For example:
> +	 *
> +	 *  A specification with the revision value ((2<<16) | (30)) would be
> +	 *  referred as 2.3;
> +	 *  A specification with the revision value ((2<<16) | (31)) would be
> +	 *  referred as 2.3.1
> +	 *
> +	 * Then later it says:
> +	 *  Related Definitions
> +	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
> +	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
> +	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
> +	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
> +	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
> +	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
> +	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
> +	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
> +	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
> +	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> +	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
> +	 *
> +	 * (Apparently this bit of the spec failed to get updated for 2.5
> +	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
> +	 *
> +	 * This of course does not match the description above at all, but it
> +	 * does match the code in Tiano.  So we decode it Tiano's way.
> +	 */
> +	u16 major = (efi.spec_version & 0xffff0000) >> 16;
> +	u16 minor = (efi.spec_version & 0x0000ffff);
> +
> +	if (!buf)
> +		return -EINVAL;

Move that check to the top of the function so that you avoid calculating
major and minor in vain if buf is NULL.

Thanks,

Lukas


> +
> +	if ((minor % 10) == 0)
> +		return sprintf(buf, "%u.%u", major, minor / 10);
> +	else
> +		return sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
> +}
> +
>  static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
>  static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>  static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>  static struct kobj_attribute efi_attr_fw_platform_size =
>  	__ATTR_RO(fw_platform_size);
> +static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
>  
>  static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_systab.attr,
> @@ -153,6 +207,7 @@ static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_runtime.attr,
>  	&efi_attr_config_table.attr,
>  	&efi_attr_fw_platform_size.attr,
> +	&efi_attr_spec_version.attr,
>  	NULL,
>  };
>  
> -- 
> 2.7.4

Re: [PATCH 2/3] efi: add firmware version information to sysfs

[Peter Jones]

On Mon, Sep 05, 2016 at 02:00:06PM +0200, Lukas Wunner wrote:
> On Fri, Sep 02, 2016 at 04:57:52PM -0400, Peter Jones wrote:
> > This adds the EFI Spec version from the system table to sysfs as
> > /sys/firmware/efi/spec_version .
> > 
> > Userland tools need this information in order to work around
> > specification deficiencies in 2.4 and earlier firmwares.  Specifically,
> > UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
> > UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
> > firmware we'll want to work around BDS's boot order management by adding
> > fwupdate entries to BootOrder.  We'd prefer not to do this on newer
> > firmware, as it is both non-sensical in terms of what it expresses, and
> > it results in more relatively failure prone flash writes.
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
> >  drivers/firmware/efi/efi.c                   | 55 ++++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > index e794eac..4eec7c2 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-efi
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > @@ -28,3 +28,9 @@ 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/spec_version
> > +Date:		August 2016
> > +Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > +Description:	Displays the UEFI Specification revision the firmware claims to
> > +		be based upon.
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 5a2631a..3ef3a2a 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -141,11 +141,65 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
> >  	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
> >  }
> >  
> > +static ssize_t spec_version_show(struct kobject *kobj,
> > +				 struct kobj_attribute *attr,
> > +				 char *buf)
> 
> The function is renamed efi_spec_version_format() in the next patch.
> Why not call it that right away in this patch?

Well, I'd rather leave the _show bit there so we can use the same macro
as everything else for the real declaration.  It's less code, and more
readable.

> I'd change the return type to void and make this function copy
> "(unknown)" to buf on error. That way you don't have to repeat
> that over and over again in the next patch.

I've done... almost that.  I'm not sure one way is really better or
worse, to be honest.

> > +	if (!buf)
> > +		return -EINVAL;
> 
> Move that check to the top of the function so that you avoid calculating
> major and minor in vain if buf is NULL.

Sure.  Updated set to follow.

-- 
  Peter

[PATCH 1/3] efi: don't call the system table version the runtime services version

[Peter Jones]

The system table's hdr.revision is not the runtime services revision,
it's the EFI Spec revision.  The runtime services revision is the one on
systab->runtime->hdr.revision.

So we shouldn't call it runtime_version throughout the code, as that's
misleading if you're *actually* looking for the runtime services
revision.

We also move some of the assignments around just a bit, in support of
making a future patch more readable.

This also fixes a minor bug where the version field was not set in the
efi structure on ia64.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/ia64/kernel/efi.c                  | 3 +++
 arch/x86/platform/efi/efi.c             | 7 ++++---
 arch/x86/platform/efi/efi_64.c          | 2 +-
 arch/x86/xen/efi.c                      | 4 ++--
 drivers/firmware/efi/arm-init.c         | 5 +++--
 drivers/firmware/efi/runtime-wrappers.c | 8 ++++----
 drivers/xen/efi.c                       | 6 +++---
 include/linux/efi.h                     | 2 +-
 8 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 1212956..2af99a8 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -513,6 +513,9 @@ efi_init (void)
 		panic("Whoa! Can't find EFI system table.\n");
 	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		panic("Whoa! EFI system table signature incorrect\n");
+
+	efi.spec_version = efi.systab->hdr.version;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		printk(KERN_WARNING "Warning: EFI system table version "
 		       "%d.%02d, expected 1.00 or greater\n",
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 1fbb408..51c091e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -325,6 +325,9 @@ static int __init efi_systab_init(void *phys)
 		pr_err("System table signature incorrect!\n");
 		return -EINVAL;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
 		       efi.systab->hdr.revision >> 16,
@@ -843,7 +846,7 @@ static void __init kexec_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
+	efi.spec_version = efi_systab.hdr.revision;
 
 	efi_native_runtime_setup();
 
@@ -939,8 +942,6 @@ static void __init __efi_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
-
 	if (efi_is_native())
 		efi_native_runtime_setup();
 	else
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 677e29e..2366cf2 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -677,7 +677,7 @@ efi_thunk_query_variable_info(u32 attr, u64 *storage_space,
 	efi_status_t status;
 	u32 phys_storage, phys_remaining, phys_max;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	phys_storage = virt_to_phys(storage_space);
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 3be0121..e7c8357 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -56,7 +56,7 @@ static efi_system_table_t efi_systab_xen __initdata = {
 
 static const struct efi efi_xen __initconst = {
 	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
+	.spec_version		  = 0,    /* Initialized_later. */
 	.mps                      = EFI_INVALID_TABLE_ADDR,
 	.acpi                     = EFI_INVALID_TABLE_ADDR,
 	.acpi20                   = EFI_INVALID_TABLE_ADDR,
@@ -131,7 +131,7 @@ static efi_system_table_t __init *xen_efi_probe(void)
 	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
 
 	if (HYPERVISOR_platform_op(&op) == 0)
-		efi.runtime_version = info->version;
+		efi.spec_version = info->version;
 
 	return &efi_systab_xen;
 }
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index c49d50e..8f4aeba 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -112,13 +112,14 @@ static int __init uefi_init(void)
 		retval = -EINVAL;
 		goto out;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
 		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
 			efi.systab->hdr.revision >> 16,
 			efi.systab->hdr.revision & 0xffff);
 
-	efi.runtime_version = efi.systab->hdr.revision;
-
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
 				sizeof(vendor) * sizeof(efi_char16_t));
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4195877..753df73 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -196,7 +196,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -214,7 +214,7 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	if (!spin_trylock(&efi_runtime_lock))
@@ -252,7 +252,7 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -268,7 +268,7 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 22f71ff..44294b6 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -192,7 +192,7 @@ efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	efi_data(op).u.query_variable_info.attr = attr;
@@ -226,7 +226,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(update_capsule);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
@@ -247,7 +247,7 @@ efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a582..c6a3126 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -826,7 +826,7 @@ typedef struct {
  */
 extern struct efi {
 	efi_system_table_t *systab;	/* EFI system table */
-	unsigned int runtime_version;	/* Runtime services version */
+	u32 spec_version;		/* EFI Spec revision of our firmware */
 	unsigned long mps;		/* MPS table */
 	unsigned long acpi;		/* ACPI table  (IA64 ext 0.71) */
 	unsigned long acpi20;		/* ACPI table  (ACPI 2.0) */
-- 
2.7.4

[PATCH 2/3] efi: add firmware version information to sysfs

[Peter Jones]

This adds the EFI Spec version from the system table to sysfs as
/sys/firmware/efi/spec_version .

Userland tools need this information in order to work around
specification deficiencies in 2.4 and earlier firmwares.  Specifically,
UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
firmware we'll want to work around BDS's boot order management by adding
fwupdate entries to BootOrder.  We'd prefer not to do this on newer
firmware, as it is both non-sensical in terms of what it expresses, and
it results in more relatively failure prone flash writes.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
 drivers/firmware/efi/efi.c                   | 66 ++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
index e794eac..4eec7c2 100644
--- a/Documentation/ABI/testing/sysfs-firmware-efi
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -28,3 +28,9 @@ 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/spec_version
+Date:		August 2016
+Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Displays the UEFI Specification revision the firmware claims to
+		be based upon.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631a..c7cdd3f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,11 +141,76 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	u16 major, minor;
+	ssize_t rc;
+
+	if (!buf)
+		return -EINVAL;
+
+	/* The spec says:
+	 *  The revision of the EFI Specification to which this table
+	 *  conforms. The upper 16 bits of this field contain the major
+	 *  revision value, and the lower 16 bits contain the minor revision
+	 *  value. The minor revision values are binary coded decimals and are
+	 *  limited to the range of 00..99.
+	 *
+	 *  When printed or displayed UEFI spec revision is referred as (Major
+	 *  revision).(Minor revision upper decimal).(Minor revision lower
+	 *  decimal) or (Major revision).(Minor revision upper decimal) in
+	 *  case Minor revision lower decimal is set to 0. For example:
+	 *
+	 *  A specification with the revision value ((2<<16) | (30)) would be
+	 *  referred as 2.3;
+	 *  A specification with the revision value ((2<<16) | (31)) would be
+	 *  referred as 2.3.1
+	 *
+	 * Then later it says:
+	 *  Related Definitions
+	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
+	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
+	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
+	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
+	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
+	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
+	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
+	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
+	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
+	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
+	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
+	 *
+	 * (Apparently this bit of the spec failed to get updated for 2.5
+	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
+	 *
+	 * This of course does not match the description above at all, but it
+	 * does match the code in Tiano.  So we decode it Tiano's way.
+	 */
+	major = (efi.spec_version & 0xffff0000) >> 16;
+	minor = (efi.spec_version & 0x0000ffff);
+
+	if ((minor % 10) == 0)
+		rc = sprintf(buf, "%u.%u", major, minor / 10);
+	else
+		rc = sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
+
+	if (rc < 0) {
+		char *str;
+
+		str = strcpy(buf, "(unknown)");
+		return str - buf;
+	}
+	return rc;
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
 static struct kobj_attribute efi_attr_fw_platform_size =
 	__ATTR_RO(fw_platform_size);
+static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
 
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
@@ -153,6 +218,7 @@ static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_runtime.attr,
 	&efi_attr_config_table.attr,
 	&efi_attr_fw_platform_size.attr,
+	&efi_attr_spec_version.attr,
 	NULL,
 };
 
-- 
2.7.4

Re: [PATCH 2/3] efi: add firmware version information to sysfs

[Lukas Wunner]

On Tue, Sep 06, 2016 at 11:51:10AM -0400, Peter Jones wrote:
> This adds the EFI Spec version from the system table to sysfs as
> /sys/firmware/efi/spec_version .
> 
> Userland tools need this information in order to work around
> specification deficiencies in 2.4 and earlier firmwares.  Specifically,
> UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
> UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
> firmware we'll want to work around BDS's boot order management by adding
> fwupdate entries to BootOrder.  We'd prefer not to do this on newer
> firmware, as it is both non-sensical in terms of what it expresses, and
> it results in more relatively failure prone flash writes.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
>  drivers/firmware/efi/efi.c                   | 66 ++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> index e794eac..4eec7c2 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -28,3 +28,9 @@ 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/spec_version
> +Date:		August 2016
> +Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:	Displays the UEFI Specification revision the firmware claims to
> +		be based upon.
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..c7cdd3f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -141,11 +141,76 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
>  	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
>  }
>  
> +static ssize_t spec_version_show(struct kobject *kobj,
> +				 struct kobj_attribute *attr,
> +				 char *buf)
> +{
> +	u16 major, minor;
> +	ssize_t rc;
> +
> +	if (!buf)
> +		return -EINVAL;
> +
> +	/* The spec says:
> +	 *  The revision of the EFI Specification to which this table
> +	 *  conforms. The upper 16 bits of this field contain the major
> +	 *  revision value, and the lower 16 bits contain the minor revision
> +	 *  value. The minor revision values are binary coded decimals and are
> +	 *  limited to the range of 00..99.
> +	 *
> +	 *  When printed or displayed UEFI spec revision is referred as (Major
> +	 *  revision).(Minor revision upper decimal).(Minor revision lower
> +	 *  decimal) or (Major revision).(Minor revision upper decimal) in
> +	 *  case Minor revision lower decimal is set to 0. For example:
> +	 *
> +	 *  A specification with the revision value ((2<<16) | (30)) would be
> +	 *  referred as 2.3;
> +	 *  A specification with the revision value ((2<<16) | (31)) would be
> +	 *  referred as 2.3.1
> +	 *
> +	 * Then later it says:
> +	 *  Related Definitions
> +	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
> +	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
> +	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
> +	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
> +	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
> +	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
> +	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
> +	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
> +	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
> +	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> +	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
> +	 *
> +	 * (Apparently this bit of the spec failed to get updated for 2.5
> +	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
> +	 *
> +	 * This of course does not match the description above at all, but it
> +	 * does match the code in Tiano.  So we decode it Tiano's way.
> +	 */
> +	major = (efi.spec_version & 0xffff0000) >> 16;
> +	minor = (efi.spec_version & 0x0000ffff);
> +
> +	if ((minor % 10) == 0)
> +		rc = sprintf(buf, "%u.%u", major, minor / 10);
> +	else
> +		rc = sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
> +
> +	if (rc < 0) {
> +		char *str;
> +
> +		str = strcpy(buf, "(unknown)");
> +		return str - buf;

str points to the same address as buf, so the return value will be 0 here...

With that addressed,

Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

> +	}
> +	return rc;
> +}
> +
>  static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
>  static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>  static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>  static struct kobj_attribute efi_attr_fw_platform_size =
>  	__ATTR_RO(fw_platform_size);
> +static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
>  
>  static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_systab.attr,
> @@ -153,6 +218,7 @@ static struct attribute *efi_subsys_attrs[] = {
>  	&efi_attr_runtime.attr,
>  	&efi_attr_config_table.attr,
>  	&efi_attr_fw_platform_size.attr,
> +	&efi_attr_spec_version.attr,
>  	NULL,
>  };
>  
> -- 
> 2.7.4

[PATCH 1/3] efi: don't call the system table version the runtime services version

[Peter Jones]

The system table's hdr.revision is not the runtime services revision,
it's the EFI Spec revision.  The runtime services revision is the one on
systab->runtime->hdr.revision.

So we shouldn't call it runtime_version throughout the code, as that's
misleading if you're *actually* looking for the runtime services
revision.

We also move some of the assignments around just a bit, in support of
making a future patch more readable.

This also fixes a minor bug where the version field was not set in the
efi structure on ia64.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/ia64/kernel/efi.c                  | 3 +++
 arch/x86/platform/efi/efi.c             | 7 ++++---
 arch/x86/platform/efi/efi_64.c          | 2 +-
 arch/x86/xen/efi.c                      | 4 ++--
 drivers/firmware/efi/arm-init.c         | 5 +++--
 drivers/firmware/efi/runtime-wrappers.c | 8 ++++----
 drivers/xen/efi.c                       | 6 +++---
 include/linux/efi.h                     | 2 +-
 8 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 1212956..2af99a8 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -513,6 +513,9 @@ efi_init (void)
 		panic("Whoa! Can't find EFI system table.\n");
 	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		panic("Whoa! EFI system table signature incorrect\n");
+
+	efi.spec_version = efi.systab->hdr.version;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		printk(KERN_WARNING "Warning: EFI system table version "
 		       "%d.%02d, expected 1.00 or greater\n",
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 1fbb408..51c091e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -325,6 +325,9 @@ static int __init efi_systab_init(void *phys)
 		pr_err("System table signature incorrect!\n");
 		return -EINVAL;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
 		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
 		       efi.systab->hdr.revision >> 16,
@@ -843,7 +846,7 @@ static void __init kexec_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
+	efi.spec_version = efi_systab.hdr.revision;
 
 	efi_native_runtime_setup();
 
@@ -939,8 +942,6 @@ static void __init __efi_enter_virtual_mode(void)
 	 *
 	 * Call EFI services through wrapper functions.
 	 */
-	efi.runtime_version = efi_systab.hdr.revision;
-
 	if (efi_is_native())
 		efi_native_runtime_setup();
 	else
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 677e29e..2366cf2 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -677,7 +677,7 @@ efi_thunk_query_variable_info(u32 attr, u64 *storage_space,
 	efi_status_t status;
 	u32 phys_storage, phys_remaining, phys_max;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	phys_storage = virt_to_phys(storage_space);
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 3be0121..e7c8357 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -56,7 +56,7 @@ static efi_system_table_t efi_systab_xen __initdata = {
 
 static const struct efi efi_xen __initconst = {
 	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
+	.spec_version		  = 0,    /* Initialized_later. */
 	.mps                      = EFI_INVALID_TABLE_ADDR,
 	.acpi                     = EFI_INVALID_TABLE_ADDR,
 	.acpi20                   = EFI_INVALID_TABLE_ADDR,
@@ -131,7 +131,7 @@ static efi_system_table_t __init *xen_efi_probe(void)
 	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
 
 	if (HYPERVISOR_platform_op(&op) == 0)
-		efi.runtime_version = info->version;
+		efi.spec_version = info->version;
 
 	return &efi_systab_xen;
 }
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index c49d50e..8f4aeba 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -112,13 +112,14 @@ static int __init uefi_init(void)
 		retval = -EINVAL;
 		goto out;
 	}
+
+	efi.spec_version = efi.systab->hdr.revision;
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
 		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
 			efi.systab->hdr.revision >> 16,
 			efi.systab->hdr.revision & 0xffff);
 
-	efi.runtime_version = efi.systab->hdr.revision;
-
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
 				sizeof(vendor) * sizeof(efi_char16_t));
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4195877..753df73 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -196,7 +196,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -214,7 +214,7 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	if (!spin_trylock(&efi_runtime_lock))
@@ -252,7 +252,7 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
@@ -268,7 +268,7 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	efi_status_t status;
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	spin_lock(&efi_runtime_lock);
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 22f71ff..44294b6 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -192,7 +192,7 @@ efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	efi_data(op).u.query_variable_info.attr = attr;
@@ -226,7 +226,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(update_capsule);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
@@ -247,7 +247,7 @@ efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 {
 	struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
 
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+	if (efi.spec_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
 	set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a582..c6a3126 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -826,7 +826,7 @@ typedef struct {
  */
 extern struct efi {
 	efi_system_table_t *systab;	/* EFI system table */
-	unsigned int runtime_version;	/* Runtime services version */
+	u32 spec_version;		/* EFI Spec revision of our firmware */
 	unsigned long mps;		/* MPS table */
 	unsigned long acpi;		/* ACPI table  (IA64 ext 0.71) */
 	unsigned long acpi20;		/* ACPI table  (ACPI 2.0) */
-- 
2.7.4

[PATCH 2/3] efi: add firmware version information to sysfs

[Peter Jones]

This adds the EFI Spec version from the system table to sysfs as
/sys/firmware/efi/spec_version .

Userland tools need this information in order to work around
specification deficiencies in 2.4 and earlier firmwares.  Specifically,
UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
firmware we'll want to work around BDS's boot order management by adding
fwupdate entries to BootOrder.  We'd prefer not to do this on newer
firmware, as it is both non-sensical in terms of what it expresses, and
it results in more relatively failure prone flash writes.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
 drivers/firmware/efi/efi.c                   | 65 ++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
index e794eac..4eec7c2 100644
--- a/Documentation/ABI/testing/sysfs-firmware-efi
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -28,3 +28,9 @@ 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/spec_version
+Date:		August 2016
+Contact:	linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Displays the UEFI Specification revision the firmware claims to
+		be based upon.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631a..4e06e54 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,11 +141,75 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	u16 major, minor;
+	ssize_t rc;
+
+	if (!buf)
+		return -EINVAL;
+
+	/* The spec says:
+	 *  The revision of the EFI Specification to which this table
+	 *  conforms. The upper 16 bits of this field contain the major
+	 *  revision value, and the lower 16 bits contain the minor revision
+	 *  value. The minor revision values are binary coded decimals and are
+	 *  limited to the range of 00..99.
+	 *
+	 *  When printed or displayed UEFI spec revision is referred as (Major
+	 *  revision).(Minor revision upper decimal).(Minor revision lower
+	 *  decimal) or (Major revision).(Minor revision upper decimal) in
+	 *  case Minor revision lower decimal is set to 0. For example:
+	 *
+	 *  A specification with the revision value ((2<<16) | (30)) would be
+	 *  referred as 2.3;
+	 *  A specification with the revision value ((2<<16) | (31)) would be
+	 *  referred as 2.3.1
+	 *
+	 * Then later it says:
+	 *  Related Definitions
+	 *   #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
+	 *   #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
+	 *   #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
+	 *   #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
+	 *   #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
+	 *   #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
+	 *   #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
+	 *   #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
+	 *   #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
+	 *   #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
+	 *   #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
+	 *
+	 * (Apparently this bit of the spec failed to get updated for 2.5
+	 * and 2.6; UefiSpec.h in Tiano has been updated, though.)
+	 *
+	 * This of course does not match the description above at all, but it
+	 * does match the code in Tiano.  So we decode it Tiano's way.
+	 */
+	major = (efi.spec_version & 0xffff0000) >> 16;
+	minor = (efi.spec_version & 0x0000ffff);
+
+	if ((minor % 10) == 0)
+		rc = sprintf(buf, "%u.%u", major, minor / 10);
+	else
+		rc = sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
+
+	if (rc < 0) {
+		strcpy(buf, "(unknown)");
+		return strlen(buf);
+	}
+
+	return rc;
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
 static struct kobj_attribute efi_attr_fw_platform_size =
 	__ATTR_RO(fw_platform_size);
+static struct kobj_attribute efi_attr_spec_version = __ATTR_RO(spec_version);
 
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
@@ -153,6 +217,7 @@ static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_runtime.attr,
 	&efi_attr_config_table.attr,
 	&efi_attr_fw_platform_size.attr,
+	&efi_attr_spec_version.attr,
 	NULL,
 };
 
-- 
2.7.4

Re: [PATCH 2/3] efi: add firmware version information to sysfs

[Matt Fleming]

On Wed, 07 Sep, at 10:56:25AM, Peter Jones wrote:
> This adds the EFI Spec version from the system table to sysfs as
> /sys/firmware/efi/spec_version .
> 
> Userland tools need this information in order to work around
> specification deficiencies in 2.4 and earlier firmwares.  Specifically,
> UEFI 2.4 and 2.5+ treat management of BootOrder very differently (See
> UEFI 2.4 section 3.1.1 vs UEFI 2.5 section 3.1), which means on older
> firmware we'll want to work around BDS's boot order management by adding
> fwupdate entries to BootOrder.  We'd prefer not to do this on newer
> firmware, as it is both non-sensical in terms of what it expresses, and
> it results in more relatively failure prone flash writes.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-firmware-efi |  6 +++
>  drivers/firmware/efi/efi.c                   | 65 ++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)

Looks fine to me.

[PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Peter Jones]

We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
conform to the standard format.

This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
old code would have produced them, and they match the nearby code in the
functions.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/ia64/kernel/efi.c          | 14 ++++++--------
 arch/x86/platform/efi/efi.c     | 20 ++++++++++++--------
 drivers/firmware/efi/arm-init.c | 13 ++++++-------
 drivers/firmware/efi/efi.c      | 16 +++++++++++++---
 include/linux/efi.h             |  2 ++
 5 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 2af99a8..f0244bf 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -473,7 +473,7 @@ efi_init (void)
 	void *efi_map_start, *efi_map_end;
 	efi_char16_t *c16;
 	u64 efi_desc_size;
-	char *cp, vendor[100] = "unknown";
+	char *cp, vendor[100] = "unknown", version[] = "65535.255.255";
 	int i;
 
 	set_bit(EFI_BOOT, &efi.flags);
@@ -516,11 +516,11 @@ efi_init (void)
 
 	efi.spec_version = efi.systab->hdr.version;
 
+	efi_spec_version_format(version);
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
-		printk(KERN_WARNING "Warning: EFI system table version "
-		       "%d.%02d, expected 1.00 or greater\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+		printk(KERN_WARNING "Warning: EFI system table version %s, expected 1.00 or greater\n",
+		       version);
 
 	/* Show what we know for posterity */
 	c16 = __va(efi.systab->fw_vendor);
@@ -530,9 +530,7 @@ efi_init (void)
 		vendor[i] = '\0';
 	}
 
-	printk(KERN_INFO "EFI v%u.%.02u by %s:",
-	       efi.systab->hdr.revision >> 16,
-	       efi.systab->hdr.revision & 0xffff, vendor);
+	printk(KERN_INFO "EFI v%s by %s\n", version, vendor);
 
 	palo_phys      = EFI_INVALID_TABLE_ADDR;
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 51c091e..df883d0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -328,10 +328,14 @@ static int __init efi_systab_init(void *phys)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
-	if ((efi.systab->hdr.revision >> 16) == 0)
-		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+	if ((efi.systab->hdr.revision >> 16) == 0) {
+		char version[] = "65535.255.255";
+
+		efi_spec_version_format(version);
+
+		pr_err("Warning: System table version %s, expected 1.00 or greater!\n",
+		       version);
+	}
 
 	return 0;
 }
@@ -447,7 +451,7 @@ static int __init efi_memmap_init(void)
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i = 0;
 	void *tmp;
 
@@ -483,9 +487,9 @@ void __init efi_init(void)
 		pr_err("Could not map the firmware vendor!\n");
 	early_memunmap(tmp, 2);
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	efi_spec_version_format(version);
+
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	if (efi_reuse_config(efi.systab->tables, efi.systab->nr_tables))
 		return;
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 8f4aeba..2be0e5a 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -90,7 +90,7 @@ static int __init uefi_init(void)
 	efi_char16_t *c16;
 	void *config_tables;
 	size_t table_size;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i, retval;
 
 	efi.systab = early_memremap_ro(efi_system_table,
@@ -115,10 +115,11 @@ static int __init uefi_init(void)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
+	efi_spec_version_format(version);
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
-		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
-			efi.systab->hdr.revision >> 16,
-			efi.systab->hdr.revision & 0xffff);
+		pr_warn("Warning: EFI system table version %s, expected 1.00 or greater\n",
+			version);
 
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
@@ -130,9 +131,7 @@ static int __init uefi_init(void)
 		early_memunmap(c16, sizeof(vendor) * sizeof(efi_char16_t));
 	}
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
 	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4e06e54..9502c07 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,9 +141,7 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
-static ssize_t spec_version_show(struct kobject *kobj,
-				 struct kobj_attribute *attr,
-				 char *buf)
+ssize_t efi_spec_version_format(char *buf)
 {
 	u16 major, minor;
 	ssize_t rc;
@@ -204,6 +202,18 @@ static ssize_t spec_version_show(struct kobject *kobj,
 	return rc;
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	char version[] = "65535.255.255";
+	ssize_t rc;
+
+	rc = efi_spec_version_format(version);
+	strncpy(buf, version, rc);
+	return rc;
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c6a3126..d2d5c13 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -535,6 +535,8 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
 
 void efi_native_runtime_setup(void);
 
+ssize_t efi_spec_version_format(char *buf);
+
 /*
  * EFI Configuration Table and GUID definitions
  *
-- 
2.7.4

Re: [PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Matt Fleming]

On Wed, 07 Sep, at 10:56:26AM, Peter Jones wrote:
> We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
> conform to the standard format.
> 
> This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
> old code would have produced them, and they match the nearby code in the
> functions.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  arch/ia64/kernel/efi.c          | 14 ++++++--------
>  arch/x86/platform/efi/efi.c     | 20 ++++++++++++--------
>  drivers/firmware/efi/arm-init.c | 13 ++++++-------
>  drivers/firmware/efi/efi.c      | 16 +++++++++++++---
>  include/linux/efi.h             |  2 ++
>  5 files changed, 39 insertions(+), 26 deletions(-)

I'm not sure about this one. Booting with this patch on my test VMs I
see the old,

  efi: EFI v2.40 by EDK II

now reading,

  efi: EFI v2.4 by EDK II

Yes, the new string is obviously more correct but gratuitous changes
to the strings we print on boot have caused trouble in the past. If
people have scripts or tests that check the EFI version that gets
printed they'll break.

Are there any additional supporting reasons for this patch?

Re: [PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Peter Jones]

On Thu, Sep 15, 2016 at 10:18:22AM +0100, Matt Fleming wrote:
> On Wed, 07 Sep, at 10:56:26AM, Peter Jones wrote:
> > We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
> > conform to the standard format.
> > 
> > This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
> > old code would have produced them, and they match the nearby code in the
> > functions.
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  arch/ia64/kernel/efi.c          | 14 ++++++--------
> >  arch/x86/platform/efi/efi.c     | 20 ++++++++++++--------
> >  drivers/firmware/efi/arm-init.c | 13 ++++++-------
> >  drivers/firmware/efi/efi.c      | 16 +++++++++++++---
> >  include/linux/efi.h             |  2 ++
> >  5 files changed, 39 insertions(+), 26 deletions(-)
> 
> I'm not sure about this one. Booting with this patch on my test VMs I
> see the old,
> 
>   efi: EFI v2.40 by EDK II
> 
> now reading,
> 
>   efi: EFI v2.4 by EDK II
> 
> Yes, the new string is obviously more correct but gratuitous changes
> to the strings we print on boot have caused trouble in the past. If
> people have scripts or tests that check the EFI version that gets
> printed they'll break.
> 
> Are there any additional supporting reasons for this patch?

Really I just prefer to have them say the same thing the spec does - but
your point is certainly valid.  Would you be happier with it if I put a
check in that only prints the new way for, say, revisions newer than
2.6, and prints them the older way otherwise?

-- 
  Peter

Re: [PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Matt Fleming]

On Thu, 15 Sep, at 09:13:05AM, Peter Jones wrote:
> 
> Really I just prefer to have them say the same thing the spec does - but
> your point is certainly valid.  Would you be happier with it if I put a
> check in that only prints the new way for, say, revisions newer than
> 2.6, and prints them the older way otherwise?

Because /sys/firmware/efi/spec_version is a new file, I'm totally
happy with printing a correct version string there. There's really no
need to maintain the version idiosyncrasy in new places.

And given the extra code required to maintain backwards compatibility
for versions <= 2.6 but only in the boot paths, I think the simplest
solution is to drop this patch.

Re: [PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Peter Jones]

On Fri, Sep 16, 2016 at 10:40:06AM +0100, Matt Fleming wrote:
> On Thu, 15 Sep, at 09:13:05AM, Peter Jones wrote:
> > 
> > Really I just prefer to have them say the same thing the spec does - but
> > your point is certainly valid.  Would you be happier with it if I put a
> > check in that only prints the new way for, say, revisions newer than
> > 2.6, and prints them the older way otherwise?
> 
> Because /sys/firmware/efi/spec_version is a new file, I'm totally
> happy with printing a correct version string there. There's really no
> need to maintain the version idiosyncrasy in new places.
> 
> And given the extra code required to maintain backwards compatibility
> for versions <= 2.6 but only in the boot paths, I think the simplest
> solution is to drop this patch.

Okay, that's fine by me since we now have the other interface.

-- 
        Peter

Re: [PATCH 1/3] efi: don't call the system table version the runtime services version

[Matt Fleming]

On Wed, 07 Sep, at 10:56:24AM, Peter Jones wrote:
> The system table's hdr.revision is not the runtime services revision,
> it's the EFI Spec revision.  The runtime services revision is the one on
> systab->runtime->hdr.revision.
> 
> So we shouldn't call it runtime_version throughout the code, as that's
> misleading if you're *actually* looking for the runtime services
> revision.
> 
> We also move some of the assignments around just a bit, in support of
> making a future patch more readable.
> 
> This also fixes a minor bug where the version field was not set in the
> efi structure on ia64.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  arch/ia64/kernel/efi.c                  | 3 +++
>  arch/x86/platform/efi/efi.c             | 7 ++++---
>  arch/x86/platform/efi/efi_64.c          | 2 +-
>  arch/x86/xen/efi.c                      | 4 ++--
>  drivers/firmware/efi/arm-init.c         | 5 +++--
>  drivers/firmware/efi/runtime-wrappers.c | 8 ++++----
>  drivers/xen/efi.c                       | 6 +++---
>  include/linux/efi.h                     | 2 +-
>  8 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 1212956..2af99a8 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -513,6 +513,9 @@ efi_init (void)
>  		panic("Whoa! Can't find EFI system table.\n");
>  	if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
>  		panic("Whoa! EFI system table signature incorrect\n");
> +
> +	efi.spec_version = efi.systab->hdr.version;
> +
>  	if ((efi.systab->hdr.revision >> 16) == 0)
>  		printk(KERN_WARNING "Warning: EFI system table version "
>  		       "%d.%02d, expected 1.00 or greater\n",

I applied this and fixed up the above typo s/hdr.version/hdr.revision/

[PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Peter Jones]

We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
conform to the standard format.

This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
old code would have produced them, and they match the nearby code in the
functions.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/ia64/kernel/efi.c          | 14 ++++++--------
 arch/x86/platform/efi/efi.c     | 21 +++++++++++++--------
 drivers/firmware/efi/arm-init.c | 13 ++++++-------
 drivers/firmware/efi/efi.c      | 13 ++++++++++---
 include/linux/efi.h             |  2 ++
 5 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 2af99a8..f0244bf 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -473,7 +473,7 @@ efi_init (void)
 	void *efi_map_start, *efi_map_end;
 	efi_char16_t *c16;
 	u64 efi_desc_size;
-	char *cp, vendor[100] = "unknown";
+	char *cp, vendor[100] = "unknown", version[] = "65535.255.255";
 	int i;
 
 	set_bit(EFI_BOOT, &efi.flags);
@@ -516,11 +516,11 @@ efi_init (void)
 
 	efi.spec_version = efi.systab->hdr.version;
 
+	efi_spec_version_format(version);
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
-		printk(KERN_WARNING "Warning: EFI system table version "
-		       "%d.%02d, expected 1.00 or greater\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+		printk(KERN_WARNING "Warning: EFI system table version %s, expected 1.00 or greater\n",
+		       version);
 
 	/* Show what we know for posterity */
 	c16 = __va(efi.systab->fw_vendor);
@@ -530,9 +530,7 @@ efi_init (void)
 		vendor[i] = '\0';
 	}
 
-	printk(KERN_INFO "EFI v%u.%.02u by %s:",
-	       efi.systab->hdr.revision >> 16,
-	       efi.systab->hdr.revision & 0xffff, vendor);
+	printk(KERN_INFO "EFI v%s by %s\n", version, vendor);
 
 	palo_phys      = EFI_INVALID_TABLE_ADDR;
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 51c091e..9ca379b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -328,10 +328,14 @@ static int __init efi_systab_init(void *phys)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
-	if ((efi.systab->hdr.revision >> 16) == 0)
-		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+	if ((efi.systab->hdr.revision >> 16) == 0) {
+		char version[] = "65535.255.255";
+
+		efi_spec_version_format(version);
+
+		pr_err("Warning: System table version %s, expected 1.00 or greater!\n",
+		       version);
+	}
 
 	return 0;
 }
@@ -447,7 +451,7 @@ static int __init efi_memmap_init(void)
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i = 0;
 	void *tmp;
 
@@ -474,6 +478,7 @@ void __init efi_init(void)
 	/*
 	 * Show what we know for posterity
 	 */
+
 	c16 = tmp = early_memremap(efi.systab->fw_vendor, 2);
 	if (c16) {
 		for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
@@ -483,9 +488,9 @@ void __init efi_init(void)
 		pr_err("Could not map the firmware vendor!\n");
 	early_memunmap(tmp, 2);
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	efi_spec_version_format(version);
+
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	if (efi_reuse_config(efi.systab->tables, efi.systab->nr_tables))
 		return;
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 8f4aeba..2be0e5a 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -90,7 +90,7 @@ static int __init uefi_init(void)
 	efi_char16_t *c16;
 	void *config_tables;
 	size_t table_size;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i, retval;
 
 	efi.systab = early_memremap_ro(efi_system_table,
@@ -115,10 +115,11 @@ static int __init uefi_init(void)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
+	efi_spec_version_format(version);
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
-		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
-			efi.systab->hdr.revision >> 16,
-			efi.systab->hdr.revision & 0xffff);
+		pr_warn("Warning: EFI system table version %s, expected 1.00 or greater\n",
+			version);
 
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
@@ -130,9 +131,7 @@ static int __init uefi_init(void)
 		early_memunmap(c16, sizeof(vendor) * sizeof(efi_char16_t));
 	}
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
 	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index c7cdd3f..a18b71f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,9 +141,7 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
-static ssize_t spec_version_show(struct kobject *kobj,
-				 struct kobj_attribute *attr,
-				 char *buf)
+ssize_t efi_spec_version_format(char *buf)
 {
 	u16 major, minor;
 	ssize_t rc;
@@ -205,6 +203,15 @@ static ssize_t spec_version_show(struct kobject *kobj,
 	return rc;
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	char version[] = "65535.255.255";
+
+	return efi_spec_version_format(version);
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c6a3126..d2d5c13 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -535,6 +535,8 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
 
 void efi_native_runtime_setup(void);
 
+ssize_t efi_spec_version_format(char *buf);
+
 /*
  * EFI Configuration Table and GUID definitions
  *
-- 
2.7.4

Re: [PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Lukas Wunner]

On Tue, Sep 06, 2016 at 11:51:11AM -0400, Peter Jones wrote:
> We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
> conform to the standard format.
> 
> This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
> old code would have produced them, and they match the nearby code in the
> functions.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/ia64/kernel/efi.c          | 14 ++++++--------
>  arch/x86/platform/efi/efi.c     | 21 +++++++++++++--------
>  drivers/firmware/efi/arm-init.c | 13 ++++++-------
>  drivers/firmware/efi/efi.c      | 13 ++++++++++---
>  include/linux/efi.h             |  2 ++
>  5 files changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 2af99a8..f0244bf 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -473,7 +473,7 @@ efi_init (void)
>  	void *efi_map_start, *efi_map_end;
>  	efi_char16_t *c16;
>  	u64 efi_desc_size;
> -	char *cp, vendor[100] = "unknown";
> +	char *cp, vendor[100] = "unknown", version[] = "65535.255.255";
>  	int i;
>  
>  	set_bit(EFI_BOOT, &efi.flags);
> @@ -516,11 +516,11 @@ efi_init (void)
>  
>  	efi.spec_version = efi.systab->hdr.version;
>  
> +	efi_spec_version_format(version);
> +
>  	if ((efi.systab->hdr.revision >> 16) == 0)
> -		printk(KERN_WARNING "Warning: EFI system table version "
> -		       "%d.%02d, expected 1.00 or greater\n",
> -		       efi.systab->hdr.revision >> 16,
> -		       efi.systab->hdr.revision & 0xffff);
> +		printk(KERN_WARNING "Warning: EFI system table version %s, expected 1.00 or greater\n",
> +		       version);
>  
>  	/* Show what we know for posterity */
>  	c16 = __va(efi.systab->fw_vendor);
> @@ -530,9 +530,7 @@ efi_init (void)
>  		vendor[i] = '\0';
>  	}
>  
> -	printk(KERN_INFO "EFI v%u.%.02u by %s:",
> -	       efi.systab->hdr.revision >> 16,
> -	       efi.systab->hdr.revision & 0xffff, vendor);
> +	printk(KERN_INFO "EFI v%s by %s\n", version, vendor);
>  
>  	palo_phys      = EFI_INVALID_TABLE_ADDR;
>  
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 51c091e..9ca379b 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -328,10 +328,14 @@ static int __init efi_systab_init(void *phys)
>  
>  	efi.spec_version = efi.systab->hdr.revision;
>  
> -	if ((efi.systab->hdr.revision >> 16) == 0)
> -		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
> -		       efi.systab->hdr.revision >> 16,
> -		       efi.systab->hdr.revision & 0xffff);
> +	if ((efi.systab->hdr.revision >> 16) == 0) {
> +		char version[] = "65535.255.255";
> +
> +		efi_spec_version_format(version);
> +
> +		pr_err("Warning: System table version %s, expected 1.00 or greater!\n",
> +		       version);
> +	}
>  
>  	return 0;
>  }
> @@ -447,7 +451,7 @@ static int __init efi_memmap_init(void)
>  void __init efi_init(void)
>  {
>  	efi_char16_t *c16;
> -	char vendor[100] = "unknown";
> +	char vendor[100] = "unknown", version[] = "65535.255.255";
>  	int i = 0;
>  	void *tmp;
>  
> @@ -474,6 +478,7 @@ void __init efi_init(void)
>  	/*
>  	 * Show what we know for posterity
>  	 */
> +

Not sure if this added newline is intentional?

>  	c16 = tmp = early_memremap(efi.systab->fw_vendor, 2);
>  	if (c16) {
>  		for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
> @@ -483,9 +488,9 @@ void __init efi_init(void)
>  		pr_err("Could not map the firmware vendor!\n");
>  	early_memunmap(tmp, 2);
>  
> -	pr_info("EFI v%u.%.02u by %s\n",
> -		efi.systab->hdr.revision >> 16,
> -		efi.systab->hdr.revision & 0xffff, vendor);
> +	efi_spec_version_format(version);
> +
> +	pr_info("EFI v%s by %s\n", version, vendor);
>  
>  	if (efi_reuse_config(efi.systab->tables, efi.systab->nr_tables))
>  		return;
> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> index 8f4aeba..2be0e5a 100644
> --- a/drivers/firmware/efi/arm-init.c
> +++ b/drivers/firmware/efi/arm-init.c
> @@ -90,7 +90,7 @@ static int __init uefi_init(void)
>  	efi_char16_t *c16;
>  	void *config_tables;
>  	size_t table_size;
> -	char vendor[100] = "unknown";
> +	char vendor[100] = "unknown", version[] = "65535.255.255";
>  	int i, retval;
>  
>  	efi.systab = early_memremap_ro(efi_system_table,
> @@ -115,10 +115,11 @@ static int __init uefi_init(void)
>  
>  	efi.spec_version = efi.systab->hdr.revision;
>  
> +	efi_spec_version_format(version);
> +
>  	if ((efi.systab->hdr.revision >> 16) < 2)
> -		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
> -			efi.systab->hdr.revision >> 16,
> -			efi.systab->hdr.revision & 0xffff);
> +		pr_warn("Warning: EFI system table version %s, expected 1.00 or greater\n",
> +			version);
>  
>  	/* Show what we know for posterity */
>  	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
> @@ -130,9 +131,7 @@ static int __init uefi_init(void)
>  		early_memunmap(c16, sizeof(vendor) * sizeof(efi_char16_t));
>  	}
>  
> -	pr_info("EFI v%u.%.02u by %s\n",
> -		efi.systab->hdr.revision >> 16,
> -		efi.systab->hdr.revision & 0xffff, vendor);
> +	pr_info("EFI v%s by %s\n", version, vendor);
>  
>  	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
>  	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index c7cdd3f..a18b71f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -141,9 +141,7 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
>  	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
>  }
>  
> -static ssize_t spec_version_show(struct kobject *kobj,
> -				 struct kobj_attribute *attr,
> -				 char *buf)
> +ssize_t efi_spec_version_format(char *buf)
>  {
>  	u16 major, minor;
>  	ssize_t rc;
> @@ -205,6 +203,15 @@ static ssize_t spec_version_show(struct kobject *kobj,
>  	return rc;
>  }
>  
> +static ssize_t spec_version_show(struct kobject *kobj,
> +				 struct kobj_attribute *attr,
> +				 char *buf)
> +{
> +	char version[] = "65535.255.255";
> +
> +	return efi_spec_version_format(version);

Hm, shouldn't version be copied to buf?

Apart from that,

Reviewed-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

> +}
> +
>  static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
>  static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>  static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c6a3126..d2d5c13 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -535,6 +535,8 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
>  
>  void efi_native_runtime_setup(void);
>  
> +ssize_t efi_spec_version_format(char *buf);
> +
>  /*
>   * EFI Configuration Table and GUID definitions
>   *
> -- 
> 2.7.4

[PATCH 3/3] efi: Format EFI version prints the way the standard says.

[Peter Jones]

We print "EFI v2.xx.yy vendor blahblah" at several places.  Make them
conform to the standard format.

This leaves 2 checkpatch warnings in arch/ia64/kernel/efi.c intact; the
old code would have produced them, and they match the nearby code in the
functions.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/ia64/kernel/efi.c          | 15 +++++++--------
 arch/x86/platform/efi/efi.c     | 23 +++++++++++++++--------
 drivers/firmware/efi/arm-init.c | 14 +++++++-------
 drivers/firmware/efi/efi.c      | 18 +++++++++++++++---
 include/linux/efi.h             |  2 ++
 5 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 2af99a8..bf16028 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -473,7 +473,7 @@ efi_init (void)
 	void *efi_map_start, *efi_map_end;
 	efi_char16_t *c16;
 	u64 efi_desc_size;
-	char *cp, vendor[100] = "unknown";
+	char *cp, vendor[100] = "unknown", version[] = "65535.255.255";
 	int i;
 
 	set_bit(EFI_BOOT, &efi.flags);
@@ -516,11 +516,12 @@ efi_init (void)
 
 	efi.spec_version = efi.systab->hdr.version;
 
+	if (efi_spec_version_format(version) <= 0)
+		strcpy(version, "(unknown)");
+
 	if ((efi.systab->hdr.revision >> 16) == 0)
-		printk(KERN_WARNING "Warning: EFI system table version "
-		       "%d.%02d, expected 1.00 or greater\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+		printk(KERN_WARNING "Warning: EFI system table version %s, expected 1.00 or greater\n",
+		       version);
 
 	/* Show what we know for posterity */
 	c16 = __va(efi.systab->fw_vendor);
@@ -530,9 +531,7 @@ efi_init (void)
 		vendor[i] = '\0';
 	}
 
-	printk(KERN_INFO "EFI v%u.%.02u by %s:",
-	       efi.systab->hdr.revision >> 16,
-	       efi.systab->hdr.revision & 0xffff, vendor);
+	printk(KERN_INFO "EFI v%s by %s\n", version, vendor);
 
 	palo_phys      = EFI_INVALID_TABLE_ADDR;
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 51c091e..245e3e0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -328,10 +328,15 @@ static int __init efi_systab_init(void *phys)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
-	if ((efi.systab->hdr.revision >> 16) == 0)
-		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
-		       efi.systab->hdr.revision >> 16,
-		       efi.systab->hdr.revision & 0xffff);
+	if ((efi.systab->hdr.revision >> 16) == 0) {
+		char version[] = "65535.255.255";
+
+		if (efi_spec_version_format(version) <= 0)
+			strcpy(version, "(unknown)");
+
+		pr_err("Warning: System table version %s, expected 1.00 or greater!\n",
+		       version);
+	}
 
 	return 0;
 }
@@ -447,7 +452,7 @@ static int __init efi_memmap_init(void)
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i = 0;
 	void *tmp;
 
@@ -474,6 +479,7 @@ void __init efi_init(void)
 	/*
 	 * Show what we know for posterity
 	 */
+
 	c16 = tmp = early_memremap(efi.systab->fw_vendor, 2);
 	if (c16) {
 		for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
@@ -483,9 +489,10 @@ void __init efi_init(void)
 		pr_err("Could not map the firmware vendor!\n");
 	early_memunmap(tmp, 2);
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	if (efi_spec_version_format(version) <= 0)
+		strcpy(version, "(unknown)");
+
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	if (efi_reuse_config(efi.systab->tables, efi.systab->nr_tables))
 		return;
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 8f4aeba..59dba34 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -90,7 +90,7 @@ static int __init uefi_init(void)
 	efi_char16_t *c16;
 	void *config_tables;
 	size_t table_size;
-	char vendor[100] = "unknown";
+	char vendor[100] = "unknown", version[] = "65535.255.255";
 	int i, retval;
 
 	efi.systab = early_memremap_ro(efi_system_table,
@@ -115,10 +115,12 @@ static int __init uefi_init(void)
 
 	efi.spec_version = efi.systab->hdr.revision;
 
+	if (efi_spec_version_format(version) <= 0)
+		strcpy(version, "(unknown)");
+
 	if ((efi.systab->hdr.revision >> 16) < 2)
-		pr_warn("Warning: EFI system table version %d.%02d, expected 2.00 or greater\n",
-			efi.systab->hdr.revision >> 16,
-			efi.systab->hdr.revision & 0xffff);
+		pr_warn("Warning: EFI system table version %s, expected 1.00 or greater\n",
+			version);
 
 	/* Show what we know for posterity */
 	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
@@ -130,9 +132,7 @@ static int __init uefi_init(void)
 		early_memunmap(c16, sizeof(vendor) * sizeof(efi_char16_t));
 	}
 
-	pr_info("EFI v%u.%.02u by %s\n",
-		efi.systab->hdr.revision >> 16,
-		efi.systab->hdr.revision & 0xffff, vendor);
+	pr_info("EFI v%s by %s\n", version, vendor);
 
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
 	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3ef3a2a..d78e3d6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -141,9 +141,7 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
 }
 
-static ssize_t spec_version_show(struct kobject *kobj,
-				 struct kobj_attribute *attr,
-				 char *buf)
+ssize_t efi_spec_version_format(char *buf)
 {
 	/* The spec says:
 	 *  The revision of the EFI Specification to which this table
@@ -194,6 +192,20 @@ static ssize_t spec_version_show(struct kobject *kobj,
 		return sprintf(buf, "%u.%u.%u", major, minor / 10, minor % 10);
 }
 
+static ssize_t spec_version_show(struct kobject *kobj,
+				 struct kobj_attribute *attr,
+				 char *buf)
+{
+	char version[] = "65535.255.255";
+	ssize_t sz;
+
+	sz = efi_spec_version_format(version);
+	if (sz < 0)
+		return sprintf(buf, "(unknown");
+
+	return sprintf(buf, "%s\n", version);
+}
+
 static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
 static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
 static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c6a3126..d2d5c13 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -535,6 +535,8 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
 
 void efi_native_runtime_setup(void);
 
+ssize_t efi_spec_version_format(char *buf);
+
 /*
  * EFI Configuration Table and GUID definitions
  *
-- 
2.7.4