Discussion:
[RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
(too old to reply)
Leif Lindholm
2018-07-09 18:49:06 UTC
Permalink
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 */
--
2.11.0
Daniel Kiper
2018-07-11 11:03:12 UTC
Permalink
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
2018-07-11 11:53:01 UTC
Permalink
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
2018-07-12 11:44:36 UTC
Permalink
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
2018-07-12 11:52:49 UTC
Permalink
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
2018-07-13 11:27:08 UTC
Permalink
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
2018-07-13 12:53:52 UTC
Permalink
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
2018-07-13 13:59:38 UTC
Permalink
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
2018-07-13 14:46:57 UTC
Permalink
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
2018-07-25 12:33:01 UTC
Permalink
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

Continue reading on narkive:
Loading...