| Message ID | 20180709184906.27831-1-leif.lindholm@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] i386: don't include lib/i386/reset.c in EFI builds | expand |
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; Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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.) / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <daniel.kiper@oracle.com> If there are no objections I will apply this in a week or so. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <daniel.kiper@oracle.com> > > 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. / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <daniel.kiper@oracle.com> > > > > 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
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 <daniel.kiper@oracle.com> > > > > > > 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
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 <daniel.kiper@oracle.com> > > > > > > > > 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
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 <daniel.kiper@oracle.com> > > > > > > > > > > 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
On Fri, Jul 13, 2018 at 03:46:57PM +0100, Leif Lindholm wrote: > 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 <daniel.kiper@oracle.com> > > > > > > > > > > > > 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. So, I have pushed original version. > Apologies for the noise. Not a problem. Thanks for posting the patch. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
diff --git a/grub-core/lib/i386/reboot.c b/grub-core/lib/i386/reboot.c index a234244dc..9dd00d421 100644 --- a/grub-core/lib/i386/reboot.c +++ b/grub-core/lib/i386/reboot.c @@ -16,6 +16,8 @@ * along with GRUB. If not, see <http://www.gnu.org/licenses/>. */ +#ifndef GRUB_MACHINE_EFI + #include <grub/relocator.h> #include <grub/cpu/relocator.h> #include <grub/misc.h> @@ -58,3 +60,5 @@ grub_reboot (void) while (1); } + +#endif /* ndef GRUB_MACHINE_EFI */
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. Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> --- grub-core/lib/i386/reboot.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.11.0 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel