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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.