Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
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 > > > > > > > > > > > > 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
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: [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
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
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.
/
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 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.
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 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
Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds
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
