[PATCH edk2-platforms 0/2] DeveloperBox: prepare for expanding the capsule payload

[PATCH edk2-platforms 0/2] DeveloperBox: prepare for expanding the capsule payload

[Ard Biesheuvel]

We intend to start distributing firmware update capsules that include the SCP
firmware partition. In order to allow for more flexibility regarding whether
a capsule contains this piece or not, update the flash access routines and
the flash partition descriptions so we can update any part of the first
4 MB, consisting of the CM3 bootstrap code, emergency flasher, pseudo
EEPROM, SCP firmware, ARM-TF and UEFI.

Note that this means we can drop the 'TEMPORARY' patch I proposed a while
ago to hack around this limitation.

Ard Biesheuvel (2):
  Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check
  Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

 .../NorFlashSynQuacerLib/NorFlashSynQuacer.c  |  5 +-
 .../SynQuacerPlatformFlashAccessLib.c         | 53 +++++++++----------
 2 files changed, 28 insertions(+), 30 deletions(-)

-- 
2.17.0

[PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check

[Ard Biesheuvel]

In commit 913fdda9f4b9 ("Silicon/SynQuacerPlatformFlashAccessLib: don't
dereference FVB header fields"), we dropped all accesses to FVB header
field, which was necessary because the flash partition may not in fact
contain such a header. Instead, only an exact match on the base address
of the FV compared to the base address of the capsule payload would
result in a match, making it difficult to create capsules that only
update a subset of the flash contents.

Given that the FVB protocol provides a GetBlockSize() method that also
returns the number of consecutive blocks of that size, and does not rely
on the FVB header contents, we can actually infer the size of the flash
partition, and use it to decide whether a capsule payload targets an
area that is covered by this partition entirely.

This optimization allows us to extend the FV description to include the
SCP firmware partition without requiring us to actually provide a
payload for that partition immediately, which is useful as a preparatory
step.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c | 53 +++++++++-----------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
index ebb6ce189aa5..a6843c949a28 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
@@ -44,8 +44,10 @@ STATIC
 EFI_STATUS
 GetFvbByAddress (
   IN  EFI_PHYSICAL_ADDRESS                Address,
+  IN  UINTN                               Length,
   OUT EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  **OutFvb,
-  OUT EFI_PHYSICAL_ADDRESS                *FvbBaseAddress
+  OUT EFI_LBA                             *Lba,
+  OUT UINTN                               *BlockSize
   )
 {
   EFI_STATUS                          Status;
@@ -54,6 +56,8 @@ GetFvbByAddress (
   UINTN                               Index;
   EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
   EFI_FVB_ATTRIBUTES_2                Attributes;
+  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
+  UINTN                               NumberOfBlocks;
 
   //
   // Locate all handles with Firmware Volume Block protocol
@@ -84,7 +88,7 @@ GetFvbByAddress (
     //
     // Checks if the address range of this handle contains parameter Address
     //
-    Status = Fvb->GetPhysicalAddress (Fvb, FvbBaseAddress);
+    Status = Fvb->GetPhysicalAddress (Fvb, &FvbBaseAddress);
     if (EFI_ERROR (Status)) {
       continue;
     }
@@ -102,8 +106,25 @@ GetFvbByAddress (
       continue;
     }
 
-    if (Address == *FvbBaseAddress) {
+    Status = Fvb->GetBlockSize (Fvb, 0, BlockSize, &NumberOfBlocks);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_INFO, "%a: failed to get FVB blocksize - %r, ignoring\n",
+        __FUNCTION__, Status));
+      continue;
+    }
+
+    if ((Length % *BlockSize) != 0) {
+      DEBUG ((DEBUG_INFO,
+        "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx, ignoring\n",
+        __FUNCTION__, Length, *BlockSize));
+      Status = EFI_INVALID_PARAMETER;
+      continue;
+    }
+
+    if (Address >= FvbBaseAddress &&
+        (Address + Length) <= (FvbBaseAddress + *BlockSize * NumberOfBlocks)) {
       *OutFvb  = Fvb;
+      *Lba = (Address - FvbBaseAddress) / *BlockSize;
       Status   = EFI_SUCCESS;
       break;
     }
@@ -190,9 +211,7 @@ PerformFlashWriteWithProgress (
   EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
   EFI_STATUS                          Status;
   UINTN                               BlockSize;
-  UINTN                               NumberOfBlocks;
   EFI_LBA                             Lba;
-  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
   UINTN                               NumBytes;
   UINTN                               Remaining;
 
@@ -216,7 +235,7 @@ PerformFlashWriteWithProgress (
   // that covers the system firmware
   //
   Fvb = NULL;
-  Status = GetFvbByAddress (FlashAddress, &Fvb, &FvbBaseAddress);
+  Status = GetFvbByAddress (FlashAddress, Length, &Fvb, &Lba, &BlockSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
       "%a: failed to locate FVB handle for address 0x%llx - %r\n",
@@ -224,28 +243,6 @@ PerformFlashWriteWithProgress (
     return Status;
   }
 
-  Status = Fvb->GetBlockSize(Fvb, 0, &BlockSize, &NumberOfBlocks);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to get FVB blocksize - %r\n",
-      __FUNCTION__, Status));
-    return Status;
-  }
-
-  if ((Length % BlockSize) != 0) {
-    DEBUG ((DEBUG_ERROR,
-      "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx\n",
-      __FUNCTION__, Length, BlockSize));
-    return EFI_INVALID_PARAMETER;
-  }
-
-  Lba = (FlashAddress - FvbBaseAddress) / BlockSize;
-  if (Lba > NumberOfBlocks - 1) {
-    DEBUG ((DEBUG_ERROR,
-      "%a: flash device with non-uniform blocksize not supported\n",
-      __FUNCTION__));
-    return EFI_UNSUPPORTED;
-  }
-
   //
   // Remap the region as device rather than uncached.
   //
-- 
2.17.0

[PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Ard Biesheuvel]

In order to allow for more flexibility when updating parts of the
firmware via capsule update, expand the description of the code FV
to cover the entire 4 MB region at the base of the NOR flash.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
index 816d8ba33f8c..357082c3d903 100644
--- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
+++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
@@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
   {
     // UEFI code region
     SYNQUACER_SPI_NOR_BASE,                             // device base
-    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
-    FixedPcdGet32 (PcdFdSize),                          // region size
+    SYNQUACER_SPI_NOR_BASE,                             // region base
+    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
+    SYNQUACER_SPI_NOR_BASE,                             // region size
     SIZE_64KB,                                          // block size
     {
       0x19c118b0, 0xc423, 0x42be, { 0xb8, 0x0f, 0x70, 0x6f, 0x1f, 0xcb, 0x59, 0x9a }
-- 
2.17.0

Re: [PATCH edk2-platforms 0/2] DeveloperBox: prepare for expanding the capsule payload

[Ard Biesheuvel]

On 7 June 2018 at 17:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> We intend to start distributing firmware update capsules that include the SCP
> firmware partition. In order to allow for more flexibility regarding whether
> a capsule contains this piece or not, update the flash access routines and
> the flash partition descriptions so we can update any part of the first
> 4 MB, consisting of the CM3 bootstrap code, emergency flasher, pseudo
> EEPROM, SCP firmware, ARM-TF and UEFI.
>
> Note that this means we can drop the 'TEMPORARY' patch I proposed a while
> ago to hack around this limitation.
>

Additional note: this approach allows you to downgrade to older
capsules as well, although
a) you will keep the newer versions of the pieces that the older
capsule does not cover
b) upgrading to the latest version will require you to go via an
intermediate version that includes these changes.

> Ard Biesheuvel (2):
>   Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check
>   Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV
>
>  .../NorFlashSynQuacerLib/NorFlashSynQuacer.c  |  5 +-
>  .../SynQuacerPlatformFlashAccessLib.c         | 53 +++++++++----------
>  2 files changed, 28 insertions(+), 30 deletions(-)
>
> --
> 2.17.0
>

Re: [PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Leif Lindholm]

On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
> In order to allow for more flexibility when updating parts of the
> firmware via capsule update, expand the description of the code FV
> to cover the entire 4 MB region at the base of the NOR flash.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> index 816d8ba33f8c..357082c3d903 100644
> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
>    {
>      // UEFI code region
>      SYNQUACER_SPI_NOR_BASE,                             // device base
> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
> -    FixedPcdGet32 (PcdFdSize),                          // region size
> +    SYNQUACER_SPI_NOR_BASE,                             // region base
> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
> +    SYNQUACER_SPI_NOR_BASE,                             // region size

Could you define the size as a macro in Platform/MemoryMap.h?

/
    Leif

>      SIZE_64KB,                                          // block size
>      {
>        0x19c118b0, 0xc423, 0x42be, { 0xb8, 0x0f, 0x70, 0x6f, 0x1f, 0xcb, 0x59, 0x9a }
> -- 
> 2.17.0
> 

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check

[Leif Lindholm]

On Thu, Jun 07, 2018 at 05:08:17PM +0200, Ard Biesheuvel wrote:
> In commit 913fdda9f4b9 ("Silicon/SynQuacerPlatformFlashAccessLib: don't
> dereference FVB header fields"), we dropped all accesses to FVB header
> field, which was necessary because the flash partition may not in fact
> contain such a header. Instead, only an exact match on the base address
> of the FV compared to the base address of the capsule payload would
> result in a match, making it difficult to create capsules that only
> update a subset of the flash contents.
> 
> Given that the FVB protocol provides a GetBlockSize() method that also
> returns the number of consecutive blocks of that size, and does not rely
> on the FVB header contents, we can actually infer the size of the flash
> partition, and use it to decide whether a capsule payload targets an
> area that is covered by this partition entirely.
> 
> This optimization allows us to extend the FV description to include the
> SCP firmware partition without requiring us to actually provide a
> payload for that partition immediately, which is useful as a preparatory
> step.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c | 53 +++++++++-----------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> index ebb6ce189aa5..a6843c949a28 100644
> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> @@ -44,8 +44,10 @@ STATIC
>  EFI_STATUS
>  GetFvbByAddress (
>    IN  EFI_PHYSICAL_ADDRESS                Address,
> +  IN  UINTN                               Length,
>    OUT EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  **OutFvb,
> -  OUT EFI_PHYSICAL_ADDRESS                *FvbBaseAddress
> +  OUT EFI_LBA                             *Lba,
> +  OUT UINTN                               *BlockSize
>    )
>  {
>    EFI_STATUS                          Status;
> @@ -54,6 +56,8 @@ GetFvbByAddress (
>    UINTN                               Index;
>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
>    EFI_FVB_ATTRIBUTES_2                Attributes;
> +  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
> +  UINTN                               NumberOfBlocks;
>  
>    //
>    // Locate all handles with Firmware Volume Block protocol
> @@ -84,7 +88,7 @@ GetFvbByAddress (
>      //
>      // Checks if the address range of this handle contains parameter Address
>      //
> -    Status = Fvb->GetPhysicalAddress (Fvb, FvbBaseAddress);
> +    Status = Fvb->GetPhysicalAddress (Fvb, &FvbBaseAddress);
>      if (EFI_ERROR (Status)) {
>        continue;
>      }
> @@ -102,8 +106,25 @@ GetFvbByAddress (
>        continue;
>      }
>  
> -    if (Address == *FvbBaseAddress) {
> +    Status = Fvb->GetBlockSize (Fvb, 0, BlockSize, &NumberOfBlocks);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "%a: failed to get FVB blocksize - %r, ignoring\n",
> +        __FUNCTION__, Status));
> +      continue;
> +    }
> +
> +    if ((Length % *BlockSize) != 0) {
> +      DEBUG ((DEBUG_INFO,
> +        "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx, ignoring\n",
> +        __FUNCTION__, Length, *BlockSize));
> +      Status = EFI_INVALID_PARAMETER;
> +      continue;
> +    }
> +
> +    if (Address >= FvbBaseAddress &&
> +        (Address + Length) <= (FvbBaseAddress + *BlockSize * NumberOfBlocks)) {

As I've already been giving Marcin a hard time about this today, could you add
parentheses around "Address >= FvbBaseAddress" and "*BlockSize * NumberOfBlocks"?

/
    Leif

>        *OutFvb  = Fvb;
> +      *Lba = (Address - FvbBaseAddress) / *BlockSize;
>        Status   = EFI_SUCCESS;
>        break;
>      }
> @@ -190,9 +211,7 @@ PerformFlashWriteWithProgress (
>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
>    EFI_STATUS                          Status;
>    UINTN                               BlockSize;
> -  UINTN                               NumberOfBlocks;
>    EFI_LBA                             Lba;
> -  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
>    UINTN                               NumBytes;
>    UINTN                               Remaining;
>  
> @@ -216,7 +235,7 @@ PerformFlashWriteWithProgress (
>    // that covers the system firmware
>    //
>    Fvb = NULL;
> -  Status = GetFvbByAddress (FlashAddress, &Fvb, &FvbBaseAddress);
> +  Status = GetFvbByAddress (FlashAddress, Length, &Fvb, &Lba, &BlockSize);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a: failed to locate FVB handle for address 0x%llx - %r\n",
> @@ -224,28 +243,6 @@ PerformFlashWriteWithProgress (
>      return Status;
>    }
>  
> -  Status = Fvb->GetBlockSize(Fvb, 0, &BlockSize, &NumberOfBlocks);
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_ERROR, "%a: failed to get FVB blocksize - %r\n",
> -      __FUNCTION__, Status));
> -    return Status;
> -  }
> -
> -  if ((Length % BlockSize) != 0) {
> -    DEBUG ((DEBUG_ERROR,
> -      "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx\n",
> -      __FUNCTION__, Length, BlockSize));
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  Lba = (FlashAddress - FvbBaseAddress) / BlockSize;
> -  if (Lba > NumberOfBlocks - 1) {
> -    DEBUG ((DEBUG_ERROR,
> -      "%a: flash device with non-uniform blocksize not supported\n",
> -      __FUNCTION__));
> -    return EFI_UNSUPPORTED;
> -  }
> -
>    //
>    // Remap the region as device rather than uncached.
>    //
> -- 
> 2.17.0
> 

Re: [PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Ard Biesheuvel]

On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
>> In order to allow for more flexibility when updating parts of the
>> firmware via capsule update, expand the description of the code FV
>> to cover the entire 4 MB region at the base of the NOR flash.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> index 816d8ba33f8c..357082c3d903 100644
>> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
>>    {
>>      // UEFI code region
>>      SYNQUACER_SPI_NOR_BASE,                             // device base
>> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
>> -    FixedPcdGet32 (PcdFdSize),                          // region size
>> +    SYNQUACER_SPI_NOR_BASE,                             // region base
>> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
>> +    SYNQUACER_SPI_NOR_BASE,                             // region size
>
> Could you define the size as a macro in Platform/MemoryMap.h?
>

The memory map currently only contains constant macros. I can add this
expression

FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE

somewhere as a #define but I would prefer it to be elsewhere, given
that it is not a SoC constant set in stone.

Re: [PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Leif Lindholm]

On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
> >> In order to allow for more flexibility when updating parts of the
> >> firmware via capsule update, expand the description of the code FV
> >> to cover the entire 4 MB region at the base of the NOR flash.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> index 816d8ba33f8c..357082c3d903 100644
> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
> >>    {
> >>      // UEFI code region
> >>      SYNQUACER_SPI_NOR_BASE,                             // device base
> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
> >> -    FixedPcdGet32 (PcdFdSize),                          // region size
> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base
> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size
> >
> > Could you define the size as a macro in Platform/MemoryMap.h?
> >
> 
> The memory map currently only contains constant macros. I can add this
> expression
> 
> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE
> 
> somewhere as a #define but I would prefer it to be elsewhere, given
> that it is not a SoC constant set in stone.

I'm OK with that, but will just throw in the argument that the fact
that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind
of set in stone. (And doesn't the Fixed bit make it constant?)

Either way, my main interest is in making this struct definition not
break my reading flow, so I'm perfectly happy with the #define
elsewhere.

/
    Leif

Re: [PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Ard Biesheuvel]

On 13 June 2018 at 12:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:
>> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
>> >> In order to allow for more flexibility when updating parts of the
>> >> firmware via capsule update, expand the description of the code FV
>> >> to cover the entire 4 MB region at the base of the NOR flash.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
>> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> >> index 816d8ba33f8c..357082c3d903 100644
>> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
>> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
>> >>    {
>> >>      // UEFI code region
>> >>      SYNQUACER_SPI_NOR_BASE,                             // device base
>> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
>> >> -    FixedPcdGet32 (PcdFdSize),                          // region size
>> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base
>> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
>> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size
>> >
>> > Could you define the size as a macro in Platform/MemoryMap.h?
>> >
>>
>> The memory map currently only contains constant macros. I can add this
>> expression
>>
>> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE
>>
>> somewhere as a #define but I would prefer it to be elsewhere, given
>> that it is not a SoC constant set in stone.
>
> I'm OK with that, but will just throw in the argument that the fact
> that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind
> of set in stone. (And doesn't the Fixed bit make it constant?)
>

The point is more that MemoryMap.h describes properties of the SoC not
properties of our firmware implementation

But that also means that I should introduce a symbolic constant for
the start of the partition, e.g.,

#define FW_CODE_REGION_BASE    SYNQUACER_SPI_NOR_BASE
#define FW_CODE_REGION_SIZE    (FixedPcdGet32 (PcdFlashNvStorageVariableBase)
                               - SYNQUACER_SPI_NOR_BASE)

but I'd still prefer to keep it local to this file.

> Either way, my main interest is in making this struct definition not
> break my reading flow, so I'm perfectly happy with the #define
> elsewhere.
>
> /
>     Leif

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check

[Ard Biesheuvel]

On 13 June 2018 at 01:03, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jun 07, 2018 at 05:08:17PM +0200, Ard Biesheuvel wrote:
>> In commit 913fdda9f4b9 ("Silicon/SynQuacerPlatformFlashAccessLib: don't
>> dereference FVB header fields"), we dropped all accesses to FVB header
>> field, which was necessary because the flash partition may not in fact
>> contain such a header. Instead, only an exact match on the base address
>> of the FV compared to the base address of the capsule payload would
>> result in a match, making it difficult to create capsules that only
>> update a subset of the flash contents.
>>
>> Given that the FVB protocol provides a GetBlockSize() method that also
>> returns the number of consecutive blocks of that size, and does not rely
>> on the FVB header contents, we can actually infer the size of the flash
>> partition, and use it to decide whether a capsule payload targets an
>> area that is covered by this partition entirely.
>>
>> This optimization allows us to extend the FV description to include the
>> SCP firmware partition without requiring us to actually provide a
>> payload for that partition immediately, which is useful as a preparatory
>> step.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c | 53 +++++++++-----------
>>  1 file changed, 25 insertions(+), 28 deletions(-)
>>
>> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
>> index ebb6ce189aa5..a6843c949a28 100644
>> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
>> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
>> @@ -44,8 +44,10 @@ STATIC
>>  EFI_STATUS
>>  GetFvbByAddress (
>>    IN  EFI_PHYSICAL_ADDRESS                Address,
>> +  IN  UINTN                               Length,
>>    OUT EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  **OutFvb,
>> -  OUT EFI_PHYSICAL_ADDRESS                *FvbBaseAddress
>> +  OUT EFI_LBA                             *Lba,
>> +  OUT UINTN                               *BlockSize
>>    )
>>  {
>>    EFI_STATUS                          Status;
>> @@ -54,6 +56,8 @@ GetFvbByAddress (
>>    UINTN                               Index;
>>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
>>    EFI_FVB_ATTRIBUTES_2                Attributes;
>> +  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
>> +  UINTN                               NumberOfBlocks;
>>
>>    //
>>    // Locate all handles with Firmware Volume Block protocol
>> @@ -84,7 +88,7 @@ GetFvbByAddress (
>>      //
>>      // Checks if the address range of this handle contains parameter Address
>>      //
>> -    Status = Fvb->GetPhysicalAddress (Fvb, FvbBaseAddress);
>> +    Status = Fvb->GetPhysicalAddress (Fvb, &FvbBaseAddress);
>>      if (EFI_ERROR (Status)) {
>>        continue;
>>      }
>> @@ -102,8 +106,25 @@ GetFvbByAddress (
>>        continue;
>>      }
>>
>> -    if (Address == *FvbBaseAddress) {
>> +    Status = Fvb->GetBlockSize (Fvb, 0, BlockSize, &NumberOfBlocks);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_INFO, "%a: failed to get FVB blocksize - %r, ignoring\n",
>> +        __FUNCTION__, Status));
>> +      continue;
>> +    }
>> +
>> +    if ((Length % *BlockSize) != 0) {
>> +      DEBUG ((DEBUG_INFO,
>> +        "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx, ignoring\n",
>> +        __FUNCTION__, Length, *BlockSize));
>> +      Status = EFI_INVALID_PARAMETER;
>> +      continue;
>> +    }
>> +
>> +    if (Address >= FvbBaseAddress &&
>> +        (Address + Length) <= (FvbBaseAddress + *BlockSize * NumberOfBlocks)) {
>
> As I've already been giving Marcin a hard time about this today, could you add
> parentheses around "Address >= FvbBaseAddress" and "*BlockSize * NumberOfBlocks"?
>

Sure. But to be consistent, shouldn't I also add parens around the
entire term after the && then?

I.e.,

+    if ((Address >= FvbBaseAddress) &&
+        ((Address + Length) <= (FvbBaseAddress + (*BlockSize *
NumberOfBlocks)))) {

I'm not sure why we like redundant parentheses so much in Tianocore,
but I don't particularly mind either.



>>        *OutFvb  = Fvb;
>> +      *Lba = (Address - FvbBaseAddress) / *BlockSize;
>>        Status   = EFI_SUCCESS;
>>        break;
>>      }
>> @@ -190,9 +211,7 @@ PerformFlashWriteWithProgress (
>>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
>>    EFI_STATUS                          Status;
>>    UINTN                               BlockSize;
>> -  UINTN                               NumberOfBlocks;
>>    EFI_LBA                             Lba;
>> -  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
>>    UINTN                               NumBytes;
>>    UINTN                               Remaining;
>>
>> @@ -216,7 +235,7 @@ PerformFlashWriteWithProgress (
>>    // that covers the system firmware
>>    //
>>    Fvb = NULL;
>> -  Status = GetFvbByAddress (FlashAddress, &Fvb, &FvbBaseAddress);
>> +  Status = GetFvbByAddress (FlashAddress, Length, &Fvb, &Lba, &BlockSize);
>>    if (EFI_ERROR (Status)) {
>>      DEBUG ((DEBUG_ERROR,
>>        "%a: failed to locate FVB handle for address 0x%llx - %r\n",
>> @@ -224,28 +243,6 @@ PerformFlashWriteWithProgress (
>>      return Status;
>>    }
>>
>> -  Status = Fvb->GetBlockSize(Fvb, 0, &BlockSize, &NumberOfBlocks);
>> -  if (EFI_ERROR (Status)) {
>> -    DEBUG ((DEBUG_ERROR, "%a: failed to get FVB blocksize - %r\n",
>> -      __FUNCTION__, Status));
>> -    return Status;
>> -  }
>> -
>> -  if ((Length % BlockSize) != 0) {
>> -    DEBUG ((DEBUG_ERROR,
>> -      "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx\n",
>> -      __FUNCTION__, Length, BlockSize));
>> -    return EFI_INVALID_PARAMETER;
>> -  }
>> -
>> -  Lba = (FlashAddress - FvbBaseAddress) / BlockSize;
>> -  if (Lba > NumberOfBlocks - 1) {
>> -    DEBUG ((DEBUG_ERROR,
>> -      "%a: flash device with non-uniform blocksize not supported\n",
>> -      __FUNCTION__));
>> -    return EFI_UNSUPPORTED;
>> -  }
>> -
>>    //
>>    // Remap the region as device rather than uncached.
>>    //
>> --
>> 2.17.0
>>

Re: [PATCH edk2-platforms 2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

[Leif Lindholm]

On Wed, Jun 13, 2018 at 02:00:39PM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 12:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:
> >> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
> >> >> In order to allow for more flexibility when updating parts of the
> >> >> firmware via capsule update, expand the description of the code FV
> >> >> to cover the entire 4 MB region at the base of the NOR flash.
> >> >>
> >> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> ---
> >> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
> >> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> >> index 816d8ba33f8c..357082c3d903 100644
> >> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
> >> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
> >> >>    {
> >> >>      // UEFI code region
> >> >>      SYNQUACER_SPI_NOR_BASE,                             // device base
> >> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
> >> >> -    FixedPcdGet32 (PcdFdSize),                          // region size
> >> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base
> >> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
> >> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size
> >> >
> >> > Could you define the size as a macro in Platform/MemoryMap.h?
> >> >
> >>
> >> The memory map currently only contains constant macros. I can add this
> >> expression
> >>
> >> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE
> >>
> >> somewhere as a #define but I would prefer it to be elsewhere, given
> >> that it is not a SoC constant set in stone.
> >
> > I'm OK with that, but will just throw in the argument that the fact
> > that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind
> > of set in stone. (And doesn't the Fixed bit make it constant?)
> >
> 
> The point is more that MemoryMap.h describes properties of the SoC not
> properties of our firmware implementation
> 
> But that also means that I should introduce a symbolic constant for
> the start of the partition, e.g.,
> 
> #define FW_CODE_REGION_BASE    SYNQUACER_SPI_NOR_BASE
> #define FW_CODE_REGION_SIZE    (FixedPcdGet32 (PcdFlashNvStorageVariableBase)
>                                - SYNQUACER_SPI_NOR_BASE)
> 
> but I'd still prefer to keep it local to this file.

Yeah, that's fine.

/
    Leif

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check

[Leif Lindholm]

On Wed, Jun 13, 2018 at 02:06:00PM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 01:03, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Jun 07, 2018 at 05:08:17PM +0200, Ard Biesheuvel wrote:
> >> In commit 913fdda9f4b9 ("Silicon/SynQuacerPlatformFlashAccessLib: don't
> >> dereference FVB header fields"), we dropped all accesses to FVB header
> >> field, which was necessary because the flash partition may not in fact
> >> contain such a header. Instead, only an exact match on the base address
> >> of the FV compared to the base address of the capsule payload would
> >> result in a match, making it difficult to create capsules that only
> >> update a subset of the flash contents.
> >>
> >> Given that the FVB protocol provides a GetBlockSize() method that also
> >> returns the number of consecutive blocks of that size, and does not rely
> >> on the FVB header contents, we can actually infer the size of the flash
> >> partition, and use it to decide whether a capsule payload targets an
> >> area that is covered by this partition entirely.
> >>
> >> This optimization allows us to extend the FV description to include the
> >> SCP firmware partition without requiring us to actually provide a
> >> payload for that partition immediately, which is useful as a preparatory
> >> step.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c | 53 +++++++++-----------
> >>  1 file changed, 25 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> >> index ebb6ce189aa5..a6843c949a28 100644
> >> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
> >> @@ -44,8 +44,10 @@ STATIC
> >>  EFI_STATUS
> >>  GetFvbByAddress (
> >>    IN  EFI_PHYSICAL_ADDRESS                Address,
> >> +  IN  UINTN                               Length,
> >>    OUT EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  **OutFvb,
> >> -  OUT EFI_PHYSICAL_ADDRESS                *FvbBaseAddress
> >> +  OUT EFI_LBA                             *Lba,
> >> +  OUT UINTN                               *BlockSize
> >>    )
> >>  {
> >>    EFI_STATUS                          Status;
> >> @@ -54,6 +56,8 @@ GetFvbByAddress (
> >>    UINTN                               Index;
> >>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
> >>    EFI_FVB_ATTRIBUTES_2                Attributes;
> >> +  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
> >> +  UINTN                               NumberOfBlocks;
> >>
> >>    //
> >>    // Locate all handles with Firmware Volume Block protocol
> >> @@ -84,7 +88,7 @@ GetFvbByAddress (
> >>      //
> >>      // Checks if the address range of this handle contains parameter Address
> >>      //
> >> -    Status = Fvb->GetPhysicalAddress (Fvb, FvbBaseAddress);
> >> +    Status = Fvb->GetPhysicalAddress (Fvb, &FvbBaseAddress);
> >>      if (EFI_ERROR (Status)) {
> >>        continue;
> >>      }
> >> @@ -102,8 +106,25 @@ GetFvbByAddress (
> >>        continue;
> >>      }
> >>
> >> -    if (Address == *FvbBaseAddress) {
> >> +    Status = Fvb->GetBlockSize (Fvb, 0, BlockSize, &NumberOfBlocks);
> >> +    if (EFI_ERROR (Status)) {
> >> +      DEBUG ((DEBUG_INFO, "%a: failed to get FVB blocksize - %r, ignoring\n",
> >> +        __FUNCTION__, Status));
> >> +      continue;
> >> +    }
> >> +
> >> +    if ((Length % *BlockSize) != 0) {
> >> +      DEBUG ((DEBUG_INFO,
> >> +        "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx, ignoring\n",
> >> +        __FUNCTION__, Length, *BlockSize));
> >> +      Status = EFI_INVALID_PARAMETER;
> >> +      continue;
> >> +    }
> >> +
> >> +    if (Address >= FvbBaseAddress &&
> >> +        (Address + Length) <= (FvbBaseAddress + *BlockSize * NumberOfBlocks)) {
> >
> > As I've already been giving Marcin a hard time about this today, could you add
> > parentheses around "Address >= FvbBaseAddress" and "*BlockSize * NumberOfBlocks"?
> >
> 
> Sure. But to be consistent, shouldn't I also add parens around the
> entire term after the && then?

Strictly speaking, yes.
The line break helps to demonstrate the separateness.

> I.e.,
> 
> +    if ((Address >= FvbBaseAddress) &&
> +        ((Address + Length) <= (FvbBaseAddress + (*BlockSize *
> NumberOfBlocks)))) {
> 
> I'm not sure why we like redundant parentheses so much in Tianocore,
> but I don't particularly mind either.

Personally, it's because I neither have nor want the evaluation order
of C expressions hard-wired into my visual cortex.

I usually give * and / vs + and - the slip because I've failed to keep
those out.

/
    Leif

> >>        *OutFvb  = Fvb;
> >> +      *Lba = (Address - FvbBaseAddress) / *BlockSize;
> >>        Status   = EFI_SUCCESS;
> >>        break;
> >>      }
> >> @@ -190,9 +211,7 @@ PerformFlashWriteWithProgress (
> >>    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  *Fvb;
> >>    EFI_STATUS                          Status;
> >>    UINTN                               BlockSize;
> >> -  UINTN                               NumberOfBlocks;
> >>    EFI_LBA                             Lba;
> >> -  EFI_PHYSICAL_ADDRESS                FvbBaseAddress;
> >>    UINTN                               NumBytes;
> >>    UINTN                               Remaining;
> >>
> >> @@ -216,7 +235,7 @@ PerformFlashWriteWithProgress (
> >>    // that covers the system firmware
> >>    //
> >>    Fvb = NULL;
> >> -  Status = GetFvbByAddress (FlashAddress, &Fvb, &FvbBaseAddress);
> >> +  Status = GetFvbByAddress (FlashAddress, Length, &Fvb, &Lba, &BlockSize);
> >>    if (EFI_ERROR (Status)) {
> >>      DEBUG ((DEBUG_ERROR,
> >>        "%a: failed to locate FVB handle for address 0x%llx - %r\n",
> >> @@ -224,28 +243,6 @@ PerformFlashWriteWithProgress (
> >>      return Status;
> >>    }
> >>
> >> -  Status = Fvb->GetBlockSize(Fvb, 0, &BlockSize, &NumberOfBlocks);
> >> -  if (EFI_ERROR (Status)) {
> >> -    DEBUG ((DEBUG_ERROR, "%a: failed to get FVB blocksize - %r\n",
> >> -      __FUNCTION__, Status));
> >> -    return Status;
> >> -  }
> >> -
> >> -  if ((Length % BlockSize) != 0) {
> >> -    DEBUG ((DEBUG_ERROR,
> >> -      "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx\n",
> >> -      __FUNCTION__, Length, BlockSize));
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> -
> >> -  Lba = (FlashAddress - FvbBaseAddress) / BlockSize;
> >> -  if (Lba > NumberOfBlocks - 1) {
> >> -    DEBUG ((DEBUG_ERROR,
> >> -      "%a: flash device with non-uniform blocksize not supported\n",
> >> -      __FUNCTION__));
> >> -    return EFI_UNSUPPORTED;
> >> -  }
> >> -
> >>    //
> >>    // Remap the region as device rather than uncached.
> >>    //
> >> --
> >> 2.17.0
> >>