Re: [PATCH v2 10/10] xen/arm: ffa: Add indirect message support

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Add support for FFA_MSG_SEND2 to send indirect messages from a VM to a
> secure partition.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2:
> - rebase
> ---
>  xen/arch/arm/tee/ffa.c         |  5 ++++
>  xen/arch/arm/tee/ffa_msg.c     | 49 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/tee/ffa_private.h |  1 +
>  3 files changed, 55 insertions(+)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 3a9525aa4598..21d41b452dc9 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -101,6 +101,7 @@ static const struct ffa_fw_abi ffa_fw_abi_needed[] = {
>      FW_ABI(FFA_MEM_RECLAIM),
>      FW_ABI(FFA_MSG_SEND_DIRECT_REQ_32),
>      FW_ABI(FFA_MSG_SEND_DIRECT_REQ_64),
> +    FW_ABI(FFA_MSG_SEND2),
>  };
>
>  /*
> @@ -195,6 +196,7 @@ static void handle_features(struct cpu_user_regs *regs)
>      case FFA_PARTITION_INFO_GET:
>      case FFA_MSG_SEND_DIRECT_REQ_32:
>      case FFA_MSG_SEND_DIRECT_REQ_64:
> +    case FFA_MSG_SEND2:
>          ffa_set_regs_success(regs, 0, 0);
>          break;
>      case FFA_MEM_SHARE_64:
> @@ -275,6 +277,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>      case FFA_MSG_SEND_DIRECT_REQ_64:
>          ffa_handle_msg_send_direct_req(regs, fid);
>          return true;
> +    case FFA_MSG_SEND2:
> +        e = ffa_handle_msg_send2(regs);
> +        break;
>      case FFA_MEM_SHARE_32:
>      case FFA_MEM_SHARE_64:
>          ffa_handle_mem_share(regs);
> diff --git a/xen/arch/arm/tee/ffa_msg.c b/xen/arch/arm/tee/ffa_msg.c
> index ae263e54890e..335f246ba657 100644
> --- a/xen/arch/arm/tee/ffa_msg.c
> +++ b/xen/arch/arm/tee/ffa_msg.c
> @@ -12,6 +12,15 @@
>
>  #include "ffa_private.h"
>
> +/* Encoding of partition message in RX/TX buffer */
> +struct ffa_part_msg_rxtx {
> +    uint32_t flags;
> +    uint32_t reserved;
> +    uint32_t msg_offset;
> +    uint32_t send_recv_id;
> +    uint32_t msg_size;
> +};
> +
>  void ffa_handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
>  {
>      struct arm_smccc_1_2_regs arg = { .a0 = fid, };
> @@ -78,3 +87,43 @@ out:
>                   resp.a4 & mask, resp.a5 & mask, resp.a6 & mask,
>                   resp.a7 & mask);
>  }
> +
> +int32_t ffa_handle_msg_send2(struct cpu_user_regs *regs)
> +{
> +    struct domain *src_d = current->domain;
> +    struct ffa_ctx *src_ctx = src_d->arch.tee;
> +    const struct ffa_part_msg_rxtx *src_msg;
> +    uint16_t dst_id, src_id;
> +    int32_t ret;
> +
> +    if ( !ffa_fw_supports_fid(FFA_MSG_SEND2) )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( !spin_trylock(&src_ctx->tx_lock) )
> +        return FFA_RET_BUSY;
> +
> +    src_msg = src_ctx->tx;
> +    src_id = src_msg->send_recv_id >> 16;
> +    dst_id = src_msg->send_recv_id & GENMASK(15,0);
> +
> +    if ( src_id != ffa_get_vm_id(src_d) || !FFA_ID_IS_SECURE(dst_id) )
> +    {
> +        ret = FFA_RET_INVALID_PARAMETERS;
> +        goto out_unlock_tx;
> +    }
> +
> +    /* check source message fits in buffer */
> +    if ( src_ctx->page_count * FFA_PAGE_SIZE <
> +         src_msg->msg_offset + src_msg->msg_size ||
> +         src_msg->msg_offset < sizeof(struct ffa_part_msg_rxtx) )
> +    {
> +        ret = FFA_RET_INVALID_PARAMETERS;
> +        goto out_unlock_tx;
> +    }

The guest can change src_mst at any moment with another CPU so these
tests are only sanity checks. The SPMC will also have to lock and do
the same tests again. So the tests here will only in the best case (in
case the guest is misbehaving) save us from entering the SPMC only to
get an error back. The lock makes sense since we could have concurrent
calls to FFA_MEM_SHARE. How about removing the tests?

> +
> +    ret = ffa_simple_call(FFA_MSG_SEND2, ((uint32_t)src_id) << 16, 0, 0, 0);

I'd rather use ffa_get_vm_id(src_d) instead of src_id.

Cheers,
Jens

> +
> +out_unlock_tx:
> +    spin_unlock(&src_ctx->tx_lock);
> +    return ret;
> +}
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 973ee55be09b..d441c0ca5598 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -359,6 +359,7 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs);
>  int ffa_handle_notification_set(struct cpu_user_regs *regs);
>
>  void ffa_handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t 
> fid);
> +int32_t ffa_handle_msg_send2(struct cpu_user_regs *regs);
>
>  static inline uint16_t ffa_get_vm_id(const struct domain *d)
>  {
> --
> 2.47.0
>