From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1c6xSE-0005rO-F3 for mharc-grub-devel@gnu.org; Wed, 16 Nov 2016 05:28:34 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6xSC-0005rH-KT for grub-devel@gnu.org; Wed, 16 Nov 2016 05:28:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6xS9-0007mu-GF for grub-devel@gnu.org; Wed, 16 Nov 2016 05:28:32 -0500 Received: from boksu.net-space.pl ([185.15.1.105]:54585) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1c6xS9-0007mc-6D for grub-devel@gnu.org; Wed, 16 Nov 2016 05:28:29 -0500 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1667668AbcKPK2W (ORCPT ); Wed, 16 Nov 2016 11:28:22 +0100 Date: Wed, 16 Nov 2016 11:28:22 +0100 From: Daniel Kiper To: agraf@suse.de, grub-devel@gnu.org Cc: dkiper@net-space.pl, arvidjaar@gmail.com, leif.lindholm@linaro.org Subject: Re: [PATCH 1/2] arm64: Move firmware fdt search into global function Message-ID: <20161116102822.GJ16470@router-fw-old.local.net-space.pl> References: <1456701744-202295-1-git-send-email-agraf@suse.de> <77c2cdca-3a1c-f5be-c60e-00a8051d047d@gmail.com> <20161115210022.GG16470@router-fw-old.local.net-space.pl> <40ae70b1-9aeb-1f5c-2cd3-8fef929d9103@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <40ae70b1-9aeb-1f5c-2cd3-8fef929d9103@suse.de> User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Nov 2016 10:28:33 -0000 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 > >>>--- > >>> 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