* [PATCH v2 0/2] efi: ARM/arm64: wire up ESRT table @ 2016-02-15 11:32 Ard Biesheuvel 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel 2016-02-15 11:32 ` [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() Ard Biesheuvel 0 siblings, 2 replies; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-15 11:32 UTC (permalink / raw) To: linux-arm-kernel As reported by Tyler, the ESRT support was not wired up yet for ARM or arm64, hence this series. Patch #1 replaces ioremap with memremap in the generic ESRT support, since what we are mapping is not I/O, and this does make a difference on ARM/arm64 Patch #2 adds the missing efi_esrt_init() call, and tweaks some memmap fields that efi_mem_desc_lookup() expects. Ard Biesheuvel (2): efi: esrt: use memremap not ioremap to access ESRT table in memory arm64/efi: esrt: add missing call to efi_esrt_init() drivers/firmware/efi/arm-init.c | 3 +++ drivers/firmware/efi/esrt.c | 16 ++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) -- 2.5.0 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-15 11:32 [PATCH v2 0/2] efi: ARM/arm64: wire up ESRT table Ard Biesheuvel @ 2016-02-15 11:32 ` Ard Biesheuvel 2016-02-15 15:45 ` Peter Jones ` (2 more replies) 2016-02-15 11:32 ` [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() Ard Biesheuvel 1 sibling, 3 replies; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-15 11:32 UTC (permalink / raw) To: linux-arm-kernel On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes. Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence. Cc: Peter Jones <pjones@redhat•com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> --- drivers/firmware/efi/esrt.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c index 22c5285f7705..f096a0a26dbd 100644 --- a/drivers/firmware/efi/esrt.c +++ b/drivers/firmware/efi/esrt.c @@ -16,6 +16,7 @@ #include <linux/device.h> #include <linux/efi.h> #include <linux/init.h> +#include <linux/io.h> #include <linux/kernel.h> #include <linux/kobject.h> #include <linux/list.h> @@ -385,15 +386,15 @@ static void cleanup_entry_list(void) static int __init esrt_sysfs_init(void) { int error; - struct efi_system_resource_table __iomem *ioesrt; + struct efi_system_resource_table *memesrt; pr_debug("esrt-sysfs: loading.\n"); if (!esrt_data || !esrt_data_size) return -ENOSYS; - ioesrt = ioremap(esrt_data, esrt_data_size); - if (!ioesrt) { - pr_err("ioremap(%pa, %zu) failed.\n", &esrt_data, + memesrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB); + if (!memesrt) { + pr_err("memremap(%pa, %zu, MEMREMAP_WB) failed.\n", &esrt_data, esrt_data_size); return -ENOMEM; } @@ -401,11 +402,12 @@ static int __init esrt_sysfs_init(void) esrt = kmalloc(esrt_data_size, GFP_KERNEL); if (!esrt) { pr_err("kmalloc failed. (wanted %zu bytes)\n", esrt_data_size); - iounmap(ioesrt); + memunmap(memesrt); return -ENOMEM; } - memcpy_fromio(esrt, ioesrt, esrt_data_size); + memcpy(esrt, memesrt, esrt_data_size); + memunmap(memesrt); esrt_kobj = kobject_create_and_add("esrt", efi_kobj); if (!esrt_kobj) { @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) if (error) goto err_cleanup_list; - memblock_remove(esrt_data, esrt_data_size); - pr_debug("esrt-sysfs: loaded.\n"); return 0; -- 2.5.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel @ 2016-02-15 15:45 ` Peter Jones 2016-02-16 19:19 ` Baicar, Tyler 2016-02-18 10:44 ` Matt Fleming 2 siblings, 0 replies; 30+ messages in thread From: Peter Jones @ 2016-02-15 15:45 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 15, 2016 at 12:32:32PM +0100, Ard Biesheuvel wrote: > On ARM and arm64, ioremap() and memremap() are not interchangeable like > on x86, and the use of ioremap() on ordinary RAM is typically flagged > as an error if the memory region being mapped is also covered by the > linear mapping, since that would lead to aliases with conflicting > cacheability attributes. > > Since what we are dealing with is not an I/O region with side effects, > using ioremap() here is arguably incorrect anyway, so let's replace > it with memremap instead. Also add a missing unmap on the success path, > and drop a memblock_remove() call which does not belong here, this far > into the boot sequence. > > Cc: Peter Jones <pjones@redhat•com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> Works on my test box and appears to be correct: Tested-by: Peter Jones <pjones@redhat•com> Acked-by: Peter Jones <pjones@redhat•com> > --- > drivers/firmware/efi/esrt.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c > index 22c5285f7705..f096a0a26dbd 100644 > --- a/drivers/firmware/efi/esrt.c > +++ b/drivers/firmware/efi/esrt.c > @@ -16,6 +16,7 @@ > #include <linux/device.h> > #include <linux/efi.h> > #include <linux/init.h> > +#include <linux/io.h> > #include <linux/kernel.h> > #include <linux/kobject.h> > #include <linux/list.h> > @@ -385,15 +386,15 @@ static void cleanup_entry_list(void) > static int __init esrt_sysfs_init(void) > { > int error; > - struct efi_system_resource_table __iomem *ioesrt; > + struct efi_system_resource_table *memesrt; > > pr_debug("esrt-sysfs: loading.\n"); > if (!esrt_data || !esrt_data_size) > return -ENOSYS; > > - ioesrt = ioremap(esrt_data, esrt_data_size); > - if (!ioesrt) { > - pr_err("ioremap(%pa, %zu) failed.\n", &esrt_data, > + memesrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB); > + if (!memesrt) { > + pr_err("memremap(%pa, %zu, MEMREMAP_WB) failed.\n", &esrt_data, > esrt_data_size); > return -ENOMEM; > } > @@ -401,11 +402,12 @@ static int __init esrt_sysfs_init(void) > esrt = kmalloc(esrt_data_size, GFP_KERNEL); > if (!esrt) { > pr_err("kmalloc failed. (wanted %zu bytes)\n", esrt_data_size); > - iounmap(ioesrt); > + memunmap(memesrt); > return -ENOMEM; > } > > - memcpy_fromio(esrt, ioesrt, esrt_data_size); > + memcpy(esrt, memesrt, esrt_data_size); > + memunmap(memesrt); > > esrt_kobj = kobject_create_and_add("esrt", efi_kobj); > if (!esrt_kobj) { > @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > if (error) > goto err_cleanup_list; > > - memblock_remove(esrt_data, esrt_data_size); > - > pr_debug("esrt-sysfs: loaded.\n"); > > return 0; > -- > 2.5.0 > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel 2016-02-15 15:45 ` Peter Jones @ 2016-02-16 19:19 ` Baicar, Tyler 2016-02-18 10:44 ` Matt Fleming 2 siblings, 0 replies; 30+ messages in thread From: Baicar, Tyler @ 2016-02-16 19:19 UTC (permalink / raw) To: linux-arm-kernel On 2/15/2016 4:32 AM, Ard Biesheuvel wrote: > On ARM and arm64, ioremap() and memremap() are not interchangeable like > on x86, and the use of ioremap() on ordinary RAM is typically flagged > as an error if the memory region being mapped is also covered by the > linear mapping, since that would lead to aliases with conflicting > cacheability attributes. > > Since what we are dealing with is not an I/O region with side effects, > using ioremap() here is arguably incorrect anyway, so let's replace > it with memremap instead. Also add a missing unmap on the success path, > and drop a memblock_remove() call which does not belong here, this far > into the boot sequence. > > Cc: Peter Jones <pjones@redhat•com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> Tested-by: Tyler Baicar<tbaicar@codeaurora•org> > --- > drivers/firmware/efi/esrt.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c > index 22c5285f7705..f096a0a26dbd 100644 > --- a/drivers/firmware/efi/esrt.c > +++ b/drivers/firmware/efi/esrt.c > @@ -16,6 +16,7 @@ > #include <linux/device.h> > #include <linux/efi.h> > #include <linux/init.h> > +#include <linux/io.h> > #include <linux/kernel.h> > #include <linux/kobject.h> > #include <linux/list.h> > @@ -385,15 +386,15 @@ static void cleanup_entry_list(void) > static int __init esrt_sysfs_init(void) > { > int error; > - struct efi_system_resource_table __iomem *ioesrt; > + struct efi_system_resource_table *memesrt; > > pr_debug("esrt-sysfs: loading.\n"); > if (!esrt_data || !esrt_data_size) > return -ENOSYS; > > - ioesrt = ioremap(esrt_data, esrt_data_size); > - if (!ioesrt) { > - pr_err("ioremap(%pa, %zu) failed.\n", &esrt_data, > + memesrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB); > + if (!memesrt) { > + pr_err("memremap(%pa, %zu, MEMREMAP_WB) failed.\n", &esrt_data, > esrt_data_size); > return -ENOMEM; > } > @@ -401,11 +402,12 @@ static int __init esrt_sysfs_init(void) > esrt = kmalloc(esrt_data_size, GFP_KERNEL); > if (!esrt) { > pr_err("kmalloc failed. (wanted %zu bytes)\n", esrt_data_size); > - iounmap(ioesrt); > + memunmap(memesrt); > return -ENOMEM; > } > > - memcpy_fromio(esrt, ioesrt, esrt_data_size); > + memcpy(esrt, memesrt, esrt_data_size); > + memunmap(memesrt); > > esrt_kobj = kobject_create_and_add("esrt", efi_kobj); > if (!esrt_kobj) { > @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > if (error) > goto err_cleanup_list; > > - memblock_remove(esrt_data, esrt_data_size); > - > pr_debug("esrt-sysfs: loaded.\n"); > > return 0; Thanks, Tyler -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel 2016-02-15 15:45 ` Peter Jones 2016-02-16 19:19 ` Baicar, Tyler @ 2016-02-18 10:44 ` Matt Fleming 2016-02-18 12:16 ` Ard Biesheuvel 2 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2016-02-18 10:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > On ARM and arm64, ioremap() and memremap() are not interchangeable like > on x86, and the use of ioremap() on ordinary RAM is typically flagged > as an error if the memory region being mapped is also covered by the > linear mapping, since that would lead to aliases with conflicting > cacheability attributes. > > Since what we are dealing with is not an I/O region with side effects, > using ioremap() here is arguably incorrect anyway, so let's replace > it with memremap instead. Also add a missing unmap on the success path, > and drop a memblock_remove() call which does not belong here, this far > into the boot sequence. > > Cc: Peter Jones <pjones@redhat•com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > --- > drivers/firmware/efi/esrt.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > [...] > @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > if (error) > goto err_cleanup_list; > > - memblock_remove(esrt_data, esrt_data_size); > - > pr_debug("esrt-sysfs: loaded.\n"); > > return 0; Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 10:44 ` Matt Fleming @ 2016-02-18 12:16 ` Ard Biesheuvel 2016-02-18 13:28 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-18 12:16 UTC (permalink / raw) To: linux-arm-kernel On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: >> On ARM and arm64, ioremap() and memremap() are not interchangeable like >> on x86, and the use of ioremap() on ordinary RAM is typically flagged >> as an error if the memory region being mapped is also covered by the >> linear mapping, since that would lead to aliases with conflicting >> cacheability attributes. >> >> Since what we are dealing with is not an I/O region with side effects, >> using ioremap() here is arguably incorrect anyway, so let's replace >> it with memremap instead. Also add a missing unmap on the success path, >> and drop a memblock_remove() call which does not belong here, this far >> into the boot sequence. >> >> Cc: Peter Jones <pjones@redhat•com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> --- >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> > > [...] > >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) >> if (error) >> goto err_cleanup_list; >> >> - memblock_remove(esrt_data, esrt_data_size); >> - >> pr_debug("esrt-sysfs: loaded.\n"); >> >> return 0; > > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > The original ESRT region is still reserved at this point, so we should > do our best to release it to the page allocator. I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ... ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 12:16 ` Ard Biesheuvel @ 2016-02-18 13:28 ` Matt Fleming 2016-02-18 13:29 ` Ard Biesheuvel 0 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2016-02-18 13:28 UTC (permalink / raw) To: linux-arm-kernel On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: > On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > >> On ARM and arm64, ioremap() and memremap() are not interchangeable like > >> on x86, and the use of ioremap() on ordinary RAM is typically flagged > >> as an error if the memory region being mapped is also covered by the > >> linear mapping, since that would lead to aliases with conflicting > >> cacheability attributes. > >> > >> Since what we are dealing with is not an I/O region with side effects, > >> using ioremap() here is arguably incorrect anyway, so let's replace > >> it with memremap instead. Also add a missing unmap on the success path, > >> and drop a memblock_remove() call which does not belong here, this far > >> into the boot sequence. > >> > >> Cc: Peter Jones <pjones@redhat•com> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > >> --- > >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > > > > [...] > > > >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > >> if (error) > >> goto err_cleanup_list; > >> > >> - memblock_remove(esrt_data, esrt_data_size); > >> - > >> pr_debug("esrt-sysfs: loaded.\n"); > >> > >> return 0; > > > > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > > The original ESRT region is still reserved at this point, so we should > > do our best to release it to the page allocator. > > I'd rather we keep it reserved. That way, the config table entry still > points to something valid, which could be useful for kexec(), I think? > At least, that is how I intended to handle config tables on ARM ... If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 13:28 ` Matt Fleming @ 2016-02-18 13:29 ` Ard Biesheuvel 2016-02-18 13:43 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-18 13:29 UTC (permalink / raw) To: linux-arm-kernel On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged >> >> as an error if the memory region being mapped is also covered by the >> >> linear mapping, since that would lead to aliases with conflicting >> >> cacheability attributes. >> >> >> >> Since what we are dealing with is not an I/O region with side effects, >> >> using ioremap() here is arguably incorrect anyway, so let's replace >> >> it with memremap instead. Also add a missing unmap on the success path, >> >> and drop a memblock_remove() call which does not belong here, this far >> >> into the boot sequence. >> >> >> >> Cc: Peter Jones <pjones@redhat•com> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> >> --- >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- >> >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> >> > >> > [...] >> > >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) >> >> if (error) >> >> goto err_cleanup_list; >> >> >> >> - memblock_remove(esrt_data, esrt_data_size); >> >> - >> >> pr_debug("esrt-sysfs: loaded.\n"); >> >> >> >> return 0; >> > >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? >> > The original ESRT region is still reserved at this point, so we should >> > do our best to release it to the page allocator. >> >> I'd rather we keep it reserved. That way, the config table entry still >> points to something valid, which could be useful for kexec(), I think? >> At least, that is how I intended to handle config tables on ARM ... > > If we're going to reserve it why do we need to copy the data out at > all in esrt_sysfs_init()? Excellent question. I don't think there is any point to doing that. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 13:29 ` Ard Biesheuvel @ 2016-02-18 13:43 ` Matt Fleming 2016-02-18 13:44 ` Ard Biesheuvel 0 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2016-02-18 13:43 UTC (permalink / raw) To: linux-arm-kernel On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: > On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: > >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like > >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged > >> >> as an error if the memory region being mapped is also covered by the > >> >> linear mapping, since that would lead to aliases with conflicting > >> >> cacheability attributes. > >> >> > >> >> Since what we are dealing with is not an I/O region with side effects, > >> >> using ioremap() here is arguably incorrect anyway, so let's replace > >> >> it with memremap instead. Also add a missing unmap on the success path, > >> >> and drop a memblock_remove() call which does not belong here, this far > >> >> into the boot sequence. > >> >> > >> >> Cc: Peter Jones <pjones@redhat•com> > >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > >> >> --- > >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- > >> >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> >> > >> > > >> > [...] > >> > > >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > >> >> if (error) > >> >> goto err_cleanup_list; > >> >> > >> >> - memblock_remove(esrt_data, esrt_data_size); > >> >> - > >> >> pr_debug("esrt-sysfs: loaded.\n"); > >> >> > >> >> return 0; > >> > > >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > >> > The original ESRT region is still reserved at this point, so we should > >> > do our best to release it to the page allocator. > >> > >> I'd rather we keep it reserved. That way, the config table entry still > >> points to something valid, which could be useful for kexec(), I think? > >> At least, that is how I intended to handle config tables on ARM ... > > > > If we're going to reserve it why do we need to copy the data out at > > all in esrt_sysfs_init()? > > Excellent question. I don't think there is any point to doing that. ... Unless the data is contained in an EFI Boot Services region ;-) Peter? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 13:43 ` Matt Fleming @ 2016-02-18 13:44 ` Ard Biesheuvel 2016-02-18 14:15 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-18 13:44 UTC (permalink / raw) To: linux-arm-kernel On 18 February 2016 at 14:43, Matt Fleming <matt@codeblueprint•co.uk> wrote: > On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: >> On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: >> >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: >> >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like >> >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged >> >> >> as an error if the memory region being mapped is also covered by the >> >> >> linear mapping, since that would lead to aliases with conflicting >> >> >> cacheability attributes. >> >> >> >> >> >> Since what we are dealing with is not an I/O region with side effects, >> >> >> using ioremap() here is arguably incorrect anyway, so let's replace >> >> >> it with memremap instead. Also add a missing unmap on the success path, >> >> >> and drop a memblock_remove() call which does not belong here, this far >> >> >> into the boot sequence. >> >> >> >> >> >> Cc: Peter Jones <pjones@redhat•com> >> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> >> >> --- >> >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- >> >> >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> >> >> >> > >> >> > [...] >> >> > >> >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) >> >> >> if (error) >> >> >> goto err_cleanup_list; >> >> >> >> >> >> - memblock_remove(esrt_data, esrt_data_size); >> >> >> - >> >> >> pr_debug("esrt-sysfs: loaded.\n"); >> >> >> >> >> >> return 0; >> >> > >> >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? >> >> > The original ESRT region is still reserved at this point, so we should >> >> > do our best to release it to the page allocator. >> >> >> >> I'd rather we keep it reserved. That way, the config table entry still >> >> points to something valid, which could be useful for kexec(), I think? >> >> At least, that is how I intended to handle config tables on ARM ... >> > >> > If we're going to reserve it why do we need to copy the data out at >> > all in esrt_sysfs_init()? >> >> Excellent question. I don't think there is any point to doing that. > > ... Unless the data is contained in an EFI Boot Services region ;-) > > Peter? Yes, it usually is. Is that a problem? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 13:44 ` Ard Biesheuvel @ 2016-02-18 14:15 ` Matt Fleming 2016-02-18 14:21 ` Ard Biesheuvel 2016-02-18 19:16 ` Peter Jones 0 siblings, 2 replies; 30+ messages in thread From: Matt Fleming @ 2016-02-18 14:15 UTC (permalink / raw) To: linux-arm-kernel On Thu, 18 Feb, at 02:44:02PM, Ard Biesheuvel wrote: > On 18 February 2016 at 14:43, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: > >> On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: > >> >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > >> >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like > >> >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged > >> >> >> as an error if the memory region being mapped is also covered by the > >> >> >> linear mapping, since that would lead to aliases with conflicting > >> >> >> cacheability attributes. > >> >> >> > >> >> >> Since what we are dealing with is not an I/O region with side effects, > >> >> >> using ioremap() here is arguably incorrect anyway, so let's replace > >> >> >> it with memremap instead. Also add a missing unmap on the success path, > >> >> >> and drop a memblock_remove() call which does not belong here, this far > >> >> >> into the boot sequence. > >> >> >> > >> >> >> Cc: Peter Jones <pjones@redhat•com> > >> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > >> >> >> --- > >> >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- > >> >> >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> >> >> > >> >> > > >> >> > [...] > >> >> > > >> >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > >> >> >> if (error) > >> >> >> goto err_cleanup_list; > >> >> >> > >> >> >> - memblock_remove(esrt_data, esrt_data_size); > >> >> >> - > >> >> >> pr_debug("esrt-sysfs: loaded.\n"); > >> >> >> > >> >> >> return 0; > >> >> > > >> >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > >> >> > The original ESRT region is still reserved at this point, so we should > >> >> > do our best to release it to the page allocator. > >> >> > >> >> I'd rather we keep it reserved. That way, the config table entry still > >> >> points to something valid, which could be useful for kexec(), I think? > >> >> At least, that is how I intended to handle config tables on ARM ... > >> > > >> > If we're going to reserve it why do we need to copy the data out at > >> > all in esrt_sysfs_init()? > >> > >> Excellent question. I don't think there is any point to doing that. > > > > ... Unless the data is contained in an EFI Boot Services region ;-) > > > > Peter? > > Yes, it usually is. Is that a problem? Yes, we free the Boot Services regions before hitting userspace on x86, see efi_free_boot_services(). We do this map/copy/unmap trick in the ACPI BGRT driver for that reason. The Boot Services regions can be many gigabytes in size, which makes leaving them alone impractical. For kexec on x86 we simply discard the BGRT table, which isn't the end of the world because who really needs access to the BGRT image on kexec reboot? However, I can see the value of preserving the ESRT. I guess we've got two options, 1) copy out the chunks of Boot Services regions we're interested in and rewrite the EFI tables to point at these new allocations and free/discard all of the original Boot Services regions or 2) only selectively free the Boot Services regions. I've always stayed clear of 2) in case there exists cross-region references in the data that isn't obvious. I'd like to think that would never happen, but, you know, dragons lurk here, etc. Though actually, now I think about it, cross-region references can't possibly exist because they'd cause issues with the current code. So maybe the best solution is actually 2), where we preserve the Boot Services regions if any of the drivers (ESRT, BGRT) request them but free all the others? What are the lifetime rules for Boot Services regions on arm*? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 14:15 ` Matt Fleming @ 2016-02-18 14:21 ` Ard Biesheuvel 2016-02-18 14:38 ` Matt Fleming 2016-02-19 9:27 ` Dave Young 2016-02-18 19:16 ` Peter Jones 1 sibling, 2 replies; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-18 14:21 UTC (permalink / raw) To: linux-arm-kernel On 18 February 2016 at 15:15, Matt Fleming <matt@codeblueprint•co.uk> wrote: > On Thu, 18 Feb, at 02:44:02PM, Ard Biesheuvel wrote: >> On 18 February 2016 at 14:43, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> > On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: >> >> On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> >> > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: >> >> >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: >> >> >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: >> >> >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like >> >> >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged >> >> >> >> as an error if the memory region being mapped is also covered by the >> >> >> >> linear mapping, since that would lead to aliases with conflicting >> >> >> >> cacheability attributes. >> >> >> >> >> >> >> >> Since what we are dealing with is not an I/O region with side effects, >> >> >> >> using ioremap() here is arguably incorrect anyway, so let's replace >> >> >> >> it with memremap instead. Also add a missing unmap on the success path, >> >> >> >> and drop a memblock_remove() call which does not belong here, this far >> >> >> >> into the boot sequence. >> >> >> >> >> >> >> >> Cc: Peter Jones <pjones@redhat•com> >> >> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> >> >> >> --- >> >> >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- >> >> >> >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> >> >> >> >> >> > >> >> >> > [...] >> >> >> > >> >> >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) >> >> >> >> if (error) >> >> >> >> goto err_cleanup_list; >> >> >> >> >> >> >> >> - memblock_remove(esrt_data, esrt_data_size); >> >> >> >> - >> >> >> >> pr_debug("esrt-sysfs: loaded.\n"); >> >> >> >> >> >> >> >> return 0; >> >> >> > >> >> >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? >> >> >> > The original ESRT region is still reserved at this point, so we should >> >> >> > do our best to release it to the page allocator. >> >> >> >> >> >> I'd rather we keep it reserved. That way, the config table entry still >> >> >> points to something valid, which could be useful for kexec(), I think? >> >> >> At least, that is how I intended to handle config tables on ARM ... >> >> > >> >> > If we're going to reserve it why do we need to copy the data out at >> >> > all in esrt_sysfs_init()? >> >> >> >> Excellent question. I don't think there is any point to doing that. >> > >> > ... Unless the data is contained in an EFI Boot Services region ;-) >> > >> > Peter? >> >> Yes, it usually is. Is that a problem? > > Yes, we free the Boot Services regions before hitting userspace on > x86, see efi_free_boot_services(). We do this map/copy/unmap trick in > the ACPI BGRT driver for that reason. > > The Boot Services regions can be many gigabytes in size, which makes > leaving them alone impractical. > > For kexec on x86 we simply discard the BGRT table, which isn't the end > of the world because who really needs access to the BGRT image on > kexec reboot? However, I can see the value of preserving the ESRT. > > I guess we've got two options, 1) copy out the chunks of Boot Services > regions we're interested in and rewrite the EFI tables to point at > these new allocations and free/discard all of the original Boot > Services regions or 2) only selectively free the Boot Services > regions. > > I've always stayed clear of 2) in case there exists cross-region > references in the data that isn't obvious. I'd like to think that > would never happen, but, you know, dragons lurk here, etc. > > Though actually, now I think about it, cross-region references can't > possibly exist because they'd cause issues with the current code. > > So maybe the best solution is actually 2), where we preserve the Boot > Services regions if any of the drivers (ESRT, BGRT) request them but > free all the others? > > What are the lifetime rules for Boot Services regions on arm*? We treat all Boot Services regions like Loader Code/Data or free regions: it is all recorded in memblock as usable memory, and only the regions that are explicitly reserved are protected from further general use. I am currently looking into the memory attribute table, and the use case is very similar. It would be very useful from our pov to simply memblock_reserve() the region right after having called efi_config_parse_tables(), and actually consume its data when we get around to it later. The ESRT handling is already split down the middle in the same way. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 14:21 ` Ard Biesheuvel @ 2016-02-18 14:38 ` Matt Fleming 2016-02-19 9:27 ` Dave Young 1 sibling, 0 replies; 30+ messages in thread From: Matt Fleming @ 2016-02-18 14:38 UTC (permalink / raw) To: linux-arm-kernel On Thu, 18 Feb, at 03:21:25PM, Ard Biesheuvel wrote: > > We treat all Boot Services regions like Loader Code/Data or free > regions: it is all recorded in memblock as usable memory, and only the > regions that are explicitly reserved are protected from further > general use. > > I am currently looking into the memory attribute table, and the use > case is very similar. It would be very useful from our pov to simply > memblock_reserve() the region right after having called > efi_config_parse_tables(), and actually consume its data when we get > around to it later. The ESRT handling is already split down the middle > in the same way. Agreed, this would also be useful in general for kexec. I've got a couple of patches in flight that try to handle the BGRT case in a different way, https://lkml.kernel.org/r/1455723910-16710-2-git-send-email-matt at codeblueprint.co.uk https://lkml.kernel.org/r/1455723910-16710-3-git-send-email-matt at codeblueprint.co.uk Let me instead take a look at how we might be able to preserve only those Boot Services regions we care out. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 14:21 ` Ard Biesheuvel 2016-02-18 14:38 ` Matt Fleming @ 2016-02-19 9:27 ` Dave Young 1 sibling, 0 replies; 30+ messages in thread From: Dave Young @ 2016-02-19 9:27 UTC (permalink / raw) To: linux-arm-kernel On 02/18/16 at 03:21pm, Ard Biesheuvel wrote: > On 18 February 2016 at 15:15, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > On Thu, 18 Feb, at 02:44:02PM, Ard Biesheuvel wrote: > >> On 18 February 2016 at 14:43, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> > On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: > >> >> On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> >> > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: > >> >> >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > >> >> >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > >> >> >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like > >> >> >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged > >> >> >> >> as an error if the memory region being mapped is also covered by the > >> >> >> >> linear mapping, since that would lead to aliases with conflicting > >> >> >> >> cacheability attributes. > >> >> >> >> > >> >> >> >> Since what we are dealing with is not an I/O region with side effects, > >> >> >> >> using ioremap() here is arguably incorrect anyway, so let's replace > >> >> >> >> it with memremap instead. Also add a missing unmap on the success path, > >> >> >> >> and drop a memblock_remove() call which does not belong here, this far > >> >> >> >> into the boot sequence. > >> >> >> >> > >> >> >> >> Cc: Peter Jones <pjones@redhat•com> > >> >> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > >> >> >> >> --- > >> >> >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- > >> >> >> >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> >> >> >> > >> >> >> > > >> >> >> > [...] > >> >> >> > > >> >> >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > >> >> >> >> if (error) > >> >> >> >> goto err_cleanup_list; > >> >> >> >> > >> >> >> >> - memblock_remove(esrt_data, esrt_data_size); > >> >> >> >> - > >> >> >> >> pr_debug("esrt-sysfs: loaded.\n"); > >> >> >> >> > >> >> >> >> return 0; > >> >> >> > > >> >> >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > >> >> >> > The original ESRT region is still reserved at this point, so we should > >> >> >> > do our best to release it to the page allocator. > >> >> >> > >> >> >> I'd rather we keep it reserved. That way, the config table entry still > >> >> >> points to something valid, which could be useful for kexec(), I think? > >> >> >> At least, that is how I intended to handle config tables on ARM ... > >> >> > > >> >> > If we're going to reserve it why do we need to copy the data out at > >> >> > all in esrt_sysfs_init()? > >> >> > >> >> Excellent question. I don't think there is any point to doing that. > >> > > >> > ... Unless the data is contained in an EFI Boot Services region ;-) > >> > > >> > Peter? > >> > >> Yes, it usually is. Is that a problem? > > > > Yes, we free the Boot Services regions before hitting userspace on > > x86, see efi_free_boot_services(). We do this map/copy/unmap trick in > > the ACPI BGRT driver for that reason. > > > > The Boot Services regions can be many gigabytes in size, which makes > > leaving them alone impractical. > > > > For kexec on x86 we simply discard the BGRT table, which isn't the end > > of the world because who really needs access to the BGRT image on > > kexec reboot? However, I can see the value of preserving the ESRT. > > > > I guess we've got two options, 1) copy out the chunks of Boot Services > > regions we're interested in and rewrite the EFI tables to point at > > these new allocations and free/discard all of the original Boot > > Services regions or 2) only selectively free the Boot Services > > regions. > > > > I've always stayed clear of 2) in case there exists cross-region > > references in the data that isn't obvious. I'd like to think that > > would never happen, but, you know, dragons lurk here, etc. > > > > Though actually, now I think about it, cross-region references can't > > possibly exist because they'd cause issues with the current code. > > > > So maybe the best solution is actually 2), where we preserve the Boot > > Services regions if any of the drivers (ESRT, BGRT) request them but > > free all the others? It is a good idea so that drivers can add their useful sections to a list or an array, they can avoid another copy of the memory also. > > > > What are the lifetime rules for Boot Services regions on arm*? > > We treat all Boot Services regions like Loader Code/Data or free > regions: it is all recorded in memblock as usable memory, and only the > regions that are explicitly reserved are protected from further > general use. > > I am currently looking into the memory attribute table, and the use > case is very similar. It would be very useful from our pov to simply > memblock_reserve() the region right after having called > efi_config_parse_tables(), and actually consume its data when we get > around to it later. The ESRT handling is already split down the middle > in the same way. A question is how can make a general way for both x86 and arm*. Maybe change the efi_free_boot_mem to something like efi_clean_boot_mem? It can first call efi_free_boot_mem, then reserve the ranges to be reserved again, but it sounds odd though. Thanks Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 14:15 ` Matt Fleming 2016-02-18 14:21 ` Ard Biesheuvel @ 2016-02-18 19:16 ` Peter Jones 2016-02-26 14:41 ` Matt Fleming 1 sibling, 1 reply; 30+ messages in thread From: Peter Jones @ 2016-02-18 19:16 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 18, 2016 at 02:15:44PM +0000, Matt Fleming wrote: > On Thu, 18 Feb, at 02:44:02PM, Ard Biesheuvel wrote: > > On 18 February 2016 at 14:43, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > > On Thu, 18 Feb, at 02:29:32PM, Ard Biesheuvel wrote: > > >> On 18 February 2016 at 14:28, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > >> > On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote: > > >> >> On 18 February 2016 at 11:44, Matt Fleming <matt@codeblueprint•co.uk> wrote: > > >> >> > On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote: > > >> >> >> On ARM and arm64, ioremap() and memremap() are not interchangeable like > > >> >> >> on x86, and the use of ioremap() on ordinary RAM is typically flagged > > >> >> >> as an error if the memory region being mapped is also covered by the > > >> >> >> linear mapping, since that would lead to aliases with conflicting > > >> >> >> cacheability attributes. > > >> >> >> > > >> >> >> Since what we are dealing with is not an I/O region with side effects, > > >> >> >> using ioremap() here is arguably incorrect anyway, so let's replace > > >> >> >> it with memremap instead. Also add a missing unmap on the success path, > > >> >> >> and drop a memblock_remove() call which does not belong here, this far > > >> >> >> into the boot sequence. > > >> >> >> > > >> >> >> Cc: Peter Jones <pjones@redhat•com> > > >> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > > >> >> >> --- > > >> >> >> drivers/firmware/efi/esrt.c | 16 ++++++++-------- > > >> >> >> 1 file changed, 8 insertions(+), 8 deletions(-) > > >> >> >> > > >> >> > > > >> >> > [...] > > >> >> > > > >> >> >> @@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void) > > >> >> >> if (error) > > >> >> >> goto err_cleanup_list; > > >> >> >> > > >> >> >> - memblock_remove(esrt_data, esrt_data_size); > > >> >> >> - > > >> >> >> pr_debug("esrt-sysfs: loaded.\n"); > > >> >> >> > > >> >> >> return 0; > > >> >> > > > >> >> > Shouldn't we be replacing memblock_remove() with free_bootmem_late()? > > >> >> > The original ESRT region is still reserved at this point, so we should > > >> >> > do our best to release it to the page allocator. > > >> >> > > >> >> I'd rather we keep it reserved. That way, the config table entry still > > >> >> points to something valid, which could be useful for kexec(), I think? > > >> >> At least, that is how I intended to handle config tables on ARM ... > > >> > > > >> > If we're going to reserve it why do we need to copy the data out at > > >> > all in esrt_sysfs_init()? > > >> > > >> Excellent question. I don't think there is any point to doing that. > > > > > > ... Unless the data is contained in an EFI Boot Services region ;-) > > > > > > Peter? > > > > Yes, it usually is. Is that a problem? > > Yes, we free the Boot Services regions before hitting userspace on > x86, see efi_free_boot_services(). We do this map/copy/unmap trick in > the ACPI BGRT driver for that reason. > > The Boot Services regions can be many gigabytes in size, which makes > leaving them alone impractical. > > For kexec on x86 we simply discard the BGRT table, which isn't the end > of the world because who really needs access to the BGRT image on > kexec reboot? However, I can see the value of preserving the ESRT. > > I guess we've got two options, 1) copy out the chunks of Boot Services > regions we're interested in and rewrite the EFI tables to point at > these new allocations and free/discard all of the original Boot > Services regions or 2) only selectively free the Boot Services > regions. I think we've got to copy them out anyway and give a new copy to any kexec'd kernel. In 5.6 in the spec, it says of the table data: The VendorGuid associated with a given VendorTable pointer defines whether or not a particular address reported in the table gets fixed up when a call to SetVirtualAddressMap() is made. It is the responsibility of the specification defining the VendorTable to specify whether to convert the addresses reported in the table. Of course the ESRT definition doesn't say anything about that, since it actually says it's in BootServicesData, and so remapping its address doesn't really make a lot of sense. Unfortunately the others are actually *less* well defined. Here's a list of the tables defined in the 2.6 spec, and what they say about either their allocation region or their remap policy: ACPI20 nada Physical Address (PA) ACPI nada PA SAL nada PA SMBIOS nada PA SMBIOS3 nada PA MPS nada PA properties nada PA memory attrs nada nada debugimage info nada nada IEIT nada nada ESRT BootServicesData nada So... for the ones specified by physical addresses, we know we can access them early on, and it probably doesn't matter what allocation pool they're in, because we're reserving them anyway. Memory attributes I would /guess/ have to also be specified by PAs; it's in the very next subsection of the spec from all the other ones, and I think it just got omitted from the list. That leaves the last three. As an aside, since these definitions are so very poor, I'm going to put it on my TODO for USST to clarify and fix these up, as well as to look at systemic fixes for this problem in the future. Anyway, the last three. I think it's safe to assume that they are mapped to virtual addresses we can read from *before* SetVirtualAddressMap() and ExitBootServices() are called, especially since we generally have an identity map before then anyway, but we really don't know anything at all after that. We also don't know the sizes without parsing the tables themselves, which continues to be a pain in the ass. > I've always stayed clear of 2) in case there exists cross-region > references in the data that isn't obvious. I'd like to think that > would never happen, but, you know, dragons lurk here, etc. > > Though actually, now I think about it, cross-region references can't > possibly exist because they'd cause issues with the current code. I would think any single table has to be entirely within the same region, yes. > So maybe the best solution is actually 2), where we preserve the Boot > Services regions if any of the drivers (ESRT, BGRT) request them but > free all the others? Note that BGRT is an ACPI table rather than an EFI Configuration Table, and as such we actually have bounds for it. The others are... special. - Debug Image Info actually has a size. That's nice of it. It's also the table we've never talked about using. - ESRT you have to compute the size as your parse it, and it has variable length members, so the driver for it needs to figure it out. - IEIT has a number of entries at the top level, and then each entry has a size. Probably still best to have the driver that's parsing it figure the size out. - the Memory Attributes Table has a number of entries and a size of each entry, so you can just multiply. > What are the lifetime rules for Boot Services regions on arm*? I'll leave this for Ard and friends. So the question I have is: would you rather chop up regions and reserve the space, or allocate a new copy and update the configuration table (after ExitBootServices()) to point to it? The latter makes it pretty easy to do from an API that all these drivers can use, and it makes the kexec case completely transparent. Up to you. -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-18 19:16 ` Peter Jones @ 2016-02-26 14:41 ` Matt Fleming 2016-03-01 23:30 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2016-02-26 14:41 UTC (permalink / raw) To: linux-arm-kernel On Thu, 18 Feb, at 02:16:24PM, Peter Jones wrote: > > So the question I have is: would you rather chop up regions and reserve > the space, or allocate a new copy and update the configuration table > (after ExitBootServices()) to point to it? The latter makes it pretty > easy to do from an API that all these drivers can use, and it makes the > kexec case completely transparent. > > Up to you. I think it makes the most sense to chop up the regions we care about (i.e. the ones we have drivers for) because not only does that save us the effort of copying out the data on every kexec reboot, it also prevents us from leaking each copy since we don't free them at the moment. Then we just need to maintain a separate list of regions to free in efi_free_boot_services() or have some other way to distinguish between them. Oh, and save_runtime_map() would need updating to save Boot Services regions too. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-02-26 14:41 ` Matt Fleming @ 2016-03-01 23:30 ` Matt Fleming 2016-03-01 23:31 ` Matt Fleming ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Matt Fleming @ 2016-03-01 23:30 UTC (permalink / raw) To: linux-arm-kernel On Fri, 26 Feb, at 02:41:14PM, Matt Fleming wrote: > On Thu, 18 Feb, at 02:16:24PM, Peter Jones wrote: > > > > So the question I have is: would you rather chop up regions and reserve > > the space, or allocate a new copy and update the configuration table > > (after ExitBootServices()) to point to it? The latter makes it pretty > > easy to do from an API that all these drivers can use, and it makes the > > kexec case completely transparent. > > > > Up to you. > > I think it makes the most sense to chop up the regions we care about > (i.e. the ones we have drivers for) because not only does that save us > the effort of copying out the data on every kexec reboot, it also > prevents us from leaking each copy since we don't free them at the > moment. > > Then we just need to maintain a separate list of regions to free in > efi_free_boot_services() or have some other way to distinguish between > them. > > Oh, and save_runtime_map() would need updating to save Boot Services > regions too. I have a very rough draft of patches that implement this strategy on the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is curious and wants to take a look. The series will be sent out soon. I wouldn't necessarily try running them or anything as there's a few things that I know are broken; ia64 build, EFI mixed mode boot, probably arm64 regressions. The upshot of the patch series is that there's a whole lot of code we can share for manipulating memory maps between all EFI architectures and by fixing this "reserve boot services regions" in a generic way it should mean kexec under EFI on arm64 should be fairly trivial to implement, along with ESRT, etc. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-03-01 23:30 ` Matt Fleming @ 2016-03-01 23:31 ` Matt Fleming 2016-03-02 1:16 ` Dave Young 2016-03-04 6:25 ` Dave Young 2 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2016-03-01 23:31 UTC (permalink / raw) To: linux-arm-kernel On Tue, 01 Mar, at 11:30:40PM, Matt Fleming wrote: > On Fri, 26 Feb, at 02:41:14PM, Matt Fleming wrote: > > On Thu, 18 Feb, at 02:16:24PM, Peter Jones wrote: > > > > > > So the question I have is: would you rather chop up regions and reserve > > > the space, or allocate a new copy and update the configuration table > > > (after ExitBootServices()) to point to it? The latter makes it pretty > > > easy to do from an API that all these drivers can use, and it makes the > > > kexec case completely transparent. > > > > > > Up to you. > > > > I think it makes the most sense to chop up the regions we care about > > (i.e. the ones we have drivers for) because not only does that save us > > the effort of copying out the data on every kexec reboot, it also > > prevents us from leaking each copy since we don't free them at the > > moment. > > > > Then we just need to maintain a separate list of regions to free in > > efi_free_boot_services() or have some other way to distinguish between > > them. > > > > Oh, and save_runtime_map() would need updating to save Boot Services > > regions too. > > I have a very rough draft of patches that implement this strategy on > the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is > curious and wants to take a look. The series will be sent out soon. Obviously that's supposed to read: experimental/efi-memmap ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-03-01 23:30 ` Matt Fleming 2016-03-01 23:31 ` Matt Fleming @ 2016-03-02 1:16 ` Dave Young 2016-03-02 10:23 ` Matt Fleming 2016-03-04 6:25 ` Dave Young 2 siblings, 1 reply; 30+ messages in thread From: Dave Young @ 2016-03-02 1:16 UTC (permalink / raw) To: linux-arm-kernel On 03/01/16 at 11:30pm, Matt Fleming wrote: > On Fri, 26 Feb, at 02:41:14PM, Matt Fleming wrote: > > On Thu, 18 Feb, at 02:16:24PM, Peter Jones wrote: > > > > > > So the question I have is: would you rather chop up regions and reserve > > > the space, or allocate a new copy and update the configuration table > > > (after ExitBootServices()) to point to it? The latter makes it pretty > > > easy to do from an API that all these drivers can use, and it makes the > > > kexec case completely transparent. > > > > > > Up to you. > > > > I think it makes the most sense to chop up the regions we care about > > (i.e. the ones we have drivers for) because not only does that save us > > the effort of copying out the data on every kexec reboot, it also > > prevents us from leaking each copy since we don't free them at the > > moment. > > > > Then we just need to maintain a separate list of regions to free in > > efi_free_boot_services() or have some other way to distinguish between > > them. > > > > Oh, and save_runtime_map() would need updating to save Boot Services > > regions too. > > I have a very rough draft of patches that implement this strategy on > the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is > curious and wants to take a look. The series will be sent out soon. > > I wouldn't necessarily try running them or anything as there's a few > things that I know are broken; ia64 build, EFI mixed mode boot, > probably arm64 regressions. > > The upshot of the patch series is that there's a whole lot of code we > can share for manipulating memory maps between all EFI architectures > and by fixing this "reserve boot services regions" in a generic way it > should mean kexec under EFI on arm64 should be fairly trivial to > implement, along with ESRT, etc. Matt, thanks. One question is for the acpi patches you sent before, do you think they are still necessary as a fix for the bgrt problem and coexist with the "reserve useful boot services" proposal? Thanks Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-03-02 1:16 ` Dave Young @ 2016-03-02 10:23 ` Matt Fleming 0 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2016-03-02 10:23 UTC (permalink / raw) To: linux-arm-kernel On Wed, 02 Mar, at 09:16:19AM, Dave Young wrote: > > Matt, thanks. One question is for the acpi patches you sent before, do you > think they are still necessary as a fix for the bgrt problem and coexist > with the "reserve useful boot services" proposal? No, the previous ACPI patches I sent can be thrown in the bin if we go with the approach outlined in this email. This approach is superior because it allows useful features such as ESRT and the new memory attributes to be used for kexec boot - the ACPI patches prevented a crash by disabling BGRT. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-03-01 23:30 ` Matt Fleming 2016-03-01 23:31 ` Matt Fleming 2016-03-02 1:16 ` Dave Young @ 2016-03-04 6:25 ` Dave Young 2016-05-18 8:36 ` Ard Biesheuvel 2 siblings, 1 reply; 30+ messages in thread From: Dave Young @ 2016-03-04 6:25 UTC (permalink / raw) To: linux-arm-kernel > I have a very rough draft of patches that implement this strategy on > the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is > curious and wants to take a look. The series will be sent out soon. > > I wouldn't necessarily try running them or anything as there's a few > things that I know are broken; ia64 build, EFI mixed mode boot, > probably arm64 regressions. Matt, I tried :). I see a build warning about print phys_addr_t without %pa anotation and booting with ioremap error messages about slot not found , and ioremap region leak messages. Looking forward to your formal posts. Thanks Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-03-04 6:25 ` Dave Young @ 2016-05-18 8:36 ` Ard Biesheuvel 2016-05-18 8:53 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2016-05-18 8:36 UTC (permalink / raw) To: linux-arm-kernel On 4 March 2016 at 07:25, Dave Young <dyoung@redhat•com> wrote: >> I have a very rough draft of patches that implement this strategy on >> the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is >> curious and wants to take a look. The series will be sent out soon. >> >> I wouldn't necessarily try running them or anything as there's a few >> things that I know are broken; ia64 build, EFI mixed mode boot, >> probably arm64 regressions. > > Matt, I tried :). I see a build warning about print phys_addr_t without > %pa anotation and booting with ioremap error messages about slot not found > , and ioremap region leak messages. > > Looking forward to your formal posts. > So what is the status here? I know Sai is working on x86 support for the memory attributes table, so it would make sense settle this matter as well. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-05-18 8:36 ` Ard Biesheuvel @ 2016-05-18 8:53 ` Matt Fleming 2016-06-16 13:47 ` Ard Biesheuvel 0 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2016-05-18 8:53 UTC (permalink / raw) To: linux-arm-kernel On Wed, 18 May, at 10:36:33AM, Ard Biesheuvel wrote: > > So what is the status here? I know Sai is working on x86 support for > the memory attributes table, so it would make sense settle this matter > as well. This got stuck behind some other work, sorry about that. Some of this was just merged by Linus (getting rid of the global memmap and switching everyone over to efi.memmap), a large chunk remains however. I should be able to get those patches mailed out once the merge window closes. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-05-18 8:53 ` Matt Fleming @ 2016-06-16 13:47 ` Ard Biesheuvel 2016-06-20 11:49 ` Matt Fleming 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2016-06-16 13:47 UTC (permalink / raw) To: linux-arm-kernel On 18 May 2016 at 10:53, Matt Fleming <matt@codeblueprint•co.uk> wrote: > On Wed, 18 May, at 10:36:33AM, Ard Biesheuvel wrote: >> >> So what is the status here? I know Sai is working on x86 support for >> the memory attributes table, so it would make sense settle this matter >> as well. > > This got stuck behind some other work, sorry about that. > > Some of this was just merged by Linus (getting rid of the global > memmap and switching everyone over to efi.memmap), a large chunk > remains however. > > I should be able to get those patches mailed out once the merge window > closes. Hi Matt, Any progress here? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory 2016-06-16 13:47 ` Ard Biesheuvel @ 2016-06-20 11:49 ` Matt Fleming 0 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2016-06-20 11:49 UTC (permalink / raw) To: linux-arm-kernel On Thu, 16 Jun, at 03:47:30PM, Ard Biesheuvel wrote: > > Hi Matt, > > Any progress here? Expect something to be posted this week. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() 2016-02-15 11:32 [PATCH v2 0/2] efi: ARM/arm64: wire up ESRT table Ard Biesheuvel 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel @ 2016-02-15 11:32 ` Ard Biesheuvel 2016-02-15 15:46 ` Peter Jones 2016-02-16 20:25 ` Baicar, Tyler 1 sibling, 2 replies; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-15 11:32 UTC (permalink / raw) To: linux-arm-kernel ESRT support is built by default for all architectures that define CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, since efi_esrt_init() was never called. So add the missing call. Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies on efi.memmap having been assigned and populated completetely, add the missing assignments of efi.memmap and efi.memmap->nr_map. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> --- drivers/firmware/efi/arm-init.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c index 9e15d571b53c..5c5e799bdb50 100644 --- a/drivers/firmware/efi/arm-init.c +++ b/drivers/firmware/efi/arm-init.c @@ -197,10 +197,13 @@ void __init efi_init(void) memmap.map_end = memmap.map + params.mmap_size; memmap.desc_size = params.desc_size; memmap.desc_version = params.desc_ver; + memmap.nr_map = params.mmap_size / params.desc_size; + efi.memmap = &memmap; if (uefi_init() < 0) return; + efi_esrt_init(); reserve_regions(); early_memunmap(memmap.map, params.mmap_size); memblock_mark_nomap(params.mmap & PAGE_MASK, -- 2.5.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() 2016-02-15 11:32 ` [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() Ard Biesheuvel @ 2016-02-15 15:46 ` Peter Jones 2016-02-16 20:25 ` Baicar, Tyler 1 sibling, 0 replies; 30+ messages in thread From: Peter Jones @ 2016-02-15 15:46 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 15, 2016 at 12:32:33PM +0100, Ard Biesheuvel wrote: > ESRT support is built by default for all architectures that define > CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, > since efi_esrt_init() was never called. So add the missing call. > > Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies > on efi.memmap having been assigned and populated completetely, add the > missing assignments of efi.memmap and efi.memmap->nr_map. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> Looks correct to me, but I can't test this one: Acked-by: Peter Jones <pjones@redhat•com> > --- > drivers/firmware/efi/arm-init.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index 9e15d571b53c..5c5e799bdb50 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -197,10 +197,13 @@ void __init efi_init(void) > memmap.map_end = memmap.map + params.mmap_size; > memmap.desc_size = params.desc_size; > memmap.desc_version = params.desc_ver; > + memmap.nr_map = params.mmap_size / params.desc_size; > + efi.memmap = &memmap; > > if (uefi_init() < 0) > return; > > + efi_esrt_init(); > reserve_regions(); > early_memunmap(memmap.map, params.mmap_size); > memblock_mark_nomap(params.mmap & PAGE_MASK, > -- > 2.5.0 > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() 2016-02-15 11:32 ` [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() Ard Biesheuvel 2016-02-15 15:46 ` Peter Jones @ 2016-02-16 20:25 ` Baicar, Tyler 2016-02-17 8:32 ` Ard Biesheuvel 2016-05-17 20:36 ` Christopher Covington 1 sibling, 2 replies; 30+ messages in thread From: Baicar, Tyler @ 2016-02-16 20:25 UTC (permalink / raw) To: linux-arm-kernel On 2/15/2016 4:32 AM, Ard Biesheuvel wrote: > ESRT support is built by default for all architectures that define > CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, > since efi_esrt_init() was never called. So add the missing call. > > Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies > on efi.memmap having been assigned and populated completetely, add the > missing assignments of efi.memmap and efi.memmap->nr_map. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> > --- > drivers/firmware/efi/arm-init.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index 9e15d571b53c..5c5e799bdb50 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -197,10 +197,13 @@ void __init efi_init(void) > memmap.map_end = memmap.map + params.mmap_size; > memmap.desc_size = params.desc_size; > memmap.desc_version = params.desc_ver; > + memmap.nr_map = params.mmap_size / params.desc_size; > + efi.memmap = &memmap; > > if (uefi_init() < 0) > return; > > + efi_esrt_init(); This call to efi_esrt_init() is failing because efi.flags does not have EFI_MEMMAP set. This flag gets set at the end of reserve_regions(). I tested moving the set_bit() call from reserve_regions() to just before efi_esrt_init() and also tested moving efi_esrt_init() to just after reserve_regions(). Both of these options worked. > reserve_regions(); > early_memunmap(memmap.map, params.mmap_size); > memblock_mark_nomap(params.mmap & PAGE_MASK, Thanks, Tyler -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() 2016-02-16 20:25 ` Baicar, Tyler @ 2016-02-17 8:32 ` Ard Biesheuvel 2016-05-17 20:36 ` Christopher Covington 1 sibling, 0 replies; 30+ messages in thread From: Ard Biesheuvel @ 2016-02-17 8:32 UTC (permalink / raw) To: linux-arm-kernel On 16 February 2016 at 21:25, Baicar, Tyler <tbaicar@codeaurora•org> wrote: > On 2/15/2016 4:32 AM, Ard Biesheuvel wrote: >> >> ESRT support is built by default for all architectures that define >> CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, >> since efi_esrt_init() was never called. So add the missing call. >> >> Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies >> on efi.memmap having been assigned and populated completetely, add the >> missing assignments of efi.memmap and efi.memmap->nr_map. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> --- >> drivers/firmware/efi/arm-init.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/firmware/efi/arm-init.c >> b/drivers/firmware/efi/arm-init.c >> index 9e15d571b53c..5c5e799bdb50 100644 >> --- a/drivers/firmware/efi/arm-init.c >> +++ b/drivers/firmware/efi/arm-init.c >> @@ -197,10 +197,13 @@ void __init efi_init(void) >> memmap.map_end = memmap.map + params.mmap_size; >> memmap.desc_size = params.desc_size; >> memmap.desc_version = params.desc_ver; >> + memmap.nr_map = params.mmap_size / params.desc_size; >> + efi.memmap = &memmap; >> if (uefi_init() < 0) >> return; >> + efi_esrt_init(); > > This call to efi_esrt_init() is failing because efi.flags does not have > EFI_MEMMAP set. This flag gets set at the end of reserve_regions(). I tested > moving the set_bit() call from reserve_regions() to just before > efi_esrt_init() and also tested moving efi_esrt_init() to just after > reserve_regions(). Both of these options worked. Thanks for spotting that. I moved the call around for some reason (I don't remember why, exactly), but it indeed belongs after the call to reserve_regions() I will send out a v2 >> >> reserve_regions(); >> early_memunmap(memmap.map, params.mmap_size); >> memblock_mark_nomap(params.mmap & PAGE_MASK, > > Thanks, > Tyler > > -- > Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() 2016-02-16 20:25 ` Baicar, Tyler 2016-02-17 8:32 ` Ard Biesheuvel @ 2016-05-17 20:36 ` Christopher Covington 1 sibling, 0 replies; 30+ messages in thread From: Christopher Covington @ 2016-05-17 20:36 UTC (permalink / raw) To: linux-arm-kernel Hi Ard, On 02/16/2016 03:25 PM, Baicar, Tyler wrote: > On 2/15/2016 4:32 AM, Ard Biesheuvel wrote: >> ESRT support is built by default for all architectures that define >> CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, >> since efi_esrt_init() was never called. So add the missing call. >> >> Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies >> on efi.memmap having been assigned and populated completetely, add the >> missing assignments of efi.memmap and efi.memmap->nr_map. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro•org> >> --- >> drivers/firmware/efi/arm-init.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/firmware/efi/arm-init.c >> b/drivers/firmware/efi/arm-init.c >> index 9e15d571b53c..5c5e799bdb50 100644 >> --- a/drivers/firmware/efi/arm-init.c >> +++ b/drivers/firmware/efi/arm-init.c >> @@ -197,10 +197,13 @@ void __init efi_init(void) >> memmap.map_end = memmap.map + params.mmap_size; >> memmap.desc_size = params.desc_size; >> memmap.desc_version = params.desc_ver; >> + memmap.nr_map = params.mmap_size / params.desc_size; >> + efi.memmap = &memmap; >> if (uefi_init() < 0) >> return; >> + efi_esrt_init(); > This call to efi_esrt_init() is failing because efi.flags does not have > EFI_MEMMAP set. This flag gets set at the end of reserve_regions(). I > tested moving the set_bit() call from reserve_regions() to just before > efi_esrt_init() and also tested moving efi_esrt_init() to just after > reserve_regions(). Both of these options worked. >> reserve_regions(); >> early_memunmap(memmap.map, params.mmap_size); >> memblock_mark_nomap(params.mmap & PAGE_MASK, Do you expect to respin this patch in the near future? Thanks, Cov -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2016-06-20 11:49 UTC | newest] Thread overview: 30+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-15 11:32 [PATCH v2 0/2] efi: ARM/arm64: wire up ESRT table Ard Biesheuvel 2016-02-15 11:32 ` [PATCH v2 1/2] efi: esrt: use memremap not ioremap to access ESRT table in memory Ard Biesheuvel 2016-02-15 15:45 ` Peter Jones 2016-02-16 19:19 ` Baicar, Tyler 2016-02-18 10:44 ` Matt Fleming 2016-02-18 12:16 ` Ard Biesheuvel 2016-02-18 13:28 ` Matt Fleming 2016-02-18 13:29 ` Ard Biesheuvel 2016-02-18 13:43 ` Matt Fleming 2016-02-18 13:44 ` Ard Biesheuvel 2016-02-18 14:15 ` Matt Fleming 2016-02-18 14:21 ` Ard Biesheuvel 2016-02-18 14:38 ` Matt Fleming 2016-02-19 9:27 ` Dave Young 2016-02-18 19:16 ` Peter Jones 2016-02-26 14:41 ` Matt Fleming 2016-03-01 23:30 ` Matt Fleming 2016-03-01 23:31 ` Matt Fleming 2016-03-02 1:16 ` Dave Young 2016-03-02 10:23 ` Matt Fleming 2016-03-04 6:25 ` Dave Young 2016-05-18 8:36 ` Ard Biesheuvel 2016-05-18 8:53 ` Matt Fleming 2016-06-16 13:47 ` Ard Biesheuvel 2016-06-20 11:49 ` Matt Fleming 2016-02-15 11:32 ` [PATCH v2 2/2] arm64/efi: esrt: add missing call to efi_esrt_init() Ard Biesheuvel 2016-02-15 15:46 ` Peter Jones 2016-02-16 20:25 ` Baicar, Tyler 2016-02-17 8:32 ` Ard Biesheuvel 2016-05-17 20:36 ` Christopher Covington
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox