[U-Boot] [PATCH] efi.h: Fix truncation of constant value

From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] efi.h: Fix truncation of constant value
Date: Sat, 14 Jul 2018 19:05:14 +0200	[thread overview]
Message-ID: <44b12995-404a-91a5-cde5-c2285646784d@gmx.de> (raw)
In-Reply-To: <20180714122053.19418-1-erosca@de.adit-jv.com>

On 07/14/2018 02:20 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
> 
> Fixes: 867a6ac86dd8 ("efi: Add start-up library code")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  include/efi.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/efi.h b/include/efi.h
> index 0fe15e65c06c..3e3f23b42f8a 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -172,11 +172,11 @@ enum {
>  	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_RUNTIME	(1ULL << EFI_MEMORY_RUNTIME_SHIFT)

Hello Eugeniu,

thanks for pointing out the problem.

We should not look at this single value only. Please eliminate the whole
enum and use #define for all values using the names and definitions
provided in the UEFI 2.7 spec (i.e. without the _SHIFT suffix).

#define EFI_MEMORY_UC            0x0000000000000001
#define EFI_MEMORY_WC            0x0000000000000002
#define EFI_MEMORY_WT            0x0000000000000004
#define EFI_MEMORY_WB            0x0000000000000008
#define EFI_MEMORY_UCE           0x0000000000000010
#define EFI_MEMORY_WP            0x0000000000001000
#define EFI_MEMORY_RP            0x0000000000002000
#define EFI_MEMORY_XP            0x0000000000004000
#define EFI_MEMORY_NV            0x0000000000008000
#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000
#define EFI_MEMORY_RO            0x0000000000020000
#define EFI_MEMORY_RUNTIME       0x8000000000000000

I think Simon used the _SHIFT values to save a few bytes in mem_attr[]
on 32bit architectures@the cost of more bytes in coding on 64bit
architectures.

In cmd/efi.c we could use the ilog2() macro to get the shift values. But
I would prefer to remove the shifting altogether and go for more
readable code.

Some values are missing in mem_attr[] (in cmd/efi.c). These should be added.

Best regards

Heinrich

> +#define EFI_MEM_DESC_VERSION	1
> +
>  #define EFI_PAGE_SHIFT		12
>  #define EFI_PAGE_SIZE		(1UL << EFI_PAGE_SHIFT)
>  #define EFI_PAGE_MASK		(EFI_PAGE_SIZE - 1)
> 