[PATCH 1/2] arm64: Move firmware fdt search into global function

[PATCH 1/2] arm64: Move firmware fdt search into global function

[Alexander Graf]

Searching for a device tree that EFI passes to us via configuration tables
is nothing architecture specific. Move it into generic code.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
 grub-core/loader/arm64/fdt.c | 24 +-----------------------
 include/grub/efi/efi.h       |  2 ++
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index e9c85de..fb90ecd 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char **path)
     }
 }
 
+void *
+grub_efi_get_firmware_fdt (void)
+{
+  grub_efi_configuration_table_t *tables;
+  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
+  void *firmware_fdt = NULL;
+  unsigned int i;
+
+  /* Look for FDT in UEFI config tables. */
+  tables = grub_efi_system_table->configuration_table;
+
+  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
+    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
+      {
+	firmware_fdt = tables[i].vendor_table;
+	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
+	break;
+      }
+
+  return firmware_fdt;
+}
+
 void
 grub_efi_fini (void)
 {
diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/arm64/fdt.c
index 5202c14..db49cf6 100644
--- a/grub-core/loader/arm64/fdt.c
+++ b/grub-core/loader/arm64/fdt.c
@@ -28,28 +28,6 @@
 static void *loaded_fdt;
 static void *fdt;
 
-static void *
-get_firmware_fdt (void)
-{
-  grub_efi_configuration_table_t *tables;
-  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
-  void *firmware_fdt = NULL;
-  unsigned int i;
-
-  /* Look for FDT in UEFI config tables. */
-  tables = grub_efi_system_table->configuration_table;
-
-  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
-    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
-      {
-	firmware_fdt = tables[i].vendor_table;
-	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
-	break;
-      }
-
-  return firmware_fdt;
-}
-
 void *
 grub_fdt_load (grub_size_t additional_size)
 {
@@ -65,7 +43,7 @@ grub_fdt_load (grub_size_t additional_size)
   if (loaded_fdt)
     raw_fdt = loaded_fdt;
   else
-    raw_fdt = get_firmware_fdt();
+    raw_fdt = grub_efi_get_firmware_fdt();
 
   size =
     raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 0e6fd86..2acf85e 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -81,6 +81,8 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 						char **device,
 						char **path);
 
+void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
+
 grub_addr_t grub_efi_modules_addr (void);
 
 void grub_efi_mm_init (void);
-- 
1.8.5.6

[PATCH 2/2] arm efi: Use fdt from firmware when available

[Alexander Graf]

If EFI is nice enough to pass us an FDT using configuration tables on 32bit
ARM, we should really try and make use of it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/grub/arm/linux.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index 059dbba..a66caad 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -40,11 +40,7 @@
 # define LINUX_PHYS_OFFSET        (0x00008000)
 # define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
 # define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
-static inline grub_addr_t
-grub_arm_firmware_get_boot_data (void)
-{
-  return 0;
-}
+# define grub_arm_firmware_get_boot_data (grub_addr_t)grub_efi_get_firmware_fdt
 static inline grub_uint32_t
 grub_arm_firmware_get_machine_type (void)
 {
-- 
1.8.5.6

Re: [PATCH 2/2] arm efi: Use fdt from firmware when available

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

Is it something relevant for 2.02 or not?

On Monday, February 29, 2016, Alexander Graf <agraf@suse.de> wrote:

> If EFI is nice enough to pass us an FDT using configuration tables on 32bit
> ARM, we should really try and make use of it.
>
> Signed-off-by: Alexander Graf <agraf@suse.de <javascript:;>>
> ---
>  include/grub/arm/linux.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 059dbba..a66caad 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -40,11 +40,7 @@
>  # define LINUX_PHYS_OFFSET        (0x00008000)
>  # define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
>  # define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
> -static inline grub_addr_t
> -grub_arm_firmware_get_boot_data (void)
> -{
> -  return 0;
> -}
> +# define grub_arm_firmware_get_boot_data
> (grub_addr_t)grub_efi_get_firmware_fdt
>  static inline grub_uint32_t
>  grub_arm_firmware_get_machine_type (void)
>  {
> --
> 1.8.5.6
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org <javascript:;>
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 1815 bytes --]

Re: [PATCH 2/2] arm efi: Use fdt from firmware when available

[Leif Lindholm]

Apologies for delay, was busy at Linaro Connect during the week, and
have been sleeping since I got back home yesterday.

This patch could with some stretch of the imagination be considered a
bugfix for the arm-efi port.

I _should_ hack together an update to arm-efi to use the arm64 Linux
loader (booting via the kernel EFI Stub) on kernels supporting it, but
since this support only went into v4.5-rc1, there is a use-case for
supporting both ways on 32-bit.

That's basically a roundabout way of saying "I wish we could just drop
the separate arm-efi Linux loader, but I don't think we can, and this
improves it to make it work more like the stub loader does".

Which is basically a roundabout way of saying "yes".

/
    Leif

On Thu, Mar 10, 2016 at 09:00:32PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Is it something relevant for 2.02 or not?
> 
> On Monday, February 29, 2016, Alexander Graf <agraf@suse.de> wrote:
> 
> > If EFI is nice enough to pass us an FDT using configuration tables on 32bit
> > ARM, we should really try and make use of it.
> >
> > Signed-off-by: Alexander Graf <agraf@suse.de <javascript:;>>
> > ---
> >  include/grub/arm/linux.h | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > index 059dbba..a66caad 100644
> > --- a/include/grub/arm/linux.h
> > +++ b/include/grub/arm/linux.h
> > @@ -40,11 +40,7 @@
> >  # define LINUX_PHYS_OFFSET        (0x00008000)
> >  # define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
> >  # define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
> > -static inline grub_addr_t
> > -grub_arm_firmware_get_boot_data (void)
> > -{
> > -  return 0;
> > -}
> > +# define grub_arm_firmware_get_boot_data
> > (grub_addr_t)grub_efi_get_firmware_fdt
> >  static inline grub_uint32_t
> >  grub_arm_firmware_get_machine_type (void)
> >  {
> > --
> > 1.8.5.6
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org <javascript:;>
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> 
> 
> -- 
> Regards
> Vladimir 'phcoder' Serbinenko

Re: [PATCH 2/2] arm efi: Use fdt from firmware when available

[Daniel Kiper]

On Mon, Feb 29, 2016 at 12:22:24AM +0100, Alexander Graf wrote:
> If EFI is nice enough to pass us an FDT using configuration tables on 32bit
> ARM, we should really try and make use of it.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Both patches are applied. Sorry for delay.

Daniel

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Andrei Borzenkov]

29.02.2016 02:22, Alexander Graf пишет:
> Searching for a device tree that EFI passes to us via configuration tables
> is nothing architecture specific. Move it into generic code.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
>  grub-core/loader/arm64/fdt.c | 24 +-----------------------
>  include/grub/efi/efi.h       |  2 ++
>  3 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index e9c85de..fb90ecd 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char **path)
>      }
>  }
>  
> +void *
> +grub_efi_get_firmware_fdt (void)
> +{
> +  grub_efi_configuration_table_t *tables;
> +  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> +  void *firmware_fdt = NULL;
> +  unsigned int i;
> +
> +  /* Look for FDT in UEFI config tables. */
> +  tables = grub_efi_system_table->configuration_table;
> +
> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
> +      {
> +	firmware_fdt = tables[i].vendor_table;
> +	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
> +	break;
> +      }
> +
> +  return firmware_fdt;
> +}
> +

Is it relevant for x86? I cannot even find anything related to FDT or
"device tree" in UEFI spec, it falls under vendor extensions. What other
use it has except in Linux loader?

I really fail to see why it should be moved into core until at least one
more non-trivial caller is present.

>  void
>  grub_efi_fini (void)
>  {
> diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/arm64/fdt.c
> index 5202c14..db49cf6 100644
> --- a/grub-core/loader/arm64/fdt.c
> +++ b/grub-core/loader/arm64/fdt.c
> @@ -28,28 +28,6 @@
>  static void *loaded_fdt;
>  static void *fdt;
>  
> -static void *
> -get_firmware_fdt (void)
> -{
> -  grub_efi_configuration_table_t *tables;
> -  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> -  void *firmware_fdt = NULL;
> -  unsigned int i;
> -
> -  /* Look for FDT in UEFI config tables. */
> -  tables = grub_efi_system_table->configuration_table;
> -
> -  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> -    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
> -      {
> -	firmware_fdt = tables[i].vendor_table;
> -	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
> -	break;
> -      }
> -
> -  return firmware_fdt;
> -}
> -
>  void *
>  grub_fdt_load (grub_size_t additional_size)
>  {
> @@ -65,7 +43,7 @@ grub_fdt_load (grub_size_t additional_size)
>    if (loaded_fdt)
>      raw_fdt = loaded_fdt;
>    else
> -    raw_fdt = get_firmware_fdt();
> +    raw_fdt = grub_efi_get_firmware_fdt();
>  
>    size =
>      raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 0e6fd86..2acf85e 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -81,6 +81,8 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
>  						char **device,
>  						char **path);
>  
> +void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
> +
>  grub_addr_t grub_efi_modules_addr (void);
>  
>  void grub_efi_mm_init (void);
> 

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Daniel Kiper]

On Sat, Nov 12, 2016 at 12:03:33PM +0300, Andrei Borzenkov wrote:
> 29.02.2016 02:22, Alexander Graf ??????????:
> > Searching for a device tree that EFI passes to us via configuration tables
> > is nothing architecture specific. Move it into generic code.
> >
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> > ---
> >  grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
> >  grub-core/loader/arm64/fdt.c | 24 +-----------------------
> >  include/grub/efi/efi.h       |  2 ++
> >  3 files changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> > index e9c85de..fb90ecd 100644
> > --- a/grub-core/kern/efi/init.c
> > +++ b/grub-core/kern/efi/init.c
> > @@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char **path)
> >      }
> >  }
> >
> > +void *
> > +grub_efi_get_firmware_fdt (void)
> > +{
> > +  grub_efi_configuration_table_t *tables;
> > +  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> > +  void *firmware_fdt = NULL;
> > +  unsigned int i;
> > +
> > +  /* Look for FDT in UEFI config tables. */
> > +  tables = grub_efi_system_table->configuration_table;
> > +
> > +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > +    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
> > +      {
> > +	firmware_fdt = tables[i].vendor_table;
> > +	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
> > +	break;
> > +      }
> > +
> > +  return firmware_fdt;
> > +}
> > +
>
> Is it relevant for x86? I cannot even find anything related to FDT or
> "device tree" in UEFI spec, it falls under vendor extensions. What other
> use it has except in Linux loader?
>
> I really fail to see why it should be moved into core until at least one
> more non-trivial caller is present.

Hmmm... It looks that I was too fast. TBH, I can agree to some extend.
Should we revert this patch or just #ifdef relevant code? Revert seems
better for me.

Daniel

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Alexander Graf]

On 15/11/2016 22:00, Daniel Kiper wrote:
> On Sat, Nov 12, 2016 at 12:03:33PM +0300, Andrei Borzenkov wrote:
>> 29.02.2016 02:22, Alexander Graf ??????????:
>>> Searching for a device tree that EFI passes to us via configuration tables
>>> is nothing architecture specific. Move it into generic code.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
>>>  grub-core/loader/arm64/fdt.c | 24 +-----------------------
>>>  include/grub/efi/efi.h       |  2 ++
>>>  3 files changed, 25 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
>>> index e9c85de..fb90ecd 100644
>>> --- a/grub-core/kern/efi/init.c
>>> +++ b/grub-core/kern/efi/init.c
>>> @@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char **path)
>>>      }
>>>  }
>>>
>>> +void *
>>> +grub_efi_get_firmware_fdt (void)
>>> +{
>>> +  grub_efi_configuration_table_t *tables;
>>> +  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
>>> +  void *firmware_fdt = NULL;
>>> +  unsigned int i;
>>> +
>>> +  /* Look for FDT in UEFI config tables. */
>>> +  tables = grub_efi_system_table->configuration_table;
>>> +
>>> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
>>> +    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
>>> +      {
>>> +	firmware_fdt = tables[i].vendor_table;
>>> +	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
>>> +	break;
>>> +      }
>>> +
>>> +  return firmware_fdt;
>>> +}
>>> +
>>
>> Is it relevant for x86? I cannot even find anything related to FDT or
>> "device tree" in UEFI spec, it falls under vendor extensions. What other
>> use it has except in Linux loader?
>>
>> I really fail to see why it should be moved into core until at least one
>> more non-trivial caller is present.
>
> Hmmm... It looks that I was too fast. TBH, I can agree to some extend.
> Should we revert this patch or just #ifdef relevant code? Revert seems
> better for me.

Uh, the point of the function was to share code between 32bit and 64bit 
arm platforms which are completely separate archs.

There's also effort underway to run with device tree on x86:

   https://plus.google.com/+ChristopherFriedt/posts/SfDifuC1xsR


Alex

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Daniel Kiper]

On Tue, Nov 15, 2016 at 10:07:39PM +0100, Alexander Graf wrote:
>
>
> On 15/11/2016 22:00, Daniel Kiper wrote:
> >On Sat, Nov 12, 2016 at 12:03:33PM +0300, Andrei Borzenkov wrote:
> >>29.02.2016 02:22, Alexander Graf ??????????:
> >>>Searching for a device tree that EFI passes to us via configuration
> >>>tables
> >>>is nothing architecture specific. Move it into generic code.
> >>>
> >>>Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>---
> >>> grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
> >>> grub-core/loader/arm64/fdt.c | 24 +-----------------------
> >>> include/grub/efi/efi.h       |  2 ++
> >>> 3 files changed, 25 insertions(+), 23 deletions(-)
> >>>
> >>>diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> >>>index e9c85de..fb90ecd 100644
> >>>--- a/grub-core/kern/efi/init.c
> >>>+++ b/grub-core/kern/efi/init.c
> >>>@@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char
> >>>**path)
> >>>     }
> >>> }
> >>>
> >>>+void *
> >>>+grub_efi_get_firmware_fdt (void)
> >>>+{
> >>>+  grub_efi_configuration_table_t *tables;
> >>>+  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> >>>+  void *firmware_fdt = NULL;
> >>>+  unsigned int i;
> >>>+
> >>>+  /* Look for FDT in UEFI config tables. */
> >>>+  tables = grub_efi_system_table->configuration_table;
> >>>+
> >>>+  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> >>>+    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof
> >>>(fdt_guid)) == 0)
> >>>+      {
> >>>+	firmware_fdt = tables[i].vendor_table;
> >>>+	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
> >>>+	break;
> >>>+      }
> >>>+
> >>>+  return firmware_fdt;
> >>>+}
> >>>+
> >>
> >>Is it relevant for x86? I cannot even find anything related to FDT or
> >>"device tree" in UEFI spec, it falls under vendor extensions. What other
> >>use it has except in Linux loader?
> >>
> >>I really fail to see why it should be moved into core until at least one
> >>more non-trivial caller is present.
> >
> >Hmmm... It looks that I was too fast. TBH, I can agree to some extend.
> >Should we revert this patch or just #ifdef relevant code? Revert seems
> >better for me.
>
> Uh, the point of the function was to share code between 32bit and 64bit
> arm platforms which are completely separate archs.
>
> There's also effort underway to run with device tree on x86:
>
>   https://plus.google.com/+ChristopherFriedt/posts/SfDifuC1xsR

OK but right know we build FDT code for every EFI platform. This
is not nice because not every EFI platform uses/knows FDT (at least
today). So, I think that we should revert that patch and then you
can provide another patch which puts FDT code in separate file, e.g.
grub-core/kern/efi/fdt.c build only on platforms supporting FDT.
Does it make sense?

Daniel

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Alexander Graf]

On 16/11/2016 11:28, Daniel Kiper wrote:
> On Tue, Nov 15, 2016 at 10:07:39PM +0100, Alexander Graf wrote:
>>
>>
>> On 15/11/2016 22:00, Daniel Kiper wrote:
>>> On Sat, Nov 12, 2016 at 12:03:33PM +0300, Andrei Borzenkov wrote:
>>>> 29.02.2016 02:22, Alexander Graf ??????????:
>>>>> Searching for a device tree that EFI passes to us via configuration
>>>>> tables
>>>>> is nothing architecture specific. Move it into generic code.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>> grub-core/kern/efi/init.c    | 22 ++++++++++++++++++++++
>>>>> grub-core/loader/arm64/fdt.c | 24 +-----------------------
>>>>> include/grub/efi/efi.h       |  2 ++
>>>>> 3 files changed, 25 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
>>>>> index e9c85de..fb90ecd 100644
>>>>> --- a/grub-core/kern/efi/init.c
>>>>> +++ b/grub-core/kern/efi/init.c
>>>>> @@ -72,6 +72,28 @@ grub_machine_get_bootlocation (char **device, char
>>>>> **path)
>>>>>     }
>>>>> }
>>>>>
>>>>> +void *
>>>>> +grub_efi_get_firmware_fdt (void)
>>>>> +{
>>>>> +  grub_efi_configuration_table_t *tables;
>>>>> +  grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
>>>>> +  void *firmware_fdt = NULL;
>>>>> +  unsigned int i;
>>>>> +
>>>>> +  /* Look for FDT in UEFI config tables. */
>>>>> +  tables = grub_efi_system_table->configuration_table;
>>>>> +
>>>>> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
>>>>> +    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof
>>>>> (fdt_guid)) == 0)
>>>>> +      {
>>>>> +	firmware_fdt = tables[i].vendor_table;
>>>>> +	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
>>>>> +	break;
>>>>> +      }
>>>>> +
>>>>> +  return firmware_fdt;
>>>>> +}
>>>>> +
>>>>
>>>> Is it relevant for x86? I cannot even find anything related to FDT or
>>>> "device tree" in UEFI spec, it falls under vendor extensions. What other
>>>> use it has except in Linux loader?
>>>>
>>>> I really fail to see why it should be moved into core until at least one
>>>> more non-trivial caller is present.
>>>
>>> Hmmm... It looks that I was too fast. TBH, I can agree to some extend.
>>> Should we revert this patch or just #ifdef relevant code? Revert seems
>>> better for me.
>>
>> Uh, the point of the function was to share code between 32bit and 64bit
>> arm platforms which are completely separate archs.
>>
>> There's also effort underway to run with device tree on x86:
>>
>>   https://plus.google.com/+ChristopherFriedt/posts/SfDifuC1xsR
>
> OK but right know we build FDT code for every EFI platform. This
> is not nice because not every EFI platform uses/knows FDT (at least
> today). So, I think that we should revert that patch and then you
> can provide another patch which puts FDT code in separate file, e.g.
> grub-core/kern/efi/fdt.c build only on platforms supporting FDT.
> Does it make sense?

It probably makes a lot more sense to move it to that file rather than 
revert the patches, as this is patch 1/2 and you would have to revert 
the actual 32bit arm enablement as well, breaking bisectability for 
32bit arm.


Alex

Re: [PATCH 1/2] arm64: Move firmware fdt search into global function

[Daniel Kiper]

On Wed, Nov 16, 2016 at 11:33:26AM +0100, Alexander Graf wrote:
> On 16/11/2016 11:28, Daniel Kiper wrote:
> >On Tue, Nov 15, 2016 at 10:07:39PM +0100, Alexander Graf wrote:

[...]

> >>Uh, the point of the function was to share code between 32bit and 64bit
> >>arm platforms which are completely separate archs.
> >>
> >>There's also effort underway to run with device tree on x86:
> >>
> >>  https://plus.google.com/+ChristopherFriedt/posts/SfDifuC1xsR
> >
> >OK but right know we build FDT code for every EFI platform. This
> >is not nice because not every EFI platform uses/knows FDT (at least
> >today). So, I think that we should revert that patch and then you
> >can provide another patch which puts FDT code in separate file, e.g.
> >grub-core/kern/efi/fdt.c build only on platforms supporting FDT.
> >Does it make sense?
>
> It probably makes a lot more sense to move it to that file rather than
> revert the patches, as this is patch 1/2 and you would have to revert
> the actual 32bit arm enablement as well, breaking bisectability for
> 32bit arm.

OK, let's go that way. I am waiting for your patch(es).

Daniel