Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 10 Apr 2024, at 16:27, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> On Wed, Apr 10, 2024 at 9:49 AM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 9 Apr 2024, at 17:36, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Add support for FF-A notifications, currently limited to an SP (Secure
>>> Partition) sending an asynchronous notification to a guest.
>>> 
>>> Guests and Xen itself are made aware of pending notifications with an
>>> interrupt. The interrupt handler retrieves the notifications using the
>>> FF-A ABI and deliver them to their destinations.
>>> 
>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>>> ---
>>> xen/arch/arm/tee/Makefile      |   1 +
>>> xen/arch/arm/tee/ffa.c         |  58 ++++++
>>> xen/arch/arm/tee/ffa_notif.c   | 319 +++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_private.h |  71 ++++++++
>>> 4 files changed, 449 insertions(+)
>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>> 
>>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>>> index f0112a2f922d..7c0f46f7f446 100644
>>> --- a/xen/arch/arm/tee/Makefile
>>> +++ b/xen/arch/arm/tee/Makefile
>>> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
>>> obj-$(CONFIG_FFA) += ffa_shm.o
>>> obj-$(CONFIG_FFA) += ffa_partinfo.o
>>> obj-$(CONFIG_FFA) += ffa_rxtx.o
>>> +obj-$(CONFIG_FFA) += ffa_notif.o
>>> obj-y += tee.o
>>> obj-$(CONFIG_OPTEE) += optee.o
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index 5209612963e1..ce9757bfeed1 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -39,6 +39,9 @@
>>> *   - at most 32 shared memory regions per guest
>>> * o FFA_MSG_SEND_DIRECT_REQ:
>>> *   - only supported from a VM to an SP
>>> + * o FFA_NOTIFICATION_*:
>>> + *   - only supports global notifications, that is, per vCPU notifications
>>> + *     are not supported
>>> *
>>> * There are some large locked sections with ffa_tx_buffer_lock and
>>> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
>>> @@ -194,6 +197,8 @@ out:
>>> 
>>> static void handle_features(struct cpu_user_regs *regs)
>>> {
>>> +    struct domain *d = current->domain;
>>> +    struct ffa_ctx *ctx = d->arch.tee;
>>>    uint32_t a1 = get_user_reg(regs, 1);
>>>    unsigned int n;
>>> 
>>> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
>>>        BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
>>>        ffa_set_regs_success(regs, 0, 0);
>>>        break;
>>> +    case FFA_FEATURE_NOTIF_PEND_INTR:
>>> +        if ( ctx->notif.enabled )
>>> +            ffa_set_regs_success(regs, FFA_NOTIF_PEND_INTR_ID, 0);
>>> +        else
>>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +        break;
>>> +    case FFA_FEATURE_SCHEDULE_RECV_INTR:
>>> +        if ( ctx->notif.enabled )
>>> +            ffa_set_regs_success(regs, FFA_NOTIF_PEND_INTR_ID, 0);
>> 
>> This should return the RECV_INTR, not the PEND one.
> 
> Thanks, I'll fix it.
> 
>> 
>>> +        else
>>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +        break;
>>> +
>>> +    case FFA_NOTIFICATION_BIND:
>>> +    case FFA_NOTIFICATION_UNBIND:
>>> +    case FFA_NOTIFICATION_GET:
>>> +    case FFA_NOTIFICATION_SET:
>>> +    case FFA_NOTIFICATION_INFO_GET_32:
>>> +    case FFA_NOTIFICATION_INFO_GET_64:
>>> +        if ( ctx->notif.enabled )
>>> +            ffa_set_regs_success(regs, 0, 0);
>>> +        else
>>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +        break;
>>>    default:
>>>        ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>>        break;
>>> @@ -305,6 +334,30 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>                                                     get_user_reg(regs, 1)),
>>>                                   get_user_reg(regs, 3));
>>>        break;
>>> +    case FFA_NOTIFICATION_BIND:
>>> +        e = ffa_handle_notification_bind(get_user_reg(regs, 1),
>>> +                                         get_user_reg(regs, 2),
>>> +                                         get_user_reg(regs, 3),
>>> +                                         get_user_reg(regs, 4));
>> 
>> I would suggest to pass regs and handle the get_user_regs in the function.
> 
> OK
> 
>> 
>>> +        break;
>>> +    case FFA_NOTIFICATION_UNBIND:
>>> +        e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
>>> +                                           get_user_reg(regs, 3),
>>> +                                           get_user_reg(regs, 4));
>> 
>> same here
> 
> OK
> 
>> 
>>> +        break;
>>> +    case FFA_NOTIFICATION_INFO_GET_32:
>>> +    case FFA_NOTIFICATION_INFO_GET_64:
>>> +        ffa_handle_notification_info_get(regs);
>>> +        return true;
>>> +    case FFA_NOTIFICATION_GET:
>>> +        ffa_handle_notification_get(regs);
>>> +        return true;
>>> +    case FFA_NOTIFICATION_SET:
>>> +        e = ffa_handle_notification_set(get_user_reg(regs, 1),
>>> +                                        get_user_reg(regs, 2),
>>> +                                        get_user_reg(regs, 3),
>>> +                                        get_user_reg(regs, 4));
>> 
>> same here
> 
> OK
> 
>> 
>>> +        break;
>>> 
>>>    default:
>>>        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
>>>    if ( !ffa_partinfo_domain_init(d) )
>>>        return -EIO;
>>> 
>>> +    if ( !ffa_notif_domain_init(d) )
>>> +        return -ENOMEM;
>> 
>> Having this function deciding on the return code is a bit weird.
>> I would suggest to have ffa_notif_domain_init returning an int
>> and deciding on the error code and this one just returning the
>> error if !=0.
>> 
>> If possible the same principle should be applied for the partinfo.
> 
> OK, I'll fix it.
> 
>> 
>>> +
>>>    return 0;
>>> }
>>> 
>>> @@ -423,6 +479,7 @@ static int ffa_domain_teardown(struct domain *d)
>>>        return 0;
>>> 
>>>    ffa_rxtx_domain_destroy(d);
>>> +    ffa_notif_domain_destroy(d);
>>> 
>>>    ffa_domain_teardown_continue(ctx, true /* first_time */);
>>> 
>>> @@ -502,6 +559,7 @@ static bool ffa_probe(void)
>>>    if ( !ffa_partinfo_init() )
>>>        goto err_rxtx_destroy;
>>> 
>>> +    ffa_notif_init();
>>>    INIT_LIST_HEAD(&ffa_teardown_head);
>>>    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>>> 
>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>> new file mode 100644
>>> index 000000000000..0173ee515362
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> @@ -0,0 +1,319 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2024  Linaro Limited
>>> + */
>>> +
>>> +#include <xen/const.h>
>>> +#include <xen/list.h>
>>> +#include <xen/spinlock.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/smccc.h>
>>> +#include <asm/regs.h>
>>> +
>>> +#include "ffa_private.h"
>>> +
>>> +static bool __ro_after_init notif_enabled;
>>> +
>>> +int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
>>> +                                 uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>> 
>> s/0xffff/0xFFFFU/
> 
> OK
> 
>> 
>>> +
>>> +    if ( flags )    /* Only global notifications are supported */
>>> +        return FFA_RET_DENIED;
>>> +
>>> +    /*
>>> +     * We only support notifications from SP so no need to check the sender
>>> +     * endpoint ID, the SPMC will take care of that for us.
>>> +     */
>>> +    return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, 
>>> bitmap_hi,
>>> +                           bitmap_lo);
>>> +}
>>> +
>>> +int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
>>> +                                   uint32_t bitmap_hi)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>> 
>> s/0xffff/0xFFFFU/
> 
> OK
> 
>> 
>>> +
>>> +    /*
>>> +     * We only support notifications from SP so no need to check the
>>> +     * destination endpoint ID, the SPMC will take care of that for us.
>>> +     */
>>> +    return  ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
>>> +                            bitmap_lo);
>>> +}
>>> +
>>> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    struct ffa_ctx *ctx = d->arch.tee;
>>> +    bool pending_global;
>>> +
>>> +    if ( !notif_enabled )
>>> +    {
>>> +        ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +        return;
>>> +    }
>>> +
>>> +    spin_lock(&ctx->notif.lock);
>>> +    pending_global = ctx->notif.secure_pending;
>>> +    ctx->notif.secure_pending = false;
>>> +    spin_unlock(&ctx->notif.lock);
>>> +
>>> +    if ( pending_global )
>>> +    {
>>> +        /* A pending global notification for the guest */
>>> +        ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>>> +                     1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, 
>>> ffa_get_vm_id(d),
>>> +                     0, 0, 0, 0);
>>> +    }
>>> +    else
>>> +    {
>>> +        /* Report an error if there where no pending global notification */
>>> +        ffa_set_regs_error(regs, FFA_RET_NO_DATA);
>>> +    }
>>> +}
>>> +
>>> +void ffa_handle_notification_get(struct cpu_user_regs *regs)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    uint32_t recv = get_user_reg(regs, 1);
>>> +    uint32_t flags = get_user_reg(regs, 2);
>>> +    uint32_t w2 = 0;
>>> +    uint32_t w3 = 0;
>>> +    uint32_t w4 = 0;
>>> +    uint32_t w5 = 0;
>>> +    uint32_t w6 = 0;
>>> +    uint32_t w7 = 0;
>>> +
>>> +    if ( !notif_enabled )
>>> +    {
>>> +        ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +        return;
>>> +    }
>>> +
>>> +    if ( (recv & 0xffff) != ffa_get_vm_id(d) )
>> s/0xffff/0xFFFFU/
> 
> OK
> 
>> 
>>> +    {
>>> +        ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
>>> +        return;
>>> +    }
>>> +
>>> +    if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
>>> +    {
>>> +        struct arm_smccc_1_2_regs arg = {
>>> +            .a0 = FFA_NOTIFICATION_GET,
>>> +            .a1 = recv,
>>> +            .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
>>> +                            FFA_NOTIF_FLAG_BITMAP_SPM ),
>>> +        };
>>> +        struct arm_smccc_1_2_regs resp;
>>> +        int32_t e;
>>> +
>>> +        arm_smccc_1_2_smc(&arg, &resp);
>>> +        e = ffa_get_ret_code(&resp);
>>> +        if ( e )
>>> +        {
>>> +            ffa_set_regs_error(regs, e);
>>> +            return;
>>> +        }
>>> +
>>> +        if ( flags & FFA_NOTIF_FLAG_BITMAP_SP )
>>> +        {
>>> +            w2 = resp.a2;
>>> +            w3 = resp.a3;
>>> +        }
>>> +
>>> +        if ( flags & FFA_NOTIF_FLAG_BITMAP_SPM )
>>> +            w6 = resp.a6;
>>> +    }
>>> +
>>> +    ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
>>> +}
>>> +
>>> +int ffa_handle_notification_set(uint32_t src_dst, uint32_t flags,
>>> +                                uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>> 
>> This needs some checking as i would have used the lowest bits here
>> for the source and not the highest. The spec is using the same description
>> for all ABIs so I am wondering if you are not using the destination instead 
>> of
>> the source here.
> 
> This is a bit tricky because not all ABI functions define Sender and
> Receiver in the same way. For FFA_NOTIFICATION_BIND it's the Sender
> and Receiver of the notification, while for instance,
> FFA_MSG_SEND_DIRECT_REQ defines it as the Sender and Receiver of the
> message.
> 
> When the Hypervisor invokes FFA_NOTIFICATION_SET it's the Sender and
> Receiver of the notification, that is, the guest is the same as the
> sender of the notification. So the guest ID should go into BIT[31:16],
> and the receiver of the notification in BIT[15:0].
> 
> When the guest invokes FFA_NOTIFICATION_SET the Hypervisor is
> requested to signal notifications to the Sender endpoint BIT[31:16].
> What's expected in BIT[15:0] isn't mentioned so I assume the
> Hypervisor should ignore it since it already knows the guest ID.
> 
> Following that analysis, we should replace the if statement above with:
> src_dst = ((uint32_t)ffa_get_vm_id(d) << 16) | (src_dst >> 16)
> 
> But I'm not certain I've understood the specification correctly, in
> particular the part where the guest invokes FFA_NOTIFICATION_SET.
> 
> What's your take on this?
> 
> I don't use this function in my tests so it's perhaps better to wait
> with the implementation of this function until it's used.

I discussed this internally and in fact we have a typo in the
 NOTIFICATION_SET description that we have to fix (first bullet of
 description should say receiver and not sender)

So the implementation should check the lowest bits to be the caller ID
for all calls except notification_set where it should check the highest bits.

In notification_set the lowest bits are encoding the ID of the endpoint to
which a notification must be generated.

Tell me if this is clearing it up :-)

Cheers
Bertrand