[U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
Starting with commit 867a6ac86dd8 ("efi: Add start-up library code"), sparse constantly complains about truncated constant value in efi.h:
include/efi.h:176:35: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
This can get quite noisy, preventing real issues to be noticed:
$ make defconfig *** Default configuration is based on 'sandbox_defconfig' $ make C=2 -j12 2>&1 | grep truncates | wc -l 441
After the patch is applied: $ make C=2 -j12 2>&1 | grep truncates | wc -l 0 $ sparse --version v0.5.2
Following the suggestion of Heinrich Schuchardt, instead of only fixing the root-cause, I replaced the whole enum of _SHIFT values by ULL defines. This matches both the UEFI 2.7 spec and the Linux kernel implementation.
Some ELF size comparison before and after the patch (gcc 7.3.0):
efi-x86_payload64_defconfig: text data bss dec hex filename 407174 29432 278676 715282 aea12 u-boot.old 407152 29464 278676 715292 aea1c u-boot.new -22 +32 0 +10
efi-x86_payload32_defconfig: text data bss dec hex filename 447075 30308 280076 757459 b8ed3 u-boot.old 447053 30340 280076 757469 b8edd u-boot.new -22 +32 0 +10
Fixes: 867a6ac86dd8 ("efi: Add start-up library code") Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com ---
v2: - Replace _SHIFT enum values by ULL defines to match UEFI 2.7 spec - Add ELF size comparison to patch description
cmd/efi.c | 22 +++++++++++----------- include/efi.h | 24 ++++++++++-------------- lib/efi_loader/efi_memory.c | 7 +++---- 3 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/cmd/efi.c b/cmd/efi.c index 6c1eb88424be..92a565f71373 100644 --- a/cmd/efi.c +++ b/cmd/efi.c @@ -28,18 +28,18 @@ static const char *const type_name[] = { };
static struct attr_info { - int shift; + u64 val; const char *name; } mem_attr[] = { - { EFI_MEMORY_UC_SHIFT, "uncached" }, - { EFI_MEMORY_WC_SHIFT, "write-coalescing" }, - { EFI_MEMORY_WT_SHIFT, "write-through" }, - { EFI_MEMORY_WB_SHIFT, "write-back" }, - { EFI_MEMORY_UCE_SHIFT, "uncached & exported" }, - { EFI_MEMORY_WP_SHIFT, "write-protect" }, - { EFI_MEMORY_RP_SHIFT, "read-protect" }, - { EFI_MEMORY_XP_SHIFT, "execute-protect" }, - { EFI_MEMORY_RUNTIME_SHIFT, "needs runtime mapping" } + { EFI_MEMORY_UC, "uncached" }, + { EFI_MEMORY_WC, "write-coalescing" }, + { EFI_MEMORY_WT, "write-through" }, + { EFI_MEMORY_WB, "write-back" }, + { EFI_MEMORY_UCE, "uncached & exported" }, + { EFI_MEMORY_WP, "write-protect" }, + { EFI_MEMORY_RP, "read-protect" }, + { EFI_MEMORY_XP, "execute-protect" }, + { EFI_MEMORY_RUNTIME, "needs runtime mapping" } };
/* Maximum different attribute values we can track */ @@ -173,7 +173,7 @@ static void efi_print_mem_table(struct efi_entry_memmap *map, printf("%c%llx: ", attr & EFI_MEMORY_RUNTIME ? 'r' : ' ', attr & ~EFI_MEMORY_RUNTIME); for (j = 0, first = true; j < ARRAY_SIZE(mem_attr); j++) { - if (attr & (1ULL << mem_attr[j].shift)) { + if (attr & mem_attr[j].val) { if (first) first = false; else diff --git a/include/efi.h b/include/efi.h index 0fe15e65c06c..eb2a569fe010 100644 --- a/include/efi.h +++ b/include/efi.h @@ -162,20 +162,16 @@ enum efi_mem_type { };
/* Attribute values */ -enum { - EFI_MEMORY_UC_SHIFT = 0, /* uncached */ - EFI_MEMORY_WC_SHIFT = 1, /* write-coalescing */ - EFI_MEMORY_WT_SHIFT = 2, /* write-through */ - EFI_MEMORY_WB_SHIFT = 3, /* write-back */ - EFI_MEMORY_UCE_SHIFT = 4, /* uncached, exported */ - EFI_MEMORY_WP_SHIFT = 12, /* write-protect */ - EFI_MEMORY_RP_SHIFT = 13, /* read-protect */ - EFI_MEMORY_XP_SHIFT = 14, /* execute-protect */ - EFI_MEMORY_RUNTIME_SHIFT = 63, /* range requires runtime mapping */ - - EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT, - EFI_MEM_DESC_VERSION = 1, -}; +#define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */ +#define EFI_MEMORY_WC ((u64)0x0000000000000002ULL) /* write-coalescing */ +#define EFI_MEMORY_WT ((u64)0x0000000000000004ULL) /* write-through */ +#define EFI_MEMORY_WB ((u64)0x0000000000000008ULL) /* write-back */ +#define EFI_MEMORY_UCE ((u64)0x0000000000000010ULL) /* uncached, exported */ +#define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ +#define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ +#define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ +#define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */ +#define EFI_MEM_DESC_VERSION 1
#define EFI_PAGE_SHIFT 12 #define EFI_PAGE_SIZE (1UL << EFI_PAGE_SHIFT) diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea8f..233333bf6b18 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -171,14 +171,13 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, switch (memory_type) { case EFI_RUNTIME_SERVICES_CODE: case EFI_RUNTIME_SERVICES_DATA: - newlist->desc.attribute = (1 << EFI_MEMORY_WB_SHIFT) | - (1ULL << EFI_MEMORY_RUNTIME_SHIFT); + newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME; break; case EFI_MMAP_IO: - newlist->desc.attribute = 1ULL << EFI_MEMORY_RUNTIME_SHIFT; + newlist->desc.attribute = EFI_MEMORY_RUNTIME; break; default: - newlist->desc.attribute = 1 << EFI_MEMORY_WB_SHIFT; + newlist->desc.attribute = EFI_MEMORY_WB; break; }
With this update, the memory attributes are in sync with Linux kernel v4.18-rc4. They also match page 190 of UEFI 2.7 spec [1].
[1] http://www.uefi.org/sites/default/files/resources/UEFI_Spec_2_7.pdf
Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com ---
v2: - Newly added
cmd/efi.c | 3 +++ include/efi.h | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/cmd/efi.c b/cmd/efi.c index 92a565f71373..366a79a96488 100644 --- a/cmd/efi.c +++ b/cmd/efi.c @@ -39,6 +39,9 @@ static struct attr_info { { EFI_MEMORY_WP, "write-protect" }, { EFI_MEMORY_RP, "read-protect" }, { EFI_MEMORY_XP, "execute-protect" }, + { EFI_MEMORY_NV, "non-volatile" }, + { EFI_MEMORY_MORE_RELIABLE, "higher reliability" }, + { EFI_MEMORY_RO, "read-only" }, { EFI_MEMORY_RUNTIME, "needs runtime mapping" } };
diff --git a/include/efi.h b/include/efi.h index eb2a569fe010..046decba3d58 100644 --- a/include/efi.h +++ b/include/efi.h @@ -170,6 +170,10 @@ enum efi_mem_type { #define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ #define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ #define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ +#define EFI_MEMORY_NV ((u64)0x0000000000008000ULL) /* non-volatile */ +#define EFI_MEMORY_MORE_RELIABLE \ + ((u64)0x0000000000010000ULL) /* higher reliability */ +#define EFI_MEMORY_RO ((u64)0x0000000000020000ULL) /* read-only */ #define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */ #define EFI_MEM_DESC_VERSION 1
On 07/14/2018 10:53 PM, Eugeniu Rosca wrote:
With this update, the memory attributes are in sync with Linux kernel v4.18-rc4. They also match page 190 of UEFI 2.7 spec [1].
[1] http://www.uefi.org/sites/default/files/resources/UEFI_Spec_2_7.pdf
Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
With this update, the memory attributes are in sync with Linux kernel v4.18-rc4. They also match page 190 of UEFI 2.7 spec [1].
[1] http://www.uefi.org/sites/default/files/resources/UEFI_Spec_2_7.pdf
Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Thanks, applied to efi-2018.09
Alex
Fix cppcheck complaint: [cmd/efi.c:173]: (style) Clarify calculation precedence for '&' and '?'.
Fixes: f1a0bafb5802 ("efi: Add a command to display the memory map") Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com ---
v2: - newly added
cmd/efi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/efi.c b/cmd/efi.c index 366a79a96488..919cb2fcfd55 100644 --- a/cmd/efi.c +++ b/cmd/efi.c @@ -173,7 +173,7 @@ static void efi_print_mem_table(struct efi_entry_memmap *map, bool first; int j;
- printf("%c%llx: ", attr & EFI_MEMORY_RUNTIME ? 'r' : ' ', + printf("%c%llx: ", (attr & EFI_MEMORY_RUNTIME) ? 'r' : ' ', attr & ~EFI_MEMORY_RUNTIME); for (j = 0, first = true; j < ARRAY_SIZE(mem_attr); j++) { if (attr & mem_attr[j].val) {
On 07/14/2018 10:53 PM, Eugeniu Rosca wrote:
Fix cppcheck complaint: [cmd/efi.c:173]: (style) Clarify calculation precedence for '&' and '?'.
Fixes: f1a0bafb5802 ("efi: Add a command to display the memory map") Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Fix cppcheck complaint: [cmd/efi.c:173]: (style) Clarify calculation precedence for '&' and '?'.
Fixes: f1a0bafb5802 ("efi: Add a command to display the memory map") Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Thanks, applied to efi-2018.09
Alex
On 07/14/2018 10:53 PM, Eugeniu Rosca wrote:
Starting with commit 867a6ac86dd8 ("efi: Add start-up library code"), sparse constantly complains about truncated constant value in efi.h:
include/efi.h:176:35: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
This can get quite noisy, preventing real issues to be noticed:
$ make defconfig *** Default configuration is based on 'sandbox_defconfig' $ make C=2 -j12 2>&1 | grep truncates | wc -l 441
After the patch is applied: $ make C=2 -j12 2>&1 | grep truncates | wc -l 0 $ sparse --version v0.5.2
Following the suggestion of Heinrich Schuchardt, instead of only fixing the root-cause, I replaced the whole enum of _SHIFT values by ULL defines. This matches both the UEFI 2.7 spec and the Linux kernel implementation.
Some ELF size comparison before and after the patch (gcc 7.3.0):
efi-x86_payload64_defconfig: text data bss dec hex filename 407174 29432 278676 715282 aea12 u-boot.old 407152 29464 278676 715292 aea1c u-boot.new -22 +32 0 +10
efi-x86_payload32_defconfig: text data bss dec hex filename 447075 30308 280076 757459 b8ed3 u-boot.old 447053 30340 280076 757469 b8edd u-boot.new -22 +32 0 +10
Fixes: 867a6ac86dd8 ("efi: Add start-up library code") Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
v2:
- Replace _SHIFT enum values by ULL defines to match UEFI 2.7 spec
- Add ELF size comparison to patch description
cmd/efi.c | 22 +++++++++++----------- include/efi.h | 24 ++++++++++-------------- lib/efi_loader/efi_memory.c | 7 +++---- 3 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/cmd/efi.c b/cmd/efi.c index 6c1eb88424be..92a565f71373 100644 --- a/cmd/efi.c +++ b/cmd/efi.c @@ -28,18 +28,18 @@ static const char *const type_name[] = { };
static struct attr_info {
- int shift;
- u64 val; const char *name;
} mem_attr[] = {
- { EFI_MEMORY_UC_SHIFT, "uncached" },
- { EFI_MEMORY_WC_SHIFT, "write-coalescing" },
- { EFI_MEMORY_WT_SHIFT, "write-through" },
- { EFI_MEMORY_WB_SHIFT, "write-back" },
- { EFI_MEMORY_UCE_SHIFT, "uncached & exported" },
- { EFI_MEMORY_WP_SHIFT, "write-protect" },
- { EFI_MEMORY_RP_SHIFT, "read-protect" },
- { EFI_MEMORY_XP_SHIFT, "execute-protect" },
- { EFI_MEMORY_RUNTIME_SHIFT, "needs runtime mapping" }
- { EFI_MEMORY_UC, "uncached" },
- { EFI_MEMORY_WC, "write-coalescing" },
- { EFI_MEMORY_WT, "write-through" },
- { EFI_MEMORY_WB, "write-back" },
- { EFI_MEMORY_UCE, "uncached & exported" },
- { EFI_MEMORY_WP, "write-protect" },
- { EFI_MEMORY_RP, "read-protect" },
- { EFI_MEMORY_XP, "execute-protect" },
- { EFI_MEMORY_RUNTIME, "needs runtime mapping" }
};
/* Maximum different attribute values we can track */ @@ -173,7 +173,7 @@ static void efi_print_mem_table(struct efi_entry_memmap *map, printf("%c%llx: ", attr & EFI_MEMORY_RUNTIME ? 'r' : ' ', attr & ~EFI_MEMORY_RUNTIME); for (j = 0, first = true; j < ARRAY_SIZE(mem_attr); j++) {
if (attr & (1ULL << mem_attr[j].shift)) {
if (attr & mem_attr[j].val) { if (first) first = false; elsediff --git a/include/efi.h b/include/efi.h index 0fe15e65c06c..eb2a569fe010 100644 --- a/include/efi.h +++ b/include/efi.h @@ -162,20 +162,16 @@ enum efi_mem_type { };
/* Attribute values */ -enum {
- EFI_MEMORY_UC_SHIFT = 0, /* uncached */
- EFI_MEMORY_WC_SHIFT = 1, /* write-coalescing */
- EFI_MEMORY_WT_SHIFT = 2, /* write-through */
- EFI_MEMORY_WB_SHIFT = 3, /* write-back */
- EFI_MEMORY_UCE_SHIFT = 4, /* uncached, exported */
- EFI_MEMORY_WP_SHIFT = 12, /* write-protect */
- EFI_MEMORY_RP_SHIFT = 13, /* read-protect */
- EFI_MEMORY_XP_SHIFT = 14, /* execute-protect */
- EFI_MEMORY_RUNTIME_SHIFT = 63, /* range requires runtime mapping */
- EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT,
- EFI_MEM_DESC_VERSION = 1,
-}; +#define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */
Unsigned long long int (ULL) is at least 64bit wide and unsigned (cf. https://gcc.gnu.org/onlinedocs/gcc/Long-Long.html). Why do you introduce the (u64) conversion?
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
+#define EFI_MEMORY_WC ((u64)0x0000000000000002ULL) /* write-coalescing */ +#define EFI_MEMORY_WT ((u64)0x0000000000000004ULL) /* write-through */ +#define EFI_MEMORY_WB ((u64)0x0000000000000008ULL) /* write-back */ +#define EFI_MEMORY_UCE ((u64)0x0000000000000010ULL) /* uncached, exported */ +#define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ +#define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ +#define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ +#define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */ +#define EFI_MEM_DESC_VERSION 1
#define EFI_PAGE_SHIFT 12 #define EFI_PAGE_SIZE (1UL << EFI_PAGE_SHIFT) diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea8f..233333bf6b18 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -171,14 +171,13 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, switch (memory_type) { case EFI_RUNTIME_SERVICES_CODE: case EFI_RUNTIME_SERVICES_DATA:
newlist->desc.attribute = (1 << EFI_MEMORY_WB_SHIFT) |(1ULL << EFI_MEMORY_RUNTIME_SHIFT);
newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
newlist->desc.attribute = 1ULL << EFI_MEMORY_RUNTIME_SHIFT;
newlist->desc.attribute = EFI_MEMORY_RUNTIME;
newlist->desc.attribute = 1 << EFI_MEMORY_WB_SHIFT;
newlist->desc.attribute = EFI_MEMORY_WB;
Hi Heinrich,
Thanks for your review comments. See my reply below.
On Mon, Jul 16, 2018 at 07:52:20AM +0200, Heinrich Schuchardt wrote: [--snip--]
diff --git a/include/efi.h b/include/efi.h index 0fe15e65c06c..eb2a569fe010 100644 --- a/include/efi.h +++ b/include/efi.h @@ -162,20 +162,16 @@ enum efi_mem_type { };
/* Attribute values */ -enum {
- EFI_MEMORY_UC_SHIFT = 0, /* uncached */
- EFI_MEMORY_WC_SHIFT = 1, /* write-coalescing */
- EFI_MEMORY_WT_SHIFT = 2, /* write-through */
- EFI_MEMORY_WB_SHIFT = 3, /* write-back */
- EFI_MEMORY_UCE_SHIFT = 4, /* uncached, exported */
- EFI_MEMORY_WP_SHIFT = 12, /* write-protect */
- EFI_MEMORY_RP_SHIFT = 13, /* read-protect */
- EFI_MEMORY_XP_SHIFT = 14, /* execute-protect */
- EFI_MEMORY_RUNTIME_SHIFT = 63, /* range requires runtime mapping */
- EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT,
- EFI_MEM_DESC_VERSION = 1,
-}; +#define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */
Unsigned long long int (ULL) is at least 64bit wide and unsigned (cf. https://gcc.gnu.org/onlinedocs/gcc/Long-Long.html). Why do you introduce the (u64) conversion?
Because this is how it appears in Linux kernel:
$ git blame ./include/linux/efi.h | grep 'define EFI_MEMORY_.*u64' ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 90) #define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 91) #define EFI_MEMORY_WC ((u64)0x0000000000000002ULL) /* write-coalescing */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 92) #define EFI_MEMORY_WT ((u64)0x0000000000000004ULL) /* write-through */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 93) #define EFI_MEMORY_WB ((u64)0x0000000000000008ULL) /* write-back */ 9c97e0bdd4b4a (Laszlo Ersek 2014-09-03 13:32:19 +0200 94) #define EFI_MEMORY_UCE ((u64)0x0000000000000010ULL) /* uncached, exported */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 95) #define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 96) #define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 97) #define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ c016ca08f89c6 (Robert Elliott 2016-02-01 22:07:06 +0000 98) #define EFI_MEMORY_NV ((u64)0x0000000000008000ULL) /* non-volatile */ 87db73aebf555 (Ard Biesheuvel 2015-08-07 09:36:54 +0100 101) #define EFI_MEMORY_RO ((u64)0x0000000000020000ULL) /* read-only */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 102) #define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */
Unless we see a potential issue with that (and I don't see), IMO we should stick to kernel sources as much as possible, since this makes integration/re-sync process easier.
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
+#define EFI_MEMORY_WC ((u64)0x0000000000000002ULL) /* write-coalescing */ +#define EFI_MEMORY_WT ((u64)0x0000000000000004ULL) /* write-through */ +#define EFI_MEMORY_WB ((u64)0x0000000000000008ULL) /* write-back */ +#define EFI_MEMORY_UCE ((u64)0x0000000000000010ULL) /* uncached, exported */ +#define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ +#define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ +#define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ +#define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */ +#define EFI_MEM_DESC_VERSION 1
#define EFI_PAGE_SHIFT 12 #define EFI_PAGE_SIZE (1UL << EFI_PAGE_SHIFT)
Best regards, Eugeniu.
On 07/16/2018 08:12 AM, Eugeniu Rosca wrote:
Hi Heinrich,
Thanks for your review comments. See my reply below.
On Mon, Jul 16, 2018 at 07:52:20AM +0200, Heinrich Schuchardt wrote: [--snip--]
diff --git a/include/efi.h b/include/efi.h index 0fe15e65c06c..eb2a569fe010 100644 --- a/include/efi.h +++ b/include/efi.h @@ -162,20 +162,16 @@ enum efi_mem_type { };
/* Attribute values */ -enum {
- EFI_MEMORY_UC_SHIFT = 0, /* uncached */
- EFI_MEMORY_WC_SHIFT = 1, /* write-coalescing */
- EFI_MEMORY_WT_SHIFT = 2, /* write-through */
- EFI_MEMORY_WB_SHIFT = 3, /* write-back */
- EFI_MEMORY_UCE_SHIFT = 4, /* uncached, exported */
- EFI_MEMORY_WP_SHIFT = 12, /* write-protect */
- EFI_MEMORY_RP_SHIFT = 13, /* read-protect */
- EFI_MEMORY_XP_SHIFT = 14, /* execute-protect */
- EFI_MEMORY_RUNTIME_SHIFT = 63, /* range requires runtime mapping */
- EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT,
- EFI_MEM_DESC_VERSION = 1,
-}; +#define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */
Unsigned long long int (ULL) is at least 64bit wide and unsigned (cf. https://gcc.gnu.org/onlinedocs/gcc/Long-Long.html). Why do you introduce the (u64) conversion?
Because this is how it appears in Linux kernel:
$ git blame ./include/linux/efi.h | grep 'define EFI_MEMORY_.*u64' ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 90) #define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 91) #define EFI_MEMORY_WC ((u64)0x0000000000000002ULL) /* write-coalescing */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 92) #define EFI_MEMORY_WT ((u64)0x0000000000000004ULL) /* write-through */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 93) #define EFI_MEMORY_WB ((u64)0x0000000000000008ULL) /* write-back */ 9c97e0bdd4b4a (Laszlo Ersek 2014-09-03 13:32:19 +0200 94) #define EFI_MEMORY_UCE ((u64)0x0000000000000010ULL) /* uncached, exported */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 95) #define EFI_MEMORY_WP ((u64)0x0000000000001000ULL) /* write-protect */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 96) #define EFI_MEMORY_RP ((u64)0x0000000000002000ULL) /* read-protect */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 97) #define EFI_MEMORY_XP ((u64)0x0000000000004000ULL) /* execute-protect */ c016ca08f89c6 (Robert Elliott 2016-02-01 22:07:06 +0000 98) #define EFI_MEMORY_NV ((u64)0x0000000000008000ULL) /* non-volatile */ 87db73aebf555 (Ard Biesheuvel 2015-08-07 09:36:54 +0100 101) #define EFI_MEMORY_RO ((u64)0x0000000000020000ULL) /* read-only */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 102) #define EFI_MEMORY_RUNTIME ((u64)0x8000000000000000ULL) /* range requires runtime mapping */
Unless we see a potential issue with that (and I don't see), IMO we should stick to kernel sources as much as possible, since this makes integration/re-sync process easier.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Hello Alexander,
Heinrich was kind to have a look at [1] and already provided his Reviewed-by. Could you please state your further expectations to accept the patches?
[1] https://patchwork.ozlabs.org/patch/944004/
Thanks, Eugeniu.
Hello Tom, Simon, Alexander, Heinrich,
On Wed, Jul 25, 2018 at 03:30:16PM +0200, Eugeniu Rosca wrote:
Hello Alexander,
Heinrich was kind to have a look at [1] and already provided his Reviewed-by. Could you please state your further expectations to accept the patches?
[1] https://patchwork.ozlabs.org/patch/944004/
Thanks, Eugeniu.
Any idea how to move forward with these patches?
Best regards, Eugeniu.
Hello Tom, Alexander,
On Wed, Aug 01, 2018 at 01:25:54PM +0200, Eugeniu Rosca wrote:
Hello Tom, Simon, Alexander, Heinrich,
On Wed, Jul 25, 2018 at 03:30:16PM +0200, Eugeniu Rosca wrote:
Hello Alexander,
Heinrich was kind to have a look at [1] and already provided his Reviewed-by. Could you please state your further expectations to accept the patches?
[1] https://patchwork.ozlabs.org/patch/944004/
Thanks, Eugeniu.
Any idea how to move forward with these patches?
I apologize for this second reminder. Could you please suggest the next steps for this simple set of three fixes? Thank you.
Best regards, Eugeniu.
Hi,
On 8 August 2018 at 14:41, Eugeniu Rosca roscaeugeniu@gmail.com wrote:
Hello Tom, Alexander,
On Wed, Aug 01, 2018 at 01:25:54PM +0200, Eugeniu Rosca wrote:
Hello Tom, Simon, Alexander, Heinrich,
On Wed, Jul 25, 2018 at 03:30:16PM +0200, Eugeniu Rosca wrote:
Hello Alexander,
Heinrich was kind to have a look at [1] and already provided his Reviewed-by. Could you please state your further expectations to accept the patches?
[1] https://patchwork.ozlabs.org/patch/944004/
Thanks, Eugeniu.
Any idea how to move forward with these patches?
I apologize for this second reminder. Could you please suggest the next steps for this simple set of three fixes? Thank you.
Best regards, Eugeniu.
I believe Alex should pick these up.
Regards, Simon
Starting with commit 867a6ac86dd8 ("efi: Add start-up library code"), sparse constantly complains about truncated constant value in efi.h:
include/efi.h:176:35: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
This can get quite noisy, preventing real issues to be noticed:
$ make defconfig *** Default configuration is based on 'sandbox_defconfig' $ make C=2 -j12 2>&1 | grep truncates | wc -l 441
After the patch is applied: $ make C=2 -j12 2>&1 | grep truncates | wc -l 0 $ sparse --version v0.5.2
Following the suggestion of Heinrich Schuchardt, instead of only fixing the root-cause, I replaced the whole enum of _SHIFT values by ULL defines. This matches both the UEFI 2.7 spec and the Linux kernel implementation.
Some ELF size comparison before and after the patch (gcc 7.3.0):
efi-x86_payload64_defconfig: text data bss dec hex filename 407174 29432 278676 715282 aea12 u-boot.old 407152 29464 278676 715292 aea1c u-boot.new -22 +32 0 +10
efi-x86_payload32_defconfig: text data bss dec hex filename 447075 30308 280076 757459 b8ed3 u-boot.old 447053 30340 280076 757469 b8edd u-boot.new -22 +32 0 +10
Fixes: 867a6ac86dd8 ("efi: Add start-up library code") Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Thanks, applied to efi-2018.09
Alex
participants (5)
-
Alexander Graf -
Eugeniu Rosca -
Eugeniu Rosca -
Heinrich Schuchardt -
Simon Glass