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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Up to Jens to say if he is ok with it.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall