| Message ID | 20180714205332.17709-1-erosca@de.adit-jv.com |
|---|---|
| State | Accepted |
| Commit | 9b89183b97f3d906a8df1050707d48a74e35caed |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | [U-Boot,v2,1/3] efi: Fix truncation of constant value | expand |
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; > } > >
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
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.
> 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
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; }
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(-)