* Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <1445593826-4578-1-git-send-email-izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org> @ 2015-10-23 8:37 ` Ard Biesheuvel 2015-10-23 8:50 ` Ingo Molnar [not found] ` <CAKv+Gu8WPvG5rzBB57eK1_Ehj1wB19wR=zzBDRoQbmhRfpCGng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 2 replies; 14+ messages in thread From: Ard Biesheuvel @ 2015-10-23 8:37 UTC (permalink / raw) To: Taku Izumi, Matt Fleming, Ingo Molnar Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-23 8:37 ` [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Ard Biesheuvel @ 2015-10-23 8:50 ` Ingo Molnar 2015-10-23 9:48 ` [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map Ard Biesheuvel [not found] ` <CAKv+Gu8WPvG5rzBB57eK1_Ehj1wB19wR=zzBDRoQbmhRfpCGng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2015-10-23 8:50 UTC (permalink / raw) To: Ard Biesheuvel Cc: Taku Izumi, Matt Fleming, Ingo Molnar, linux-kernel, kamezawa.hiroyu, linux-efi * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map 2015-10-23 8:50 ` Ingo Molnar @ 2015-10-23 9:48 ` Ard Biesheuvel [not found] ` <1445593697-1342-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2015-10-27 21:09 ` [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map Matt Fleming 0 siblings, 2 replies; 14+ messages in thread From: Ard Biesheuvel @ 2015-10-23 9:48 UTC (permalink / raw) To: linux-efi, izumi.taku, matt.fleming, linux-kernel, mingo Cc: kamezawa.hiroyu, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
[parent not found: <1445593697-1342-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <1445593697-1342-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> @ 2015-10-23 9:48 ` Ard Biesheuvel 2015-10-23 10:27 ` Ingo Molnar 2015-10-27 21:11 ` Matt Fleming 0 siblings, 2 replies; 14+ messages in thread From: Ard Biesheuvel @ 2015-10-23 9:48 UTC (permalink / raw) To: linux-efi-u79uwXL29TY76Z2rM5mHXA, izumi.taku-+CUm20s59erQFUHtdCDX3A, matt.fleming-ral2JQCrhuEAvxtiuMwx3w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, mingo-DgEjT+Ai2ygdnm+yROfE0A Cc: kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-23 9:48 ` [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Ard Biesheuvel @ 2015-10-23 10:27 ` Ingo Molnar [not found] ` <20151023102728.GA1974-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-27 21:11 ` Matt Fleming 1 sibling, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2015-10-23 10:27 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi, izumi.taku, matt.fleming, linux-kernel, kamezawa.hiroyu * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151023102728.GA1974-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <20151023102728.GA1974-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-23 10:30 ` Ingo Molnar [not found] ` <20151023103022.GA2297-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2015-10-23 10:30 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, izumi.taku-+CUm20s59erQFUHtdCDX3A, matt.fleming-ral2JQCrhuEAvxtiuMwx3w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151023103022.GA2297-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <20151023103022.GA2297-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-23 10:33 ` Ingo Molnar 2015-10-27 21:12 ` Matt Fleming 0 siblings, 1 reply; 14+ messages in thread From: Ingo Molnar @ 2015-10-23 10:33 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, izumi.taku-+CUm20s59erQFUHtdCDX3A, matt.fleming-ral2JQCrhuEAvxtiuMwx3w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-23 10:33 ` Ingo Molnar @ 2015-10-27 21:12 ` Matt Fleming 2015-10-28 11:28 ` Ingo Molnar 0 siblings, 1 reply; 14+ messages in thread From: Matt Fleming @ 2015-10-27 21:12 UTC (permalink / raw) To: Ingo Molnar Cc: Ard Biesheuvel, linux-efi, izumi.taku, linux-kernel, kamezawa.hiroyu 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-27 21:12 ` Matt Fleming @ 2015-10-28 11:28 ` Ingo Molnar 0 siblings, 0 replies; 14+ messages in thread From: Ingo Molnar @ 2015-10-28 11:28 UTC (permalink / raw) To: Matt Fleming Cc: Ard Biesheuvel, linux-efi, izumi.taku, linux-kernel, kamezawa.hiroyu * 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-23 9:48 ` [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Ard Biesheuvel 2015-10-23 10:27 ` Ingo Molnar @ 2015-10-27 21:11 ` Matt Fleming 1 sibling, 0 replies; 14+ messages in thread From: Matt Fleming @ 2015-10-27 21:11 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi, izumi.taku, linux-kernel, mingo, kamezawa.hiroyu 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map 2015-10-23 9:48 ` [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map Ard Biesheuvel [not found] ` <1445593697-1342-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> @ 2015-10-27 21:09 ` Matt Fleming 1 sibling, 0 replies; 14+ messages in thread From: Matt Fleming @ 2015-10-27 21:09 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi, izumi.taku, linux-kernel, mingo, kamezawa.hiroyu 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAKv+Gu8WPvG5rzBB57eK1_Ehj1wB19wR=zzBDRoQbmhRfpCGng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <CAKv+Gu8WPvG5rzBB57eK1_Ehj1wB19wR=zzBDRoQbmhRfpCGng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-10-26 21:02 ` Matt Fleming 2015-10-27 2:33 ` Ard Biesheuvel 0 siblings, 1 reply; 14+ messages in thread From: Matt Fleming @ 2015-10-26 21:02 UTC (permalink / raw) To: Ard Biesheuvel Cc: Taku Izumi, Ingo Molnar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds 2015-10-26 21:02 ` [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Matt Fleming @ 2015-10-27 2:33 ` Ard Biesheuvel [not found] ` <CAKv+Gu-1MfUJiF5idd0KoziUxhrDStmRwXQZHuX+gwnJvq083g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: Ard Biesheuvel @ 2015-10-27 2:33 UTC (permalink / raw) To: Matt Fleming Cc: Taku Izumi, Ingo Molnar, linux-kernel, kamezawa.hiroyu, linux-efi 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAKv+Gu-1MfUJiF5idd0KoziUxhrDStmRwXQZHuX+gwnJvq083g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds [not found] ` <CAKv+Gu-1MfUJiF5idd0KoziUxhrDStmRwXQZHuX+gwnJvq083g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-10-27 21:08 ` Matt Fleming 0 siblings, 0 replies; 14+ messages in thread From: Matt Fleming @ 2015-10-27 21:08 UTC (permalink / raw) To: Ard Biesheuvel Cc: Taku Izumi, Ingo Molnar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-10-28 11:28 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1445593826-4578-1-git-send-email-izumi.taku@jp.fujitsu.com> [not found] ` <1445593826-4578-1-git-send-email-izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org> 2015-10-23 8:37 ` [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Ard Biesheuvel 2015-10-23 8:50 ` Ingo Molnar 2015-10-23 9:48 ` [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map Ard Biesheuvel [not found] ` <1445593697-1342-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2015-10-23 9:48 ` [PATCH 2/2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Ard Biesheuvel 2015-10-23 10:27 ` Ingo Molnar [not found] ` <20151023102728.GA1974-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-23 10:30 ` Ingo Molnar [not found] ` <20151023103022.GA2297-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-23 10:33 ` Ingo Molnar 2015-10-27 21:12 ` Matt Fleming 2015-10-28 11:28 ` Ingo Molnar 2015-10-27 21:11 ` Matt Fleming 2015-10-27 21:09 ` [PATCH 1/2] efi: use correct type for struct efi_memory_map::phys_map Matt Fleming [not found] ` <CAKv+Gu8WPvG5rzBB57eK1_Ehj1wB19wR=zzBDRoQbmhRfpCGng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-10-26 21:02 ` [PATCH v2] efi: Fix warning of int-to-pointer-cast on x86 32-bit builds Matt Fleming 2015-10-27 2:33 ` Ard Biesheuvel [not found] ` <CAKv+Gu-1MfUJiF5idd0KoziUxhrDStmRwXQZHuX+gwnJvq083g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-10-27 21:08 ` Matt Fleming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).