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

 * 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);
+
+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

(+ 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)

>  * 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);
> +
> +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

Hi Ard,

2018-04-16 7:40 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> (+ 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 added NULL class lib hook for VariableRuntimeDxe and it still works.
I'll send third patch on top of already submitted (the branch is
updated as well).

Thanks,
Marcin


>
>>  * 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);
>> +
>> +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

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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Laszlo,

2018-04-16 21:41 GMT+02:00 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:
>
>
> 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".
>

Right, I'll correct it.

> 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.
>

We had some discussions last year - I followed the coding standards:

Function (
  Argument1,
  Argument2,
  Argument3
  );

But was requested to place Argument1 to the function line and the last
bracket to Argument3 line:

Function (Argument1,
  Argument2,
  Argument3);

Afair, there were some attempts to modify coding standards at the
time, but I see the original version persisted. In fact I can do
whatever line-breaking necessary:

Ard - what style do you prefer in future patches?

Best regards,
Marcin

> 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
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Laszlo,
>
> 2018-04-16 21:41 GMT+02:00 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:
>>
>>
>> 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".
>>
>
> Right, I'll correct it.
>

Ah, I missed that. Thanks for spotting that Laszlo

>> 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.
>>
>
> We had some discussions last year - I followed the coding standards:
>
> Function (
>   Argument1,
>   Argument2,
>   Argument3
>   );
>
> But was requested to place Argument1 to the function line and the last
> bracket to Argument3 line:
>
> Function (Argument1,
>   Argument2,
>   Argument3);
>
> Afair, there were some attempts to modify coding standards at the
> time, but I see the original version persisted. In fact I can do
> whatever line-breaking necessary:
>
> Ard - what style do you prefer in future patches?
>

I tend to treat the coding style document as a guideline rather than
rule of law, given that it is not entirely consistent with current
practice to begin with. In my opinion, self consistency and legibility
are more important than adhering to some rule, although I realize
legibility is a subjective quality

Personally, I think the former takes up too much space in general, but
with complex expressions as arguments, it is more readable than the
latter.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Ard,

2018-04-17 7:32 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
>> Hi Laszlo,
>>
>> 2018-04-16 21:41 GMT+02:00 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:
>>>
>>>
>>> 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".
>>>
>>
>> Right, I'll correct it.
>>
>
> Ah, I missed that. Thanks for spotting that Laszlo
>
>>> 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.
>>>
>>
>> We had some discussions last year - I followed the coding standards:
>>
>> Function (
>>   Argument1,
>>   Argument2,
>>   Argument3
>>   );
>>
>> But was requested to place Argument1 to the function line and the last
>> bracket to Argument3 line:
>>
>> Function (Argument1,
>>   Argument2,
>>   Argument3);
>>
>> Afair, there were some attempts to modify coding standards at the
>> time, but I see the original version persisted. In fact I can do
>> whatever line-breaking necessary:
>>
>> Ard - what style do you prefer in future patches?
>>
>
> I tend to treat the coding style document as a guideline rather than
> rule of law, given that it is not entirely consistent with current
> practice to begin with. In my opinion, self consistency and legibility
> are more important than adhering to some rule, although I realize
> legibility is a subjective quality
>
> Personally, I think the former takes up too much space in general, but
> with complex expressions as arguments, it is more readable than the
> latter.

Well, I see it can be treated sort of as a matter of taste. In order
to be consistent with my previous code, I'd keep variant2 as the line
breaking scheme in edk2-platforms. In edk2 I don't suspect much code
submitted, but to be on a safe side I will use variant1. Is it ok for
you?

Thanks,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 17 April 2018 at 08:04, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Ard,
>
> 2018-04-17 7:32 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>> On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
>>> Hi Laszlo,
>>>
>>> 2018-04-16 21:41 GMT+02:00 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:
>>>>
>>>>
>>>> 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".
>>>>
>>>
>>> Right, I'll correct it.
>>>
>>
>> Ah, I missed that. Thanks for spotting that Laszlo
>>
>>>> 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.
>>>>
>>>
>>> We had some discussions last year - I followed the coding standards:
>>>
>>> Function (
>>>   Argument1,
>>>   Argument2,
>>>   Argument3
>>>   );
>>>
>>> But was requested to place Argument1 to the function line and the last
>>> bracket to Argument3 line:
>>>
>>> Function (Argument1,
>>>   Argument2,
>>>   Argument3);
>>>
>>> Afair, there were some attempts to modify coding standards at the
>>> time, but I see the original version persisted. In fact I can do
>>> whatever line-breaking necessary:
>>>
>>> Ard - what style do you prefer in future patches?
>>>
>>
>> I tend to treat the coding style document as a guideline rather than
>> rule of law, given that it is not entirely consistent with current
>> practice to begin with. In my opinion, self consistency and legibility
>> are more important than adhering to some rule, although I realize
>> legibility is a subjective quality
>>
>> Personally, I think the former takes up too much space in general, but
>> with complex expressions as arguments, it is more readable than the
>> latter.
>
> Well, I see it can be treated sort of as a matter of taste. In order
> to be consistent with my previous code, I'd keep variant2 as the line
> breaking scheme in edk2-platforms. In edk2 I don't suspect much code
> submitted, but to be on a safe side I will use variant1. Is it ok for
> you?
>

Works for me
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 04/17/18 07:15, Marcin Wojtas wrote:
> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:

>> 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.
>>
> 
> We had some discussions last year - I followed the coding standards:
> 
> Function (
>   Argument1,
>   Argument2,
>   Argument3
>   );

Right, this is option #1.

> But was requested to place Argument1 to the function line and the last
> bracket to Argument3 line:
> 
> Function (Argument1,
>   Argument2,
>   Argument3);

In this case I prefer compressing the arguments as much as possible, for
example

  Status = ProtocolInstance->LongMemberFunctionName (Argument1,
                               Argument2, Argument3, Argument4,
                               Argument5, Argument6);

So either break the first argument and the closing paren as well to new
lines, or else keep all arguments compressed -- break them to new lines
only when the max line width dictates so.

No need to resubmit the patch because of this, I just wanted to clarify
my remark.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel