Re: [XEN PATCH v7 10/20] xen/arm: ffa: add direct request support

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Wed, Mar 1, 2023 at 4:50 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 1 Mar 2023, at 11:55, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Mon, Feb 27, 2023 at 4:28 PM Bertrand Marquis
> > <Bertrand.Marquis@xxxxxxx> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> 
> >>> 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 | 119 +++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 119 insertions(+)
> >>>
> >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >>> index 463fd7730573..a5d8a12635b6 100644
> >>> --- a/xen/arch/arm/tee/ffa.c
> >>> +++ b/xen/arch/arm/tee/ffa.c
> >>> @@ -142,6 +142,7 @@
> >>>
> >>> struct ffa_ctx {
> >>>    uint32_t guest_vers;
> >>> +    bool interrupted;
> >>
> >> This is added and set here for one special error code but is never used.
> >> I would suggest to introduce this when there will be an action based on it.
> >
> > I'm sorry, I forgot about completing this. I'll add code to deal with
> > FFA_INTERRUPT. This will be tricky to test though since we don't use
> > FFA_INTERRUPT like this with OP-TEE. The Hypervisor is required by the
> > FF-A standard to support it so I better add something.
> >
> >>
> >>> };
> >>>
> >>> /* Negotiated FF-A version to use with the SPMC */
> >>> @@ -167,6 +168,55 @@ 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)
> >>> +{
> >>> +    uint32_t ret = ffa_features(id);
> >>> +
> >>> +    if (ret)
> >>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing\n", id);
> >>
> >> It might be useful here to actually print the error code.
> >> Are we sure that all errors actually mean not supported ?
> >
> > Yes, that's what the standard says.
> >
> >>
> >>> +
> >>> +    return !ret;
> >>> +}
> >>> +
> >>> static uint16_t get_vm_id(const struct domain *d)
> >>> {
> >>>    /* +1 since 0 is reserved for the hypervisor in FF-A */
> >>> @@ -208,6 +258,66 @@ 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;
> >>> +    struct ffa_ctx *ctx = d->arch.tee;
> >>> +    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;
> >>> +
> >>> +    while ( true )
> >>> +    {
> >>> +        arm_smccc_1_2_smc(&arg, &resp);
> >>> +
> >>> +        switch ( resp.a0 )
> >>> +        {
> >>> +        case FFA_INTERRUPT:
> >>> +            ctx->interrupted = true;
> >>> +            goto out;
> >>> +        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:
> >>> +            goto out;
> >>> +        default:
> >>> +            /* Bad fid, report back. */
> >>> +            memset(&arg, 0, sizeof(arg));
> >>> +            arg.a0 = FFA_ERROR;
> >>> +            arg.a1 = src_dst;
> >>> +            arg.a2 = FFA_RET_NOT_SUPPORTED;
> >>> +            continue;
> >>
> >> There is a potential infinite loop here and i do not understand
> >> why this needs to be done.
> >> Here if something is returning a value that you do not understand
> >> you send back an ERROR to it. I do not find in the spec where this
> >> is supposed to be done.
> >> Can you explain a bit here ?
> >
> > This should normally not happen, but the SP/SPMC is responding with a
> > request that we don't know what to do with. The standard doesn't say
> > how to handle that as far as I understand. However, returning back to
> > the VM at this point with an error may leave the SP/SPMC in a strange
> > state. So I think it's better to report back to the SP/SPMC that the
> > request isn't understood and hopefully it can at least return back
> > with an error in a sane state.
> >
> > I'll add something to the comment.
>
> I discussed that with Achin and Marc today at Arm and if we get an invalid
> fid we do not need to report it back like you did.
> We should instead report this as an error to the requester.
>
> This is good as it will remove the :-)

Great, thanks.
/Jens