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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72499): https://edk2.groups.io/g/devel/message/72499
Mute This Topic: https://groups.io/mt/80588994/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
