[PATCH 2/3] loader/ia64/linux: use central copy of grub_efi_find_mmap_size
Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.
Signed-off-by: Leif Lindholm
---
grub-core/loader/ia64/efi/linux.c | 46 ++-
1 file changed, 2 insertions(+), 44 deletions(-)
diff --git a/grub-core/loader/ia64/efi/linux.c
b/grub-core/loader/ia64/efi/linux.c
index 750330d45..96fce713a 100644
--- a/grub-core/loader/ia64/efi/linux.c
+++ b/grub-core/loader/ia64/efi/linux.c
@@ -133,48 +133,6 @@ query_fpswa (void)
}
}
-/* Find the optimal number of pages for the memory map. Is it better to
- move this code to efi/mm.c? */
-static grub_efi_uintn_t
-find_mmap_size (void)
-{
- static grub_efi_uintn_t mmap_size = 0;
-
- if (mmap_size != 0)
-return mmap_size;
-
- mmap_size = (1 << 12);
- while (1)
-{
- int ret;
- grub_efi_memory_descriptor_t *mmap;
- grub_efi_uintn_t desc_size;
-
- mmap = grub_malloc (mmap_size);
- if (! mmap)
- return 0;
-
- ret = grub_efi_get_memory_map (_size, mmap, 0, _size, 0);
- grub_free (mmap);
-
- if (ret < 0)
- {
- grub_error (GRUB_ERR_IO, "cannot get memory map");
- return 0;
- }
- else if (ret > 0)
- break;
-
- mmap_size += (1 << 12);
-}
-
- /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more. */
- mmap_size += (1 << 12);
-
- return page_align (mmap_size);
-}
-
static void
free_pages (void)
{
@@ -212,7 +170,7 @@ allocate_pages (grub_uint64_t align, grub_uint64_t
size_pages,
size = size_pages << 12;
- mmap_size = find_mmap_size ();
+ mmap_size = grub_efi_find_mmap_size ();
if (!mmap_size)
return 0;
@@ -323,7 +281,7 @@ grub_linux_boot (void)
/* MDT.
Must be done after grub_machine_fini because map_key is used by
exit_boot_services. */
- mmap_size = find_mmap_size ();
+ mmap_size = grub_efi_find_mmap_size ();
if (! mmap_size)
return grub_errno;
mmap_buf = grub_efi_allocate_any_pages (page_align (mmap_size) >> 12);
--
2.11.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/3] loader/multiboot_mbi2: use central copy of grub_efi_find_mmap_size
Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.
Signed-off-by: Leif Lindholm
---
grub-core/loader/multiboot_mbi2.c | 38 +-
1 file changed, 1 insertion(+), 37 deletions(-)
diff --git a/grub-core/loader/multiboot_mbi2.c
b/grub-core/loader/multiboot_mbi2.c
index 4df659595..747e10850 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -407,42 +407,6 @@ acpiv2_size (void)
static grub_efi_uintn_t efi_mmap_size = 0;
-/* Find the optimal number of pages for the memory map. Is it better to
- move this code to efi/mm.c? */
-static void
-find_efi_mmap_size (void)
-{
- efi_mmap_size = (1 << 12);
- while (1)
-{
- int ret;
- grub_efi_memory_descriptor_t *mmap;
- grub_efi_uintn_t desc_size;
- grub_efi_uintn_t cur_mmap_size = efi_mmap_size;
-
- mmap = grub_malloc (cur_mmap_size);
- if (! mmap)
- return;
-
- ret = grub_efi_get_memory_map (_mmap_size, mmap, 0, _size, 0);
- grub_free (mmap);
-
- if (ret < 0)
- return;
- else if (ret > 0)
- break;
-
- if (efi_mmap_size < cur_mmap_size)
- efi_mmap_size = cur_mmap_size;
- efi_mmap_size += (1 << 12);
-}
-
- /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more. */
- efi_mmap_size += (3 << 12);
-
- efi_mmap_size = ALIGN_UP (efi_mmap_size, 4096);
-}
#endif
static grub_size_t
@@ -463,7 +427,7 @@ grub_multiboot2_get_mbi_size (void)
{
#ifdef GRUB_MACHINE_EFI
if (!keep_bs && !efi_mmap_size)
-find_efi_mmap_size ();
+efi_mmap_size = grub_efi_find_mmap_size ();
#endif
return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
+ sizeof (struct multiboot_tag)
--
2.11.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/3] use central copy of grub_efi_find_mmap_size
There were multiple local implementations of functions to determine the size of buffer required to hold a UEFI memory map. Drop these and switch to grub_efi_find_mmap_size() in kern/efi/mm.c. I'm not going to lie. I no longer have an ia64 cross-toolchain, so that one is not even compile tested. Leif Lindholm (3): loader/i386/linux: use central copy of grub_efi_find_mmap_size loader/ia64/linux: use central copy of grub_efi_find_mmap_size loader/multiboot_mbi2: use central copy of grub_efi_find_mmap_size grub-core/loader/i386/linux.c | 51 +-- grub-core/loader/ia64/efi/linux.c | 46 ++- grub-core/loader/multiboot_mbi2.c | 38 + 3 files changed, 4 insertions(+), 131 deletions(-) -- 2.11.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/3] loader/i386/linux: use central copy of grub_efi_find_mmap_size
Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.
Signed-off-by: Leif Lindholm
---
grub-core/loader/i386/linux.c | 51 +--
1 file changed, 1 insertion(+), 50 deletions(-)
diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 44301e126..ab02add53 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -101,55 +101,6 @@ page_align (grub_size_t size)
return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
}
-#ifdef GRUB_MACHINE_EFI
-/* Find the optimal number of pages for the memory map. Is it better to
- move this code to efi/mm.c? */
-static grub_efi_uintn_t
-find_efi_mmap_size (void)
-{
- static grub_efi_uintn_t mmap_size = 0;
-
- if (mmap_size != 0)
-return mmap_size;
-
- mmap_size = (1 << 12);
- while (1)
-{
- int ret;
- grub_efi_memory_descriptor_t *mmap;
- grub_efi_uintn_t desc_size;
- grub_efi_uintn_t cur_mmap_size = mmap_size;
-
- mmap = grub_malloc (cur_mmap_size);
- if (! mmap)
- return 0;
-
- ret = grub_efi_get_memory_map (_mmap_size, mmap, 0, _size, 0);
- grub_free (mmap);
-
- if (ret < 0)
- {
- grub_error (GRUB_ERR_IO, "cannot get memory map");
- return 0;
- }
- else if (ret > 0)
- break;
-
- if (mmap_size < cur_mmap_size)
- mmap_size = cur_mmap_size;
- mmap_size += (1 << 12);
-}
-
- /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more. */
- mmap_size += (3 << 12);
-
- mmap_size = page_align (mmap_size);
- return mmap_size;
-}
-
-#endif
-
/* Helper for find_mmap_size. */
static int
count_hook (grub_uint64_t addr __attribute__ ((unused)),
@@ -560,7 +511,7 @@ grub_linux_boot (void)
ctx.real_size = ALIGN_UP (cl_offset + maximal_cmdline_size, 4096);
#ifdef GRUB_MACHINE_EFI
- efi_mmap_size = find_efi_mmap_size ();
+ efi_mmap_size = grub_efi_find_mmap_size ();
if (efi_mmap_size == 0)
return grub_errno;
#endif
--
2.11.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
On Fri, Jul 13, 2018 at 03:59:38PM +0200, Daniel Kiper wrote: > On Fri, Jul 13, 2018 at 01:53:52PM +0100, Leif Lindholm wrote: > > On Fri, Jul 13, 2018 at 01:27:08PM +0200, Daniel Kiper wrote: > > > > > > (i386_)ieee1275 implements its own grub_reboot(), so that should be > > > > > > fine. (This does mean that i386_ieee1275 may currently be unable to > > > > > > load the reboot module on master.) > > > > > > > > > > Hmmm... So, it looks that your solution is safer. Then > > > > > > > > > > Reviewed-by: Daniel Kiper > > > > > > > > > > If there are no objections I will apply this in a week or so. > > > > > > > > In that case, I think it may be worth extending the test to > > > > > > > > #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275) > > > > > > > > I had not noticed that bit when I sent the original patch. > > > > > > > > But this is theorising based on looking at source code without > > > > testing. > > > > > > Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC > > > only. > > > So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" > > > here. > > > > Oh, right. > > > > Then I think we still have a problem with I386_IEEE1275, but am less > > sure how to deal with it. > > I have just build the i386-ieee1275 platform. It looks that the platform > uses standard i386 reboot mechanism. So, I would put #ifndef GRUB_MACHINE_EFI > like it was in original patch. Yes, you are correct. This is handled (as you said) by i386_ieee1275 not including lib/ieee1275/reboot.c. And indeed, since these are both in the reboot module (which I had somehow managed to miss), it would have triggered a build-time fault if it had been an issue. Apologies for the noise. / Leif ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
On Fri, Jul 13, 2018 at 01:53:52PM +0100, Leif Lindholm wrote: > On Fri, Jul 13, 2018 at 01:27:08PM +0200, Daniel Kiper wrote: > > > > > (i386_)ieee1275 implements its own grub_reboot(), so that should be > > > > > fine. (This does mean that i386_ieee1275 may currently be unable to > > > > > load the reboot module on master.) > > > > > > > > Hmmm... So, it looks that your solution is safer. Then > > > > > > > > Reviewed-by: Daniel Kiper > > > > > > > > If there are no objections I will apply this in a week or so. > > > > > > In that case, I think it may be worth extending the test to > > > > > > #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275) > > > > > > I had not noticed that bit when I sent the original patch. > > > > > > But this is theorising based on looking at source code without > > > testing. > > > > Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC > > only. > > So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" > > here. > > Oh, right. > > Then I think we still have a problem with I386_IEEE1275, but am less > sure how to deal with it. I have just build the i386-ieee1275 platform. It looks that the platform uses standard i386 reboot mechanism. So, I would put #ifndef GRUB_MACHINE_EFI like it was in original patch. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
On Fri, Jul 13, 2018 at 01:27:08PM +0200, Daniel Kiper wrote: > > > > (i386_)ieee1275 implements its own grub_reboot(), so that should be > > > > fine. (This does mean that i386_ieee1275 may currently be unable to > > > > load the reboot module on master.) > > > > > > Hmmm... So, it looks that your solution is safer. Then > > > > > > Reviewed-by: Daniel Kiper > > > > > > If there are no objections I will apply this in a week or so. > > > > In that case, I think it may be worth extending the test to > > > > #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275) > > > > I had not noticed that bit when I sent the original patch. > > > > But this is theorising based on looking at source code without > > testing. > > Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC > only. > So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" > here. Oh, right. Then I think we still have a problem with I386_IEEE1275, but am less sure how to deal with it. / Leif ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
RE: [PATCH v3] i386/linux: add support for ext_lfb_base
> -Original Message- > From: Daniel Kiper > Sent: Friday, July 13, 2018 5:20 PM > To: Nath, Arindam > Cc: grub-devel@gnu.org; Daniel Kiper ; Arindam Nath > > Subject: Re: [PATCH v3] i386/linux: add support for ext_lfb_base > > On Thu, Jul 12, 2018 at 07:02:49PM +0530, Arindam Nath wrote: > > From: Arindam Nath > > > > Signed-off-by: Arindam Nath > > --- > > Cc: Daniel Kiper > > --- > > v1: > > > > The EFI Graphics Output Protocol can return a 64-bit > > linear frame buffer address in some firmware/BIOS > > implementations. We currently only store the lower > > 32-bits in the lfb_base. This will eventually be > > passed to Linux kernel and the efifb driver will > > incorrectly interpret the framebuffer address as > > 32-bit address. > > > > The Linux kernel has already added support to handle > > 64-bit linear framebuffer address in the efifb driver > > since quite some time now. > > > > This patch adds the support for 64-bit linear frame > > buffer address in GRUB to address the above mentioned > > scenario. > > > > v2: changes suggested by Daniel > > > > - added #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__) > > - moved constant definitions to the beginning of header file > > > > v3: changes suggested by Daniel > > > > - moved patch version info below SOB > > - added empty lines above and below the modified lines > > - removed unnecessary #if and #endif from header file > > Code looks good but the commit message is a mess. The commit message > should look like this in your editor. > > > i386/linux: add support for ext_lfb_base > > The EFI Graphics Output Protocol can return a 64-bit > linear frame buffer address in some firmware/BIOS > implementations. We currently only store the lower > 32-bits in the lfb_base. This will eventually be > passed to Linux kernel and the efifb driver will > incorrectly interpret the framebuffer address as > 32-bit address. > > The Linux kernel has already added support to handle > 64-bit linear framebuffer address in the efifb driver > since quite some time now. > > This patch adds the support for 64-bit linear frame > buffer address in GRUB to address the above mentioned > scenario. > > Signed-off-by: Arindam Nath > --- > Cc: Daniel Kiper > > v2: changes suggested by Daniel > - added #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__) > - moved constant definitions to the beginning of header file > > v3: changes suggested by Daniel > - moved patch version info below SOB > - added empty lines above and below the modified lines > - removed unnecessary #if and #endif from header file > > > I will fix that before committing the patch. I will do > that in a week or so if there are no objections. Thanks. > > Next time please adhere to the commit message layout presented above. Will do. Arindam > > Thank you for doing the work. > > Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] i386/linux: add support for ext_lfb_base
On Thu, Jul 12, 2018 at 07:02:49PM +0530, Arindam Nath wrote: > From: Arindam Nath > > Signed-off-by: Arindam Nath > --- > Cc: Daniel Kiper > --- > v1: > > The EFI Graphics Output Protocol can return a 64-bit > linear frame buffer address in some firmware/BIOS > implementations. We currently only store the lower > 32-bits in the lfb_base. This will eventually be > passed to Linux kernel and the efifb driver will > incorrectly interpret the framebuffer address as > 32-bit address. > > The Linux kernel has already added support to handle > 64-bit linear framebuffer address in the efifb driver > since quite some time now. > > This patch adds the support for 64-bit linear frame > buffer address in GRUB to address the above mentioned > scenario. > > v2: changes suggested by Daniel > > - added #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__) > - moved constant definitions to the beginning of header file > > v3: changes suggested by Daniel > > - moved patch version info below SOB > - added empty lines above and below the modified lines > - removed unnecessary #if and #endif from header file Code looks good but the commit message is a mess. The commit message should look like this in your editor. i386/linux: add support for ext_lfb_base The EFI Graphics Output Protocol can return a 64-bit linear frame buffer address in some firmware/BIOS implementations. We currently only store the lower 32-bits in the lfb_base. This will eventually be passed to Linux kernel and the efifb driver will incorrectly interpret the framebuffer address as 32-bit address. The Linux kernel has already added support to handle 64-bit linear framebuffer address in the efifb driver since quite some time now. This patch adds the support for 64-bit linear frame buffer address in GRUB to address the above mentioned scenario. Signed-off-by: Arindam Nath --- Cc: Daniel Kiper v2: changes suggested by Daniel - added #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__) - moved constant definitions to the beginning of header file v3: changes suggested by Daniel - moved patch version info below SOB - added empty lines above and below the modified lines - removed unnecessary #if and #endif from header file I will fix that before committing the patch. I will do that in a week or so if there are no objections. Next time please adhere to the commit message layout presented above. Thank you for doing the work. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
On Thu, Jul 12, 2018 at 12:52:49PM +0100, Leif Lindholm wrote:
> On Thu, Jul 12, 2018 at 01:44:36PM +0200, Daniel Kiper wrote:
> > On Wed, Jul 11, 2018 at 12:53:01PM +0100, Leif Lindholm wrote:
> > > On Wed, Jul 11, 2018 at 01:03:12PM +0200, Daniel Kiper wrote:
> > > > On Mon, Jul 09, 2018 at 07:49:06PM +0100, Leif Lindholm wrote:
> > > > > Commit 0ba90a7f0178 ("efi: Move grub_reboot() into kernel") broke
> > > > > the build on i386-efi - genmoddep.awk bails out with message
> > > > > grub_reboot in reboot is duplicated in kernel
> > > > > This is because both lib/i386/reset.c and kern/efi/efi.c now provide
> > > > > this function.
> > > > >
> > > > > Rather than explicitly list each i386 platform variant in
> > > > > Makefile.core.def, include the contents of lib/i386/reset.c only when
> > > > > GRUB_MACHINE_EFI is not set.
> > > >
> > > > Could you try the patch below? It seems better to me.
> > > >
> > > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > > index fc4767f..820ddc3 100644
> > > > --- a/grub-core/Makefile.core.def
> > > > +++ b/grub-core/Makefile.core.def
> > > > @@ -870,8 +870,8 @@ module = {
> > > >
> > > > module = {
> > > >name = reboot;
> > > > - i386 = lib/i386/reboot.c;
> > > > - i386 = lib/i386/reboot_trampoline.S;
> > > > + i386_pc = lib/i386/reboot.c;
> > > > + i386_pc = lib/i386/reboot_trampoline.S;
> > > >powerpc_ieee1275 = lib/ieee1275/reboot.c;
> > > >sparc64_ieee1275 = lib/ieee1275/reboot.c;
> > > >mips_arc = lib/mips/arc/reboot.c;
> > >
> > > I agree this looks a lot nicer, but I don't know enough to say whether
> > > that's valid for i386_coreboot, i386_multiboot and i386_qemu, which
> > > don't seem to have any other grub_reset() implementations.
> > > I also don't know how to test those beyond just compiling.
> > >
> > > (i386_)ieee1275 implements its own grub_reboot(), so that should be
> > > fine. (This does mean that i386_ieee1275 may currently be unable to
> > > load the reboot module on master.)
> >
> > Hmmm... So, it looks that your solution is safer. Then
> >
> > Reviewed-by: Daniel Kiper
> >
> > If there are no objections I will apply this in a week or so.
>
> In that case, I think it may be worth extending the test to
>
> #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275)
>
> I had not noticed that bit when I sent the original patch.
>
> But this is theorising based on looking at source code without
> testing.
Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC only.
So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" here.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
