Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Laszlo Ersek <lersek@redhat.com>]

On 04/16/18 07:40, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>> Recent changes in the EDK2 mainline resulted in breaking
>> of compilation and booting of Armada platforms.
>> This patch adjust the MvFvbDxe driver by:
>>
>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>
> 
> Hello Marcin,
> 
> Installing this GUID is only necessary if you update your platform
> .DSC to make the generic variable runtime driver depend on it by
> adding a NULL library class resolution using NvVarStoreFormattedLib.
> So I think this patch is correct, but you'll need an additional change
> to make it work as expected. (Otherwise, the variable runtime driver
> could still be dispatched early and invoked for read access before the
> variable store is reformatted)

I agree.

I'd also like to point out another frequent pitfall in this patch:

>>  * making explicit dependency to ArmPkg/Drivers/CpuDxe drivers in order
>>    to enable successful calling of gDS->SetMemorySpaceAttributes
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 21 ++++++++++++++++++++
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  7 ++-----
>>  2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> index 252ef67..6e583a3 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> @@ -26,6 +26,7 @@
>>  #include <Library/UefiLib.h>
>>  #include <Library/UefiRuntimeLib.h>
>>
>> +#include <Guid/NvVarStoreFormatted.h>
>>  #include <Guid/SystemNvDataGuid.h>
>>  #include <Guid/VariableFormat.h>
>>
>> @@ -1076,6 +1077,21 @@ MvFvbEntryPoint (
>>    }
>>
>>    //
>> +  // The driver implementing the variable read service can now be dispatched;
>> +  // the varstore headers are in place.
>> +  //
>> +  Status = gBS->InstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Failed to install gEdkiiNvVarStoreFormattedGuid\n",
>> +      __FUNCTION__));
>> +    goto ErrorInstallNvVarStoreFormatted;
>> +  }
>> +
>> +  //
>>    // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
>>    //
>>    RuntimeMmioRegionSize = mFvbDevice->FvbSize;
>> @@ -1126,6 +1142,11 @@ ErrorSetMemAttr:
>>    gDS->RemoveMemorySpace (RegionBaseAddress, RuntimeMmioRegionSize);
>>
>>  ErrorAddSpace:
>> +  gBS->UninstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  NULL);
>> +

While gBS->InstallProtocolInterface() takes a *pointer* to a handle
(because it can *create* a handle, if the handle is NULL on input, and
the first protocol interface is installed on it),
gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
protocol interface is uninstalled from the handle, then the handle is
destroyed, but gBS->UninstallProtocolInterface() does not attempt to
NULL the handle itself. So, here you should pass "gImageHandle", not
"&gImageHandle".

There's also a bit of whitespace mangling here that's not compatible
with either multiline function call style that we like in edk2, but
perhaps edk2-platforms treats that more laxly.

Thanks
Laszlo

>> +ErrorInstallNvVarStoreFormatted:
>>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>>           &gEfiDevicePathProtocolGuid,
>>           &gEfiFirmwareVolumeBlockProtocolGuid,
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> index 117fe8b..fd3f2f7 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> @@ -63,6 +63,7 @@
>>    UefiRuntimeServicesTableLib
>>
>>  [Guids]
>> +  gEdkiiNvVarStoreFormattedGuid
>>    gEfiAuthenticatedVariableGuid
>>    gEfiEventVirtualAddressChangeGuid
>>    gEfiSystemNvDataFvGuid
>> @@ -84,8 +85,4 @@
>>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>>
>>  [Depex]
>> -  #
>> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
>> -  # flash needs populating with default values.
>> -  #
>> -  BEFORE gVariableRuntimeDxeFileGuid
>> +  gEfiCpuArchProtocolGuid
>> --
>> 2.7.4
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>