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

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Ilias,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Ilias Apalodimas <ilias.apalodimas@linaro.org> 
Sent: 01 February 2021 02:01 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>
Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

Hi Sami,


[...]
> > +STATIC
> > +EFI_STATUS
> > +ReadWriteRpmb (
> > +  UINTN  SvcAct,
> > +  UINTN  Addr,
> > +  UINTN  NumBytes,
> > +  UINTN  Offset
> > +  )
> > +{
> > +  ARM_SVC_ARGS  SvcArgs;
> > +  EFI_STATUS    Status;
> > +
> > +  ZeroMem (&SvcArgs, sizeof (SvcArgs));
> > +
> > +  SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
> > +  SvcArgs.Arg1 = storage_id;
> > +  SvcArgs.Arg2 = 0;
> > +  SvcArgs.Arg3 = SvcAct;
> > +  SvcArgs.Arg4 = Addr;
> > +  SvcArgs.Arg5 = NumBytes;
> > +  SvcArgs.Arg6 = Offset;
> > +
> > +  ArmCallSvc (&SvcArgs);
> > +  if (SvcArgs.Arg3) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n",
> > +     __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3));
> > +  }
> > +
> > +  switch (SvcArgs.Arg3) {
> > +  case ARM_SVC_SPM_RET_SUCCESS:
> > +    Status = EFI_SUCCESS;
> > +    break;
> > +
> > +  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
> > +    Status = EFI_UNSUPPORTED;
> > +    break;
> > +
> > +  case ARM_SVC_SPM_RET_INVALID_PARAMS:
> > +    Status = EFI_INVALID_PARAMETER;
> > +    break;
> > +
> > +  case ARM_SVC_SPM_RET_DENIED:
> > +    Status = EFI_ACCESS_DENIED;
> > +    break;
> > +
> > +  case ARM_SVC_SPM_RET_NO_MEMORY:
> > +    Status = EFI_BAD_BUFFER_SIZE;
> > +    break;
> > +
> > +  default:
> > +    Status = EFI_ACCESS_DENIED;
> > +  }
> > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches?
> > [/SAMI]
>
> I actually picked up the error handling from the previous non-FFA code.
> I'll check what's on Sughosh latest patches and fix it if there are
> any differences.
> Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of
> memory properly anyway.
>

Had another look at this. This seems fine if I just change
EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only
using these errors from FFA. Eventually the OP-TEE code that launches
StMM today, will move to FFA and become a separate SP, so that will
naturally be handled once that's done. I don't see a point of adding
unused error cases.

[SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the 
error codes being handled above would be returned in SvcArgs.Arg2. 
The message flow would be as follows:
    - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint.
    - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2)
    - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces:
	* FFA_MSG_SEND_DIRECT_RESP
	* FFA_INTERRUPT
	* FFA_SUCCESS
    This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3).
[/SAMI]

Regards
/Ilias