* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
@ 2018-07-14 20:53 Eugeniu Rosca
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2018-07-14 20:53 UTC (permalink / raw)
To: u-boot
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;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes
2018-07-14 20:53 [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Eugeniu Rosca
@ 2018-07-14 20:53 ` Eugeniu Rosca
2018-07-16 5:58 ` Heinrich Schuchardt
2018-08-20 22:05 ` [U-Boot] [U-Boot, v2, " Alexander Graf
2018-07-14 20:53 ` [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?' Eugeniu Rosca
` (2 subsequent siblings)
3 siblings, 2 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2018-07-14 20:53 UTC (permalink / raw)
To: u-boot
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
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?'
2018-07-14 20:53 [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Eugeniu Rosca
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
@ 2018-07-14 20:53 ` Eugeniu Rosca
2018-07-16 6:00 ` Heinrich Schuchardt
2018-08-20 22:06 ` [U-Boot] [U-Boot, v2, " Alexander Graf
2018-07-16 5:52 ` [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Heinrich Schuchardt
2018-08-20 22:05 ` [U-Boot] [U-Boot,v2,1/3] " Alexander Graf
3 siblings, 2 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2018-07-14 20:53 UTC (permalink / raw)
To: u-boot
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) {
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-07-14 20:53 [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Eugeniu Rosca
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
2018-07-14 20:53 ` [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?' Eugeniu Rosca
@ 2018-07-16 5:52 ` Heinrich Schuchardt
2018-07-16 6:12 ` Eugeniu Rosca
2018-08-20 22:05 ` [U-Boot] [U-Boot,v2,1/3] " Alexander Graf
3 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2018-07-16 5:52 UTC (permalink / raw)
To: u-boot
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;
> 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 */
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;
> 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;
> }
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
@ 2018-07-16 5:58 ` Heinrich Schuchardt
2018-08-20 22:05 ` [U-Boot] [U-Boot, v2, " Alexander Graf
1 sibling, 0 replies; 16+ messages in thread
From: Heinrich Schuchardt @ 2018-07-16 5:58 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?'
2018-07-14 20:53 ` [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?' Eugeniu Rosca
@ 2018-07-16 6:00 ` Heinrich Schuchardt
2018-08-20 22:06 ` [U-Boot] [U-Boot, v2, " Alexander Graf
1 sibling, 0 replies; 16+ messages in thread
From: Heinrich Schuchardt @ 2018-07-16 6:00 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-07-16 5:52 ` [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Heinrich Schuchardt
@ 2018-07-16 6:12 ` Eugeniu Rosca
2018-07-16 7:39 ` Heinrich Schuchardt
0 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2018-07-16 6:12 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-07-16 6:12 ` Eugeniu Rosca
@ 2018-07-16 7:39 ` Heinrich Schuchardt
2018-07-25 13:30 ` Eugeniu Rosca
0 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2018-07-16 7:39 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-07-16 7:39 ` Heinrich Schuchardt
@ 2018-07-25 13:30 ` Eugeniu Rosca
2018-08-01 11:25 ` Eugeniu Rosca
0 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2018-07-25 13:30 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-07-25 13:30 ` Eugeniu Rosca
@ 2018-08-01 11:25 ` Eugeniu Rosca
2018-08-08 20:41 ` Eugeniu Rosca
0 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2018-08-01 11:25 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-08-01 11:25 ` Eugeniu Rosca
@ 2018-08-08 20:41 ` Eugeniu Rosca
2018-08-17 12:48 ` Simon Glass
0 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2018-08-08 20:41 UTC (permalink / raw)
To: u-boot
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.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-08-08 20:41 ` Eugeniu Rosca
@ 2018-08-17 12:48 ` Simon Glass
2018-08-17 12:55 ` Eugeniu Rosca
0 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2018-08-17 12:48 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value
2018-08-17 12:48 ` Simon Glass
@ 2018-08-17 12:55 ` Eugeniu Rosca
0 siblings, 0 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2018-08-17 12:55 UTC (permalink / raw)
To: u-boot
On Fri, Aug 17, 2018 at 06:48:52AM -0600, Simon Glass wrote:
> I believe Alex should pick these up.
>
> Regards,
> Simon
Thanks, Simon!
I then look forward for some feedback from Alex.
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [U-Boot, v2, 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
2018-07-16 5:58 ` Heinrich Schuchardt
@ 2018-08-20 22:05 ` Alexander Graf
1 sibling, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2018-08-20 22:05 UTC (permalink / raw)
To: u-boot
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [U-Boot,v2,1/3] efi: Fix truncation of constant value
2018-07-14 20:53 [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Eugeniu Rosca
` (2 preceding siblings ...)
2018-07-16 5:52 ` [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Heinrich Schuchardt
@ 2018-08-20 22:05 ` Alexander Graf
3 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2018-08-20 22:05 UTC (permalink / raw)
To: u-boot
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [U-Boot, v2, 3/3] cmd: efi: Clarify calculation precedence for '&' and '?'
2018-07-14 20:53 ` [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?' Eugeniu Rosca
2018-07-16 6:00 ` Heinrich Schuchardt
@ 2018-08-20 22:06 ` Alexander Graf
1 sibling, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2018-08-20 22:06 UTC (permalink / raw)
To: u-boot
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-08-20 22:06 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-14 20:53 [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Eugeniu Rosca
2018-07-14 20:53 ` [U-Boot] [PATCH v2 2/3] efi: Add EFI_MEMORY_{NV, MORE_RELIABLE, RO} attributes Eugeniu Rosca
2018-07-16 5:58 ` Heinrich Schuchardt
2018-08-20 22:05 ` [U-Boot] [U-Boot, v2, " Alexander Graf
2018-07-14 20:53 ` [U-Boot] [PATCH v2 3/3] cmd: efi: Clarify calculation precedence for '&' and '?' Eugeniu Rosca
2018-07-16 6:00 ` Heinrich Schuchardt
2018-08-20 22:06 ` [U-Boot] [U-Boot, v2, " Alexander Graf
2018-07-16 5:52 ` [U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value Heinrich Schuchardt
2018-07-16 6:12 ` Eugeniu Rosca
2018-07-16 7:39 ` Heinrich Schuchardt
2018-07-25 13:30 ` Eugeniu Rosca
2018-08-01 11:25 ` Eugeniu Rosca
2018-08-08 20:41 ` Eugeniu Rosca
2018-08-17 12:48 ` Simon Glass
2018-08-17 12:55 ` Eugeniu Rosca
2018-08-20 22:05 ` [U-Boot] [U-Boot,v2,1/3] " Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.