Re: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Thu, Apr 13, 2023 at 3:44 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi,
>
> > On 13 Apr 2023, at 15:27, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 13/04/2023 14:20, Bertrand Marquis wrote:
> >> Hi Julien,
> >>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xxxxxxx> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 13/04/2023 08:14, Jens Wiklander wrote:
> >>>> Adds support for sending a FF-A direct request. Checks that the SP also
> >>>> supports handling a 32-bit direct request. 64-bit direct requests are
> >>>> not used by the mediator itself so there is not need to check for that.
> >>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 112 insertions(+)
> >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >>>> index f129879c5b81..f2cce955d981 100644
> >>>> --- a/xen/arch/arm/tee/ffa.c
> >>>> +++ b/xen/arch/arm/tee/ffa.c
> >>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
> >>>>      return true;
> >>>>  }
> >>>>  +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> >>>> +{
> >>>> +    switch ( resp->a0 )
> >>>> +    {
> >>>> +    case FFA_ERROR:
> >>>> +        if ( resp->a2 )
> >>>> +            return resp->a2;
> >>>> +        else
> >>>> +            return FFA_RET_NOT_SUPPORTED;
> >>>> +    case FFA_SUCCESS_32:
> >>>> +    case FFA_SUCCESS_64:
> >>>> +        return FFA_RET_OK;
> >>>> +    default:
> >>>> +        return FFA_RET_NOT_SUPPORTED;
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t 
> >>>> a2,
> >>>> +                               register_t a3, register_t a4)
> >>>> +{
> >>>> +    const struct arm_smccc_1_2_regs arg = {
> >>>> +        .a0 = fid,
> >>>> +        .a1 = a1,
> >>>> +        .a2 = a2,
> >>>> +        .a3 = a3,
> >>>> +        .a4 = a4,
> >>>> +    };
> >>>> +    struct arm_smccc_1_2_regs resp;
> >>>> +
> >>>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>>> +
> >>>> +    return get_ffa_ret_code(&resp);
> >>>> +}
> >>>> +
> >>>> +static int32_t ffa_features(uint32_t id)
> >>>> +{
> >>>> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> >>>> +}
> >>>> +
> >>>> +static bool check_mandatory_feature(uint32_t id)
> >>>> +{
> >>>> +    int32_t ret = ffa_features(id);
> >>>> +
> >>>> +    if (ret)
> >>>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error 
> >>>> %d\n",
> >>>> +               id, ret);
> >>>> +
> >>>> +    return !ret;
> >>>> +}
> >>>> +
> >>>>  static uint16_t get_vm_id(const struct domain *d)
> >>>>  {
> >>>>      /* +1 since 0 is reserved for the hypervisor in FF-A */
> >>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs 
> >>>> *regs)
> >>>>      set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
> >>>>  }
> >>>>  +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, 
> >>>> uint32_t fid)
> >>>> +{
> >>>> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
> >>>> +    struct arm_smccc_1_2_regs resp = { };
> >>>> +    struct domain *d = current->domain;
> >>>> +    uint32_t src_dst;
> >>>> +    uint64_t mask;
> >>>> +
> >>>> +    if ( smccc_is_conv_64(fid) )
> >>>> +        mask = GENMASK_ULL(63, 0);
> >>>> +    else
> >>>> +        mask = GENMASK_ULL(31, 0);
> >>>> +
> >>>> +    src_dst = get_user_reg(regs, 1);
> >>>> +    if ( (src_dst >> 16) != get_vm_id(d) )
> >>>> +    {
> >>>> +        resp.a0 = FFA_ERROR;
> >>>> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +    arg.a1 = src_dst;
> >>>> +    arg.a2 = get_user_reg(regs, 2) & mask;
> >>>> +    arg.a3 = get_user_reg(regs, 3) & mask;
> >>>> +    arg.a4 = get_user_reg(regs, 4) & mask;
> >>>> +    arg.a5 = get_user_reg(regs, 5) & mask;
> >>>> +    arg.a6 = get_user_reg(regs, 6) & mask;
> >>>> +    arg.a7 = get_user_reg(regs, 7) & mask;
> >>>> +
> >>>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>>> +    switch ( resp.a0 )
> >>>> +    {
> >>>> +    case FFA_ERROR:
> >>>> +    case FFA_SUCCESS_32:
> >>>> +    case FFA_SUCCESS_64:
> >>>> +    case FFA_MSG_SEND_DIRECT_RESP_32:
> >>>> +    case FFA_MSG_SEND_DIRECT_RESP_64:
> >>>> +        break;
> >>>> +    default:
> >>>> +        /* Bad fid, report back. */
> >>>> +        memset(&arg, 0, sizeof(arg));
> >>>> +        arg.a0 = FFA_ERROR;
> >>>> +        arg.a1 = src_dst;
> >>>> +        arg.a2 = FFA_RET_ABORTED;
> >>>> +    }
> >>>> +
> >>>> +out:
> >>>> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & 
> >>>> mask,
> >>>> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & 
> >>>> mask);
> >>>> +}
> >>>> +
> >>>>  static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>>>  {
> >>>>      uint32_t fid = get_user_reg(regs, 0);
> >>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs 
> >>>> *regs)
> >>>>      case FFA_ID_GET:
> >>>>          set_regs_success(regs, get_vm_id(d), 0);
> >>>>          return true;
> >>>> +    case FFA_MSG_SEND_DIRECT_REQ_32:
> >>>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
> >>>> +        handle_msg_send_direct_req(regs, fid);
> >>>> +        return true;
> >>>>        default:
> >>>>          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> >>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
> >>>>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
> >>>>             major_vers, minor_vers);
> >>>>  +    /*
> >>>> +     * TODO save result of checked features and use that information to
> >>>> +     * accept or reject requests from guests.
> >>>> +     */
> >>>
> >>> I am not entirely sure I understand this TODO. Does it mean a guest can 
> >>> currently use a request that is not supported by FFA?
> >> In fact this is a bit the opposite: in the following patch we check that 
> >> all features we could need are supported but if a guest is only using a 
> >> subset we might not need to have all of them.
> >> Idea of this TODO would be to save the features supported and refuse guest 
> >> requests depending on the features needed for them.
> >
> > Thanks. I would suggest the following comment:
> >
> > /*
> > * At the moment domains must supports the same features used by Xen.
> > * TODO: Rework the code to allow domain to use a subset of the features
> > * supported.
> > */
> >
> > Note that I am using "domains" rather than "guests" because the latter 
> > doesn't include dom0.
>
> Makes sense and new comment is nice.

That's better, I'll update it.

Thanks,
Jens

>
> Up to Jens to say if he is ok with it.
>
> Cheers
> Bertrand
>
> >
> > Cheers,
> >
> > --
> > Julien Grall
>
>