[RFC] i386: don't include lib/i386/reset.c in EFI builds

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

Commit Message

Leif Lindholm July 9, 2018, 6:49 p.m.
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

Comments

Daniel Kiper July 11, 2018, 11:03 a.m. | #1
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
Leif Lindholm July 11, 2018, 11:53 a.m. | #2
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
Daniel Kiper July 12, 2018, 11:44 a.m. | #3
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
Leif Lindholm July 12, 2018, 11:52 a.m. | #4
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
Daniel Kiper July 13, 2018, 11:27 a.m. | #5
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
Leif Lindholm July 13, 2018, 12:53 p.m. | #6
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
Daniel Kiper July 13, 2018, 1:59 p.m. | #7
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
Leif Lindholm July 13, 2018, 2:46 p.m. | #8
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

Patch

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