From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by mx.groups.io with SMTP id smtpd.web11.31877.1618232628208184920 for ; Mon, 12 Apr 2021 06:03:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=g4cT3SBo; spf=pass (domain: akeo.ie, ip: 209.85.128.48, mailfrom: pete@akeo.ie) Received: by mail-wm1-f48.google.com with SMTP id j20-20020a05600c1914b029010f31e15a7fso8644092wmq.1 for ; Mon, 12 Apr 2021 06:03:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=wUyJr1iKLbnHi9KtSAsx9vpGNohczzKiSxWRkPoQcJs=; b=g4cT3SBodTD1KRfupxpxQQwmzOS+orhHgdKkxvAJhFhiKGxTW5Jqf0mzcyNRJ4toIZ FRzD6iye//pwwzYjUi5dt8YIORMKS/qLiDuUrFE/tua+zxnSYkKnom109YCNsEXF4A+h EBK7ruIYcPG1N1xbzf2MA3obj48zByxwO2lobg3cJPEAUBpnwof5gdmF3hlubnrB35m3 8BHtVbFRKUpMP/Il94yup3NnCT5YDeOh+qtRobV5Q3ezhc7BB6fnHUw6HhMHTwsrmI4H fhPK4bAUZ19R0FqZF+uinNryXt3nsHDXV0i3lQKIohddxvna12UjR6XPuSEWa60vsgTm nISQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wUyJr1iKLbnHi9KtSAsx9vpGNohczzKiSxWRkPoQcJs=; b=ZDz8omYg8LO3VawqPX8tf0vjsEuLnJg/Qb1D284AjgLP5xyfxKIKbwDoVnEvKk8MPs WuQCtSvSH0o1ldW6HOhfCOQ5wDRam1dmDizs7JrsMWdM+NIMX4ine7mpm03Jz9OenTS7 5cF/T2dIu7oRzNkZrcAiAwi9iQAhqf2HqzpzfINrSiKD1FRVvCv44z0Si7gIV60gzYPx Ua5XitCRPLAKcrViwpi8YQGvcTdnUr1hbwK9T1DqF9Hl0BK2DR3uis/eqCZx/4i9K7+Y kLEIqW+2t7a1hAEe36xu1NFA7sBFkQZ1Kjp5RxS/eT/dUuRsYxnURWvSHhxAAydnMNT3 4OOg== X-Gm-Message-State: AOAM531jl28XGJIMcRxF3Yfsvw2eXDuO+VNncG/jhOonfif5jVmpSpL7 L0DsHl24nGJBdKqDKl+rby7l7w== X-Google-Smtp-Source: ABdhPJz6/tKIipmmbzkX8LZv+2NptenTrCdlnKu9iB7N7juTv6yYmsdhwgt2xLqR0jCV3QxDcjFFqw== X-Received: by 2002:a1c:7707:: with SMTP id t7mr5020934wmi.76.1618232626705; Mon, 12 Apr 2021 06:03:46 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.85.228]) by smtp.googlemail.com with ESMTPSA id a2sm326558wrt.82.2021.04.12.06.03.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Apr 2021 06:03:46 -0700 (PDT) Subject: Re: [PATCH 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot To: Sunny Wang , devel@edk2.groups.io Cc: Samer El-Haj-Mahmoud , Jeremy Linton , Ard Biesheuvel References: <20210412090545.2130-1-Sunny.Wang@arm.com> From: "Pete Batard" Message-ID: Date: Mon, 12 Apr 2021 14:03:44 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <20210412090545.2130-1-Sunny.Wang@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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? 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 > Cc: Jeremy Linton > Cc: Pete Batard > Cc: Ard Biesheuvel > Signed-off-by: Sunny Wang > --- > .../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 > > * > > * 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 > > # > > # 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 > > - * 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 > > - * 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 > > - * 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 > > * 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 > > #include > > > > +#include I'd remove the extra line between and > > + > > #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 > > @@ -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 > > # 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 > > +# 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 >