Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 1/4/21 7:24 PM, Pete Batard wrote:
> Hi Ard,
> 
> Thanks for pushing the patch. But I think it will need some amending
> indeed, as per comments below:
> 

Hmm, I did not intend to merge it, but apparently I did ...

Please follow up with a fix that addresses the crash, thanks.

-- 
Ard.

> 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 <pete@akeo.ie>
>>> ---
>>>  
>>> 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 <Library/BaseLib.h>
>>>   #include <Library/DebugLib.h>
>>> +#include <Library/TimerLib.h>
>>>   #include <Library/EfiResetSystemLib.h>
>>>   #include <Library/ArmSmcLib.h>
>>>   #include <Library/UefiLib.h>
>>> @@ -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
>>>
>>
>