From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by mx.groups.io with SMTP id smtpd.web08.513.1609784701428658307 for ; Mon, 04 Jan 2021 10:25:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=wjs6xFFI; spf=pass (domain: akeo.ie, ip: 209.85.221.52, mailfrom: pete@akeo.ie) Received: by mail-wr1-f52.google.com with SMTP id 91so33127733wrj.7 for ; Mon, 04 Jan 2021 10:25:01 -0800 (PST) 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=RLMpyFC36SHiUgS2ANFI/kPzjpDAMLjGekwgbDODreA=; b=wjs6xFFI0YEy0ENY9+e+V2914IWuk5xxddVuW0kURD8E2+Fqb6D4etq5VN6oHrTHiV SgpgxTBUC26sI77e/wQbKYA1mQ3rS30Se4hj5jWrjPC+25pHjy8BAfeEW8yDSPhmSxn/ V/t9HWJGJ7UIHNbexuGAmMT6b5vmlGGTHxLHGWsFDEy+zszVdszfIvuBCYCXxhphRLpo Ub+6oUCK/gnAzrs/xvSS32+5uWhaW30xWuqvEuRzsny3noBMVz2MGkbkhePF5bdvZAXw HVGGCIcM2zQmjyHXfW54Vi0ZwWPQMDqjNUEtg9PCo0gEXub/r4hNIrvPId82V4VkQs5Q lW7A== 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=RLMpyFC36SHiUgS2ANFI/kPzjpDAMLjGekwgbDODreA=; b=aBkabWp75XbgKd4i573RziwSzf3Gmo0YMroALYx33WHUrhTn2EZ1dTcOblwWwNiyY+ n7VaSgKP9waHcQcjKzzLqE/EQJXztFqxEqYGTYPMsMPmTfjizISB4kECgBx93C1TSIEc zFPY3zkL33FTQ9LA5LgzfBiC5BrlnJg1IC9ANDr4s8zoV7BPnunh3pF32VkSWzDQirAX pyLL3sCVD6XqP/Rv4bGTvzEsrXZpUHg7AXYn405d3F3WTblvNd34+6WZZe6BkvyHPChR grW6XWchl+Rw1nKIJojDZTKf3rmUpvSyGWsEtY/jWtKWr2TsUE8Zd4VHvQGBhg0Mesjc CaoQ== X-Gm-Message-State: AOAM533xoA3ji07Ks4WB9LGb5A6g/JnXdGfzyZVe/Vd7jQCn/VQdgvTW rUdRM3dBwkrhQf6iqO+P+dTLkg== X-Google-Smtp-Source: ABdhPJzo1BcvPgjmyR+1Uh5ruJW1xdDITblKNMzbEee9QxrNOHHcf3Q6SjA7ELNhP9T5nZQpmbOUtA== X-Received: by 2002:a05:6000:368:: with SMTP id f8mr81049981wrf.150.1609784699928; Mon, 04 Jan 2021 10:24:59 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.63.11]) by smtp.googlemail.com with ESMTPSA id g5sm90144825wro.60.2021.01.04.10.24.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Jan 2021 10:24:59 -0800 (PST) Subject: Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay To: Ard Biesheuvel , devel@edk2.groups.io Cc: leif@nuviainc.com, samer.el-haj-mahmoud@arm.com, awarkentin@vmware.com References: <20201222134528.793-1-pete@akeo.ie> <8236dcd5-fa4f-130c-6e43-d3aed2e79e4a@arm.com> From: "Pete Batard" Message-ID: <8e9411f9-d0b4-415b-bd51-272b421fe4a1@akeo.ie> Date: Mon, 4 Jan 2021 18:24:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <8236dcd5-fa4f-130c-6e43-d3aed2e79e4a@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Hi Ard, Thanks for pushing the patch. But I think it will need some amending indeed, as per comments below: On 2021.01.04 09:44, Ard Biesheuvel wrote: > On 12/22/20 2:45 PM, Pete Batard wrote: >> Due to the method in which NV variables are stored on removable media >> for the Raspberry Pi platform, and the manner in which we dump updated >> variables right before reset, it is possible, and has been repeatedly >> demonstrated with SSD-based USB 3.0 devices, that the updated file does >> not actually end up being written to permanent storage, due to the >> device write-cache not having enough time to be flushed before reset. >> >> To compensate for this, since we don't know of a generic method that >> would allow turning off USB mass storage devices write cache (and also >> because we are seeing an issue that seems related for SD-based media), >> we add a new reset delay PCD, which can be set by the user, and which >> we also set as required when NV variables are being dumped. >> >> Our testing show that, with more than 3 seconds of extra delay, the >> storage media has enough time to finalize its internal write, thus >> solving the issue of configuration changes not being persisted. >> >> Signed-off-by: Pete Batard >> --- >> Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c | 24 ++++++++++++++++++++ >> Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf | 1 + >> Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 11 +++++++++ >> Platform/RaspberryPi/Library/ResetLib/ResetLib.inf | 5 ++++ >> Platform/RaspberryPi/RPi3/RPi3.dsc | 6 +++++ >> Platform/RaspberryPi/RPi4/RPi4.dsc | 6 +++++ >> Platform/RaspberryPi/RaspberryPi.dec | 1 + >> 7 files changed, 54 insertions(+) >> >> diff --git a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c >> index 07f3f1c24295..4071a3fca468 100644 >> --- a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c >> +++ b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c >> @@ -10,6 +10,18 @@ >> >> #include "VarBlockService.h" >> >> +// >> +// Minimum delay to enact before reset, when variables are dirty (in μs). >> +// Needed to ensure that SSD-based USB 3.0 devices have time to flush their >> +// write cache after updating the NV vars. A much smaller delay is applied >> +// on Pi 3 compared to Pi 4, as we haven't had reports of issues there yet. >> +// >> +#if (RPI_MODEL == 3) >> +#define PLATFORM_RESET_DELAY 500000 >> +#else >> +#define PLATFORM_RESET_DELAY 3500000 >> +#endif >> + >> VOID *mSFSRegistration; >> >> >> @@ -154,6 +166,7 @@ DumpVars ( >> ) >> { >> EFI_STATUS Status; >> + RETURN_STATUS PcdStatus; >> >> if (mFvInstance->Device == NULL) { >> DEBUG ((DEBUG_INFO, "Variable store not found?\n")); >> @@ -173,6 +186,17 @@ DumpVars ( >> } >> >> DEBUG ((DEBUG_INFO, "Variables dumped!\n")); >> + >> + // >> + // Add a reset delay to give time for slow/cached devices >> + // to flush the NV variables write to permanent storage. >> + // But only do so if this won't reduce an existing user-set delay. >> + // >> + if (PcdGet32 (PcdPlatformResetDelay) < PLATFORM_RESET_DELAY) { >> + PcdStatus = PcdSet32S (PcdPlatformResetDelay, PLATFORM_RESET_DELAY); >> + ASSERT_RETURN_ERROR (PcdStatus); >> + } >> + >> mFvInstance->Dirty = FALSE; >> } >> >> diff --git a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf >> index ecfb8f85c9c1..c2edb25bd41d 100644 >> --- a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf >> +++ b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf >> @@ -79,6 +79,7 @@ [Pcd] >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase >> gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase >> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 >> >> [FeaturePcd] >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> index c62a92321ecb..4a50166dd63b 100644 >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> @@ -16,6 +16,7 @@ >> >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -44,6 +45,7 @@ LibResetSystem ( >> ) >> { >> ARM_SMC_ARGS ArmSmcArgs; >> + UINT32 Delay; >> >> if (!EfiAtRuntime ()) { >> /* >> @@ -52,6 +54,15 @@ LibResetSystem ( >> EfiEventGroupSignal (&gRaspberryPiEventResetGuid); >> } >> >> + Delay = PcdGet32 (PcdPlatformResetDelay); >> + if (Delay != 0) { >> + DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", >> + Delay / 1000000, (Delay % 1000000) / 100000)); >> + MicroSecondDelay (Delay); >> + } > > Doesn't this cause a crash at runtime on the DEBUG build due to the fact > that the UART is no longer 1:1 mapped? You are bringing a good point that made me test this patch a bit more comprehensively. Whereas I saw no ill outcome from leaving the DEBUG statements in the DEBUG firmware, I am seeing a kernel panic in Linux (I mostly tested with Debian 11) on reset/poweroff, with either the RELEASE or DEBUG UEFI firmware, which I have isolated to the PcdGet32 () line. My understanding is that this occurs because we shouldn't use that call after ExitBootServices (), and therefore the 6 new lines above should have been inside the "if (!EfiAtRuntime ())" block, rather than as an unconditional section of code. It also makes sense to only have that code there anyway, since, even if I tried to introduce a generic PCD, this patch is mostly a workaround for variables not being stored after the user changes them though the firmware UI, and therefore where platform reset happens before we exit boot services. Thus, unless someone sees it differently, I'll be sending an addendum patch, that moves the 6 lines above into the "if (!EfiAtRuntime ())" block. Regards, /Pete > > >> + DEBUG ((DEBUG_INFO, "Platform %a.\n", >> + (ResetType == EfiResetShutdown) ? "shutdown" : "reset")); >> + >> switch (ResetType) { >> case EfiResetPlatformSpecific: >> // Map the platform specific reset as reboot >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf >> index b02a06d9d0bf..9bdb94a52ebf 100644 >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf >> @@ -33,8 +33,13 @@ [LibraryClasses] >> DebugLib >> BaseLib >> ArmSmcLib >> + PcdLib >> + TimerLib >> UefiLib >> UefiRuntimeLib >> >> [Guids] >> gRaspberryPiEventResetGuid >> + >> +[Pcd] >> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay ## CONSUMES >> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc >> index 9408138d0a09..530b42796a0d 100644 >> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc >> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc >> @@ -508,6 +508,12 @@ [PcdsDynamicHii.common.DEFAULT] >> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 >> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0 >> >> + # >> + # Reset-related. >> + # >> + >> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRaspberryPiTokenSpaceGuid|0x0|0 >> + >> # >> # Common UEFI ones. >> # >> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc >> index ddf4dd6a416e..5f8452aa0b76 100644 >> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc >> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc >> @@ -522,6 +522,12 @@ [PcdsDynamicHii.common.DEFAULT] >> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 >> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60 >> >> + # >> + # Reset-related. >> + # >> + >> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRaspberryPiTokenSpaceGuid|0x0|0 >> + >> # >> # Common UEFI ones. >> # >> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec >> index c64c61930ea8..10723036aa31 100644 >> --- a/Platform/RaspberryPi/RaspberryPi.dec >> +++ b/Platform/RaspberryPi/RaspberryPi.dec >> @@ -68,3 +68,4 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >> gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A >> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C >> gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D >> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001E >> >