[U-Boot] [PATCH v2 1/3] efi: Fix truncation of constant value

Heinrich Schuchardt xypron.glpk at gmx.de
Mon Jul 16 05:52:20 UTC 2018


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 at gmx.de>
> Signed-off-by: Eugeniu Rosca <erosca at 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 at 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;
>  	}
>  
> 



More information about the U-Boot mailing list