[PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Taku Izumi]

commit-0f96a99 introduces the following warning message:

  drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
  from integer of different size [-Wint-to-pointer-cast]

new_memmap_phy was defined as a u64 value and casted to void*.
This causes a warning of int-to-pointer-cast on x86 32-bit
environment.

This patch changes the type of "new_memmap_phy" variable
from "u64" into "ulong" to avoid it.

v1 -> v2:
 - change the type of "new_memmap_phy" from phys_addr_t
   into ulong according to Ard's comment

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/firmware/efi/fake_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
index 32bcb14..1f483b4 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/drivers/firmware/efi/fake_mem.c
@@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
 	u64 start, end, m_start, m_end, m_attr;
 	int new_nr_map = memmap.nr_map;
 	efi_memory_desc_t *md;
-	u64 new_memmap_phy;
+	ulong new_memmap_phy;
 	void *new_memmap;
 	void *old, *new;
 	int i;
-- 
1.8.3.1

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ard Biesheuvel]

On 23 October 2015 at 11:50, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> commit-0f96a99 introduces the following warning message:
>
>   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
>   from integer of different size [-Wint-to-pointer-cast]
>
> new_memmap_phy was defined as a u64 value and casted to void*.
> This causes a warning of int-to-pointer-cast on x86 32-bit
> environment.
>
> This patch changes the type of "new_memmap_phy" variable
> from "u64" into "ulong" to avoid it.
>
> v1 -> v2:
>  - change the type of "new_memmap_phy" from phys_addr_t
>    into ulong according to Ard's comment
>
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/firmware/efi/fake_mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> index 32bcb14..1f483b4 100644
> --- a/drivers/firmware/efi/fake_mem.c
> +++ b/drivers/firmware/efi/fake_mem.c
> @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
>         u64 start, end, m_start, m_end, m_attr;
>         int new_nr_map = memmap.nr_map;
>         efi_memory_desc_t *md;
> -       u64 new_memmap_phy;
> +       ulong new_memmap_phy;
>         void *new_memmap;
>         void *old, *new;
>         int i;


After looking at the original (already merged) patch 11/11 again, I
realize this is still not right: the problem is that efi_memory_map's
phys_map member uses a void* type to hold a physical address, which
happens to be correct in the normal case even when phys_addr_t is
larger than void* (like on ARM with LPAE enabled) since the address it
holds is the address of an allocation performed by the firmware, which
only uses 1:1 addressable memory.

However, overwriting memmap.phys_map with a value produced my
memblock_alloc() is problematic, since the allocation may be above 4
GB on 32-bit (L)PAE platforms. So the correct way to do this would be
to set the memblock limit to 4GB before memblock_alloc() on 32-bit
platforms, and restore it afterwards. This is a bit of a kludge,
though, and it would be more correct to change the type of
efi_memory_map::phys_map to phys_addr_t, although I don't know what
the potential fallout of that change is. Matt?

So that means your v1 of this patch is correct after all, and the
warning spotted by Ingo uncovered a problem with the original series
that requires an additional fix.

Regards,
Ard.

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 23 October 2015 at 11:50, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> > commit-0f96a99 introduces the following warning message:
> >
> >   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
> >   from integer of different size [-Wint-to-pointer-cast]
> >
> > new_memmap_phy was defined as a u64 value and casted to void*.
> > This causes a warning of int-to-pointer-cast on x86 32-bit
> > environment.
> >
> > This patch changes the type of "new_memmap_phy" variable
> > from "u64" into "ulong" to avoid it.
> >
> > v1 -> v2:
> >  - change the type of "new_memmap_phy" from phys_addr_t
> >    into ulong according to Ard's comment
> >
> > Reported-by: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > ---
> >  drivers/firmware/efi/fake_mem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> > index 32bcb14..1f483b4 100644
> > --- a/drivers/firmware/efi/fake_mem.c
> > +++ b/drivers/firmware/efi/fake_mem.c
> > @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
> >         u64 start, end, m_start, m_end, m_attr;
> >         int new_nr_map = memmap.nr_map;
> >         efi_memory_desc_t *md;
> > -       u64 new_memmap_phy;
> > +       ulong new_memmap_phy;
> >         void *new_memmap;
> >         void *old, *new;
> >         int i;
> 
> 
> After looking at the original (already merged) patch 11/11 again, I
> realize this is still not right: the problem is that efi_memory_map's
> phys_map member uses a void* type to hold a physical address, which
> happens to be correct in the normal case even when phys_addr_t is
> larger than void* (like on ARM with LPAE enabled) since the address it
> holds is the address of an allocation performed by the firmware, which
> only uses 1:1 addressable memory.
> 
> However, overwriting memmap.phys_map with a value produced my
> memblock_alloc() is problematic, since the allocation may be above 4
> GB on 32-bit (L)PAE platforms. So the correct way to do this would be
> to set the memblock limit to 4GB before memblock_alloc() on 32-bit
> platforms, and restore it afterwards. This is a bit of a kludge,
> though, and it would be more correct to change the type of
> efi_memory_map::phys_map to phys_addr_t, although I don't know what
> the potential fallout of that change is. Matt?
> 
> So that means your v1 of this patch is correct after all, and the
> warning spotted by Ingo uncovered a problem with the original series
> that requires an additional fix.

No, v1 is not right either. This type cast loses information:

        memmap.phys_map = (void *)new_memmap_phy;

Because there are countless platforms where 'void *' is 32-bit while physical 
addresses are wider.

No way of fudging around the type of 'new_memmap_phy' will solve that bug, it 
might only make the symptoms and the warning go away ...

The real problem is with the inappropriate (too narrow) type of memmap.phys_map, 
not with the type of new_memmap_phy. The cast just hides it.

One more page in the '1000 real-life examples of why type casts are evil' book.

Thanks,

	Ingo

[PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map

[Ard Biesheuvel]

We have been getting away with using a void* for the physical
address of the UEFI memory map, since, even on 32-bit platforms
with 64-bit physical addresses, no truncation takes place if the
memory map has been allocated by the firmware (which only uses
1:1 virtually addressable memory), which is usually the case.

However, commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
adds code that clones and modifies the UEFI memory map, and the
clone may live above 4 GB on 32-bit platforms. This means our use
of void* for struct efi_memory_map::phys_map has graduated from
'incorrect but working' to 'incorrect and broken', and we need to
fix it.

So redefine struct efi_memory_map::phys_map as phys_addr_t, and
get rid of a bunch of casts that are now unneeded.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi.c     | 4 ++--
 arch/x86/platform/efi/efi.c | 4 ++--
 drivers/firmware/efi/efi.c  | 8 ++++----
 include/linux/efi.h         | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 4b7df346e388..61eb1d17586a 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -208,7 +208,7 @@ void __init efi_init(void)
 
 	memblock_reserve(params.mmap & PAGE_MASK,
 			 PAGE_ALIGN(params.mmap_size + (params.mmap & ~PAGE_MASK)));
-	memmap.phys_map = (void *)params.mmap;
+	memmap.phys_map = params.mmap;
 	memmap.map = early_memremap(params.mmap, params.mmap_size);
 	memmap.map_end = memmap.map + params.mmap_size;
 	memmap.desc_size = params.desc_size;
@@ -282,7 +282,7 @@ static int __init arm64_enable_runtime_services(void)
 	pr_info("Remapping and enabling EFI services.\n");
 
 	mapsize = memmap.map_end - memmap.map;
-	memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
+	memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
 						   mapsize);
 	if (!memmap.map) {
 		pr_err("Failed to remap EFI memory map\n");
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3e1d09e885c0..ad285404ea7f 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -194,7 +194,7 @@ static void __init do_add_efi_memmap(void)
 int __init efi_memblock_x86_reserve_range(void)
 {
 	struct efi_info *e = &boot_params.efi_info;
-	unsigned long pmap;
+	phys_addr_t pmap;
 
 	if (efi_enabled(EFI_PARAVIRT))
 		return 0;
@@ -209,7 +209,7 @@ int __init efi_memblock_x86_reserve_range(void)
 #else
 	pmap = (e->efi_memmap |	((__u64)e->efi_memmap_hi << 32));
 #endif
-	memmap.phys_map		= (void *)pmap;
+	memmap.phys_map		= pmap;
 	memmap.nr_map		= e->efi_memmap_size /
 				  e->efi_memdesc_size;
 	memmap.desc_size	= e->efi_memdesc_size;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 31fc864eb037..027ca212179f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -254,7 +254,7 @@ subsys_initcall(efisubsys_init);
 int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 {
 	struct efi_memory_map *map = efi.memmap;
-	void *p, *e;
+	phys_addr_t p, e;
 
 	if (!efi_enabled(EFI_MEMMAP)) {
 		pr_err_once("EFI_MEMMAP is not enabled.\n");
@@ -286,10 +286,10 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 		 * So just always get our own virtual map on the CPU.
 		 *
 		 */
-		md = early_memremap((phys_addr_t)p, sizeof (*md));
+		md = early_memremap(p, sizeof (*md));
 		if (!md) {
-			pr_err_once("early_memremap(%p, %zu) failed.\n",
-				    p, sizeof (*md));
+			pr_err_once("early_memremap(%pa, %zu) failed.\n",
+				    &p, sizeof (*md));
 			return -ENOMEM;
 		}
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4d01c1033fce..569b5a866bb1 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -680,7 +680,7 @@ typedef struct {
 } efi_system_table_t;
 
 struct efi_memory_map {
-	void *phys_map;
+	phys_addr_t phys_map;
 	void *map;
 	void *map_end;
 	int nr_map;
-- 
2.1.4

[PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ard Biesheuvel]

From: Taku Izumi <izumi.taku@jp.fujitsu.com>

Commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
introduces the following warning message:

  drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
  from integer of different size [-Wint-to-pointer-cast]

new_memmap_phy was defined as a u64 value and cast to void*, causing
a int-to-pointer-cast warning on x86 32-bit builds.
However, since the void* type is inappropriate for a physical
address, the definition of struct efi_memory_map::phys_map has been
changed to phys_addr_t in the previous patch, and so the cast can be
dropped entirely.

This patch also changes the type of the "new_memmap_phy" variable
from "u64" to "phys_addr_t" to align with the types of
memblock_alloc() and struct efi_memory_map::phys_map.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
[ard.biesheuvel: removed void* cast, updated commit log]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/fake_mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
index 32bcb14df2c8..ed3a854950cc 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/drivers/firmware/efi/fake_mem.c
@@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
 	u64 start, end, m_start, m_end, m_attr;
 	int new_nr_map = memmap.nr_map;
 	efi_memory_desc_t *md;
-	u64 new_memmap_phy;
+	phys_addr_t new_memmap_phy;
 	void *new_memmap;
 	void *old, *new;
 	int i;
@@ -183,7 +183,7 @@ void __init efi_fake_memmap(void)
 	/* swap into new EFI memmap */
 	efi_unmap_memmap();
 	memmap.map = new_memmap;
-	memmap.phys_map = (void *)new_memmap_phy;
+	memmap.phys_map = new_memmap_phy;
 	memmap.nr_map = new_nr_map;
 	memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size;
 	set_bit(EFI_MEMMAP, &efi.flags);
-- 
2.1.4

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> From: Taku Izumi <izumi.taku@jp.fujitsu.com>
> 
> Commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
> introduces the following warning message:
> 
>   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
>   from integer of different size [-Wint-to-pointer-cast]
> 
> new_memmap_phy was defined as a u64 value and cast to void*, causing
> a int-to-pointer-cast warning on x86 32-bit builds.
> However, since the void* type is inappropriate for a physical
> address, the definition of struct efi_memory_map::phys_map has been
> changed to phys_addr_t in the previous patch, and so the cast can be
> dropped entirely.
> 
> This patch also changes the type of the "new_memmap_phy" variable
> from "u64" to "phys_addr_t" to align with the types of
> memblock_alloc() and struct efi_memory_map::phys_map.
> 
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> [ard.biesheuvel: removed void* cast, updated commit log]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/fake_mem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> index 32bcb14df2c8..ed3a854950cc 100644
> --- a/drivers/firmware/efi/fake_mem.c
> +++ b/drivers/firmware/efi/fake_mem.c
> @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
>  	u64 start, end, m_start, m_end, m_attr;
>  	int new_nr_map = memmap.nr_map;
>  	efi_memory_desc_t *md;
> -	u64 new_memmap_phy;
> +	phys_addr_t new_memmap_phy;
>  	void *new_memmap;
>  	void *old, *new;
>  	int i;
> @@ -183,7 +183,7 @@ void __init efi_fake_memmap(void)
>  	/* swap into new EFI memmap */
>  	efi_unmap_memmap();
>  	memmap.map = new_memmap;
> -	memmap.phys_map = (void *)new_memmap_phy;
> +	memmap.phys_map = new_memmap_phy;
>  	memmap.nr_map = new_nr_map;
>  	memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size;

Please guys, think for a change!

How is this supposed to work with:

   include/linux/efi.h:    void *phys_map;

?

Thanks,

	Ingo

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> > From: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > 
> > Commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
> > introduces the following warning message:
> > 
> >   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
> >   from integer of different size [-Wint-to-pointer-cast]
> > 
> > new_memmap_phy was defined as a u64 value and cast to void*, causing
> > a int-to-pointer-cast warning on x86 32-bit builds.
> > However, since the void* type is inappropriate for a physical
> > address, the definition of struct efi_memory_map::phys_map has been
> > changed to phys_addr_t in the previous patch, and so the cast can be
> > dropped entirely.
> > 
> > This patch also changes the type of the "new_memmap_phy" variable
> > from "u64" to "phys_addr_t" to align with the types of
> > memblock_alloc() and struct efi_memory_map::phys_map.
> > 
> > Reported-by: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > [ard.biesheuvel: removed void* cast, updated commit log]
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  drivers/firmware/efi/fake_mem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> > index 32bcb14df2c8..ed3a854950cc 100644
> > --- a/drivers/firmware/efi/fake_mem.c
> > +++ b/drivers/firmware/efi/fake_mem.c
> > @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
> >  	u64 start, end, m_start, m_end, m_attr;
> >  	int new_nr_map = memmap.nr_map;
> >  	efi_memory_desc_t *md;
> > -	u64 new_memmap_phy;
> > +	phys_addr_t new_memmap_phy;
> >  	void *new_memmap;
> >  	void *old, *new;
> >  	int i;
> > @@ -183,7 +183,7 @@ void __init efi_fake_memmap(void)
> >  	/* swap into new EFI memmap */
> >  	efi_unmap_memmap();
> >  	memmap.map = new_memmap;
> > -	memmap.phys_map = (void *)new_memmap_phy;
> > +	memmap.phys_map = new_memmap_phy;
> >  	memmap.nr_map = new_nr_map;
> >  	memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size;
> 
> Please guys, think for a change!
> 
> How is this supposed to work with:
> 
>    include/linux/efi.h:    void *phys_map;
> 
> ?

Ah, I take that back, now I see your patch 1/2 that changes it to phys_addr_t.

That's the right solution, and it also cleans up the code:

  Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > 
> > > From: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > > 
> > > Commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
> > > introduces the following warning message:
> > > 
> > >   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
> > >   from integer of different size [-Wint-to-pointer-cast]
> > > 
> > > new_memmap_phy was defined as a u64 value and cast to void*, causing
> > > a int-to-pointer-cast warning on x86 32-bit builds.
> > > However, since the void* type is inappropriate for a physical
> > > address, the definition of struct efi_memory_map::phys_map has been
> > > changed to phys_addr_t in the previous patch, and so the cast can be
> > > dropped entirely.
> > > 
> > > This patch also changes the type of the "new_memmap_phy" variable
> > > from "u64" to "phys_addr_t" to align with the types of
> > > memblock_alloc() and struct efi_memory_map::phys_map.
> > > 
> > > Reported-by: Ingo Molnar <mingo@kernel.org>
> > > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > > [ard.biesheuvel: removed void* cast, updated commit log]
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  drivers/firmware/efi/fake_mem.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> > > index 32bcb14df2c8..ed3a854950cc 100644
> > > --- a/drivers/firmware/efi/fake_mem.c
> > > +++ b/drivers/firmware/efi/fake_mem.c
> > > @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
> > >  	u64 start, end, m_start, m_end, m_attr;
> > >  	int new_nr_map = memmap.nr_map;
> > >  	efi_memory_desc_t *md;
> > > -	u64 new_memmap_phy;
> > > +	phys_addr_t new_memmap_phy;
> > >  	void *new_memmap;
> > >  	void *old, *new;
> > >  	int i;
> > > @@ -183,7 +183,7 @@ void __init efi_fake_memmap(void)
> > >  	/* swap into new EFI memmap */
> > >  	efi_unmap_memmap();
> > >  	memmap.map = new_memmap;
> > > -	memmap.phys_map = (void *)new_memmap_phy;
> > > +	memmap.phys_map = new_memmap_phy;
> > >  	memmap.nr_map = new_nr_map;
> > >  	memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size;
> > 
> > Please guys, think for a change!
> > 
> > How is this supposed to work with:
> > 
> >    include/linux/efi.h:    void *phys_map;
> > 
> > ?
> 
> Ah, I take that back, now I see your patch 1/2 that changes it to phys_addr_t.
> 
> That's the right solution, and it also cleans up the code:
> 
>   Reviewed-by: Ingo Molnar <mingo@kernel.org>

Matt, do you want to take these fixes, or should I apply them directly?

Thanks,

	Ingo

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Matt Fleming]

On Fri, 23 Oct, at 12:33:29PM, Ingo Molnar wrote:
> 
> Matt, do you want to take these fixes, or should I apply them directly?

Could you please apply them directly with my Reviewed-by tag?

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Fri, 23 Oct, at 12:33:29PM, Ingo Molnar wrote:
> > 
> > Matt, do you want to take these fixes, or should I apply them directly?
> 
> Could you please apply them directly with my Reviewed-by tag?

Sure, done!

Thanks,

	Ingo

Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Matt Fleming]

On Fri, 23 Oct, at 11:48:17AM, Ard Biesheuvel wrote:
> From: Taku Izumi <izumi.taku@jp.fujitsu.com>
> 
> Commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
> introduces the following warning message:
> 
>   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
>   from integer of different size [-Wint-to-pointer-cast]
> 
> new_memmap_phy was defined as a u64 value and cast to void*, causing
> a int-to-pointer-cast warning on x86 32-bit builds.
> However, since the void* type is inappropriate for a physical
> address, the definition of struct efi_memory_map::phys_map has been
> changed to phys_addr_t in the previous patch, and so the cast can be
> dropped entirely.
> 
> This patch also changes the type of the "new_memmap_phy" variable
> from "u64" to "phys_addr_t" to align with the types of
> memblock_alloc() and struct efi_memory_map::phys_map.
> 
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> [ard.biesheuvel: removed void* cast, updated commit log]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/fake_mem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

[tip:core/efi] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[tip-bot for Taku Izumi]

Commit-ID:  78b9bc947b18ed16b6c2c573d774e6d54ad9452d
Gitweb:     http://git.kernel.org/tip/78b9bc947b18ed16b6c2c573d774e6d54ad9452d
Author:     Taku Izumi <izumi.taku@jp.fujitsu.com>
AuthorDate: Fri, 23 Oct 2015 11:48:17 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 28 Oct 2015 12:28:06 +0100

efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

Commit:

  0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")

introduced the following warning message:

  drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

new_memmap_phy was defined as a u64 value and cast to void*,
causing a int-to-pointer-cast warning on x86 32-bit builds.
However, since the void* type is inappropriate for a physical
address, the definition of struct efi_memory_map::phys_map has
been changed to phys_addr_t in the previous patch, and so the
cast can be dropped entirely.

This patch also changes the type of the "new_memmap_phy"
variable from "u64" to "phys_addr_t" to align with the types of
memblock_alloc() and struct efi_memory_map::phys_map.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
[ Removed void* cast, updated commit log]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: kamezawa.hiroyu@jp.fujitsu.com
Cc: linux-efi@vger.kernel.org
Cc: matt.fleming@intel.com
Link: http://lkml.kernel.org/r/1445593697-1342-2-git-send-email-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/fake_mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
index 32bcb14..ed3a854 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/drivers/firmware/efi/fake_mem.c
@@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
 	u64 start, end, m_start, m_end, m_attr;
 	int new_nr_map = memmap.nr_map;
 	efi_memory_desc_t *md;
-	u64 new_memmap_phy;
+	phys_addr_t new_memmap_phy;
 	void *new_memmap;
 	void *old, *new;
 	int i;
@@ -183,7 +183,7 @@ void __init efi_fake_memmap(void)
 	/* swap into new EFI memmap */
 	efi_unmap_memmap();
 	memmap.map = new_memmap;
-	memmap.phys_map = (void *)new_memmap_phy;
+	memmap.phys_map = new_memmap_phy;
 	memmap.nr_map = new_nr_map;
 	memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size;
 	set_bit(EFI_MEMMAP, &efi.flags);

Re: [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map

[Matt Fleming]

On Fri, 23 Oct, at 11:48:16AM, Ard Biesheuvel wrote:
> We have been getting away with using a void* for the physical
> address of the UEFI memory map, since, even on 32-bit platforms
> with 64-bit physical addresses, no truncation takes place if the
> memory map has been allocated by the firmware (which only uses
> 1:1 virtually addressable memory), which is usually the case.
> 
> However, commit 0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")
> adds code that clones and modifies the UEFI memory map, and the
> clone may live above 4 GB on 32-bit platforms. This means our use
> of void* for struct efi_memory_map::phys_map has graduated from
> 'incorrect but working' to 'incorrect and broken', and we need to
> fix it.
> 
> So redefine struct efi_memory_map::phys_map as phys_addr_t, and
> get rid of a bunch of casts that are now unneeded.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi.c     | 4 ++--
>  arch/x86/platform/efi/efi.c | 4 ++--
>  drivers/firmware/efi/efi.c  | 8 ++++----
>  include/linux/efi.h         | 2 +-
>  4 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

[tip:core/efi] efi: Use correct type for struct efi_memory_map:: phys_map

[tip-bot for Ard Biesheuvel]

Commit-ID:  44511fb9e55ada760822b0b0d7be9d150576f17f
Gitweb:     http://git.kernel.org/tip/44511fb9e55ada760822b0b0d7be9d150576f17f
Author:     Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Fri, 23 Oct 2015 11:48:16 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 28 Oct 2015 12:28:06 +0100

efi: Use correct type for struct efi_memory_map::phys_map

We have been getting away with using a void* for the physical
address of the UEFI memory map, since, even on 32-bit platforms
with 64-bit physical addresses, no truncation takes place if the
memory map has been allocated by the firmware (which only uses
1:1 virtually addressable memory), which is usually the case.

However, commit:

  0f96a99dab36 ("efi: Add "efi_fake_mem" boot option")

adds code that clones and modifies the UEFI memory map, and the
clone may live above 4 GB on 32-bit platforms.

This means our use of void* for struct efi_memory_map::phys_map has
graduated from 'incorrect but working' to 'incorrect and
broken', and we need to fix it.

So redefine struct efi_memory_map::phys_map as phys_addr_t, and
get rid of a bunch of casts that are now unneeded.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: izumi.taku@jp.fujitsu.com
Cc: kamezawa.hiroyu@jp.fujitsu.com
Cc: linux-efi@vger.kernel.org
Cc: matt.fleming@intel.com
Link: http://lkml.kernel.org/r/1445593697-1342-1-git-send-email-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/arm64/kernel/efi.c     | 4 ++--
 arch/x86/platform/efi/efi.c | 4 ++--
 drivers/firmware/efi/efi.c  | 8 ++++----
 include/linux/efi.h         | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 4b7df34..61eb1d1 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -208,7 +208,7 @@ void __init efi_init(void)
 
 	memblock_reserve(params.mmap & PAGE_MASK,
 			 PAGE_ALIGN(params.mmap_size + (params.mmap & ~PAGE_MASK)));
-	memmap.phys_map = (void *)params.mmap;
+	memmap.phys_map = params.mmap;
 	memmap.map = early_memremap(params.mmap, params.mmap_size);
 	memmap.map_end = memmap.map + params.mmap_size;
 	memmap.desc_size = params.desc_size;
@@ -282,7 +282,7 @@ static int __init arm64_enable_runtime_services(void)
 	pr_info("Remapping and enabling EFI services.\n");
 
 	mapsize = memmap.map_end - memmap.map;
-	memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
+	memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
 						   mapsize);
 	if (!memmap.map) {
 		pr_err("Failed to remap EFI memory map\n");
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3e1d09e..ad28540 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -194,7 +194,7 @@ static void __init do_add_efi_memmap(void)
 int __init efi_memblock_x86_reserve_range(void)
 {
 	struct efi_info *e = &boot_params.efi_info;
-	unsigned long pmap;
+	phys_addr_t pmap;
 
 	if (efi_enabled(EFI_PARAVIRT))
 		return 0;
@@ -209,7 +209,7 @@ int __init efi_memblock_x86_reserve_range(void)
 #else
 	pmap = (e->efi_memmap |	((__u64)e->efi_memmap_hi << 32));
 #endif
-	memmap.phys_map		= (void *)pmap;
+	memmap.phys_map		= pmap;
 	memmap.nr_map		= e->efi_memmap_size /
 				  e->efi_memdesc_size;
 	memmap.desc_size	= e->efi_memdesc_size;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 31fc864..027ca21 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -254,7 +254,7 @@ subsys_initcall(efisubsys_init);
 int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 {
 	struct efi_memory_map *map = efi.memmap;
-	void *p, *e;
+	phys_addr_t p, e;
 
 	if (!efi_enabled(EFI_MEMMAP)) {
 		pr_err_once("EFI_MEMMAP is not enabled.\n");
@@ -286,10 +286,10 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 		 * So just always get our own virtual map on the CPU.
 		 *
 		 */
-		md = early_memremap((phys_addr_t)p, sizeof (*md));
+		md = early_memremap(p, sizeof (*md));
 		if (!md) {
-			pr_err_once("early_memremap(%p, %zu) failed.\n",
-				    p, sizeof (*md));
+			pr_err_once("early_memremap(%pa, %zu) failed.\n",
+				    &p, sizeof (*md));
 			return -ENOMEM;
 		}
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4d01c10..569b5a8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -680,7 +680,7 @@ typedef struct {
 } efi_system_table_t;
 
 struct efi_memory_map {
-	void *phys_map;
+	phys_addr_t phys_map;
 	void *map;
 	void *map_end;
 	int nr_map;

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Matt Fleming]

On Fri, 23 Oct, at 10:37:46AM, Ard Biesheuvel wrote:
> 
> After looking at the original (already merged) patch 11/11 again, I
> realize this is still not right: the problem is that efi_memory_map's
> phys_map member uses a void* type to hold a physical address, which
> happens to be correct in the normal case even when phys_addr_t is
> larger than void* (like on ARM with LPAE enabled) since the address it
> holds is the address of an allocation performed by the firmware, which
> only uses 1:1 addressable memory.
> 
> However, overwriting memmap.phys_map with a value produced my
> memblock_alloc() is problematic, since the allocation may be above 4
> GB on 32-bit (L)PAE platforms. So the correct way to do this would be
> to set the memblock limit to 4GB before memblock_alloc() on 32-bit
> platforms, and restore it afterwards. This is a bit of a kludge,
> though, and it would be more correct to change the type of
> efi_memory_map::phys_map to phys_addr_t, although I don't know what
> the potential fallout of that change is. Matt?

I think that should be fine. The only potentially tricky situation we
could encounter is where 32-bit x86 firmware uses PAE but the kernel
is built without support.

But that's not something I've ever seen enabled in the firmware and
there's a bunch of assumptions in the kernel already that would break
in that case.

Given that addresses are always 64-bit in the UEFI spec, even on
32-bit platforms, what's the downside of picking u64 instead of
phys_addr_t for efi_memory_map::phys_map?

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ard Biesheuvel]

On 27 October 2015 at 06:02, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 23 Oct, at 10:37:46AM, Ard Biesheuvel wrote:
>>
>> After looking at the original (already merged) patch 11/11 again, I
>> realize this is still not right: the problem is that efi_memory_map's
>> phys_map member uses a void* type to hold a physical address, which
>> happens to be correct in the normal case even when phys_addr_t is
>> larger than void* (like on ARM with LPAE enabled) since the address it
>> holds is the address of an allocation performed by the firmware, which
>> only uses 1:1 addressable memory.
>>
>> However, overwriting memmap.phys_map with a value produced my
>> memblock_alloc() is problematic, since the allocation may be above 4
>> GB on 32-bit (L)PAE platforms. So the correct way to do this would be
>> to set the memblock limit to 4GB before memblock_alloc() on 32-bit
>> platforms, and restore it afterwards. This is a bit of a kludge,
>> though, and it would be more correct to change the type of
>> efi_memory_map::phys_map to phys_addr_t, although I don't know what
>> the potential fallout of that change is. Matt?
>
> I think that should be fine. The only potentially tricky situation we
> could encounter is where 32-bit x86 firmware uses PAE but the kernel
> is built without support.
>
> But that's not something I've ever seen enabled in the firmware and
> there's a bunch of assumptions in the kernel already that would break
> in that case.
>

Does UEFI even allow that? Even if it can describe memory over 4 GB,
it uses a flat mapping so allocations done by the stub (which
retrieves the memory map) should never reside at addresses over 4 GB.

-- 
Ard.

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Matt Fleming]

On Tue, 27 Oct, at 11:33:55AM, Ard Biesheuvel wrote:
> 
> Does UEFI even allow that? Even if it can describe memory over 4 GB,
> it uses a flat mapping so allocations done by the stub (which
> retrieves the memory map) should never reside at addresses over 4 GB.

Right, exactly. We're good with phys_addr_t.

Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds

[Ingo Molnar]

* Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:

> commit-0f96a99 introduces the following warning message:
> 
>   drivers/firmware/efi/fake_mem.c:186:20: warning: cast to pointer
>   from integer of different size [-Wint-to-pointer-cast]
> 
> new_memmap_phy was defined as a u64 value and casted to void*.
> This causes a warning of int-to-pointer-cast on x86 32-bit
> environment.
> 
> This patch changes the type of "new_memmap_phy" variable
> from "u64" into "ulong" to avoid it.
> 
> v1 -> v2:
>  - change the type of "new_memmap_phy" from phys_addr_t
>    into ulong according to Ard's comment
> 
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/firmware/efi/fake_mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> index 32bcb14..1f483b4 100644
> --- a/drivers/firmware/efi/fake_mem.c
> +++ b/drivers/firmware/efi/fake_mem.c
> @@ -59,7 +59,7 @@ void __init efi_fake_memmap(void)
>  	u64 start, end, m_start, m_end, m_attr;
>  	int new_nr_map = memmap.nr_map;
>  	efi_memory_desc_t *md;
> -	u64 new_memmap_phy;
> +	ulong new_memmap_phy;
>  	void *new_memmap;
>  	void *old, *new;
>  	int i;

Sight, this just makes the code outright buggy in another place.

Please don't just try to 'avoid a warning', that's the wrong mindset!

You need to look at how the various values are used, what their natural types are, 
how they are used in other places, and accordingly fix the code.

Thanks,

	Ingo