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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

You can do that in a different patch then and just remove this from this patch ?

> 
>> 
>>> };
>>> 
>>> /* 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.

The error code might still be useful in the print.

> 
>> 
>>> +
>>> +    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.

Please also make sure that the code is not looping infinitely on this.

> 
>> 
>>> +        }
>>> +    }
>>> +
>>> +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);
>>> @@ -225,6 +335,12 @@ 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:
>>> +#ifdef CONFIG_ARM_64
>>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
>>> +#endif
>>> +        handle_msg_send_direct_req(regs, fid);
>>> +        return true;
>>> 
>>>    default:
>>>        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -310,6 +426,9 @@ static bool ffa_probe(void)
>>>    printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>>>           major_vers, minor_vers);
>>> 
>>> +    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>>> +        return false;
>> 
>> One could not need this feature and here this will make everything 
>> unavailable instead.
>> Why not just reporting back the unsupported error to clients using 
>> unsupported interfaces ?
> 
> One could perhaps argue that this check should be moved to a later
> patch in this series. Perhaps there's some future configuration that
> might make sense without this feature, but for now, it doesn't make
> sense to initialize without it.

I am starting to wonder if we should not at boot scan for available features, 
save them
somewhere and then accept/reject calls depending on the supported features.

Maybe just add a TODO here so that we remember that this is something that 
could be
checked/modified one day. That would also give an insight if someone has such a 
usecase
one day.

Cheers
Bertrand

> 
> Thanks,
> Jens
> 
>> 
>> Cheers
>> Bertrand
>> 
>>> +
>>>    ffa_version = vers;
>>> 
>>>    return true;
>>> --
>>> 2.34.1