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

[Eugeniu Rosca]

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 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)

Best regards,
Eugeniu.