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

[Leif Lindholm]

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 <***@linaro.org>
---
grub-core/lib/i386/reboot.c | 4 ++++
1 file changed, 4 insertions(+)

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

[Daniel Kiper]

Post by Leif Lindholm
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

[Leif Lindholm]

Post by Daniel Kiper

Post by Leif Lindholm
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

[Daniel Kiper]

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
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 <***@oracle.com>

If there are no objections I will apply this in a week or so.

Daniel

[Leif Lindholm]

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
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
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

[Daniel Kiper]

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
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
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

[Leif Lindholm]

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
(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
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

[Daniel Kiper]

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
(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
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

[Leif Lindholm]

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
(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
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

[Daniel Kiper]

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm

Post by Daniel Kiper

Post by Leif Lindholm
(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
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.

Post by Leif Lindholm
Apologies for the noise.

Not a problem. Thanks for posting the patch.

Daniel