"arm_coreboot: Support DMA" breaks arm64

Message ID CAF7UmSw0GfWDiLEwf2f6JnWrJ-Qpx3Rp1OZJ-DNUJx1VrerX3w@mail.gmail.com
State New
Headers show

Commit Message

Leif Lindholm May 11, 2017, 3:13 p.m.
Commit 265292f ("arm_coreboot: Support DMA") breaks arm64 grub-mkimage with:
/work/local/bin/grub-mkimage: error: undefined symbol grub_arch_sync_dma_caches.

This appears to be caused purely by the false symbol dependency
created by the non-x86 version being an EXPORT_FUNC, in order to be
usable by modules.

A not very pretty but functional workaround is:

 #endif
 #endif

Thoughts?

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Vladimir 'phcoder' Serbinenko May 11, 2017, 6:33 p.m. | #1
On Thu, May 11, 2017, 17:35 Leif Lindholm <leif.lindholm@linaro.org> wrote:

> Commit 265292f ("arm_coreboot: Support DMA") breaks arm64 grub-mkimage

> with:

> /work/local/bin/grub-mkimage: error: undefined symbol

> grub_arch_sync_dma_caches.

>

> This appears to be caused purely by the false symbol dependency

> created by the non-x86 version being an EXPORT_FUNC, in order to be

> usable by modules.

>

> A not very pretty but functional workaround is:

>

> diff --git a/include/grub/cache.h b/include/grub/cache.h

> index 1c98ce2..cc4c833 100644

> --- a/include/grub/cache.h

> +++ b/include/grub/cache.h

> @@ -40,7 +40,7 @@ grub_arch_sync_dma_caches (volatile void *address

> __attribute__ ((unused)),

>                            grub_size_t len __attribute__ ((unused)))

>  {

>  }

> -#else

> +#elif !defined (__aarch64__)

>  void EXPORT_FUNC(grub_arch_sync_dma_caches) (volatile void *address,

> grub_size_t len);

>  #endif

>  #endif

>

> Thoughts?

>

I don't think that functions in cache.h are used outside of kernel on arm64
currently. We can just remove cache.h from the list of exported headers

>

> /

>     Leif

>

> _______________________________________________

> Grub-devel mailing list

> Grub-devel@gnu.org

> https://lists.gnu.org/mailman/listinfo/grub-devel

>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm May 24, 2017, 11:35 a.m. | #2
Apologies for delay, got knocked out by a bad cold.

On Thu, May 11, 2017 at 06:33:34PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> > Thoughts?

> >

> I don't think that functions in cache.h are used outside of kernel on arm64

> currently. We can just remove cache.h from the list of exported headers


That's true. So, would something like the below be acceptable?

/
    Leif


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-develdiff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 1045138..622f946 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -66,7 +66,9 @@ CLEANFILES += grub_script.yy.c grub_script.yy.h

 include $(srcdir)/Makefile.core.am

+if !COND_arm64
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/cache.h
+endif
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/device.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/disk.h

Leif Lindholm May 30, 2017, 3:28 p.m. | #3
On Wed, May 24, 2017 at 12:35:51PM +0100, Leif Lindholm wrote:
> Apologies for delay, got knocked out by a bad cold.

> 

> On Thu, May 11, 2017 at 06:33:34PM +0000, Vladimir 'phcoder' Serbinenko wrote:

> > > Thoughts?

> > >

> > I don't think that functions in cache.h are used outside of kernel on arm64

> > currently. We can just remove cache.h from the list of exported headers

> 

> That's true. So, would something like the below be acceptable?


Or would a new AM_CONDITIONAL be preferable?
Once the 32/64-bit ARM UEFI linux loaders are unified, the same change
should probably be made for the 32-bit port, so there is a case for
doing it that way.
Maybe COND_efi? Although that could require a workaround for ia64.

/
    Leif

> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am

> index 1045138..622f946 100644

> --- a/grub-core/Makefile.am

> +++ b/grub-core/Makefile.am

> @@ -66,7 +66,9 @@ CLEANFILES += grub_script.yy.c grub_script.yy.h

> 

>  include $(srcdir)/Makefile.core.am

> 

> +if !COND_arm64

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/cache.h

> +endif

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/device.h

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/disk.h


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Vladimir 'phcoder' Serbinenko May 30, 2017, 4:06 p.m. | #4
LGTM

On Wed, May 24, 2017, 13:36 Leif Lindholm <leif.lindholm@linaro.org> wrote:

> Apologies for delay, got knocked out by a bad cold.

>

> On Thu, May 11, 2017 at 06:33:34PM +0000, Vladimir 'phcoder' Serbinenko

> wrote:

> > > Thoughts?

> > >

> > I don't think that functions in cache.h are used outside of kernel on

> arm64

> > currently. We can just remove cache.h from the list of exported headers

>

> That's true. So, would something like the below be acceptable?

>

> /

>     Leif

>

> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am

> index 1045138..622f946 100644

> --- a/grub-core/Makefile.am

> +++ b/grub-core/Makefile.am

> @@ -66,7 +66,9 @@ CLEANFILES += grub_script.yy.c grub_script.yy.h

>

>  include $(srcdir)/Makefile.core.am

>

> +if !COND_arm64

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/cache.h

> +endif

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/device.h

>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/disk.h

>

> _______________________________________________

> Grub-devel mailing list

> Grub-devel@gnu.org

> https://lists.gnu.org/mailman/listinfo/grub-devel

>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm June 5, 2017, 10:15 a.m. | #5
On Tue, May 30, 2017 at 04:06:52PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> LGTM


Thanks - so am I OK to push the following?:

From bb9aa73e172ee284d9b41a0a4657154c355d6f63 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>

Date: Mon, 5 Jun 2017 10:47:35 +0100
Subject: [PATCH] arm64: don't export cache.h

arm64 does not implement grub_arch_sync_dma_caches(), which causes an
error when running grub-mkimage due to the missing exported symbol.
Make the exporting of cache.h conditional on !COND_arm64.
---
 grub-core/Makefile.am | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.1.4


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-develdiff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 1045138..622f946 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -66,7 +66,9 @@ CLEANFILES += grub_script.yy.c grub_script.yy.h
 
 include $(srcdir)/Makefile.core.am
 
+if !COND_arm64
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/cache.h
+endif
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/device.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/disk.h

Patch

diff --git a/include/grub/cache.h b/include/grub/cache.h
index 1c98ce2..cc4c833 100644
--- a/include/grub/cache.h
+++ b/include/grub/cache.h
@@ -40,7 +40,7 @@  grub_arch_sync_dma_caches (volatile void *address
__attribute__ ((unused)),
                           grub_size_t len __attribute__ ((unused)))
 {
 }
-#else
+#elif !defined (__aarch64__)
 void EXPORT_FUNC(grub_arch_sync_dma_caches) (volatile void *address,
grub_size_t len);