Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

["Ilias Apalodimas" <ilias.apalodimas@linaro.org>]

Hi Pierre, 

On Wed, Mar 03, 2021 at 10:20:17AM +0000, Pierre wrote:
> Hello Ilias,
> 
> My review is mostly about coding style, but I would have some (inlined)
> remarks about your patch,
> 
> Regards,
> 
> Pierre
> 
> On 3/2/21 3:35 PM, Pierre Gondois wrote:
> 
> > 
> > 

[...]

> > +  offer that from an RPMB, the driver allocates memory on init and
> > passes that
> > 
> > +  on the upper layers. Since the memory is dynamically allocated and we
> > can't set the
> > 
> > +  PCD is StMM context, we need to patch it correctly on each access
> I think: is -> in

Ok

> > 
> > +
> > 
> > +  @retval EFI_SUCCESS Protocol was found and PCDs patched up
> > 
> > +
> > 
> > + **/
> 
> I think there should not be a space here.

Ok 

> 
> > 
> > +EFI_STATUS
> > 
> > +EFIAPI
> > 
> > +FixPcdMemory (
> > 

[...]

> > +
> > 
> > +  // Patch PCDs with the the correct values
> I think it s
> 'the the' -> the

Yea

> > 
> > +  PatchPcdSet64 (PcdFlashNvStorageVariableBase64,
> > Instance->MemBaseAddress);

[...]

> > +  @retval           EFI_OUT_OF_RESOURCES op-tee out of memory
> > 
> > +**/
> > 
> > +STATIC
> > 
> > +EFI_STATUS
> > 
> > +ReadWriteRpmb (
> > 
> > +  UINTN  SvcAct,
> > 
> > +  UINTN  Addr,
> > 
> > +  UINTN  NumBytes,
> > 
> > +  UINTN  Offset
> > 
> > +  )
> 
> I think the parameters should have IN/OUT indications, and the function
> header aswell (cf https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/62_comments#6-2-1-only-use-c-style-comments-on-the-same-line-as-pre-processor-directives-and-in-doxygen-style-file-and-function-header-comment-blocks).
> There are other functions with missing IN/OUT parameters.

Sure, (did you c/p the wrong link?)

> 
> > 
> > +{
> > 

[...]

> > +OpTeeRpmbFvbSetAttributes (
> > 
> > +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
> > 
> > +  IN OUT    EFI_FVB_ATTRIBUTES_2 *Attributes
> Parameters should be aligned. (I think this is valid at other places, like
> for OpTeeRpmbFvbGetPhysicalAddress())

Ok 

> > 
> > +  )

[...]

> > +  The GetBlockSize() function retrieves the size of the requested
> > 
> > +  block. It also returns the number of additional blocks with
> > 
> > +  the identical size. The GetBlockSize() function is used to
> > 
> > +  retrieve the block map (see EFI_FIRMWARE_VOLUME_HEADER).
> > 
> > +
> > 
> > +
> > 
> > +  @param This           Indicates the
> > EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance.
> > 
> > +
> > 
> > +  @param Lba            Indicates the block for which to return the size.
> > 
> > +
> > 
> > +  @param BlockSize      Pointer to a caller-allocated UINTN in which
> > 
> > +                        the size of the block is returned.
> > 
> > +
> > 
> > +  @param NumberOfBlocks Pointer to a caller-allocated UINTN in
> > 
> > +                        which the number of consecutive blocks,
> > 
> > +                        starting with Lba, is returned. All
> > 
> > +                        blocks in this range have a size of
> > 
> > +                        BlockSize.
> > 
> > +
> > 
> > +
> > 
> > +  @retval EFI_SUCCESS             The firmware volume base address was
> > returned.
> > 
> > +
> > 
> > +  @retval EFI_INVALID_PARAMETER   The requested LBA is out of range.
> > 
> > +
> > 
> > +**/
> > 
> > +STATIC
> > 
> > +EFI_STATUS
> > 
> > +OpTeeRpmbFvbGetBlockSize (
> > 
> > +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
> > 
> > +  IN        EFI_LBA                            Lba,
> > 
> > +  OUT       UINTN *BlockSize,
> > 
> > +  OUT       UINTN *NumberOfBlocks
> > 
> > +  )
> > 
> > +{
> > 
> > +  MEM_INSTANCE *Instance;
> > 
> > +
> > 
> > +  Instance = INSTANCE_FROM_FVB_THIS (This);
> > 
> > +  if (Lba > Instance->NBlocks) {
> > 
> > +    return EFI_INVALID_PARAMETER;
> > 
> > +  }
> > 
> > +
> > 
> > +  *NumberOfBlocks = Instance->NBlocks - (UINTN)Lba;
> > 
> > +  *BlockSize = Instance->BlockSize;
> > 
> > +
> > 
> > +  return EFI_SUCCESS;
> > 
> > +}
> > 
> > +
> > 
> > +/**
> > 
> > +  Reads the specified number of bytes into a buffer from the specified
> > block.
> > 
> > +
> > 
> > +  The Read() function reads the requested number of bytes from the
> > 
> > +  requested block and stores them in the provided buffer.
> > 
> > +  Implementations should be mindful that the firmware volume
> > 
> > +  might be in the ReadDisabled state. If it is in this state,
> > 
> > +  the Read() function must return the status code
> > 
> > +  EFI_ACCESS_DENIED without modifying the contents of the
> > 
> > +  buffer. The Read() function must also prevent spanning block
> > 
> > +  boundaries. If a read is requested that would span a block
> > 
> > +  boundary, the read must read up to the boundary but not
> > 
> > +  beyond. The output parameter NumBytes must be set to correctly
> > 
> > +  indicate the number of bytes actually read. The caller must be
> > 
> > +  aware that a read may be partially completed.
> > 
> > +
> > 
> > +  @param This     Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL
> > instance.
> > 
> > +
> > 
> > +  @param Lba      The starting logical block index
> > 
> > +                  from which to read.
> > 
> > +
> > 
> > +  @param Offset   Offset into the block at which to begin reading.
> > 
> > +
> > 
> > +  @param NumBytes Pointer to a UINTN. At entry, *NumBytes
> > 
> > +                  contains the total size of the buffer. At
> > 
> > +                  exit, *NumBytes contains the total number of
> > 
> > +                  bytes read.
> > 
> > +
> > 
> > +  @param Buffer   Pointer to a caller-allocated buffer that will
> > 
> > +                  be used to hold the data that is read.
> > 
> > +
> > 
> > +  @retval EFI_SUCCESS         The firmware volume was read successfully,
> > 
> > +                              and contents are in Buffer.
> > 
> > +
> > 
> > +  @retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA
> > 
> > +                              boundary. On output, NumBytes
> > 
> > +                              contains the total number of bytes
> > 
> > +                              returned in Buffer.
> > 
> > +
> > 
> > +  @retval EFI_ACCESS_DENIED   The firmware volume is in the
> > 
> > +                              ReadDisabled state.
> > 
> > +
> > 
> > +  @retval EFI_DEVICE_ERROR    The block device is not
> > 
> > +                              functioning correctly and could
> > 
> > +                              not be read.
> > 
> I think one new line should be enough. And would it be possible to align the
> return codes comments ?

What's misaligned here? the parameters and the retval comments?
This was pretty much copied from a different driver, so I assumed that was the
'right way'. I can of course align them.

> > +
> > 
> > +**/
> > 
> > +STATIC
> > 
> > +EFI_STATUS
> > 
> > 
> > +  ReadAddr = Instance->MemBaseAddress;
> > 
> > +  // There's no need to check if the read failed here. The upper EDK2
> > layers
> > 
> > +  // will initialize the flash correctly if the in-memory copy is wrong
> > 
> > +  ReadWriteRpmb (
> ReadWriteRpmb() returns an error code, I think we should return it aswell.

There's a reason the return code is not checked here.  This is only called in
FvbInitialize().  That function will always run, even if the RPMB, the files
and filesystem that OP-TEE creates haven't been created yet.  In that case
the OP-TEE API will return an 'error' which is 'file not found' though.
You want to initialize properly with the FVB headers etc and continue with
those contents. 

The code that follows does that, so if we checked the return code and exited
early we would never be able to build the flash contents correctly to begin
with.
In any case after the read the headers are checked for validity/correctness
and if an error is found the contents will be rebuilt.  So we don't really care
about the return here.  Worst case scenario the flash is broken and we need
to rebuild it.  That's what the comment is trying to explain.

Is there a better way to initialize the flash and the contents?
Is there a callback in EDK2 to explicitly rebuild the flash if an error is 
found during the Fvbinitiaze phase?

> > 
> > +    SP_SVC_RPMB_READ,
> > +  @retval EFI_OUT_OF_RESOURCES      op-tee out of memory

[...]

> > 
> > +
> > 
> > +
> > 
> > +
> I think one new line should be enough.

Ok

> > 
> > 

[...]

> > +    mInstance.MemBaseAddress +
> > 
> > +    PcdGet32 (PcdFlashNvStorageVariableSize) +
> > 
> > +    PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
> > 
> > +    );
> It seems this part is really similar to what is FixupPcd.c:FixPcdMemory(),
> maybe this would be worth re-using it.

This is calculating the offsets of dynamic memory allocation that 'maps' to
our RPMB and it is indeed identical. But since they run into completely
different contexts I tried to avoid introducing dependencies between the 2
files. Is that a big problem?

> > 
> > +
> > 

[...]

> > +/**
> > 
> > +  This struct is used by the RPMB driver. Since the upper EDK2 layers
> > 
> > +  expect byte addressable memory, we allocate a memory area of certain
> > 
> > +  size and sync it to the hardware on reads/writes.
> > 
> > +
> > 
> > +  @param[in] Signature        Internal signature used to discover the
> > instance
> > 
> > +  @param[in] Initialize       Function used to initialize the instance
> > 
> > +  @param[in] Initialized      Set to true if initialized
> > 
> > +  @param[in] FvbProtocol      FVB protocol of the instance
> > 
> > +  @param[in] Handle           Handle to install
> > 
> > +  @param[in] MemBaseAddress   Physical address of the beggining of the
> > allocated memory
> > 
> > +  @param[in] BlockSize        Blocksize
> > 
> > +  @param[in] NBlocks          Number of allocated blocks
> 
> I think that for doxygen, '@param[in]' is more for functions declarations.
> For structures, it should be as:
> 
> /**
>   * This struct is used by the RPMB driver. Since the upper EDK2 layers
>   * expect byte addressable memory, we allocate a memory area of certain
>   * size and sync it to the hardware on reads/writes.
> **/
> struct _MEM_INSTANCE {
>     ///  Signature        Internal signature used to discover the instance
>     UINT32                              Signature;
> 
> /// Initialize       Function used to initialize the instance
>     MEM_INITIALIZE                      Initialize;
> 
> [...]
> };
> 
> Cf: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference
> 

Ok 


Thanks
/Ilias