On 13 June 2018 at 01:03, Leif Lindholm <leif.lindh...@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.biesheu...@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
