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>,
Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot
Date: Mon, 12 Apr 2021 19:27:04 +0200 [thread overview]
Message-ID: <CAMj1kXHtfrd6_QuzrerecdNhnGfDBiqww1xAFNpdKi9HFoi5AQ@mail.gmail.com> (raw)
In-Reply-To: <e48e43da-8734-0d9c-a222-f29eed291816@akeo.ie>
On Mon, 12 Apr 2021 at 15:03, Pete Batard <pete@akeo.ie> wrote:
>
> Hi Sunny,
>
> Functionaly, I see no issues with this patch, and it's indeed a good
> solution to the problem of trying to satisfy everyone while fixing the
> issues we are seeing.
>
> There are however a few minor typos and whitespace issues, that I'm
> detailing below. So could you please send a v2 to address these?
>
Agree with Pete. Thanks for providing a fix for this, and please
incorporate the changes suggested by Pete.
Then, could we *please* figure out a way to get this patch on the
mailing list without Windows whitespace damage? Not sure if anything
changed recently, but not a single EDK2 patch has arrived in my
mailbox recently in a form that I could apply with 'git am'.
Thanks,
Ard.
> On 2021.04.12 10:05, 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: 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 | 15 ++++++++++++++-
> > Platform/RaspberryPi/Include/ConfigVars.h | 12 +++++++++++-
> > .../Library/PlatformBootManagerLib/PlatformBm.c | 16 ++++++++++++++--
> > .../PlatformBootManagerLib.inf | 1 +
> > Platform/RaspberryPi/RPi3/RPi3.dsc | 10 +++++++++-
> > Platform/RaspberryPi/RPi4/RPi4.dsc | 9 ++++++++-
> > Platform/RaspberryPi/RaspberryPi.dec | 2 ++
> > 10 files changed, 80 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..7b14fdf39f 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"
> >
> >
>
> Personally, I would move the Boot Policy section between "System Table
> Selection" and "ACPI Fan Control" if we try to keep the advanced
> settings in the order we think they will be most used.
>
> >
> > +#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 discoverd for reducing "
>
> "discoverd" -> "discovered"
>
> >
> > + "the boot time."
>
> Missing a space after "the boot time.". It should be "the boot time. ",
> else the help text will display as "(...) the boot time.When Full
> Discovery (...)"
>
> >
> > + "When Full Discovery is selected, all the devices will be discoverd for some "
>
> "discoverd" -> "discovered"
>
> >
> > + "scenarios such as system deployement and diagnostic tests"
>
> "deployement" -> "deployment"
>
> >
> > +#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..5dc558ec08 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);
> >
> > @@ -220,6 +225,14 @@ formset
> > minsize = 0,
> >
> > maxsize = ASSET_TAG_STR_MAX_LEN,
> >
> > endstring;
> >
> > +
> >
> > + 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;
>
> "STRING_TOKEN(STR_FAST_BOOT )" -> "STRING_TOKEN(STR_FAST_BOOT)"
>
> >
> > + option text = STRING_TOKEN(STR_FULL_DISCOVERY), value = FULL_DISCOVERY, flags = DEFAULT;
> >
> > + endoneof;
> >
> > endform;
> >
> >
> >
> > form formid = 0x1003,
> >
> > diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
> > index 142317985a..3347f899df 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 Discovrey (Connect All)
>
> "Discovrey" -> "Discovery"
>
> >
> > + */
> >
> > + 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..abbe4fb3d0 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
> >
> > @@ -26,9 +26,11 @@
> > #include <Guid/EventGroup.h>
> >
> > #include <Guid/TtyTerm.h>
> >
> >
> >
> > +#include <ConfigVars.h>
>
> I'd remove the extra line between <Guid/TtyTerm.h> and <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"
>
> Why are you adding the carriage return? Was there an issue with the
> display of this prompt?
>
> >
> >
> >
> > #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
> >
> >
> >
> > @@ -633,6 +635,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"));
>
> "All" -> "all"
>
> >
> > + 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..ddb03e405f 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,14 @@
> > gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
> >
> > gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0
> >
> >
> >
> > + #
> >
> > + # Boot Policy
> >
> > + # 0 - Fast Boot
> >
> > + # 1 - Full Discovrey (Connect All)
>
> "Discovrey" -> "Discovery"
>
> >
> > + #
> >
> > + gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1
> >
> > +
> >
> > +
>
> Please remove the extra line. There should be just one blank like
> between the sections.
>
> >
> > #
> >
> > # Reset-related.
> >
> > #
> >
> > diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> > index ff802d8347..8ee1922a44 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 Discovrey (Connect All)
>
> "Discovrey" -> "Discovery"
>
> Regards,
>
> /Pete
>
> >
> > + #
> >
> > + 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
> >
>
next prev parent reply other threads:[~2021-04-12 17:27 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-12 9:05 [PATCH 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot Sunny Wang
2021-04-12 13:03 ` Pete Batard
2021-04-12 17:27 ` Ard Biesheuvel [this message]
2021-04-13 6:01 ` Sunny Wang
[not found] ` <16755592E0799AE6.2444@groups.io>
2021-04-13 7:22 ` [edk2-devel] " Sunny Wang
2021-04-13 4:33 ` Sunny Wang
2021-04-12 21:28 ` Samer El-Haj-Mahmoud
2021-04-13 7:51 ` Sunny Wang
2021-04-16 13:45 ` 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=CAMj1kXHtfrd6_QuzrerecdNhnGfDBiqww1xAFNpdKi9HFoi5AQ@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