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

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-+CUm20s59erQFUHtdCDX3A@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
>  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-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
[ard.biesheuvel: removed void* cast, updated commit log]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> 
> * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > From: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > 
> > 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > [ard.biesheuvel: removed void* cast, updated commit log]
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> 
> * Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > 
> > * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > 
> > > From: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > > 
> > > 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > Signed-off-by: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > > [ard.biesheuvel: removed void* cast, updated commit log]
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

Thanks,

	Ingo

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

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>

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