Re: [PATCH] EmbeddedPkg/AndroidFastbootApp: support sparse image

[Haojian Zhuang <haojian.zhuang@linaro.org>]

On 2017/5/4 18:48, Leif Lindholm wrote:
> On Fri, Apr 14, 2017 at 05:55:32PM +0800, Haojian Zhuang wrote:
>> Support sparse image that is reformatted from large raw image and
>> splitted into a few smaller images. The sparse image follows the
>> rule of Android Sparse Image format.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  .../AndroidFastboot/AndroidFastbootApp.c           | 127 +++++++++++++++++++--
>>  .../Include/Protocol/AndroidFastbootPlatform.h     |  23 ++++
>>  2 files changed, 142 insertions(+), 8 deletions(-)
>>
>> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
>> index 61abc67..4883a77 100644
>> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
>> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
>> @@ -25,7 +25,34 @@
>>  #include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/UefiRuntimeServicesTableLib.h>
>>
>> -#define ANDROID_FASTBOOT_VERSION "0.4"
>> +#define ANDROID_FASTBOOT_VERSION "0.5"
>
> Could this #define, as well as the ones below, and the typedefs, and
> the macros, all be moved into AndroidFastbootApp.h?
>
> Actually, I'm confused. In current upstream EDK2, this #define _is_ in
> AndroidFastbootApp.h. What is this patch against?
>

Maybe the EDK2 that I'm using is out of date. I'll sync to the latest one.

>> +
>> +#define SPARSE_HEADER_MAGIC         0xED26FF3A
>> +#define CHUNK_TYPE_RAW              0xCAC1
>> +#define CHUNK_TYPE_FILL             0xCAC2
>> +#define CHUNK_TYPE_DONT_CARE        0xCAC3
>> +#define CHUNK_TYPE_CRC32            0xCAC4
>> +
>> +#define FILL_BUF_SIZE               1024
>
> Is this meeting some specification limit, or an arbitrary value?
>

Yes, it's defined by android sparse image. Here's the reference 
(http://www.2net.co.uk/tutorial/android-sparse-image-format).

The image is split into chunks of multiples of 4096 bytes.

1024 * sizeof (UINT32) = 4096 bytes

>> +
>> +typedef struct _SPARSE_HEADER {
>> +  UINT32       Magic;
>> +  UINT16       MajorVersion;
>> +  UINT16       MinorVersion;
>> +  UINT16       FileHeaderSize;
>> +  UINT16       ChunkHeaderSize;
>> +  UINT32       BlockSize;
>> +  UINT32       TotalBlocks;
>> +  UINT32       TotalChunks;
>> +  UINT32       ImageChecksum;
>> +} SPARSE_HEADER;
>> +
>> +typedef struct _CHUNK_HEADER {
>> +  UINT16       ChunkType;
>> +  UINT16       Reserved1;
>> +  UINT32       ChunkSize;
>> +  UINT32       TotalSize;
>> +} CHUNK_HEADER;
>>
>>  /*
>>   * UEFI Application using the FASTBOOT_TRANSPORT_PROTOCOL and
>> @@ -139,13 +166,83 @@ HandleDownload (
>>  }
>>
>>  STATIC
>> +EFI_STATUS
>> +FlashSparseImage (
>> +  IN CHAR8         *PartitionName,
>> +  IN SPARSE_HEADER *SparseHeader
>> +  )
>> +{
>> +  EFI_STATUS        Status;
>> +  UINTN             Chunk, Offset = 0, Index;
>> +  VOID             *Image;
>> +  CHUNK_HEADER     *ChunkHeader;
>> +  UINT32            FillBuf[FILL_BUF_SIZE];
>> +  CHAR16            OutputString[FASTBOOT_STRING_MAX_LENGTH];
>> +
>> +  Image = (VOID *)SparseHeader;
>> +  Image += SparseHeader->FileHeaderSize;
>> +  for (Chunk = 0; Chunk < SparseHeader->TotalChunks; Chunk++) {
>> +    ChunkHeader = (CHUNK_HEADER *)Image;
>> +    DEBUG ((DEBUG_INFO, "Chunk #%d - Type: 0x%x Size: %d TotalSize: %d Offset %d\n",
>> +            (Chunk+1), ChunkHeader->ChunkType, ChunkHeader->ChunkSize,
>> +            ChunkHeader->TotalSize, Offset));
>> +    Image += sizeof (CHUNK_HEADER);
>> +    switch (ChunkHeader->ChunkType) {
>> +    case CHUNK_TYPE_RAW:
>> +      Status = mPlatform->FlashPartitionEx (
>> +                            PartitionName,
>> +                            Offset,
>> +                            ChunkHeader->ChunkSize * SparseHeader->BlockSize,
>> +                            Image
>> +                            );
>> +      if (EFI_ERROR (Status)) {
>> +        return Status;
>> +      }
>> +      Image += ChunkHeader->ChunkSize * SparseHeader->BlockSize;
>> +      Offset += ChunkHeader->ChunkSize * SparseHeader->BlockSize;
>> +      break;
>> +    case CHUNK_TYPE_FILL:
>> +      SetMem32 (FillBuf, FILL_BUF_SIZE * sizeof (UINT32), *(UINT32 *)Image);
>
> It's in scope, "sizeof (FillBuf)" would be more clear (and less error prone).
>

OK

>> +      Image += sizeof (UINT32);
>> +      for (Index = 0; Index < ChunkHeader->ChunkSize; Index++) {
>> +        Status = mPlatform->FlashPartitionEx (
>> +                              PartitionName,
>> +                              Offset,
>> +                              SparseHeader->BlockSize,
>> +                              FillBuf
>> +                              );
>> +        if (EFI_ERROR (Status)) {
>> +          return Status;
>> +        }
>> +        Offset += SparseHeader->BlockSize;
>> +      }
>> +      break;
>> +    case CHUNK_TYPE_DONT_CARE:
>> +      Offset += ChunkHeader->ChunkSize * SparseHeader->BlockSize;
>> +      break;
>> +    default:
>> +      UnicodeSPrint (
>> +        OutputString,
>> +        sizeof (OutputString),
>> +        L"Unsupported Chunk Type:0x%x\n",
>> +        ChunkHeader->ChunkType
>> +        );
>> +      mTextOut->OutputString (mTextOut, OutputString);
>> +      break;
>> +    }
>> +  }
>> +  return Status;
>> +}
>> +
>> +STATIC
>>  VOID
>>  HandleFlash (
>>    IN CHAR8 *PartitionName
>>    )
>>  {
>> -  EFI_STATUS  Status;
>> -  CHAR16      OutputString[FASTBOOT_STRING_MAX_LENGTH];
>> +  EFI_STATUS        Status;
>> +  CHAR16            OutputString[FASTBOOT_STRING_MAX_LENGTH];
>> +  SPARSE_HEADER    *SparseHeader;
>>
>>    // Build output string
>>    UnicodeSPrint (OutputString, sizeof (OutputString), L"Flashing partition %a\r\n", PartitionName);
>> @@ -157,11 +254,25 @@ HandleFlash (
>>      return;
>>    }
>>
>> -  Status = mPlatform->FlashPartition (
>> -                        PartitionName,
>> -                        mNumDataBytes,
>> -                        mDataBuffer
>> -                        );
>> +  SparseHeader = (SPARSE_HEADER *)mDataBuffer;
>> +  if (SparseHeader->Magic == SPARSE_HEADER_MAGIC) {
>> +    DEBUG ((DEBUG_INFO, "Sparse Magic: 0x%x Major: %d Minor: %d fhs: %d chs: %d bs: %d tbs: %d tcs: %d checksum: %d \n",
>> +                SparseHeader->Magic, SparseHeader->MajorVersion, SparseHeader->MinorVersion,  SparseHeader->FileHeaderSize,
>> +                SparseHeader->ChunkHeaderSize, SparseHeader->BlockSize, SparseHeader->TotalBlocks,
>> +                SparseHeader->TotalChunks, SparseHeader->ImageChecksum));
>> +    if (SparseHeader->MajorVersion != 1) {
>> +        DEBUG ((DEBUG_ERROR, "Sparse image version %d.%d not supported.\n",
>
> Indent is two spaces.
>

OK

>> +                    SparseHeader->MajorVersion, SparseHeader->MinorVersion));
>
> These indents (two DEBUG statements above) look a bit funky.
> Should the continuation lines not line up with DEBUG_?
>
> /
>     Leif
>
>> +        return;
>> +    }
>> +    Status = FlashSparseImage (PartitionName, SparseHeader);
>> +  } else {
>> +    Status = mPlatform->FlashPartition (
>> +                          PartitionName,
>> +                          mNumDataBytes,
>> +                          mDataBuffer
>> +                          );
>> +  }
>>    if (Status == EFI_NOT_FOUND) {
>>      SEND_LITERAL ("FAILNo such partition.");
>>      mTextOut->OutputString (mTextOut, L"No such partition.\r\n");
>> diff --git a/EmbeddedPkg/Include/Protocol/AndroidFastbootPlatform.h b/EmbeddedPkg/Include/Protocol/AndroidFastbootPlatform.h
>> index a9b4aac..2cb51eb 100644
>> --- a/EmbeddedPkg/Include/Protocol/AndroidFastbootPlatform.h
>> +++ b/EmbeddedPkg/Include/Protocol/AndroidFastbootPlatform.h
>> @@ -133,6 +133,28 @@ EFI_STATUS
>>    IN  CHAR8   *Command
>>    );
>>
>> +/*
>> +  Flash the partition named (according to a platform-specific scheme)
>> +  PartitionName, with partition offset and the image pointed to by Buffer,
>> +  whose size is BufferSize.
>> +
>> +  @param[in] PartitionName  Null-terminated name of partition to write.
>> +  @param[in] Offset         Offset of partition.
>> +  @param[in] BufferSize     Size of Buffer in byets.
>> +  @param[in] Buffer         Data to write to partition.
>> +
>> +  @retval EFI_NOT_FOUND     No such partition.
>> +  @retval EFI_DEVICE_ERROR  Flashing failed.
>> +*/
>> +typedef
>> +EFI_STATUS
>> +(*FASTBOOT_PLATFORM_FLASH_EX) (
>> +  IN CHAR8   *PartitionName,
>> +  IN UINTN    Offset,
>> +  IN UINTN    BufferSize,
>> +  IN VOID    *Buffer
>> +  );
>> +
>>  typedef struct _FASTBOOT_PLATFORM_PROTOCOL {
>>    FASTBOOT_PLATFORM_INIT          Init;
>>    FASTBOOT_PLATFORM_UN_INIT       UnInit;
>> @@ -140,6 +162,7 @@ typedef struct _FASTBOOT_PLATFORM_PROTOCOL {
>>    FASTBOOT_PLATFORM_ERASE         ErasePartition;
>>    FASTBOOT_PLATFORM_GETVAR        GetVar;
>>    FASTBOOT_PLATFORM_OEM_COMMAND   DoOemCommand;
>> +  FASTBOOT_PLATFORM_FLASH_EX      FlashPartitionEx;
>>  } FASTBOOT_PLATFORM_PROTOCOL;
>>
>>  #endif
>> --
>> 2.7.4
>>