[edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Mar 3 20:02:31 UTC 2021
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72404): https://edk2.groups.io/g/devel/message/72404
Mute This Topic: https://groups.io/mt/80588994/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list