From: "Ard Biesheuvel" <ardb@kernel.org>
To: Pete Batard <pete@akeo.ie>
Cc: Sunny Wang <Sunny.Wang@arm.com>,
edk2-devel-groups-io <devel@edk2.groups.io>,
Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>,
Jeremy Linton <jeremy.linton@arm.com>,
Sami Mujawar <sami.mujawar@arm.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot
Date: Thu, 15 Apr 2021 20:50:40 +0200 [thread overview]
Message-ID: <CAMj1kXEeZRRsp3vZXZr_dwpqkvmPZse+2Qh89fREry7yVgVAHQ@mail.gmail.com> (raw)
In-Reply-To: <e15b6865-03a1-cd32-0a44-9ebaca36803f@akeo.ie>
On Tue, 13 Apr 2021 at 17:24, Pete Batard <pete@akeo.ie> wrote:
>
> On 2021.04.13 16:22, Ard Biesheuvel wrote:
> > On Tue, 13 Apr 2021 at 14:13, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> Thanks for the v2. Everything looks good now.
> >>
> >> On 2021.04.13 08:14, Sunny Wang wrote:
> >>> This is a fix for https://github.com/pftf/RPi4/issues/114.
> >>>
> >>> Changes:
> >>> 1. Add a setup option called Boot Policy and consume the setting
> >>> during boot to whether perform or skip ConnectAll.
> >>> 2. The Default setting is set to Full discovery because it is not
> >>> worth enabling Fast boot by default on RaspberryPi systems.
> >>> Enabling it just saves boot time about 1 second, but caused a
> >>> lot of issues.
> >>>
> >>> Testing Done:
> >>> - Booted to Standalone UEFI shell on SD card and use drivers
> >>> command to check the result with Fast Boot and Full discovery
> >>> settings. Then, child/device handles are created as expected.
> >>>
> >>> Note and to-do items:
> >>> - The root cause looks like that boot loaders and some tools like
> >>> grub and iPXE haven't supported selective connect/Fast boot.
> >>> However, system firmware should still provide a setup option for
> >>> user to enable Fast boot with old version boot loaders and tools
> >>> , which is why we proposed this change. We will also report this
> >>> issue to boot loader and tool vendors/open source GitHubs.
> >>> - We Will add more options for connecting specific type devices so
> >>> that we can still have the shortest boot time for all use cases.
> >>>
> >>> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
> >>> Cc: Jeremy Linton <jeremy.linton@arm.com>
> >>> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >>> Cc: Pete Batard <pete@akeo.ie>
> >>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >>> Signed-off-by: Sunny Wang <sunny.wang@arm.com>
> >>> ---
> >>> .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 11 ++++++++++-
> >>> .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf | 3 ++-
> >>> .../Drivers/ConfigDxe/ConfigDxeHii.uni | 10 +++++++++-
> >>> .../Drivers/ConfigDxe/ConfigDxeHii.vfr | 16 +++++++++++++++-
> >>> Platform/RaspberryPi/Include/ConfigVars.h | 12 +++++++++++-
> >>> .../Library/PlatformBootManagerLib/PlatformBm.c | 15 +++++++++++++--
> >>> .../PlatformBootManagerLib.inf | 1 +
> >>> Platform/RaspberryPi/RPi3/RPi3.dsc | 9 ++++++++-
> >>> Platform/RaspberryPi/RPi4/RPi4.dsc | 9 ++++++++-
> >>> Platform/RaspberryPi/RaspberryPi.dec | 2 ++
> >>> 10 files changed, 79 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> >>> index 22f86d4d44..d3c5869949 100644
> >>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> >>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> >>> @@ -1,6 +1,6 @@
> >>> /** @file
> >>> *
> >>> - * Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> >>> + * Copyright (c) 2019 - 2021, ARM Limited. All rights reserved.
> >>> * Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> *
> >>> * SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> @@ -281,6 +281,15 @@ SetupVariables (
> >>> );
> >>> }
> >>>
> >>> + Size = sizeof (UINT32);
> >>> + Status = gRT->GetVariable (L"BootPolicy",
> >>> + &gConfigDxeFormSetGuid,
> >>> + NULL, &Size, &Var32);
> >>> + if (EFI_ERROR (Status)) {
> >>> + Status = PcdSet32S (PcdBootPolicy, PcdGet32 (PcdBootPolicy));
> >>> + ASSERT_EFI_ERROR (Status);
> >>> + }
> >>> +
> >>> Size = sizeof (UINT32);
> >>> Status = gRT->GetVariable (L"SdIsArasan",
> >>> &gConfigDxeFormSetGuid,
> >>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> >>> index d51e54e010..032e40b0c3 100644
> >>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> >>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> >>> @@ -2,7 +2,7 @@
> >>> #
> >>> # Component description file for the RasbperryPi DXE platform config driver.
> >>> #
> >>> -# Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> >>> +# Copyright (c) 2019 - 2021, ARM Limited. All rights reserved.
> >>> # Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> #
> >>> # SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> @@ -93,6 +93,7 @@
> >>> gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
> >>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio
> >>> gRaspberryPiTokenSpaceGuid.PcdFanTemp
> >>> + gRaspberryPiTokenSpaceGuid.PcdBootPolicy
> >>>
> >>> [Depex]
> >>> gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
> >>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> >>> index 466fa852cb..81761d64bb 100644
> >>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> >>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> >>> @@ -1,7 +1,7 @@
> >>> /** @file
> >>> *
> >>> * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> - * Copyright (c) 2020, ARM Limited. All rights reserved.
> >>> + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> >>> *
> >>> * SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> *
> >>> @@ -60,6 +60,14 @@
> >>> #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"
> >>> #string STR_ADVANCED_ASSET_TAG_HELP #language en-US "Set the system Asset Tag"
> >>>
> >>> +#string STR_BOOT_POLICY_PROMPT #language en-US "Boot Policy"
> >>> +#string STR_BOOT_POLICY_HELP #language en-US "When Fast Boot is selected, only required devices will be discovered for reducing "
> >>> + "the boot time. "
> >>> + "When Full Discovery is selected, all the devices will be discovered for some "
> >>> + "scenarios such as system deployment and diagnostic tests."
> >>> +#string STR_FAST_BOOT #language en-US "Fast Boot"
> >>> +#string STR_FULL_DISCOVERY #language en-US "Full Discovery"
> >>> +
> >>> /*
> >>> * MMC/SD configuration.
> >>> */
> >>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> >>> index cc7a09cfb7..aa124e4e31 100644
> >>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> >>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> >>> @@ -1,7 +1,7 @@
> >>> /** @file
> >>> *
> >>> * Copyright (c) 2018 Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> - * Copyright (c) 2020, ARM Limited. All rights reserved.
> >>> + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> >>> *
> >>> * SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> *
> >>> @@ -116,6 +116,11 @@ formset
> >>> name = DisplayEnableSShot,
> >>> guid = CONFIGDXE_FORM_SET_GUID;
> >>>
> >>> + efivarstore BOOT_POLICY_VARSTORE_DATA,
> >>> + attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> >>> + name = BootPolicy,
> >>> + guid = CONFIGDXE_FORM_SET_GUID;
> >>> +
> >>> form formid = 1,
> >>> title = STRING_TOKEN(STR_FORM_SET_TITLE);
> >>> subtitle text = STRING_TOKEN(STR_NULL_STRING);
> >>> @@ -190,6 +195,14 @@ formset
> >>> option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
> >>> endoneof;
> >>>
> >>> + oneof varid = BootPolicy.BootPolicy,
> >>> + prompt = STRING_TOKEN(STR_BOOT_POLICY_PROMPT),
> >>> + help = STRING_TOKEN(STR_BOOT_POLICY_HELP),
> >>> + flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
> >>> + option text = STRING_TOKEN(STR_FAST_BOOT), value = FAST_BOOT , flags = 0;
> >>> + option text = STRING_TOKEN(STR_FULL_DISCOVERY), value = FULL_DISCOVERY, flags = DEFAULT;
> >>> + endoneof;
> >>> +
> >>> #if (RPI_MODEL == 4)
> >>> grayoutif NOT ideqval SystemTableMode.Mode == SYSTEM_TABLE_MODE_ACPI;
> >>> oneof varid = FanOnGpio.Enabled,
> >>> @@ -220,6 +233,7 @@ formset
> >>> minsize = 0,
> >>> maxsize = ASSET_TAG_STR_MAX_LEN,
> >>> endstring;
> >>> +
> >>> endform;
> >>>
> >>> form formid = 0x1003,
> >>> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
> >>> index 142317985a..9ef62b7a6e 100644
> >>> --- a/Platform/RaspberryPi/Include/ConfigVars.h
> >>> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
> >>> @@ -1,7 +1,7 @@
> >>> /** @file
> >>> *
> >>> * Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> - * Copyright (c) 2020, ARM Limited. All rights reserved.
> >>> + * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> >>> *
> >>> * SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> *
> >>> @@ -143,4 +143,14 @@ typedef struct {
> >>> UINT32 EnableDma;
> >>> } MMC_EMMC_DMA_VARSTORE_DATA;
> >>>
> >>> +#define FAST_BOOT 0
> >>> +#define FULL_DISCOVERY 1
> >>> +typedef struct {
> >>> + /*
> >>> + * 0 - Fast Boot
> >>> + * 1 - Full Discovery (Connect All)
> >>> + */
> >>> + UINT32 BootPolicy;
> >>> +} BOOT_POLICY_VARSTORE_DATA;
> >>> +
> >>> #endif /* CONFIG_VARS_H */
> >>> diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> >>> index c2fc40b8ea..d081fdae63 100644
> >>> --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> >>> +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> >>> @@ -4,7 +4,7 @@
> >>> * Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> * Copyright (c) 2016, Linaro Ltd. All rights reserved.
> >>> * Copyright (c) 2015-2016, Red Hat, Inc.
> >>> - * Copyright (c) 2014-2020, ARM Ltd. All rights reserved.
> >>> + * Copyright (c) 2014-2021, ARM Ltd. All rights reserved.
> >>> * Copyright (c) 2004-2016, Intel Corporation. All rights reserved.
> >>> *
> >>> * SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> @@ -25,10 +25,11 @@
> >>> #include <Protocol/LoadedImage.h>
> >>> #include <Guid/EventGroup.h>
> >>> #include <Guid/TtyTerm.h>
> >>> +#include <ConfigVars.h>
> >>>
> >>> #include "PlatformBm.h"
> >>>
> >>> -#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)"
> >>> +#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)\n"
> >>>
> >>> #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
> >>>
> >>> @@ -633,6 +634,16 @@ PlatformBootManagerAfterConsole (
> >>> Print (BOOT_PROMPT);
> >>> }
> >>>
> >>> + //
> >>> + // Connect the rest of the devices if the boot polcy is set to Full discovery
> >>> + //
> >>> + if (PcdGet32 (PcdBootPolicy) == FULL_DISCOVERY) {
> >>> + DEBUG ((DEBUG_INFO, "Boot Policy is Full Discovery. Connect all devices\n"));
> >>> + EfiBootManagerConnectAll ();
> >>> + } else if (PcdGet32 (PcdBootPolicy) == FAST_BOOT) {
> >>> + DEBUG ((DEBUG_INFO, "Boot Policy is Fast Boot. Skip connecting all devices\n"));
> >>> + }
> >>> +
> >>> Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL, (VOID**)&EsrtManagement);
> >>> if (!EFI_ERROR (Status)) {
> >>> EsrtManagement->SyncEsrtFmp ();
> >>> diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> >>> index 88f6f8fe09..fbf510ab96 100644
> >>> --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> >>> +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> >>> @@ -63,6 +63,7 @@
> >>> [Pcd]
> >>> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> >>> gRaspberryPiTokenSpaceGuid.PcdSdIsArasan
> >>> + gRaspberryPiTokenSpaceGuid.PcdBootPolicy
> >>>
> >>> [Guids]
> >>> gEfiFileInfoGuid
> >>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> >>> index 0961133ae9..425c7ff9ec 100644
> >>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> >>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> >>> @@ -1,6 +1,6 @@
> >>> # @file
> >>> #
> >>> -# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved.
> >>> +# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved.
> >>> # Copyright (c) 2014, Linaro Limited. All rights reserved.
> >>> # Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
> >>> # Copyright (c) 2017 - 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> @@ -512,6 +512,13 @@
> >>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
> >>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0
> >>>
> >>> + #
> >>> + # Boot Policy
> >>> + # 0 - Fast Boot
> >>> + # 1 - Full Discovery (Connect All)
> >>> + #
> >>> + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1
> >>> +
> >>> #
> >>> # Reset-related.
> >>> #
> >>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> >>> index ff802d8347..5c6783eae7 100644
> >>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> >>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> >>> @@ -1,6 +1,6 @@
> >>> # @file
> >>> #
> >>> -# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved.
> >>> +# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved.
> >>> # Copyright (c) 2017 - 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> # Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
> >>> # Copyright (c) 2014, Linaro Limited. All rights reserved.
> >>> @@ -528,6 +528,13 @@
> >>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
> >>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60
> >>>
> >>> + #
> >>> + # Boot Policy
> >>> + # 0 - Fast Boot
> >>> + # 1 - Full Discovery (Connect All)
> >>> + #
> >>> + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1
> >>> +
> >>> #
> >>> # Reset-related.
> >>> #
> >>> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> >>> index 08135717ed..8eb1c2bac7 100644
> >>> --- a/Platform/RaspberryPi/RaspberryPi.dec
> >>> +++ b/Platform/RaspberryPi/RaspberryPi.dec
> >>> @@ -2,6 +2,7 @@
> >>> #
> >>> # Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> >>> # Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >>> +# Copyright (c) 2021, ARM Limited. All rights reserved.
> >>> #
> >>> # SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> #
> >>> @@ -70,3 +71,4 @@
> >>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
> >>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001E
> >>> gRaspberryPiTokenSpaceGuid.PcdMmcEnableDma|0|UINT32|0x0000001F
> >>> + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|0|UINT32|0x00000020
> >>>
> >>
> >> Reviewed-by: Pete Batard <pete@akeo.ie>
> >
> > Thanks Pete.
> >
> > So which patch is the one I should be applying?
> >
>
> The one I reviewed is the most recent I saw on the mailing list.
> I believe both v2 are identical though.
>
OK
Pushed as 7545558886de..efdc159ef7c9
Thanks all,
next prev parent reply other threads:[~2021-04-15 18:50 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-13 7:14 [PATCH v2 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot Sunny Wang
2021-04-13 12:12 ` Pete Batard
2021-04-13 15:22 ` Ard Biesheuvel
2021-04-13 15:24 ` Pete Batard
2021-04-15 18:50 ` Ard Biesheuvel [this message]
-- strict thread matches above, loose matches on Subject: below --
2021-04-13 4:20 Sunny Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMj1kXEeZRRsp3vZXZr_dwpqkvmPZse+2Qh89fREry7yVgVAHQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox