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

["PierreGondois" <pierre.gondois@arm.com>]

Hi Ilias,

Thanks for the answers, I am re-reviewing the patch because I think I 
missed some things. I answered to your comments (inlined),

Regards,

Pierre


On 3/3/21 8:02 PM, Ilias Apalodimas wrote:
> 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?)
Yes it seems so ...
>
>>> +{
>>>
> [...]
>
>>> +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.
My comment did not belong to the right place I believe, I was referring 
to the '@retval' of OpTeeRpmbFvbErase(). The parameters of 
OpTeeRpmbFvbRead() do not appear as aligned though.
>
>>> +
>>>
>>> +**/
>>>
>>> +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?
Thanks for the explanation. I am not aware of such mechanism.
>
>>> +��� 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 is not a big problem, but it seems the 'FixupPcd' and 
'OpTeeRpmbFvb' modules are currently tied up by the definitions in 
'OpTeeRpmbFvb.h'. If there is a chance for them to be separated in the 
future, dependencies should effectively avoided. Otherwise Iit might be 
better to factorize the code.
>
>>> +
>>>
> [...]
>
>>> +/**
>>>
>>> +� 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