From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web08.14102.1614960530489233969
 for <devel@edk2.groups.io>;
 Fri, 05 Mar 2021 08:08:50 -0800
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0604A11FB;
	Fri,  5 Mar 2021 08:08:50 -0800 (PST)
Received: from [192.168.1.169] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 1BAE43F73B;
	Fri,  5 Mar 2021 08:08:48 -0800 (PST)
Subject: Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: devel@edk2.groups.io, Sami.Mujawar@arm.com
References: <20210212173459.508205-1-ilias.apalodimas@linaro.org>
 <20210212173459.508205-2-ilias.apalodimas@linaro.org>
 <51d77291-571b-1635-caa4-590f66c75b56@arm.com> <YD/rV2w2e/ID6EMj@apalos.home>
From: "PierreGondois" <pierre.gondois@arm.com>
Message-ID: <a061cd15-bf06-d827-1e53-0bc2a554dc41@arm.com>
Date: Fri, 5 Mar 2021 16:08:40 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <YD/rV2w2e/ID6EMj@apalos.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

Hi Ilias,

Thanks for the answers, I am re-reviewing the patch because I think I=20
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:
>>
>>>
> [...]
>
>>> +=C3=AF=C2=BF=C2=BD offer that from an RPMB, the driver allocates mem=
ory on init and
>>> passes that
>>>
>>> +=C3=AF=C2=BF=C2=BD on the upper layers. Since the memory is dynamica=
lly allocated and we
>>> can't set the
>>>
>>> +=C3=AF=C2=BF=C2=BD PCD is StMM context, we need to patch it correctl=
y on each access
>> I think: is -> in
> Ok
>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_SUCCESS Protocol was found and PCDs p=
atched up
>>>
>>> +
>>>
>>> + **/
>> I think there should not be a space here.
> Ok
>
>>> +EFI_STATUS
>>>
>>> +EFIAPI
>>>
>>> +FixPcdMemory (
>>>
> [...]
>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD // Patch PCDs with the the correct values
>> I think it s
>> 'the the' -> the
> Yea
>
>>> +=C3=AF=C2=BF=C2=BD PatchPcdSet64 (PcdFlashNvStorageVariableBase64,
>>> Instance->MemBaseAddress);
> [...]
>
>>> +=C3=AF=C2=BF=C2=BD @retval=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD EFI_OUT_O=
F_RESOURCES op-tee out of memory
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +ReadWriteRpmb (
>>>
>>> +=C3=AF=C2=BF=C2=BD UINTN=C3=AF=C2=BF=C2=BD SvcAct,
>>>
>>> +=C3=AF=C2=BF=C2=BD UINTN=C3=AF=C2=BF=C2=BD Addr,
>>>
>>> +=C3=AF=C2=BF=C2=BD UINTN=C3=AF=C2=BF=C2=BD NumBytes,
>>>
>>> +=C3=AF=C2=BF=C2=BD UINTN=C3=AF=C2=BF=C2=BD Offset
>>>
>>> +=C3=AF=C2=BF=C2=BD )
>> I think the parameters should have IN/OUT indications, and the functio=
n
>> header aswell (cf https://edk2-docs.gitbook.io/edk-ii-c-coding-standar=
ds-specification/6_documenting_software/62_comments#6-2-1-only-use-c-styl=
e-comments-on-the-same-line-as-pre-processor-directives-and-in-doxygen-st=
yle-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 (
>>>
>>> +=C3=AF=C2=BF=C2=BD IN CONST=C3=AF=C2=BF=C2=BD EFI_FIRMWARE_VOLUME_BL=
OCK_PROTOCOL *This,
>>>
>>> +=C3=AF=C2=BF=C2=BD IN OUT=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD EFI_FVB_ATTRIBUTES_2 *Attributes
>> Parameters should be aligned. (I think this is valid at other places, =
like
>> for OpTeeRpmbFvbGetPhysicalAddress())
> Ok
>
>>> +=C3=AF=C2=BF=C2=BD )
> [...]
>
>>> +=C3=AF=C2=BF=C2=BD The GetBlockSize() function retrieves the size of=
 the requested
>>>
>>> +=C3=AF=C2=BF=C2=BD block. It also returns the number of additional b=
locks with
>>>
>>> +=C3=AF=C2=BF=C2=BD the identical size. The GetBlockSize() function i=
s used to
>>>
>>> +=C3=AF=C2=BF=C2=BD retrieve the block map (see EFI_FIRMWARE_VOLUME_H=
EADER).
>>>
>>> +
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param This=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Indica=
tes the
>>> EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param Lba=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD Indicates the block for which to return the size.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param BlockSize=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Pointer to a ca=
ller-allocated UINTN in which
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD the size of the block is re=
turned.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param NumberOfBlocks Pointer to a caller-allocat=
ed UINTN in
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD which the number of consecu=
tive blocks,
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD starting with Lba, is retur=
ned. All
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD blocks in this range have a=
 size of
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD BlockSize.
>>>
>>> +
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_SUCCESS=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD The firmware volume base address was
>>> returned.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_INVALID_PARAMETER=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD The requested LBA is out of range.
>>>
>>> +
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +OpTeeRpmbFvbGetBlockSize (
>>>
>>> +=C3=AF=C2=BF=C2=BD IN CONST=C3=AF=C2=BF=C2=BD EFI_FIRMWARE_VOLUME_BL=
OCK_PROTOCOL *This,
>>>
>>> +=C3=AF=C2=BF=C2=BD IN=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD EFI_LBA=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Lba,
>>>
>>> +=C3=AF=C2=BF=C2=BD OUT=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD UINTN *Block=
Size,
>>>
>>> +=C3=AF=C2=BF=C2=BD OUT=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD UINTN *Numbe=
rOfBlocks
>>>
>>> +=C3=AF=C2=BF=C2=BD )
>>>
>>> +{
>>>
>>> +=C3=AF=C2=BF=C2=BD MEM_INSTANCE *Instance;
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD Instance =3D INSTANCE_FROM_FVB_THIS (This);
>>>
>>> +=C3=AF=C2=BF=C2=BD if (Lba > Instance->NBlocks) {
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD return EFI_IN=
VALID_PARAMETER;
>>>
>>> +=C3=AF=C2=BF=C2=BD }
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD *NumberOfBlocks =3D Instance->NBlocks - (UINTN)Lb=
a;
>>>
>>> +=C3=AF=C2=BF=C2=BD *BlockSize =3D Instance->BlockSize;
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD return EFI_SUCCESS;
>>>
>>> +}
>>>
>>> +
>>>
>>> +/**
>>>
>>> +=C3=AF=C2=BF=C2=BD Reads the specified number of bytes into a buffer=
 from the specified
>>> block.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD The Read() function reads the requested number of=
 bytes from the
>>>
>>> +=C3=AF=C2=BF=C2=BD requested block and stores them in the provided b=
uffer.
>>>
>>> +=C3=AF=C2=BF=C2=BD Implementations should be mindful that the firmwa=
re volume
>>>
>>> +=C3=AF=C2=BF=C2=BD might be in the ReadDisabled state. If it is in t=
his state,
>>>
>>> +=C3=AF=C2=BF=C2=BD the Read() function must return the status code
>>>
>>> +=C3=AF=C2=BF=C2=BD EFI_ACCESS_DENIED without modifying the contents =
of the
>>>
>>> +=C3=AF=C2=BF=C2=BD buffer. The Read() function must also prevent spa=
nning block
>>>
>>> +=C3=AF=C2=BF=C2=BD boundaries. If a read is requested that would spa=
n a block
>>>
>>> +=C3=AF=C2=BF=C2=BD boundary, the read must read up to the boundary b=
ut not
>>>
>>> +=C3=AF=C2=BF=C2=BD beyond. The output parameter NumBytes must be set=
 to correctly
>>>
>>> +=C3=AF=C2=BF=C2=BD indicate the number of bytes actually read. The c=
aller must be
>>>
>>> +=C3=AF=C2=BF=C2=BD aware that a read may be partially completed.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param This=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Indicates the EFI_FIRMWARE_VOLUME_BLOCK=
_PROTOCOL
>>> instance.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param Lba=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD The starting logical =
block index
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD from which to read.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param Offset=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
 Offset into the block at which to begin reading.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param NumBytes Pointer to a UINTN. At entry, *Nu=
mBytes
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD contains the total size of the buffer. At
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD exit, *NumBytes contains the total number of
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD bytes read.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param Buffer=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
 Pointer to a caller-allocated buffer that will
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD be used to hold the data that is read.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_SUCCESS=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD The firmware volume was read succ=
essfully,
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD and contents are in Buffer.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_BAD_BUFFER_SIZE Read attempted across=
 an LBA
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD boundary. On output, NumBytes
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD contains the total number of bytes
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD returned in Buffer.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_ACCESS_DENIED=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD The firmware volume is in the
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD ReadDisabled state.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_DEVICE_ERROR=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD The block device is not
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD functioning correctly and could
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD not be read.
>>>
>> I think one new line should be enough. And would it be possible to ali=
gn 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=20
to the '@retval' of OpTeeRpmbFvbErase(). The parameters of=20
OpTeeRpmbFvbRead() do not appear as aligned though.
>
>>> +
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>>
>>> +=C3=AF=C2=BF=C2=BD ReadAddr =3D Instance->MemBaseAddress;
>>>
>>> +=C3=AF=C2=BF=C2=BD // There's no need to check if the read failed he=
re. The upper EDK2
>>> layers
>>>
>>> +=C3=AF=C2=BF=C2=BD // will initialize the flash correctly if the in-=
memory copy is wrong
>>>
>>> +=C3=AF=C2=BF=C2=BD ReadWriteRpmb (
>> ReadWriteRpmb() returns an error code, I think we should return it asw=
ell.
> There's a reason the return code is not checked here.  This is only cal=
led 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 c=
ase
> 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 w=
ith
> those contents.
>
> The code that follows does that, so if we checked the return code and e=
xited
> early we would never be able to build the flash contents correctly to b=
egin
> with.
> In any case after the read the headers are checked for validity/correct=
ness
> and if an error is found the contents will be rebuilt.  So we don't rea=
lly 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.
>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD SP_SVC_RPMB_R=
EAD,
>>> +=C3=AF=C2=BF=C2=BD @retval EFI_OUT_OF_RESOURCES=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD op-=
tee out of memory
> [...]
>
>>> +
>>>
>>> +
>>>
>>> +
>> I think one new line should be enough.
> Ok
>
>>>
> [...]
>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD mInstance.Mem=
BaseAddress +
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD PcdGet32 (Pcd=
FlashNvStorageVariableSize) +
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD PcdGet32 (Pcd=
FlashNvStorageFtwWorkingSize)
>>>
>>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD );
>> It seems this part is really similar to what is FixupPcd.c:FixPcdMemor=
y(),
>> 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 th=
e 2
> files. Is that a big problem?
This is not a big problem, but it seems the 'FixupPcd' and=20
'OpTeeRpmbFvb' modules are currently tied up by the definitions in=20
'OpTeeRpmbFvb.h'. If there is a chance for them to be separated in the=20
future, dependencies should effectively avoided. Otherwise Iit might be=20
better to factorize the code.
>
>>> +
>>>
> [...]
>
>>> +/**
>>>
>>> +=C3=AF=C2=BF=C2=BD This struct is used by the RPMB driver. Since the=
 upper EDK2 layers
>>>
>>> +=C3=AF=C2=BF=C2=BD expect byte addressable memory, we allocate a mem=
ory area of certain
>>>
>>> +=C3=AF=C2=BF=C2=BD size and sync it to the hardware on reads/writes.
>>>
>>> +
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] Signature=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD Internal signature used to discover the
>>> instance
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] Initialize=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD Function used to initialize the instance
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] Initialized=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Set to tr=
ue if initialized
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] FvbProtocol=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD FVB proto=
col of the instance
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] Handle=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD =
Handle to install
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] MemBaseAddress=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD Physical address of the beggining of the
>>> allocated memory
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] BlockSize=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD Blocksize
>>>
>>> +=C3=AF=C2=BF=C2=BD @param[in] NBlocks=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Number of allocate=
d blocks
>> I think that for doxygen, '@param[in]' is more for functions declarati=
ons.
>> For structures, it should be as:
>>
>> /**
>> =C3=AF=C2=BF=C2=BD * This struct is used by the RPMB driver. Since the=
 upper EDK2 layers
>> =C3=AF=C2=BF=C2=BD * expect byte addressable memory, we allocate a mem=
ory area of certain
>> =C3=AF=C2=BF=C2=BD * size and sync it to the hardware on reads/writes.
>> **/
>> struct _MEM_INSTANCE {
>> =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD ///=C3=AF=C2=BF=
=C2=BD Signature=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Int=
ernal signature used to discover the instance
>> =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD UINT32=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Signature=
;
>>
>> /// Initialize=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Function used to init=
ialize the instance
>> =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD MEM_INITIALIZE=C3=
=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=
=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=
=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=
=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=
=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=
 Initialize;
>>
>> [...]
>> };
>>
>> Cf: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specificati=
on/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration=
-with-forward-reference-or-self-reference
>>
> Ok
>
>
> Thanks
> /Ilias