Hi Bhupesh, > -----Original Message----- > From: Bhupesh Sharma [mailto:bhsharma@xxxxxxxxxx] > Sent: Monday, March 26, 2018 2:15 PM > To: Pankaj Bansal <pankaj.bansal@xxxxxxx> > Cc: Varun Sethi <V.Sethi@xxxxxxx>; Wasim Khan <wasim.khan@xxxxxxx>; > Udit Kumar <udit.kumar@xxxxxxx>; linux-efi@xxxxxxxxxxxxxxx; Ard Biesheuvel > <ard.biesheuvel@xxxxxxxxxx> > Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration > table > > On 03/26/2018 11:13 AM, Pankaj Bansal wrote: > > Hello Bhupesh/Ard, > > > > Thank you for reviewing my changes. > > > > > -----Original Message----- > > > From: Bhupesh Sharma [mailto:bhsharma@xxxxxxxxxx] > > > Sent: Monday, March 26, 2018 10:51 AM > > > To: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > > Cc: Varun Sethi <V.Sethi@xxxxxxx>; Wasim Khan <wasim.khan@xxxxxxx>; > > > Udit Kumar <udit.kumar@xxxxxxx>; Ard Biesheuvel > > > <ard.biesheuvel@xxxxxxxxxx>; linux-efi@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in > > > configuration table > > > > > > Hi Pankaj, > > > > > > On 03/25/2018 07:26 PM, Pankaj Bansal wrote: > > >> Bootloader may need to fixup the device tree before OS can use it. > > >> e.g. a UEFI/DXE driver that has initialized a controller can add > > >> controller's clock frequency in controller node. This way OS need > > >> not to call get/set clock for that controller. > > > Its usually better to include a cover letter to provide a > > > description of what the patchset is adding/fixing. > > > > Sure. Will follow this practice now onwards. > > > > > What I understand from looking at this patch is that you are > > > providing the device tree to the kernel (via dtb= ? command) and > > > trying to fix it via the bootloader - is this understanding correct? > > > > That is correct. > > > > > > > > If yes, I am not sure why the bootloader would not pass the dtb in > > > the first place to the kernel via the standard interface defined in > > > <https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > www > > > > .kernel.org%2Fdoc%2FDocumentation%2Farm64%2Fbooting.txt&data=02%7C0 > > > > 1%7Cpankaj.bansal%40nxp.com%7Ce917e5770a544261e9ee08d592d971a0%7C > > > > 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636576384962510170&sd > > > ata=GlBJchtRXnrX2IyoaKqe25k1ZDMmy0bH8ZileRFFBqo%3D&reserved=0>, > i.e.: > > > > > > x0 = physical address of device tree blob (dtb) in system RAM. > > > > > > Normally bootloaders like UEFI/u-boot pass a fixed up dtb to the > > > kernel, in such a case, the dtb would be fixed up by the bootloader > > > itself and the kernel needs not worry about the same. > > > > Yes this is the default case. But we have seen such cases when the linux kernel > fails to boot with the UEFI provided dtb. > > e.g. say UEFI dtb binary is generated with linux v4.4. When we try to > > boot linux v4.9 or v4.14 without updating dtb, then kernel boot fails. Maybe > this is because device trees are not backward compatible as Ard pointed out. > > > > I see the boot behavior in other boot loader u-boot. In that bootloader we can > provide the kernel, initrd, dtb triad. > > > > Usage: > > bootm [addr [arg ...]] > > - boot application image stored in memory > > passing arguments 'arg ...'; when booting a Linux kernel, > > 'arg' can be the address of an initrd image > > When booting a Linux kernel which requires a flat device-tree > > a third argument is required which is the address of the > > device-tree blob. To boot that kernel without an initrd image, > > use a '-' for the second argument. If you do not pass a third > > a bd_info struct will be passed instead > > > > The u-boot fixes up the dtb provided in bootm command before booting the > kernel image. > > This same behavior we want to achieve with UEFI boot. i.e. UEFI will provide a > dtb image, but it can be overridden using dtb = <path/to/dtb>. > But there are several limitations to this approach. One of them is that dtb= is > ignored from kernel command line when UEFI Secure Boot is enabled. > See > <https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir. > bootlin.com%2Flinux%2Fv4.4.41%2Fsource%2Fdrivers%2Ffirmware%2Fefi%2Flib > stub%2Farm- > stub.c%23L235&data=02%7C01%7Cpankaj.bansal%40nxp.com%7C49f4c16493c > b4cd66a7108d592f5d2b9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1 > %7C636576506854276136&sdata=HAjJP8mZYQ7UWMoQ1AwOfUQdAK2ztsbdks > hZFRMxn4s%3D&reserved=0> for details. > Right now we are not testing secure boot. But in my opinion for secure boot the dtb binary would be made part of Efi FD image and the whole FD image would be signed. Then if we want to boot a signed kernel image other than whose dtb is present in Firmware, we need to generate FD image again with new dtb and sign it. > So you won't be able to use this method while attempting a UEFI secure boot. > > Which would bring-up a question - why don't we fix up the device tree in the > UEFI bootloader itself as we need to provide the dtb to the kernel via the > bootloader itself. > For e.g. we can make the bootloader read the dtb from a storage source (e.g.: > sd card?) and patch it up as per the kernel version we are trying to boot. > This is same that dtb = ... would do, the only difference is that dtb image would be read by efi firmware rather by efi stub. BUT, still the Ard's statement "you should not use the dtbs shipped with the kernel" would not hold true. > Regards, > Bhupesh > > > > > >> Therefore, install fdt used by OS in configuration tables and > > >> associate it with device tree guid. > > >> > > >> UEFI/DXE drivers can fixup this device tree in their respective > > >> ExitBootServices events. > > >> > > >> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > >> Cc: linux-efi@xxxxxxxxxxxxxxx > > >> Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > >> --- > > >> drivers/firmware/efi/libstub/fdt.c | 8 ++++++++ > > >> 1 file changed, 8 insertions(+) > > >> > > >> diff --git a/drivers/firmware/efi/libstub/fdt.c > > >> b/drivers/firmware/efi/libstub/fdt.c > > >> index 177654e..df862e6 100644 > > >> --- a/drivers/firmware/efi/libstub/fdt.c > > >> +++ b/drivers/firmware/efi/libstub/fdt.c > > >> @@ -265,6 +265,7 @@ efi_status_t > > > allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg, > > >> int runtime_entry_count = 0; > > >> struct efi_boot_memmap map; > > >> struct exit_boot_struct priv; > > >> + efi_guid_t fdt_guid = DEVICE_TREE_GUID; > > >> > > >> map.map = &runtime_map; > > >> map.map_size = &map_size; > > >> @@ -314,6 +315,13 @@ efi_status_t > > > allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg, > > >> goto fail_free_new_fdt; > > >> } > > >> > > >> + status = efi_call_early(install_configuration_table, &fdt_guid, > > >> + (void *)*new_fdt_addr); > > >> + if (status != EFI_SUCCESS) { > > >> + pr_efi_err(sys_table_arg, "Unable to install new device tree.\n"); > > >> + goto fail_free_new_fdt; > > >> + } > > >> + > > >> priv.runtime_map = runtime_map; > > >> priv.runtime_entry_count = &runtime_entry_count; > > >> priv.new_fdt_addr = (void *)*new_fdt_addr; > > >> > > ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
