[XEN,v3,5/5] xen/arm: ffa: support notification

Commit Message

Jens Wiklander April 26, 2024, 8:47 a.m. UTC

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.

Update ffa_partinfo_domain_init() to return error code like
ffa_notif_domain_init().

Signed-off-by: Jens Wiklander <[e-mail]>
---
v2->v3:
- Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
- Register the Xen SRI handler on each CPU using on_selected_cpus() and
  setup_irq()
- Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
  doesn't conflict with static SGI handlers

v1->v2:
- Addressing review comments
- Change ffa_handle_notification_{bind,unbind,set}() to take struct
  cpu_user_regs *regs as argument.
- Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
  an error code.
- Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
---
 xen/arch/arm/irq.c              |   2 +-
 xen/arch/arm/tee/Makefile       |   1 +
 xen/arch/arm/tee/ffa.c          |  55 ++++-
 xen/arch/arm/tee/ffa_notif.c    | 378 ++++++++++++++++++++++++++++++++
 xen/arch/arm/tee/ffa_partinfo.c |   9 +-
 xen/arch/arm/tee/ffa_private.h  |  56 ++++-
 xen/include/public/arch-arm.h   |  14 ++
 7 files changed, 507 insertions(+), 8 deletions(-)
 create mode 100644 xen/arch/arm/tee/ffa_notif.c

Comments

Bertrand Marquis April 26, 2024, 9:20 a.m. UTC | #1

Hi Jens,

> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> 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.
> 
> Update ffa_partinfo_domain_init() to return error code like
> ffa_notif_domain_init().
> 
> Signed-off-by: Jens Wiklander <[e-mail]>
> ---
> v2->v3:
> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>  setup_irq()
> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>  doesn't conflict with static SGI handlers
> 
> v1->v2:
> - Addressing review comments
> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>  cpu_user_regs *regs as argument.
> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>  an error code.
> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> ---
> xen/arch/arm/irq.c              |   2 +-
> xen/arch/arm/tee/Makefile       |   1 +
> xen/arch/arm/tee/ffa.c          |  55 ++++-
> xen/arch/arm/tee/ffa_notif.c    | 378 ++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> xen/arch/arm/tee/ffa_private.h  |  56 ++++-
> xen/include/public/arch-arm.h   |  14 ++
> 7 files changed, 507 insertions(+), 8 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d7306aa64d50..5224898265a5 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>     {
>         /* SGIs are always edge-triggered */
>         if ( irq < NR_GIC_SGI )
> -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>         else
>             local_irqs_type[irq] = IRQ_TYPE_INVALID;
>     }
> 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> +        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,22 @@ 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(regs);
> +        break;
> +    case FFA_NOTIFICATION_UNBIND:
> +        e = ffa_handle_notification_unbind(regs);
> +        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(regs);
> +        break;
> 
>     default:
>         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> static int ffa_domain_init(struct domain *d)
> {
>     struct ffa_ctx *ctx;
> +    int ret;
> 
>     if ( !ffa_version )
>         return -ENODEV;
> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>      * error, so no need for cleanup in this function.
>      */
> 
> -    if ( !ffa_partinfo_domain_init(d) )
> -        return -EIO;
> +    ret = ffa_partinfo_domain_init(d);
> +    if ( ret )
> +        return ret;
> 
> -    return 0;
> +    return ffa_notif_domain_init(d);
> }
> 
> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> @@ -423,6 +470,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 +550,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..6bb0804ee2f8
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,378 @@
> +/* 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(struct cpu_user_regs *regs)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t src_dst = get_user_reg(regs, 1);
> +    uint32_t flags = get_user_reg(regs, 2);
> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> +    if ( !notif_enabled )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    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(struct cpu_user_regs *regs)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t src_dst = get_user_reg(regs, 1);
> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> +    if ( !notif_enabled )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    /*
> +     * 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 & 0xFFFFU) != ffa_get_vm_id(d) )
> +    {
> +        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;
> +    }

In here you never clean the secure_pending flag but in practice there would be
no more pending notification if SP and SPM flags are set so the response to
notification_info_get would wrongly say there is something pending.

I am not completely sure how this could be handled if both SP and SPM are
not set so i would say for now we should at least put a comment in info_get
to keep a note of this fact.
Do you agree ?

> +
> +    ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> +}
> +
> +int ffa_handle_notification_set(struct cpu_user_regs *regs)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t src_dst = get_user_reg(regs, 1);
> +    uint32_t flags = get_user_reg(regs, 2);
> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> +    if ( !notif_enabled )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    /* Let the SPMC check the destination of the notification */
> +    return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
> +                           bitmap_hi);
> +}
> +
> +/*
> + * Extract a 16-bit ID (index n) from the successful return value from
> + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
> + * returned in registers 3 to 7 with four IDs per register for 64-bit
> + * calling convention and two IDs per register for 32-bit calling
> + * convention.
> + */
> +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
> +                                 unsigned int n)
> +{
> +    unsigned int ids_per_reg;
> +    unsigned int reg_idx;
> +    unsigned int reg_shift;
> +
> +    if ( smccc_is_conv_64(resp->a0) )
> +        ids_per_reg = 4;
> +    else
> +        ids_per_reg = 2;
> +
> +    reg_idx = n / ids_per_reg + 3;
> +    reg_shift = ( n % ids_per_reg ) * 16;
> +
> +    switch ( reg_idx )
> +    {
> +    case 3:
> +        return resp->a3 >> reg_shift;
> +    case 4:
> +        return resp->a4 >> reg_shift;
> +    case 5:
> +        return resp->a5 >> reg_shift;
> +    case 6:
> +        return resp->a6 >> reg_shift;
> +    case 7:
> +        return resp->a7 >> reg_shift;
> +    default:
> +        ASSERT(0); /* "Can't happen" */
> +        return 0;
> +    }
> +}
> +
> +static void notif_irq_handler(int irq, void *data)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> +    };
> +    struct arm_smccc_1_2_regs resp;
> +    unsigned int id_pos;
> +    unsigned int list_count;
> +    uint64_t ids_count;
> +    unsigned int n;
> +    int32_t res;
> +
> +    do {
> +        arm_smccc_1_2_smc(&arg, &resp);
> +        res = ffa_get_ret_code(&resp);
> +        if ( res )
> +        {
> +            if ( res != FFA_RET_NO_DATA )
> +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> +                       res);
> +            return;
> +        }
> +
> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> +
> +        id_pos = 0;
> +        for ( n = 0; n < list_count; n++ )
> +        {
> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> +            struct domain *d;
> +

If a notification is pending for a secure partition at this stage we are not
signaling anything so I think this fact should be listed in the limitations to
say that we do not signal any secondary scheduler if a notification is
pending for a secure partition.

> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> +
> +            if ( d )
> +            {
> +                struct ffa_ctx *ctx = d->arch.tee;
> +
> +                spin_lock(&ctx->notif.lock);
> +                ctx->notif.secure_pending = true;
> +                spin_unlock(&ctx->notif.lock);
> +
> +                /*
> +                 * Since we're only delivering global notification, always
> +                 * deliver to the first vCPU. It doesn't matter which we
> +                 * chose, as long as it's available.
> +                 */
> +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> +                                true);
> +
> +                put_domain(d);
> +            }
> +
> +            id_pos += count;
> +        }
> +
> +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}
> +
> +static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
> +                                              uint32_t vcpu_count)
> +{
> +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
> +                           0, 0);
> +}
> +
> +static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
> +{
> +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
> +}
> +
> +struct notif_irq_info {
> +    unsigned int irq;
> +    int ret;
> +    struct irqaction *action;
> +};
> +
> +static void notif_irq_enable(void *info)
> +{
> +    struct notif_irq_info *irq_info = info;
> +
> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> +    if ( irq_info->ret )
> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> +               irq_info->irq, irq_info->ret);
> +}
> +
> +void ffa_notif_init(void)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = FFA_FEATURES,
> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> +    };
> +    struct notif_irq_info irq_info = { };
> +    struct arm_smccc_1_2_regs resp;
> +    unsigned int cpu;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +    if ( resp.a0 != FFA_SUCCESS_32 )
> +        return;
> +
> +    irq_info.irq = resp.a2;
> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> +    {
> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> +               irq_info.irq);
> +        return;
> +    }
> +
> +    /*
> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> +     * IPI to call notif_irq_enable() on each CPU including the current
> +     * CPU. The struct irqaction is preallocated since we can't allocate
> +     * memory while in interrupt context.
> +     */
> +    for_each_online_cpu(cpu)
> +    {
> +        irq_info.action = xmalloc(struct irqaction);

You allocate one action per cpu but you have only one action pointer in your structure
which means you will overload the previously allocated one and lose track.

You should have a table of actions in your structure instead unless one action is
enough and can be reused on all cpus and in this case you should move out of
your loop the allocation part.

> +        if ( !irq_info.action )
> +        {
> +            irq_info.ret = -ENOMEM;
> +            break;
> +        }
> +
> +        *irq_info.action = (struct irqaction){
> +            .handler = notif_irq_handler,
> +            .name = "FF-A notif",
> +            .free_on_release = 1,
> +        };
> +
> +        on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> +        if ( irq_info.ret )
> +        {
> +            XFREE(irq_info.action);
> +            break;
> +        }
> +    }
> +
> +    notif_enabled = !irq_info.ret;
> +}
> +
> +int ffa_notif_domain_init(struct domain *d)
> +{
> +    struct ffa_ctx *ctx = d->arch.tee;
> +    int32_t res;
> +
> +    if ( !notif_enabled )
> +        return 0;
> +
> +    res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> +    if ( res )
> +        return -ENOMEM;
> +
> +    ctx->notif.enabled = true;
> +
> +    return 0;
> +}
> +
> +void ffa_notif_domain_destroy(struct domain *d)
> +{
> +    struct ffa_ctx *ctx = d->arch.tee;
> +
> +    if ( ctx->notif.enabled )
> +    {
> +        ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> +        ctx->notif.enabled = false;
> +    }
> +}
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index dc1059584828..93a03c6bc672 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
>     }
> }
> 
> -bool ffa_partinfo_domain_init(struct domain *d)
> +int ffa_partinfo_domain_init(struct domain *d)
> {
>     unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
>     struct ffa_ctx *ctx = d->arch.tee;
> @@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
> 
>     ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
>     if ( !ctx->vm_destroy_bitmap )
> -        return false;
> +        return -ENOMEM;
> 
>     for ( n = 0; n < subscr_vm_created_count; n++ )
>     {
> @@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
>     }
>     vm_destroy_bitmap_init(ctx, n);
> 
> -    return n == subscr_vm_created_count;
> +    if ( n != subscr_vm_created_count )
> +        return -EIO;
> +
> +    return 0;
> }
> 
> bool ffa_partinfo_domain_destroy(struct domain *d)
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 98236cbf14a3..a59c9887774b 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -25,6 +25,7 @@
> #define FFA_RET_DENIED                  -6
> #define FFA_RET_RETRY                   -7
> #define FFA_RET_ABORTED                 -8
> +#define FFA_RET_NO_DATA                 -9
> 
> /* FFA_VERSION helpers */
> #define FFA_VERSION_MAJOR_SHIFT         16U
> @@ -175,6 +176,21 @@
>  */
> #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
> 
> +/* Flags used in calls to FFA_NOTIFICATION_GET interface  */
> +#define FFA_NOTIF_FLAG_BITMAP_SP        BIT(0, U)
> +#define FFA_NOTIF_FLAG_BITMAP_VM        BIT(1, U)
> +#define FFA_NOTIF_FLAG_BITMAP_SPM       BIT(2, U)
> +#define FFA_NOTIF_FLAG_BITMAP_HYP       BIT(3, U)
> +
> +#define FFA_NOTIF_INFO_GET_MORE_FLAG        BIT(0, U)
> +#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT    12
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT   7
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK    0x1F
> +
> +/* Feature IDs used with FFA_FEATURES */
> +#define FFA_FEATURE_NOTIF_PEND_INTR     0x1U
> +#define FFA_FEATURE_SCHEDULE_RECV_INTR  0x2U
> +
> /* Function IDs */
> #define FFA_ERROR                       0x84000060U
> #define FFA_SUCCESS_32                  0x84000061U
> @@ -213,6 +229,27 @@
> #define FFA_MEM_FRAG_TX                 0x8400007BU
> #define FFA_MSG_SEND                    0x8400006EU
> #define FFA_MSG_POLL                    0x8400006AU
> +#define FFA_NOTIFICATION_BITMAP_CREATE  0x8400007DU
> +#define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
> +#define FFA_NOTIFICATION_BIND           0x8400007FU
> +#define FFA_NOTIFICATION_UNBIND         0x84000080U
> +#define FFA_NOTIFICATION_SET            0x84000081U
> +#define FFA_NOTIFICATION_GET            0x84000082U
> +#define FFA_NOTIFICATION_INFO_GET_32    0x84000083U
> +#define FFA_NOTIFICATION_INFO_GET_64    0xC4000083U
> +
> +struct ffa_ctx_notif {
> +    bool enabled;
> +
> +    /* Used to serialize access to the rest of this struct */
> +    spinlock_t lock;
> +
> +    /*
> +     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> +     * pending global notifications.
> +     */
> +    bool secure_pending;
> +};
> 
> struct ffa_ctx {
>     void *rx;
> @@ -228,6 +265,7 @@ struct ffa_ctx {
>     struct list_head shm_list;
>     /* Number of allocated shared memory object */
>     unsigned int shm_count;
> +    struct ffa_ctx_notif notif;
>     /*
>      * tx_lock is used to serialize access to tx
>      * rx_lock is used to serialize access to rx
> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
> 
> bool ffa_partinfo_init(void);
> -bool ffa_partinfo_domain_init(struct domain *d);
> +int ffa_partinfo_domain_init(struct domain *d);
> bool ffa_partinfo_domain_destroy(struct domain *d);
> int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
>                                       uint32_t w4, uint32_t w5, uint32_t *count,
> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> uint32_t ffa_handle_rxtx_unmap(void);
> int32_t ffa_handle_rx_release(void);
> 
> +void ffa_notif_init(void);
> +int ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> +
> static inline uint16_t ffa_get_vm_id(const struct domain *d)
> {
>     /* +1 since 0 is reserved for the hypervisor in FF-A */
>     return d->domain_id + 1;
> }
> 
> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> +{
> +    /* -1 to match ffa_get_vm_id() */
> +    return get_domain_by_id(vm_id - 1);
> +}
> +

I think there is a global discussion to have on using get_domain_by_vm_id
or rcu_lock_live_remote_domain_by_id to make sure the domain is not
dying when we try to do something with it.

It does not need to be done here as there are other places to adapt but
i wanted to raise the question.

I would like to know what you and also other maintainers think here.
@Julien/Michal/Stefano ?

> static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
>                                 register_t v1, register_t v2, register_t v3,
>                                 register_t v4, register_t v5, register_t v6,
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e167e14f8df9..1eac9802aa53 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
> #define GUEST_MAX_VCPUS 128
> 
> /* Interrupts */
> +
> #define GUEST_TIMER_VIRT_PPI    27
> #define GUEST_TIMER_PHYS_S_PPI  29
> #define GUEST_TIMER_PHYS_NS_PPI 30
> @@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
> #define GUEST_VIRTIO_MMIO_SPI_FIRST   33
> #define GUEST_VIRTIO_MMIO_SPI_LAST    43
> 
> +/*
> + * SGI is the preferred delivery mechanism of FF-A pending notifications or
> + * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
> + * as they in a non-virtualized system typically are assigned to the secure
> + * world. Here we're free to use SGI 8-15 since they are virtual and have
> + * nothing to do with the secure world.
> + *
> + * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
> + * https://developer.arm.com/documentation/den0094/
> + */
> +#define GUEST_FFA_NOTIF_PEND_INTR_ID      8
> +#define GUEST_FFA_SCHEDULE_RECV_INTR_ID   9
> +
> /* PSCI functions */
> #define PSCI_cpu_suspend 0
> #define PSCI_cpu_off     1
> -- 
> 2.34.1
> 

Cheers
Bertrand

Jens Wiklander April 26, 2024, 12:11 p.m. UTC | #2

Hi Bertrand,

On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
<[e-mail]> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> 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.
> >
> > Update ffa_partinfo_domain_init() to return error code like
> > ffa_notif_domain_init().
> >
> > Signed-off-by: Jens Wiklander <[e-mail]>
> > ---
> > v2->v3:
> > - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> >  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> > - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> >  setup_irq()
> > - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> >  doesn't conflict with static SGI handlers
> >
> > v1->v2:
> > - Addressing review comments
> > - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> >  cpu_user_regs *regs as argument.
> > - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> >  an error code.
> > - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> > ---
> > xen/arch/arm/irq.c              |   2 +-
> > xen/arch/arm/tee/Makefile       |   1 +
> > xen/arch/arm/tee/ffa.c          |  55 ++++-
> > xen/arch/arm/tee/ffa_notif.c    | 378 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> > xen/arch/arm/tee/ffa_private.h  |  56 ++++-
> > xen/include/public/arch-arm.h   |  14 ++
> > 7 files changed, 507 insertions(+), 8 deletions(-)
> > create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d7306aa64d50..5224898265a5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> >     {
> >         /* SGIs are always edge-triggered */
> >         if ( irq < NR_GIC_SGI )
> > -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> > +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> >         else
> >             local_irqs_type[irq] = IRQ_TYPE_INVALID;
> >     }
> > 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> > +        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,22 @@ 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(regs);
> > +        break;
> > +    case FFA_NOTIFICATION_UNBIND:
> > +        e = ffa_handle_notification_unbind(regs);
> > +        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(regs);
> > +        break;
> >
> >     default:
> >         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > static int ffa_domain_init(struct domain *d)
> > {
> >     struct ffa_ctx *ctx;
> > +    int ret;
> >
> >     if ( !ffa_version )
> >         return -ENODEV;
> > @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> >      * error, so no need for cleanup in this function.
> >      */
> >
> > -    if ( !ffa_partinfo_domain_init(d) )
> > -        return -EIO;
> > +    ret = ffa_partinfo_domain_init(d);
> > +    if ( ret )
> > +        return ret;
> >
> > -    return 0;
> > +    return ffa_notif_domain_init(d);
> > }
> >
> > static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> > @@ -423,6 +470,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 +550,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..6bb0804ee2f8
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
> > @@ -0,0 +1,378 @@
> > +/* 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(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    uint32_t src_dst = get_user_reg(regs, 1);
> > +    uint32_t flags = get_user_reg(regs, 2);
> > +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> > +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > +    if ( !notif_enabled )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    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(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    uint32_t src_dst = get_user_reg(regs, 1);
> > +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> > +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > +    if ( !notif_enabled )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    /*
> > +     * 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 & 0xFFFFU) != ffa_get_vm_id(d) )
> > +    {
> > +        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;
> > +    }
>
> In here you never clean the secure_pending flag but in practice there would be
> no more pending notification if SP and SPM flags are set so the response to
> notification_info_get would wrongly say there is something pending.
>
> I am not completely sure how this could be handled if both SP and SPM are
> not set so i would say for now we should at least put a comment in info_get
> to keep a note of this fact.
> Do you agree ?

Good point. I'll add a comment and clear secure_pending.

>
> > +
> > +    ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> > +}
> > +
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    uint32_t src_dst = get_user_reg(regs, 1);
> > +    uint32_t flags = get_user_reg(regs, 2);
> > +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> > +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > +    if ( !notif_enabled )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    /* Let the SPMC check the destination of the notification */
> > +    return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
> > +                           bitmap_hi);
> > +}
> > +
> > +/*
> > + * Extract a 16-bit ID (index n) from the successful return value from
> > + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
> > + * returned in registers 3 to 7 with four IDs per register for 64-bit
> > + * calling convention and two IDs per register for 32-bit calling
> > + * convention.
> > + */
> > +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
> > +                                 unsigned int n)
> > +{
> > +    unsigned int ids_per_reg;
> > +    unsigned int reg_idx;
> > +    unsigned int reg_shift;
> > +
> > +    if ( smccc_is_conv_64(resp->a0) )
> > +        ids_per_reg = 4;
> > +    else
> > +        ids_per_reg = 2;
> > +
> > +    reg_idx = n / ids_per_reg + 3;
> > +    reg_shift = ( n % ids_per_reg ) * 16;
> > +
> > +    switch ( reg_idx )
> > +    {
> > +    case 3:
> > +        return resp->a3 >> reg_shift;
> > +    case 4:
> > +        return resp->a4 >> reg_shift;
> > +    case 5:
> > +        return resp->a5 >> reg_shift;
> > +    case 6:
> > +        return resp->a6 >> reg_shift;
> > +    case 7:
> > +        return resp->a7 >> reg_shift;
> > +    default:
> > +        ASSERT(0); /* "Can't happen" */
> > +        return 0;
> > +    }
> > +}
> > +
> > +static void notif_irq_handler(int irq, void *data)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> > +    };
> > +    struct arm_smccc_1_2_regs resp;
> > +    unsigned int id_pos;
> > +    unsigned int list_count;
> > +    uint64_t ids_count;
> > +    unsigned int n;
> > +    int32_t res;
> > +
> > +    do {
> > +        arm_smccc_1_2_smc(&arg, &resp);
> > +        res = ffa_get_ret_code(&resp);
> > +        if ( res )
> > +        {
> > +            if ( res != FFA_RET_NO_DATA )
> > +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> > +                       res);
> > +            return;
> > +        }
> > +
> > +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> > +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> > +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> > +
> > +        id_pos = 0;
> > +        for ( n = 0; n < list_count; n++ )
> > +        {
> > +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> > +            struct domain *d;
> > +
>
> If a notification is pending for a secure partition at this stage we are not
> signaling anything so I think this fact should be listed in the limitations to
> say that we do not signal any secondary scheduler if a notification is
> pending for a secure partition.

I agree, I'll add a note in the limitation.

>
> > +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> > +
> > +            if ( d )
> > +            {
> > +                struct ffa_ctx *ctx = d->arch.tee;
> > +
> > +                spin_lock(&ctx->notif.lock);
> > +                ctx->notif.secure_pending = true;
> > +                spin_unlock(&ctx->notif.lock);
> > +
> > +                /*
> > +                 * Since we're only delivering global notification, always
> > +                 * deliver to the first vCPU. It doesn't matter which we
> > +                 * chose, as long as it's available.
> > +                 */
> > +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> > +                                true);
> > +
> > +                put_domain(d);
> > +            }
> > +
> > +            id_pos += count;
> > +        }
> > +
> > +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> > +}
> > +
> > +static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
> > +                                              uint32_t vcpu_count)
> > +{
> > +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
> > +                           0, 0);
> > +}
> > +
> > +static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
> > +{
> > +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
> > +}
> > +
> > +struct notif_irq_info {
> > +    unsigned int irq;
> > +    int ret;
> > +    struct irqaction *action;
> > +};
> > +
> > +static void notif_irq_enable(void *info)
> > +{
> > +    struct notif_irq_info *irq_info = info;
> > +
> > +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> > +    if ( irq_info->ret )
> > +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > +               irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_FEATURES,
> > +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > +    };
> > +    struct notif_irq_info irq_info = { };
> > +    struct arm_smccc_1_2_regs resp;
> > +    unsigned int cpu;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +    if ( resp.a0 != FFA_SUCCESS_32 )
> > +        return;
> > +
> > +    irq_info.irq = resp.a2;
> > +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> > +    {
> > +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> > +               irq_info.irq);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> > +     * IPI to call notif_irq_enable() on each CPU including the current
> > +     * CPU. The struct irqaction is preallocated since we can't allocate
> > +     * memory while in interrupt context.
> > +     */
> > +    for_each_online_cpu(cpu)
> > +    {
> > +        irq_info.action = xmalloc(struct irqaction);
>
> You allocate one action per cpu but you have only one action pointer in your structure
> which means you will overload the previously allocated one and lose track.
>
> You should have a table of actions in your structure instead unless one action is
> enough and can be reused on all cpus and in this case you should move out of
> your loop the allocation part.

That shouldn't be needed because this is done in sequence only one CPU
at a time.

>
> > +        if ( !irq_info.action )
> > +        {
> > +            irq_info.ret = -ENOMEM;
> > +            break;
> > +        }
> > +
> > +        *irq_info.action = (struct irqaction){
> > +            .handler = notif_irq_handler,
> > +            .name = "FF-A notif",
> > +            .free_on_release = 1,
> > +        };
> > +
> > +        on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> > +        if ( irq_info.ret )
> > +        {
> > +            XFREE(irq_info.action);
> > +            break;
> > +        }
> > +    }
> > +
> > +    notif_enabled = !irq_info.ret;
> > +}
> > +
> > +int ffa_notif_domain_init(struct domain *d)
> > +{
> > +    struct ffa_ctx *ctx = d->arch.tee;
> > +    int32_t res;
> > +
> > +    if ( !notif_enabled )
> > +        return 0;
> > +
> > +    res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> > +    if ( res )
> > +        return -ENOMEM;
> > +
> > +    ctx->notif.enabled = true;
> > +
> > +    return 0;
> > +}
> > +
> > +void ffa_notif_domain_destroy(struct domain *d)
> > +{
> > +    struct ffa_ctx *ctx = d->arch.tee;
> > +
> > +    if ( ctx->notif.enabled )
> > +    {
> > +        ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> > +        ctx->notif.enabled = false;
> > +    }
> > +}
> > diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> > index dc1059584828..93a03c6bc672 100644
> > --- a/xen/arch/arm/tee/ffa_partinfo.c
> > +++ b/xen/arch/arm/tee/ffa_partinfo.c
> > @@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> >     }
> > }
> >
> > -bool ffa_partinfo_domain_init(struct domain *d)
> > +int ffa_partinfo_domain_init(struct domain *d)
> > {
> >     unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
> >     struct ffa_ctx *ctx = d->arch.tee;
> > @@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
> >
> >     ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
> >     if ( !ctx->vm_destroy_bitmap )
> > -        return false;
> > +        return -ENOMEM;
> >
> >     for ( n = 0; n < subscr_vm_created_count; n++ )
> >     {
> > @@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
> >     }
> >     vm_destroy_bitmap_init(ctx, n);
> >
> > -    return n == subscr_vm_created_count;
> > +    if ( n != subscr_vm_created_count )
> > +        return -EIO;
> > +
> > +    return 0;
> > }
> >
> > bool ffa_partinfo_domain_destroy(struct domain *d)
> > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> > index 98236cbf14a3..a59c9887774b 100644
> > --- a/xen/arch/arm/tee/ffa_private.h
> > +++ b/xen/arch/arm/tee/ffa_private.h
> > @@ -25,6 +25,7 @@
> > #define FFA_RET_DENIED                  -6
> > #define FFA_RET_RETRY                   -7
> > #define FFA_RET_ABORTED                 -8
> > +#define FFA_RET_NO_DATA                 -9
> >
> > /* FFA_VERSION helpers */
> > #define FFA_VERSION_MAJOR_SHIFT         16U
> > @@ -175,6 +176,21 @@
> >  */
> > #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
> >
> > +/* Flags used in calls to FFA_NOTIFICATION_GET interface  */
> > +#define FFA_NOTIF_FLAG_BITMAP_SP        BIT(0, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_VM        BIT(1, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_SPM       BIT(2, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_HYP       BIT(3, U)
> > +
> > +#define FFA_NOTIF_INFO_GET_MORE_FLAG        BIT(0, U)
> > +#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT    12
> > +#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT   7
> > +#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK    0x1F
> > +
> > +/* Feature IDs used with FFA_FEATURES */
> > +#define FFA_FEATURE_NOTIF_PEND_INTR     0x1U
> > +#define FFA_FEATURE_SCHEDULE_RECV_INTR  0x2U
> > +
> > /* Function IDs */
> > #define FFA_ERROR                       0x84000060U
> > #define FFA_SUCCESS_32                  0x84000061U
> > @@ -213,6 +229,27 @@
> > #define FFA_MEM_FRAG_TX                 0x8400007BU
> > #define FFA_MSG_SEND                    0x8400006EU
> > #define FFA_MSG_POLL                    0x8400006AU
> > +#define FFA_NOTIFICATION_BITMAP_CREATE  0x8400007DU
> > +#define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
> > +#define FFA_NOTIFICATION_BIND           0x8400007FU
> > +#define FFA_NOTIFICATION_UNBIND         0x84000080U
> > +#define FFA_NOTIFICATION_SET            0x84000081U
> > +#define FFA_NOTIFICATION_GET            0x84000082U
> > +#define FFA_NOTIFICATION_INFO_GET_32    0x84000083U
> > +#define FFA_NOTIFICATION_INFO_GET_64    0xC4000083U
> > +
> > +struct ffa_ctx_notif {
> > +    bool enabled;
> > +
> > +    /* Used to serialize access to the rest of this struct */
> > +    spinlock_t lock;
> > +
> > +    /*
> > +     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> > +     * pending global notifications.
> > +     */
> > +    bool secure_pending;
> > +};
> >
> > struct ffa_ctx {
> >     void *rx;
> > @@ -228,6 +265,7 @@ struct ffa_ctx {
> >     struct list_head shm_list;
> >     /* Number of allocated shared memory object */
> >     unsigned int shm_count;
> > +    struct ffa_ctx_notif notif;
> >     /*
> >      * tx_lock is used to serialize access to tx
> >      * rx_lock is used to serialize access to rx
> > @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> > int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
> >
> > bool ffa_partinfo_init(void);
> > -bool ffa_partinfo_domain_init(struct domain *d);
> > +int ffa_partinfo_domain_init(struct domain *d);
> > bool ffa_partinfo_domain_destroy(struct domain *d);
> > int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> >                                       uint32_t w4, uint32_t w5, uint32_t *count,
> > @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> > uint32_t ffa_handle_rxtx_unmap(void);
> > int32_t ffa_handle_rx_release(void);
> >
> > +void ffa_notif_init(void);
> > +int ffa_notif_domain_init(struct domain *d);
> > +void ffa_notif_domain_destroy(struct domain *d);
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> > +
> > static inline uint16_t ffa_get_vm_id(const struct domain *d)
> > {
> >     /* +1 since 0 is reserved for the hypervisor in FF-A */
> >     return d->domain_id + 1;
> > }
> >
> > +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> > +{
> > +    /* -1 to match ffa_get_vm_id() */
> > +    return get_domain_by_id(vm_id - 1);
> > +}
> > +
>
> I think there is a global discussion to have on using get_domain_by_vm_id
> or rcu_lock_live_remote_domain_by_id to make sure the domain is not
> dying when we try to do something with it.
>
> It does not need to be done here as there are other places to adapt but
> i wanted to raise the question.
>
> I would like to know what you and also other maintainers think here.
> @Julien/Michal/Stefano ?
>
> > static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> >                                 register_t v1, register_t v2, register_t v3,
> >                                 register_t v4, register_t v5, register_t v6,
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index e167e14f8df9..1eac9802aa53 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
> > #define GUEST_MAX_VCPUS 128
> >
> > /* Interrupts */
> > +
> > #define GUEST_TIMER_VIRT_PPI    27
> > #define GUEST_TIMER_PHYS_S_PPI  29
> > #define GUEST_TIMER_PHYS_NS_PPI 30
> > @@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
> > #define GUEST_VIRTIO_MMIO_SPI_FIRST   33
> > #define GUEST_VIRTIO_MMIO_SPI_LAST    43
> >
> > +/*
> > + * SGI is the preferred delivery mechanism of FF-A pending notifications or
> > + * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
> > + * as they in a non-virtualized system typically are assigned to the secure
> > + * world. Here we're free to use SGI 8-15 since they are virtual and have
> > + * nothing to do with the secure world.
> > + *
> > + * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
> > + * https://developer.arm.com/documentation/den0094/
> > + */
> > +#define GUEST_FFA_NOTIF_PEND_INTR_ID      8
> > +#define GUEST_FFA_SCHEDULE_RECV_INTR_ID   9
> > +
> > /* PSCI functions */
> > #define PSCI_cpu_suspend 0
> > #define PSCI_cpu_off     1
> > --
> > 2.34.1
> >

Thanks,
Jens

Bertrand Marquis April 26, 2024, 12:19 p.m. UTC | #3

Hi Jens,

> On 26 Apr 2024, at 14:11, Jens Wiklander <[e-mail]> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> <[e-mail]> wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> 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.
>>> 
>>> Update ffa_partinfo_domain_init() to return error code like
>>> ffa_notif_domain_init().
>>> 
>>> Signed-off-by: Jens Wiklander <[e-mail]>
>>> ---
>>> v2->v3:
>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>> setup_irq()
>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>> doesn't conflict with static SGI handlers
>>> 
>>> v1->v2:
>>> - Addressing review comments
>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>> cpu_user_regs *regs as argument.
>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>> an error code.
>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>> ---
>>> xen/arch/arm/irq.c              |   2 +-
>>> xen/arch/arm/tee/Makefile       |   1 +
>>> xen/arch/arm/tee/ffa.c          |  55 ++++-
>>> xen/arch/arm/tee/ffa_notif.c    | 378 ++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
>>> xen/arch/arm/tee/ffa_private.h  |  56 ++++-
>>> xen/include/public/arch-arm.h   |  14 ++
>>> 7 files changed, 507 insertions(+), 8 deletions(-)
>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>> 
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index d7306aa64d50..5224898265a5 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>>>    {
>>>        /* SGIs are always edge-triggered */
>>>        if ( irq < NR_GIC_SGI )
>>> -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
>>> +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>>>        else
>>>            local_irqs_type[irq] = IRQ_TYPE_INVALID;
>>>    }
>>> 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>>> +        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,22 @@ 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(regs);
>>> +        break;
>>> +    case FFA_NOTIFICATION_UNBIND:
>>> +        e = ffa_handle_notification_unbind(regs);
>>> +        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(regs);
>>> +        break;
>>> 
>>>    default:
>>>        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> static int ffa_domain_init(struct domain *d)
>>> {
>>>    struct ffa_ctx *ctx;
>>> +    int ret;
>>> 
>>>    if ( !ffa_version )
>>>        return -ENODEV;
>>> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>>>     * error, so no need for cleanup in this function.
>>>     */
>>> 
>>> -    if ( !ffa_partinfo_domain_init(d) )
>>> -        return -EIO;
>>> +    ret = ffa_partinfo_domain_init(d);
>>> +    if ( ret )
>>> +        return ret;
>>> 
>>> -    return 0;
>>> +    return ffa_notif_domain_init(d);
>>> }
>>> 
>>> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
>>> @@ -423,6 +470,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 +550,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..6bb0804ee2f8
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> @@ -0,0 +1,378 @@
>>> +/* 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(struct cpu_user_regs *regs)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    uint32_t src_dst = get_user_reg(regs, 1);
>>> +    uint32_t flags = get_user_reg(regs, 2);
>>> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    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(struct cpu_user_regs *regs)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    uint32_t src_dst = get_user_reg(regs, 1);
>>> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    /*
>>> +     * 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 & 0xFFFFU) != ffa_get_vm_id(d) )
>>> +    {
>>> +        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;
>>> +    }
>> 
>> In here you never clean the secure_pending flag but in practice there would be
>> no more pending notification if SP and SPM flags are set so the response to
>> notification_info_get would wrongly say there is something pending.
>> 
>> I am not completely sure how this could be handled if both SP and SPM are
>> not set so i would say for now we should at least put a comment in info_get
>> to keep a note of this fact.
>> Do you agree ?
> 
> Good point. I'll add a comment and clear secure_pending.
> 
>> 
>>> +
>>> +    ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
>>> +}
>>> +
>>> +int ffa_handle_notification_set(struct cpu_user_regs *regs)
>>> +{
>>> +    struct domain *d = current->domain;
>>> +    uint32_t src_dst = get_user_reg(regs, 1);
>>> +    uint32_t flags = get_user_reg(regs, 2);
>>> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> +    if ( !notif_enabled )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    /* Let the SPMC check the destination of the notification */
>>> +    return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
>>> +                           bitmap_hi);
>>> +}
>>> +
>>> +/*
>>> + * Extract a 16-bit ID (index n) from the successful return value from
>>> + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
>>> + * returned in registers 3 to 7 with four IDs per register for 64-bit
>>> + * calling convention and two IDs per register for 32-bit calling
>>> + * convention.
>>> + */
>>> +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
>>> +                                 unsigned int n)
>>> +{
>>> +    unsigned int ids_per_reg;
>>> +    unsigned int reg_idx;
>>> +    unsigned int reg_shift;
>>> +
>>> +    if ( smccc_is_conv_64(resp->a0) )
>>> +        ids_per_reg = 4;
>>> +    else
>>> +        ids_per_reg = 2;
>>> +
>>> +    reg_idx = n / ids_per_reg + 3;
>>> +    reg_shift = ( n % ids_per_reg ) * 16;
>>> +
>>> +    switch ( reg_idx )
>>> +    {
>>> +    case 3:
>>> +        return resp->a3 >> reg_shift;
>>> +    case 4:
>>> +        return resp->a4 >> reg_shift;
>>> +    case 5:
>>> +        return resp->a5 >> reg_shift;
>>> +    case 6:
>>> +        return resp->a6 >> reg_shift;
>>> +    case 7:
>>> +        return resp->a7 >> reg_shift;
>>> +    default:
>>> +        ASSERT(0); /* "Can't happen" */
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> +static void notif_irq_handler(int irq, void *data)
>>> +{
>>> +    const struct arm_smccc_1_2_regs arg = {
>>> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
>>> +    };
>>> +    struct arm_smccc_1_2_regs resp;
>>> +    unsigned int id_pos;
>>> +    unsigned int list_count;
>>> +    uint64_t ids_count;
>>> +    unsigned int n;
>>> +    int32_t res;
>>> +
>>> +    do {
>>> +        arm_smccc_1_2_smc(&arg, &resp);
>>> +        res = ffa_get_ret_code(&resp);
>>> +        if ( res )
>>> +        {
>>> +            if ( res != FFA_RET_NO_DATA )
>>> +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
>>> +                       res);
>>> +            return;
>>> +        }
>>> +
>>> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
>>> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
>>> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
>>> +
>>> +        id_pos = 0;
>>> +        for ( n = 0; n < list_count; n++ )
>>> +        {
>>> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
>>> +            struct domain *d;
>>> +
>> 
>> If a notification is pending for a secure partition at this stage we are not
>> signaling anything so I think this fact should be listed in the limitations to
>> say that we do not signal any secondary scheduler if a notification is
>> pending for a secure partition.
> 
> I agree, I'll add a note in the limitation.
> 
>> 
>>> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
>>> +
>>> +            if ( d )
>>> +            {
>>> +                struct ffa_ctx *ctx = d->arch.tee;
>>> +
>>> +                spin_lock(&ctx->notif.lock);
>>> +                ctx->notif.secure_pending = true;
>>> +                spin_unlock(&ctx->notif.lock);
>>> +
>>> +                /*
>>> +                 * Since we're only delivering global notification, always
>>> +                 * deliver to the first vCPU. It doesn't matter which we
>>> +                 * chose, as long as it's available.
>>> +                 */
>>> +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
>>> +                                true);
>>> +
>>> +                put_domain(d);
>>> +            }
>>> +
>>> +            id_pos += count;
>>> +        }
>>> +
>>> +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
>>> +}
>>> +
>>> +static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
>>> +                                              uint32_t vcpu_count)
>>> +{
>>> +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
>>> +                           0, 0);
>>> +}
>>> +
>>> +static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
>>> +{
>>> +    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
>>> +}
>>> +
>>> +struct notif_irq_info {
>>> +    unsigned int irq;
>>> +    int ret;
>>> +    struct irqaction *action;
>>> +};
>>> +
>>> +static void notif_irq_enable(void *info)
>>> +{
>>> +    struct notif_irq_info *irq_info = info;
>>> +
>>> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>> +    if ( irq_info->ret )
>>> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>> +               irq_info->irq, irq_info->ret);
>>> +}
>>> +
>>> +void ffa_notif_init(void)
>>> +{
>>> +    const struct arm_smccc_1_2_regs arg = {
>>> +        .a0 = FFA_FEATURES,
>>> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>> +    };
>>> +    struct notif_irq_info irq_info = { };
>>> +    struct arm_smccc_1_2_regs resp;
>>> +    unsigned int cpu;
>>> +
>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>> +    if ( resp.a0 != FFA_SUCCESS_32 )
>>> +        return;
>>> +
>>> +    irq_info.irq = resp.a2;
>>> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>> +    {
>>> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>> +               irq_info.irq);
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>> +     * IPI to call notif_irq_enable() on each CPU including the current
>>> +     * CPU. The struct irqaction is preallocated since we can't allocate
>>> +     * memory while in interrupt context.
>>> +     */
>>> +    for_each_online_cpu(cpu)
>>> +    {
>>> +        irq_info.action = xmalloc(struct irqaction);
>> 
>> You allocate one action per cpu but you have only one action pointer in your structure
>> which means you will overload the previously allocated one and lose track.
>> 
>> You should have a table of actions in your structure instead unless one action is
>> enough and can be reused on all cpus and in this case you should move out of
>> your loop the allocation part.
> 
> That shouldn't be needed because this is done in sequence only one CPU
> at a time.

Sorry i do not understand here.
You have a loop over each online cpu and on each loop you are assigning
irq_info.action with a newly allocated struct irqaction so you are in practice
overloading on cpu 2 the action that was allocated for cpu 1.

What do you mean by sequence here ?

Cheers
Bertrand

Jens Wiklander April 26, 2024, 12:32 p.m. UTC | #4

Hi Bertrand,

On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
<[e-mail]> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:11, Jens Wiklander <[e-mail]> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> > <[e-mail]> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> wrote:
> >>>
[...]
> >>> +struct notif_irq_info {
> >>> +    unsigned int irq;
> >>> +    int ret;
> >>> +    struct irqaction *action;
> >>> +};
> >>> +
> >>> +static void notif_irq_enable(void *info)
> >>> +{
> >>> +    struct notif_irq_info *irq_info = info;
> >>> +
> >>> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> >>> +    if ( irq_info->ret )
> >>> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>> +               irq_info->irq, irq_info->ret);
> >>> +}
> >>> +
> >>> +void ffa_notif_init(void)
> >>> +{
> >>> +    const struct arm_smccc_1_2_regs arg = {
> >>> +        .a0 = FFA_FEATURES,
> >>> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> >>> +    };
> >>> +    struct notif_irq_info irq_info = { };
> >>> +    struct arm_smccc_1_2_regs resp;
> >>> +    unsigned int cpu;
> >>> +
> >>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>> +    if ( resp.a0 != FFA_SUCCESS_32 )
> >>> +        return;
> >>> +
> >>> +    irq_info.irq = resp.a2;
> >>> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> >>> +    {
> >>> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> >>> +               irq_info.irq);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    /*
> >>> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> >>> +     * IPI to call notif_irq_enable() on each CPU including the current
> >>> +     * CPU. The struct irqaction is preallocated since we can't allocate
> >>> +     * memory while in interrupt context.
> >>> +     */
> >>> +    for_each_online_cpu(cpu)
> >>> +    {
> >>> +        irq_info.action = xmalloc(struct irqaction);
> >>
> >> You allocate one action per cpu but you have only one action pointer in your structure
> >> which means you will overload the previously allocated one and lose track.
> >>
> >> You should have a table of actions in your structure instead unless one action is
> >> enough and can be reused on all cpus and in this case you should move out of
> >> your loop the allocation part.
> >
> > That shouldn't be needed because this is done in sequence only one CPU
> > at a time.
>
> Sorry i do not understand here.
> You have a loop over each online cpu and on each loop you are assigning
> irq_info.action with a newly allocated struct irqaction so you are in practice
> overloading on cpu 2 the action that was allocated for cpu 1.
>
> What do you mean by sequence here ?
>

My understanding is that for_each_online_cpu(cpu) loops over each cpu,
one at a time. The call
on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
returns after notif_irq_enable() has returned on the CPU in question
thanks to the "1" (wait) parameter. So once it has returned &irq_info
isn't used by the other CPU any longer and we can assign a new value
to irq_info.action.

Thanks,
Jens

Bertrand Marquis April 26, 2024, 12:41 p.m. UTC | #5

Hi Jens,

> On 26 Apr 2024, at 14:32, Jens Wiklander <[e-mail]> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
> <[e-mail]> wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 14:11, Jens Wiklander <[e-mail]> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>>> <[e-mail]> wrote:
>>>> 
>>>> Hi Jens,
>>>> 
>>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> wrote:
>>>>> 
> [...]
>>>>> +struct notif_irq_info {
>>>>> +    unsigned int irq;
>>>>> +    int ret;
>>>>> +    struct irqaction *action;
>>>>> +};
>>>>> +
>>>>> +static void notif_irq_enable(void *info)
>>>>> +{
>>>>> +    struct notif_irq_info *irq_info = info;
>>>>> +
>>>>> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>>>> +    if ( irq_info->ret )
>>>>> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>>>> +               irq_info->irq, irq_info->ret);
>>>>> +}
>>>>> +
>>>>> +void ffa_notif_init(void)
>>>>> +{
>>>>> +    const struct arm_smccc_1_2_regs arg = {
>>>>> +        .a0 = FFA_FEATURES,
>>>>> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>>>> +    };
>>>>> +    struct notif_irq_info irq_info = { };
>>>>> +    struct arm_smccc_1_2_regs resp;
>>>>> +    unsigned int cpu;
>>>>> +
>>>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>>>> +    if ( resp.a0 != FFA_SUCCESS_32 )
>>>>> +        return;
>>>>> +
>>>>> +    irq_info.irq = resp.a2;
>>>>> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>>>> +    {
>>>>> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>>>> +               irq_info.irq);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>>>> +     * IPI to call notif_irq_enable() on each CPU including the current
>>>>> +     * CPU. The struct irqaction is preallocated since we can't allocate
>>>>> +     * memory while in interrupt context.
>>>>> +     */
>>>>> +    for_each_online_cpu(cpu)
>>>>> +    {
>>>>> +        irq_info.action = xmalloc(struct irqaction);
>>>> 
>>>> You allocate one action per cpu but you have only one action pointer in your structure
>>>> which means you will overload the previously allocated one and lose track.
>>>> 
>>>> You should have a table of actions in your structure instead unless one action is
>>>> enough and can be reused on all cpus and in this case you should move out of
>>>> your loop the allocation part.
>>> 
>>> That shouldn't be needed because this is done in sequence only one CPU
>>> at a time.
>> 
>> Sorry i do not understand here.
>> You have a loop over each online cpu and on each loop you are assigning
>> irq_info.action with a newly allocated struct irqaction so you are in practice
>> overloading on cpu 2 the action that was allocated for cpu 1.
>> 
>> What do you mean by sequence here ?
>> 
> 
> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> one at a time. The call
> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> returns after notif_irq_enable() has returned on the CPU in question
> thanks to the "1" (wait) parameter. So once it has returned &irq_info
> isn't used by the other CPU any longer and we can assign a new value
> to irq_info.action.

Right so you loose track of what was assigned so you are not able to
free it.
If that is wanted then why saving this in irq.action as you will only have
there the one allocated for the last online cpu.

Cheers
Bertrand

> 
> Thanks,
> Jens

Jens Wiklander April 26, 2024, 1:02 p.m. UTC | #6

On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
<[e-mail]> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:32, Jens Wiklander <[e-mail]> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
> > <[e-mail]> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 14:11, Jens Wiklander <[e-mail]> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> >>> <[e-mail]> wrote:
> >>>>
> >>>> Hi Jens,
> >>>>
> >>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> wrote:
> >>>>>
> > [...]
> >>>>> +struct notif_irq_info {
> >>>>> +    unsigned int irq;
> >>>>> +    int ret;
> >>>>> +    struct irqaction *action;
> >>>>> +};
> >>>>> +
> >>>>> +static void notif_irq_enable(void *info)
> >>>>> +{
> >>>>> +    struct notif_irq_info *irq_info = info;
> >>>>> +
> >>>>> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> >>>>> +    if ( irq_info->ret )
> >>>>> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>>>> +               irq_info->irq, irq_info->ret);
> >>>>> +}
> >>>>> +
> >>>>> +void ffa_notif_init(void)
> >>>>> +{
> >>>>> +    const struct arm_smccc_1_2_regs arg = {
> >>>>> +        .a0 = FFA_FEATURES,
> >>>>> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> >>>>> +    };
> >>>>> +    struct notif_irq_info irq_info = { };
> >>>>> +    struct arm_smccc_1_2_regs resp;
> >>>>> +    unsigned int cpu;
> >>>>> +
> >>>>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>>>> +    if ( resp.a0 != FFA_SUCCESS_32 )
> >>>>> +        return;
> >>>>> +
> >>>>> +    irq_info.irq = resp.a2;
> >>>>> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> >>>>> +    {
> >>>>> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> >>>>> +               irq_info.irq);
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>> +    /*
> >>>>> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> >>>>> +     * IPI to call notif_irq_enable() on each CPU including the current
> >>>>> +     * CPU. The struct irqaction is preallocated since we can't allocate
> >>>>> +     * memory while in interrupt context.
> >>>>> +     */
> >>>>> +    for_each_online_cpu(cpu)
> >>>>> +    {
> >>>>> +        irq_info.action = xmalloc(struct irqaction);
> >>>>
> >>>> You allocate one action per cpu but you have only one action pointer in your structure
> >>>> which means you will overload the previously allocated one and lose track.
> >>>>
> >>>> You should have a table of actions in your structure instead unless one action is
> >>>> enough and can be reused on all cpus and in this case you should move out of
> >>>> your loop the allocation part.
> >>>
> >>> That shouldn't be needed because this is done in sequence only one CPU
> >>> at a time.
> >>
> >> Sorry i do not understand here.
> >> You have a loop over each online cpu and on each loop you are assigning
> >> irq_info.action with a newly allocated struct irqaction so you are in practice
> >> overloading on cpu 2 the action that was allocated for cpu 1.
> >>
> >> What do you mean by sequence here ?
> >>
> >
> > My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> > one at a time. The call
> > on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> > returns after notif_irq_enable() has returned on the CPU in question
> > thanks to the "1" (wait) parameter. So once it has returned &irq_info
> > isn't used by the other CPU any longer and we can assign a new value
> > to irq_info.action.
>
> Right so you loose track of what was assigned so you are not able to
> free it.
> If that is wanted then why saving this in irq.action as you will only have
> there the one allocated for the last online cpu.

Wouldn't release_irq() free it? An error here is unlikely, but we may
be left with a few installed struct irqaction if it occurs. I can add
a more elaborate error path if it's worth the added complexity.

Thanks,
Jens

Bertrand Marquis April 26, 2024, 3:12 p.m. UTC | #7

Hi Jens,

> On 26 Apr 2024, at 15:02, Jens Wiklander <[e-mail]> wrote:
> 
> On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
> <[e-mail]> wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 14:32, Jens Wiklander <[e-mail]> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
>>> <[e-mail]> wrote:
>>>> 
>>>> Hi Jens,
>>>> 
>>>>> On 26 Apr 2024, at 14:11, Jens Wiklander <[e-mail]> wrote:
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>>>>> <[e-mail]> wrote:
>>>>>> 
>>>>>> Hi Jens,
>>>>>> 
>>>>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <[e-mail]> wrote:
>>>>>>> 
>>> [...]
>>>>>>> +struct notif_irq_info {
>>>>>>> +    unsigned int irq;
>>>>>>> +    int ret;
>>>>>>> +    struct irqaction *action;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static void notif_irq_enable(void *info)
>>>>>>> +{
>>>>>>> +    struct notif_irq_info *irq_info = info;
>>>>>>> +
>>>>>>> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>>>>>> +    if ( irq_info->ret )
>>>>>>> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>>>>>> +               irq_info->irq, irq_info->ret);
>>>>>>> +}
>>>>>>> +
>>>>>>> +void ffa_notif_init(void)
>>>>>>> +{
>>>>>>> +    const struct arm_smccc_1_2_regs arg = {
>>>>>>> +        .a0 = FFA_FEATURES,
>>>>>>> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>>>>>> +    };
>>>>>>> +    struct notif_irq_info irq_info = { };
>>>>>>> +    struct arm_smccc_1_2_regs resp;
>>>>>>> +    unsigned int cpu;
>>>>>>> +
>>>>>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>>>>>> +    if ( resp.a0 != FFA_SUCCESS_32 )
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    irq_info.irq = resp.a2;
>>>>>>> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>>>>>> +    {
>>>>>>> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>>>>>> +               irq_info.irq);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>>>>>> +     * IPI to call notif_irq_enable() on each CPU including the current
>>>>>>> +     * CPU. The struct irqaction is preallocated since we can't allocate
>>>>>>> +     * memory while in interrupt context.
>>>>>>> +     */
>>>>>>> +    for_each_online_cpu(cpu)
>>>>>>> +    {
>>>>>>> +        irq_info.action = xmalloc(struct irqaction);
>>>>>> 
>>>>>> You allocate one action per cpu but you have only one action pointer in your structure
>>>>>> which means you will overload the previously allocated one and lose track.
>>>>>> 
>>>>>> You should have a table of actions in your structure instead unless one action is
>>>>>> enough and can be reused on all cpus and in this case you should move out of
>>>>>> your loop the allocation part.
>>>>> 
>>>>> That shouldn't be needed because this is done in sequence only one CPU
>>>>> at a time.
>>>> 
>>>> Sorry i do not understand here.
>>>> You have a loop over each online cpu and on each loop you are assigning
>>>> irq_info.action with a newly allocated struct irqaction so you are in practice
>>>> overloading on cpu 2 the action that was allocated for cpu 1.
>>>> 
>>>> What do you mean by sequence here ?
>>>> 
>>> 
>>> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
>>> one at a time. The call
>>> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
>>> returns after notif_irq_enable() has returned on the CPU in question
>>> thanks to the "1" (wait) parameter. So once it has returned &irq_info
>>> isn't used by the other CPU any longer and we can assign a new value
>>> to irq_info.action.
>> 
>> Right so you loose track of what was assigned so you are not able to
>> free it.
>> If that is wanted then why saving this in irq.action as you will only have
>> there the one allocated for the last online cpu.
> 
> Wouldn't release_irq() free it? An error here is unlikely, but we may
> be left with a few installed struct irqaction if it occurs. I can add
> a more elaborate error path if it's worth the added complexity.

I think just add a comment saying that the irqaction will be freed
upon release_irq so we do not keep a reference to it or something
like that and this will be ok.

The code is in fact a bit misleading because the irqaction is used
inside the function called on other cores through the IPI and there
you actually pass the action. Your structure is only there to transport
the information for the IPI handler.
So please add a comment on top of the notif_irq_info to say that
this structure is used to pass information to and back the notif_irq_enable
executed using an IPI on other cores.

Cheers
Bertrand


> 
> Thanks,
> Jens

Julien Grall April 26, 2024, 5:58 p.m. UTC | #8

Hi Jens,

On 26/04/2024 09:47, Jens Wiklander wrote:
> +static void notif_irq_enable(void *info)
> +{
> +    struct notif_irq_info *irq_info = info;
> +
> +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
In v2, you were using request_irq(). But now you seem to be open-coding 
it. Can you explain why?

> +    if ( irq_info->ret )
> +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> +               irq_info->irq, irq_info->ret);
> +}
> +
> +void ffa_notif_init(void)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = FFA_FEATURES,
> +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> +    };
> +    struct notif_irq_info irq_info = { };
> +    struct arm_smccc_1_2_regs resp;
> +    unsigned int cpu;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +    if ( resp.a0 != FFA_SUCCESS_32 )
> +        return;
> +
> +    irq_info.irq = resp.a2;
> +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> +    {
> +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> +               irq_info.irq);
> +        return;
> +    }
> +
> +    /*
> +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> +     * IPI to call notif_irq_enable() on each CPU including the current
> +     * CPU. The struct irqaction is preallocated since we can't allocate
> +     * memory while in interrupt context.
> +     */
> +    for_each_online_cpu(cpu)
Even though we currently don't support CPU hotplug, you want to add a 
CPU Notifier to also register the IRQ when a CPU is onlined 
ffa_notif_init().

For an example, see time.c. We may also want to consider to enable TEE 
in presmp_initcalls() so we don't need to have a for_each_online_cpu().

Cheers,

Julien Grall April 26, 2024, 6:31 p.m. UTC | #9

Hi Bertrand,

On 26/04/2024 10:20, Bertrand Marquis wrote:
>> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
>> +{
>> +    /* -1 to match ffa_get_vm_id() */
>> +    return get_domain_by_id(vm_id - 1);
>> +}
>> +
> 
> I think there is a global discussion to have on using get_domain_by_vm_id
> or rcu_lock_live_remote_domain_by_id to make sure the domain is not
> dying when we try to do something with it.
> 
> It does not need to be done here as there are other places to adapt but
> i wanted to raise the question.
> 
> I would like to know what you and also other maintainers think here.
> @Julien/Michal/Stefano ?

Good question. I think the intention is get_domain_by_id() should be 
called when you need a reference for longer.

I would consider to involve the REST in this discussion.

Cheers,

Julien Grall April 26, 2024, 7:07 p.m. UTC | #10

Hi Jens,

On 26/04/2024 09:47, Jens Wiklander wrote:
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d7306aa64d50..5224898265a5 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>       {
>           /* SGIs are always edge-triggered */
>           if ( irq < NR_GIC_SGI )
> -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;

This changes seems unrelated to this patch. This wants to be separate 
(if you actually intended to change it).

>           else
>               local_irqs_type[irq] = IRQ_TYPE_INVALID;
>       }
> 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> +        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,22 @@ 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(regs);
> +        break;
> +    case FFA_NOTIFICATION_UNBIND:
> +        e = ffa_handle_notification_unbind(regs);
> +        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(regs);
> +        break;
>   
>       default:
>           gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>   static int ffa_domain_init(struct domain *d)
>   {
>       struct ffa_ctx *ctx;
> +    int ret;
>   
>       if ( !ffa_version )
>           return -ENODEV;
> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>        * error, so no need for cleanup in this function.
>        */
>   
> -    if ( !ffa_partinfo_domain_init(d) )
> -        return -EIO;
> +    ret = ffa_partinfo_domain_init(d);
> +    if ( ret )
> +        return ret;
>   
> -    return 0;
> +    return ffa_notif_domain_init(d);
>   }
>   
>   static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> @@ -423,6 +470,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 +550,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..6bb0804ee2f8
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,378 @@
> +/* 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(struct cpu_user_regs *regs)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t src_dst = get_user_reg(regs, 1);
> +    uint32_t flags = get_user_reg(regs, 2);
> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> +    if ( !notif_enabled )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    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(struct cpu_user_regs *regs)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t src_dst = get_user_reg(regs, 1);
> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> +    if ( !notif_enabled )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    /*
> +     * 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);
> +    }
> +}
> +static void notif_irq_handler(int irq, void *data)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> +    };
> +    struct arm_smccc_1_2_regs resp;
> +    unsigned int id_pos;
> +    unsigned int list_count;
> +    uint64_t ids_count;
> +    unsigned int n;
> +    int32_t res;
> +
> +    do {
> +        arm_smccc_1_2_smc(&arg, &resp);
> +        res = ffa_get_ret_code(&resp);
> +        if ( res )
> +        {
> +            if ( res != FFA_RET_NO_DATA )
> +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> +                       res);
> +            return;
> +        }
> +
> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> +
> +        id_pos = 0;
> +        for ( n = 0; n < list_count; n++ )
> +        {
> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> +            struct domain *d;
> +
> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));

Thinking a bit more about the question from Bertrand about 
get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure 
whether either are ok here.

If I am not mistaken, d->arch.tee will be freed as part of the domain 
teardown process. This means you can have the following scenario:

CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)

CPU1: call domain_kill()
CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)

d->arch.tee is now a dangling pointer

CPU0: access d->arch.tee

This implies you may need to gain a global lock (I don't have a better 
idea so far) to protect the IRQ handler against domains teardown.

Did I miss anything?

> +
> +            if ( d )
> +            {
> +                struct ffa_ctx *ctx = d->arch.tee;
> +
> +                spin_lock(&ctx->notif.lock);
> +                ctx->notif.secure_pending = true;
> +                spin_unlock(&ctx->notif.lock);


AFAICT, the spinlock is used with IRQ enabled (see 
ffa_handle_notification_info_get()) but also in an IRQ handler. So to 
prevent deadlock, you will want to use spin_lock_irq* helpers.

That said, I don't think you need a spin_lock(). You could use atomic 
operations instead. For the first hunk, you could use 
test_and_clear_bool(). E.g.:

if ( test_and_clear_bool(ctx.notif.secure_pending) )

For the second part, it might be fine to use ACCESS_ONCE().

> +
> +                /*
> +                 * Since we're only delivering global notification, always
> +                 * deliver to the first vCPU. It doesn't matter which we
> +                 * chose, as long as it's available.

What if vCPU0 is offlined?

> +                 */
> +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> +                                true);
> +
> +                put_domain(d);
> +            }
> +
> +            id_pos += count;
> +        }
> +
> +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}

[..]

> +struct ffa_ctx_notif {
> +    bool enabled;
> +
> +    /* Used to serialize access to the rest of this struct */
> +    spinlock_t lock;

Regardless what I wrote above, I can't seem to find a call to 
spin_lock_init(). You will want it to initialize.

Also, it would be best if we keep the two booleans close to each other 
so we limit the amount of padding in the structure.

> +
> +    /*
> +     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> +     * pending global notifications.
> +     */
> +    bool secure_pending;
> +};
>   
>   struct ffa_ctx {
>       void *rx;
> @@ -228,6 +265,7 @@ struct ffa_ctx {
>       struct list_head shm_list;
>       /* Number of allocated shared memory object */
>       unsigned int shm_count;
> +    struct ffa_ctx_notif notif;
>       /*
>        * tx_lock is used to serialize access to tx
>        * rx_lock is used to serialize access to rx
> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
>   int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>   
>   bool ffa_partinfo_init(void);
> -bool ffa_partinfo_domain_init(struct domain *d);
> +int ffa_partinfo_domain_init(struct domain *d);
>   bool ffa_partinfo_domain_destroy(struct domain *d);
>   int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
>                                         uint32_t w4, uint32_t w5, uint32_t *count,
> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
>   uint32_t ffa_handle_rxtx_unmap(void);
>   int32_t ffa_handle_rx_release(void);
>   
> +void ffa_notif_init(void);
> +int ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> +
>   static inline uint16_t ffa_get_vm_id(const struct domain *d)
>   {
>       /* +1 since 0 is reserved for the hypervisor in FF-A */
>       return d->domain_id + 1;
>   }
>   
> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> +{

Is this expected to be called with vm_id == 0? If not, then I would 
consider to add an ASSERT(vm_id != 0). If yes, then I please add a 
runtime check and return NULL.

> +    /* -1 to match ffa_get_vm_id() */
> +    return get_domain_by_id(vm_id - 1);
> +}
> +
>   static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
>                                   register_t v1, register_t v2, register_t v3,
>                                   register_t v4, register_t v5, register_t v6,

Cheers,

Bertrand Marquis April 29, 2024, 7:20 a.m. UTC | #11

Hi Julien,

> On 26 Apr 2024, at 21:07, Julien Grall <[e-mail]> wrote:
> 
> Hi Jens,
> 
> On 26/04/2024 09:47, Jens Wiklander wrote:
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index d7306aa64d50..5224898265a5 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>>      {
>>          /* SGIs are always edge-triggered */
>>          if ( irq < NR_GIC_SGI )
>> -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
>> +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> 
> This changes seems unrelated to this patch. This wants to be separate (if you actually intended to change it).
> 
>>          else
>>              local_irqs_type[irq] = IRQ_TYPE_INVALID;
>>      }
>> 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>> +        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,22 @@ 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(regs);
>> +        break;
>> +    case FFA_NOTIFICATION_UNBIND:
>> +        e = ffa_handle_notification_unbind(regs);
>> +        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(regs);
>> +        break;
>>        default:
>>          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>  static int ffa_domain_init(struct domain *d)
>>  {
>>      struct ffa_ctx *ctx;
>> +    int ret;
>>        if ( !ffa_version )
>>          return -ENODEV;
>> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>>       * error, so no need for cleanup in this function.
>>       */
>>  -    if ( !ffa_partinfo_domain_init(d) )
>> -        return -EIO;
>> +    ret = ffa_partinfo_domain_init(d);
>> +    if ( ret )
>> +        return ret;
>>  -    return 0;
>> +    return ffa_notif_domain_init(d);
>>  }
>>    static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
>> @@ -423,6 +470,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 +550,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..6bb0804ee2f8
>> --- /dev/null
>> +++ b/xen/arch/arm/tee/ffa_notif.c
>> @@ -0,0 +1,378 @@
>> +/* 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(struct cpu_user_regs *regs)
>> +{
>> +    struct domain *d = current->domain;
>> +    uint32_t src_dst = get_user_reg(regs, 1);
>> +    uint32_t flags = get_user_reg(regs, 2);
>> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
>> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
>> +
>> +    if ( !notif_enabled )
>> +        return FFA_RET_NOT_SUPPORTED;
>> +
>> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>> +        return FFA_RET_INVALID_PARAMETERS;
>> +
>> +    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(struct cpu_user_regs *regs)
>> +{
>> +    struct domain *d = current->domain;
>> +    uint32_t src_dst = get_user_reg(regs, 1);
>> +    uint32_t bitmap_lo = get_user_reg(regs, 3);
>> +    uint32_t bitmap_hi = get_user_reg(regs, 4);
>> +
>> +    if ( !notif_enabled )
>> +        return FFA_RET_NOT_SUPPORTED;
>> +
>> +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>> +        return FFA_RET_INVALID_PARAMETERS;
>> +
>> +    /*
>> +     * 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);
>> +    }
>> +}
>> +static void notif_irq_handler(int irq, void *data)
>> +{
>> +    const struct arm_smccc_1_2_regs arg = {
>> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
>> +    };
>> +    struct arm_smccc_1_2_regs resp;
>> +    unsigned int id_pos;
>> +    unsigned int list_count;
>> +    uint64_t ids_count;
>> +    unsigned int n;
>> +    int32_t res;
>> +
>> +    do {
>> +        arm_smccc_1_2_smc(&arg, &resp);
>> +        res = ffa_get_ret_code(&resp);
>> +        if ( res )
>> +        {
>> +            if ( res != FFA_RET_NO_DATA )
>> +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
>> +                       res);
>> +            return;
>> +        }
>> +
>> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
>> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
>> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
>> +
>> +        id_pos = 0;
>> +        for ( n = 0; n < list_count; n++ )
>> +        {
>> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
>> +            struct domain *d;
>> +
>> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> 
> Thinking a bit more about the question from Bertrand about get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok here.
> 
> If I am not mistaken, d->arch.tee will be freed as part of the domain teardown process. This means you can have the following scenario:
> 
> CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
> 
> CPU1: call domain_kill()
> CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
> 
> d->arch.tee is now a dangling pointer
> 
> CPU0: access d->arch.tee
> 
> This implies you may need to gain a global lock (I don't have a better idea so far) to protect the IRQ handler against domains teardown.
> 
> Did I miss anything?

I think you are right which is why I was thinking to use rcu_lock_live_remote_domain_by_id to only get a reference
to the domain if it is not dying.

From the comment in sched.h:
/*
 * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
 * This is the preferred function if the returned domain reference
 * is short lived,  but it cannot be used if the domain reference needs
 * to be kept beyond the current scope (e.g., across a softirq).
 * The returned domain reference must be discarded using rcu_unlock_domain().
 */

Now the question of short lived should be challenged but I do not think we can
consider the current code as "long lived".

It would be a good idea to start a mailing list thread discussion with other
maintainers on the subject to confirm.
@Jens: as i will be off for some time, would you mind doing it ?

> 
>> +
>> +            if ( d )
>> +            {
>> +                struct ffa_ctx *ctx = d->arch.tee;
>> +
>> +                spin_lock(&ctx->notif.lock);
>> +                ctx->notif.secure_pending = true;
>> +                spin_unlock(&ctx->notif.lock);
> 
> 
> AFAICT, the spinlock is used with IRQ enabled (see ffa_handle_notification_info_get()) but also in an IRQ handler. So to prevent deadlock, you will want to use spin_lock_irq* helpers.
> 
> That said, I don't think you need a spin_lock(). You could use atomic operations instead. For the first hunk, you could use test_and_clear_bool(). E.g.:
> 
> if ( test_and_clear_bool(ctx.notif.secure_pending) )
> 
> For the second part, it might be fine to use ACCESS_ONCE().
> 
>> +
>> +                /*
>> +                 * Since we're only delivering global notification, always
>> +                 * deliver to the first vCPU. It doesn't matter which we
>> +                 * chose, as long as it's available.
> 
> What if vCPU0 is offlined?
> 
>> +                 */
>> +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
>> +                                true);
>> +
>> +                put_domain(d);
>> +            }
>> +
>> +            id_pos += count;
>> +        }
>> +
>> +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
>> +}
> 
> [..]
> 
>> +struct ffa_ctx_notif {
>> +    bool enabled;
>> +
>> +    /* Used to serialize access to the rest of this struct */
>> +    spinlock_t lock;
> 
> Regardless what I wrote above, I can't seem to find a call to spin_lock_init(). You will want it to initialize.
> 
> Also, it would be best if we keep the two booleans close to each other so we limit the amount of padding in the structure.
> 
>> +
>> +    /*
>> +     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
>> +     * pending global notifications.
>> +     */
>> +    bool secure_pending;
>> +};
>>    struct ffa_ctx {
>>      void *rx;
>> @@ -228,6 +265,7 @@ struct ffa_ctx {
>>      struct list_head shm_list;
>>      /* Number of allocated shared memory object */
>>      unsigned int shm_count;
>> +    struct ffa_ctx_notif notif;
>>      /*
>>       * tx_lock is used to serialize access to tx
>>       * rx_lock is used to serialize access to rx
>> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
>>  int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>>    bool ffa_partinfo_init(void);
>> -bool ffa_partinfo_domain_init(struct domain *d);
>> +int ffa_partinfo_domain_init(struct domain *d);
>>  bool ffa_partinfo_domain_destroy(struct domain *d);
>>  int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
>>                                        uint32_t w4, uint32_t w5, uint32_t *count,
>> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
>>  uint32_t ffa_handle_rxtx_unmap(void);
>>  int32_t ffa_handle_rx_release(void);
>>  +void ffa_notif_init(void);
>> +int ffa_notif_domain_init(struct domain *d);
>> +void ffa_notif_domain_destroy(struct domain *d);
>> +
>> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
>> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
>> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
>> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
>> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
>> +
>>  static inline uint16_t ffa_get_vm_id(const struct domain *d)
>>  {
>>      /* +1 since 0 is reserved for the hypervisor in FF-A */
>>      return d->domain_id + 1;
>>  }
>>  +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
>> +{
> 
> Is this expected to be called with vm_id == 0? If not, then I would consider to add an ASSERT(vm_id != 0). If yes, then I please add a runtime check and return NULL.
> 
>> +    /* -1 to match ffa_get_vm_id() */
>> +    return get_domain_by_id(vm_id - 1);
>> +}
>> +
>>  static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
>>                                  register_t v1, register_t v2, register_t v3,
>>                                  register_t v4, register_t v5, register_t v6,

Cheers
Bertrand

Jens Wiklander April 29, 2024, 8:43 a.m. UTC | #12

Hi Julien,

On Fri, Apr 26, 2024 at 9:07 PM Julien Grall <[e-mail]> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d7306aa64d50..5224898265a5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> >       {
> >           /* SGIs are always edge-triggered */
> >           if ( irq < NR_GIC_SGI )
> > -            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> > +            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>
> This changes seems unrelated to this patch. This wants to be separate
> (if you actually intended to change it).

I'm sorry, my bad. I meant for this to go into "xen/arm: allow
dynamically assigned SGI handlers".

>
> >           else
> >               local_irqs_type[irq] = IRQ_TYPE_INVALID;
> >       }
> > 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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> > +        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,22 @@ 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(regs);
> > +        break;
> > +    case FFA_NOTIFICATION_UNBIND:
> > +        e = ffa_handle_notification_unbind(regs);
> > +        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(regs);
> > +        break;
> >
> >       default:
> >           gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >   static int ffa_domain_init(struct domain *d)
> >   {
> >       struct ffa_ctx *ctx;
> > +    int ret;
> >
> >       if ( !ffa_version )
> >           return -ENODEV;
> > @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> >        * error, so no need for cleanup in this function.
> >        */
> >
> > -    if ( !ffa_partinfo_domain_init(d) )
> > -        return -EIO;
> > +    ret = ffa_partinfo_domain_init(d);
> > +    if ( ret )
> > +        return ret;
> >
> > -    return 0;
> > +    return ffa_notif_domain_init(d);
> >   }
> >
> >   static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> > @@ -423,6 +470,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 +550,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..6bb0804ee2f8
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
> > @@ -0,0 +1,378 @@
> > +/* 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(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    uint32_t src_dst = get_user_reg(regs, 1);
> > +    uint32_t flags = get_user_reg(regs, 2);
> > +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> > +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > +    if ( !notif_enabled )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    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(struct cpu_user_regs *regs)
> > +{
> > +    struct domain *d = current->domain;
> > +    uint32_t src_dst = get_user_reg(regs, 1);
> > +    uint32_t bitmap_lo = get_user_reg(regs, 3);
> > +    uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > +    if ( !notif_enabled )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    /*
> > +     * 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);
> > +    }
> > +}
> > +static void notif_irq_handler(int irq, void *data)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> > +    };
> > +    struct arm_smccc_1_2_regs resp;
> > +    unsigned int id_pos;
> > +    unsigned int list_count;
> > +    uint64_t ids_count;
> > +    unsigned int n;
> > +    int32_t res;
> > +
> > +    do {
> > +        arm_smccc_1_2_smc(&arg, &resp);
> > +        res = ffa_get_ret_code(&resp);
> > +        if ( res )
> > +        {
> > +            if ( res != FFA_RET_NO_DATA )
> > +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> > +                       res);
> > +            return;
> > +        }
> > +
> > +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> > +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> > +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> > +
> > +        id_pos = 0;
> > +        for ( n = 0; n < list_count; n++ )
> > +        {
> > +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> > +            struct domain *d;
> > +
> > +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
>
> Thinking a bit more about the question from Bertrand about
> get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure
> whether either are ok here.
>
> If I am not mistaken, d->arch.tee will be freed as part of the domain
> teardown process. This means you can have the following scenario:
>
> CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
>
> CPU1: call domain_kill()
> CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
>
> d->arch.tee is now a dangling pointer
>
> CPU0: access d->arch.tee
>
> This implies you may need to gain a global lock (I don't have a better
> idea so far) to protect the IRQ handler against domains teardown.
>
> Did I miss anything?

Thanks for the explanation. I'll reply to Bertrands answer.

>
> > +
> > +            if ( d )
> > +            {
> > +                struct ffa_ctx *ctx = d->arch.tee;
> > +
> > +                spin_lock(&ctx->notif.lock);
> > +                ctx->notif.secure_pending = true;
> > +                spin_unlock(&ctx->notif.lock);
>
>
> AFAICT, the spinlock is used with IRQ enabled (see
> ffa_handle_notification_info_get()) but also in an IRQ handler. So to
> prevent deadlock, you will want to use spin_lock_irq* helpers.
>
> That said, I don't think you need a spin_lock(). You could use atomic
> operations instead. For the first hunk, you could use
> test_and_clear_bool(). E.g.:
>
> if ( test_and_clear_bool(ctx.notif.secure_pending) )
>
> For the second part, it might be fine to use ACCESS_ONCE().

Thanks, I'll update the code accordingly.

>
> > +
> > +                /*
> > +                 * Since we're only delivering global notification, always
> > +                 * deliver to the first vCPU. It doesn't matter which we
> > +                 * chose, as long as it's available.
>
> What if vCPU0 is offlined?

Good question, would the following work?

for_each_vcpu(d, vcpu)
{
    if ( is_vcpu_online(vcpu)
    {
        vgic_inject_irq(d, vcpu, GUEST_FFA_NOTIF_PEND_INTR_ID, true);
        break;
    }
}
if ( !vcpu )
    printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs offline\");

>
> > +                 */
> > +                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> > +                                true);
> > +
> > +                put_domain(d);
> > +            }
> > +
> > +            id_pos += count;
> > +        }
> > +
> > +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> > +}
>
> [..]
>
> > +struct ffa_ctx_notif {
> > +    bool enabled;
> > +
> > +    /* Used to serialize access to the rest of this struct */
> > +    spinlock_t lock;
>
> Regardless what I wrote above, I can't seem to find a call to
> spin_lock_init(). You will want it to initialize.
>
> Also, it would be best if we keep the two booleans close to each other
> so we limit the amount of padding in the structure.

Good point, I'll remove the spinlock and update the code accordingly.

>
> > +
> > +    /*
> > +     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> > +     * pending global notifications.
> > +     */
> > +    bool secure_pending;
> > +};
> >
> >   struct ffa_ctx {
> >       void *rx;
> > @@ -228,6 +265,7 @@ struct ffa_ctx {
> >       struct list_head shm_list;
> >       /* Number of allocated shared memory object */
> >       unsigned int shm_count;
> > +    struct ffa_ctx_notif notif;
> >       /*
> >        * tx_lock is used to serialize access to tx
> >        * rx_lock is used to serialize access to rx
> > @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> >   int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
> >
> >   bool ffa_partinfo_init(void);
> > -bool ffa_partinfo_domain_init(struct domain *d);
> > +int ffa_partinfo_domain_init(struct domain *d);
> >   bool ffa_partinfo_domain_destroy(struct domain *d);
> >   int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> >                                         uint32_t w4, uint32_t w5, uint32_t *count,
> > @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> >   uint32_t ffa_handle_rxtx_unmap(void);
> >   int32_t ffa_handle_rx_release(void);
> >
> > +void ffa_notif_init(void);
> > +int ffa_notif_domain_init(struct domain *d);
> > +void ffa_notif_domain_destroy(struct domain *d);
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> > +
> >   static inline uint16_t ffa_get_vm_id(const struct domain *d)
> >   {
> >       /* +1 since 0 is reserved for the hypervisor in FF-A */
> >       return d->domain_id + 1;
> >   }
> >
> > +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> > +{
>
> Is this expected to be called with vm_id == 0? If not, then I would
> consider to add an ASSERT(vm_id != 0). If yes, then I please add a
> runtime check and return NULL.

vm_id should not be 0, I'll add an ASSERT() and a check in
notif_irq_handler() that vm_id isn't 0.

Thanks,
Jens

>
> > +    /* -1 to match ffa_get_vm_id() */
> > +    return get_domain_by_id(vm_id - 1);
> > +}
> > +
> >   static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> >                                   register_t v1, register_t v2, register_t v3,
> >                                   register_t v4, register_t v5, register_t v6,
>
> Cheers,
>
> --
> Julien Grall

Jens Wiklander April 29, 2024, 9:49 a.m. UTC | #13

Hi Bertrand,

On Mon, Apr 29, 2024 at 9:20 AM Bertrand Marquis
<[e-mail]> wrote:
[...]
> >> +static void notif_irq_handler(int irq, void *data)
> >> +{
> >> +    const struct arm_smccc_1_2_regs arg = {
> >> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> >> +    };
> >> +    struct arm_smccc_1_2_regs resp;
> >> +    unsigned int id_pos;
> >> +    unsigned int list_count;
> >> +    uint64_t ids_count;
> >> +    unsigned int n;
> >> +    int32_t res;
> >> +
> >> +    do {
> >> +        arm_smccc_1_2_smc(&arg, &resp);
> >> +        res = ffa_get_ret_code(&resp);
> >> +        if ( res )
> >> +        {
> >> +            if ( res != FFA_RET_NO_DATA )
> >> +                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> >> +                       res);
> >> +            return;
> >> +        }
> >> +
> >> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> >> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> >> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> >> +
> >> +        id_pos = 0;
> >> +        for ( n = 0; n < list_count; n++ )
> >> +        {
> >> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> >> +            struct domain *d;
> >> +
> >> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> >
> > Thinking a bit more about the question from Bertrand about get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok here.
> >
> > If I am not mistaken, d->arch.tee will be freed as part of the domain teardown process. This means you can have the following scenario:
> >
> > CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
> >
> > CPU1: call domain_kill()
> > CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
> >
> > d->arch.tee is now a dangling pointer
> >
> > CPU0: access d->arch.tee
> >
> > This implies you may need to gain a global lock (I don't have a better idea so far) to protect the IRQ handler against domains teardown.
> >
> > Did I miss anything?
>
> I think you are right which is why I was thinking to use rcu_lock_live_remote_domain_by_id to only get a reference
> to the domain if it is not dying.
>
> From the comment in sched.h:
> /*
>  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>  * This is the preferred function if the returned domain reference
>  * is short lived,  but it cannot be used if the domain reference needs
>  * to be kept beyond the current scope (e.g., across a softirq).
>  * The returned domain reference must be discarded using rcu_unlock_domain().
>  */
>
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".
>
> It would be a good idea to start a mailing list thread discussion with other
> maintainers on the subject to confirm.
> @Jens: as i will be off for some time, would you mind doing it ?

Sure, first I'll send out a new patch set with the current comments
addressed. I'll update to use rcu_lock_live_remote_domain_by_id() both
because it makes more sense than the current code, and also to have a
good base for the discussion.

Thanks,
Jens

Jens Wiklander April 29, 2024, 9:55 a.m. UTC | #14

Hi Julien,

On Fri, Apr 26, 2024 at 7:58 PM Julien Grall <[e-mail]> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
> > +static void notif_irq_enable(void *info)
> > +{
> > +    struct notif_irq_info *irq_info = info;
> > +
> > +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> In v2, you were using request_irq(). But now you seem to be open-coding
> it. Can you explain why?

It's because request_irq() does a memory allocation that can't be done
in interrupt context.
>
> > +    if ( irq_info->ret )
> > +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > +               irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_FEATURES,
> > +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > +    };
> > +    struct notif_irq_info irq_info = { };
> > +    struct arm_smccc_1_2_regs resp;
> > +    unsigned int cpu;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +    if ( resp.a0 != FFA_SUCCESS_32 )
> > +        return;
> > +
> > +    irq_info.irq = resp.a2;
> > +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> > +    {
> > +        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> > +               irq_info.irq);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> > +     * IPI to call notif_irq_enable() on each CPU including the current
> > +     * CPU. The struct irqaction is preallocated since we can't allocate
> > +     * memory while in interrupt context.
> > +     */
> > +    for_each_online_cpu(cpu)
> Even though we currently don't support CPU hotplug, you want to add a
> CPU Notifier to also register the IRQ when a CPU is onlined
> ffa_notif_init().
>
> For an example, see time.c. We may also want to consider to enable TEE
> in presmp_initcalls() so we don't need to have a for_each_online_cpu().

I was considering that too. I'll update the code.

Thanks,
Jens

Julien Grall April 29, 2024, 8:55 p.m. UTC | #15

Hi Bertrand,

On 29/04/2024 08:20, Bertrand Marquis wrote:
>  From the comment in sched.h:
> /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>   * This is the preferred function if the returned domain reference
>   * is short lived,  but it cannot be used if the domain reference needs
>   * to be kept beyond the current scope (e.g., across a softirq).
>   * The returned domain reference must be discarded using rcu_unlock_domain().
>   */
> 
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".

Actually, I am not entirely sure you can use put_domain() in interrupt 
context. If you look at the implementation of domain_destroy() it takes 
a spin lock without disabling the interrupts.

The same spinlock is taken in domain_create(). So there is a potential 
deadlock.

Which means the decision between the two is not only about liveness.

Cheers,

Patch

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d7306aa64d50..5224898265a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -155,7 +155,7 @@  void __init init_IRQ(void)
     {
         /* SGIs are always edge-triggered */
         if ( irq < NR_GIC_SGI )
-            local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+            local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
         else
             local_irqs_type[irq] = IRQ_TYPE_INVALID;
     }
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..912e905a27bd 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, GUEST_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, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
+        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,22 @@  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(regs);
+        break;
+    case FFA_NOTIFICATION_UNBIND:
+        e = ffa_handle_notification_unbind(regs);
+        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(regs);
+        break;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -322,6 +367,7 @@  static bool ffa_handle_call(struct cpu_user_regs *regs)
 static int ffa_domain_init(struct domain *d)
 {
     struct ffa_ctx *ctx;
+    int ret;
 
     if ( !ffa_version )
         return -ENODEV;
@@ -345,10 +391,11 @@  static int ffa_domain_init(struct domain *d)
      * error, so no need for cleanup in this function.
      */
 
-    if ( !ffa_partinfo_domain_init(d) )
-        return -EIO;
+    ret = ffa_partinfo_domain_init(d);
+    if ( ret )
+        return ret;
 
-    return 0;
+    return ffa_notif_domain_init(d);
 }
 
 static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
@@ -423,6 +470,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 +550,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..6bb0804ee2f8
--- /dev/null
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -0,0 +1,378 @@ 
+/* 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(struct cpu_user_regs *regs)
+{
+    struct domain *d = current->domain;
+    uint32_t src_dst = get_user_reg(regs, 1);
+    uint32_t flags = get_user_reg(regs, 2);
+    uint32_t bitmap_lo = get_user_reg(regs, 3);
+    uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+    if ( !notif_enabled )
+        return FFA_RET_NOT_SUPPORTED;
+
+    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    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(struct cpu_user_regs *regs)
+{
+    struct domain *d = current->domain;
+    uint32_t src_dst = get_user_reg(regs, 1);
+    uint32_t bitmap_lo = get_user_reg(regs, 3);
+    uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+    if ( !notif_enabled )
+        return FFA_RET_NOT_SUPPORTED;
+
+    if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    /*
+     * 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 & 0xFFFFU) != ffa_get_vm_id(d) )
+    {
+        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(struct cpu_user_regs *regs)
+{
+    struct domain *d = current->domain;
+    uint32_t src_dst = get_user_reg(regs, 1);
+    uint32_t flags = get_user_reg(regs, 2);
+    uint32_t bitmap_lo = get_user_reg(regs, 3);
+    uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+    if ( !notif_enabled )
+        return FFA_RET_NOT_SUPPORTED;
+
+    if ( (src_dst >> 16) != ffa_get_vm_id(d) )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    /* Let the SPMC check the destination of the notification */
+    return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
+                           bitmap_hi);
+}
+
+/*
+ * Extract a 16-bit ID (index n) from the successful return value from
+ * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
+ * returned in registers 3 to 7 with four IDs per register for 64-bit
+ * calling convention and two IDs per register for 32-bit calling
+ * convention.
+ */
+static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
+                                 unsigned int n)
+{
+    unsigned int ids_per_reg;
+    unsigned int reg_idx;
+    unsigned int reg_shift;
+
+    if ( smccc_is_conv_64(resp->a0) )
+        ids_per_reg = 4;
+    else
+        ids_per_reg = 2;
+
+    reg_idx = n / ids_per_reg + 3;
+    reg_shift = ( n % ids_per_reg ) * 16;
+
+    switch ( reg_idx )
+    {
+    case 3:
+        return resp->a3 >> reg_shift;
+    case 4:
+        return resp->a4 >> reg_shift;
+    case 5:
+        return resp->a5 >> reg_shift;
+    case 6:
+        return resp->a6 >> reg_shift;
+    case 7:
+        return resp->a7 >> reg_shift;
+    default:
+        ASSERT(0); /* "Can't happen" */
+        return 0;
+    }
+}
+
+static void notif_irq_handler(int irq, void *data)
+{
+    const struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_NOTIFICATION_INFO_GET_64,
+    };
+    struct arm_smccc_1_2_regs resp;
+    unsigned int id_pos;
+    unsigned int list_count;
+    uint64_t ids_count;
+    unsigned int n;
+    int32_t res;
+
+    do {
+        arm_smccc_1_2_smc(&arg, &resp);
+        res = ffa_get_ret_code(&resp);
+        if ( res )
+        {
+            if ( res != FFA_RET_NO_DATA )
+                printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
+                       res);
+            return;
+        }
+
+        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
+        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
+                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
+
+        id_pos = 0;
+        for ( n = 0; n < list_count; n++ )
+        {
+            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
+            struct domain *d;
+
+            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
+
+            if ( d )
+            {
+                struct ffa_ctx *ctx = d->arch.tee;
+
+                spin_lock(&ctx->notif.lock);
+                ctx->notif.secure_pending = true;
+                spin_unlock(&ctx->notif.lock);
+
+                /*
+                 * Since we're only delivering global notification, always
+                 * deliver to the first vCPU. It doesn't matter which we
+                 * chose, as long as it's available.
+                 */
+                vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
+                                true);
+
+                put_domain(d);
+            }
+
+            id_pos += count;
+        }
+
+    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
+}
+
+static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
+                                              uint32_t vcpu_count)
+{
+    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
+                           0, 0);
+}
+
+static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
+{
+    return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
+}
+
+struct notif_irq_info {
+    unsigned int irq;
+    int ret;
+    struct irqaction *action;
+};
+
+static void notif_irq_enable(void *info)
+{
+    struct notif_irq_info *irq_info = info;
+
+    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
+    if ( irq_info->ret )
+        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
+               irq_info->irq, irq_info->ret);
+}
+
+void ffa_notif_init(void)
+{
+    const struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_FEATURES,
+        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+    };
+    struct notif_irq_info irq_info = { };
+    struct arm_smccc_1_2_regs resp;
+    unsigned int cpu;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+    if ( resp.a0 != FFA_SUCCESS_32 )
+        return;
+
+    irq_info.irq = resp.a2;
+    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
+    {
+        printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
+               irq_info.irq);
+        return;
+    }
+
+    /*
+     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
+     * IPI to call notif_irq_enable() on each CPU including the current
+     * CPU. The struct irqaction is preallocated since we can't allocate
+     * memory while in interrupt context.
+     */
+    for_each_online_cpu(cpu)
+    {
+        irq_info.action = xmalloc(struct irqaction);
+        if ( !irq_info.action )
+        {
+            irq_info.ret = -ENOMEM;
+            break;
+        }
+
+        *irq_info.action = (struct irqaction){
+            .handler = notif_irq_handler,
+            .name = "FF-A notif",
+            .free_on_release = 1,
+        };
+
+        on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
+        if ( irq_info.ret )
+        {
+            XFREE(irq_info.action);
+            break;
+        }
+    }
+
+    notif_enabled = !irq_info.ret;
+}
+
+int ffa_notif_domain_init(struct domain *d)
+{
+    struct ffa_ctx *ctx = d->arch.tee;
+    int32_t res;
+
+    if ( !notif_enabled )
+        return 0;
+
+    res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
+    if ( res )
+        return -ENOMEM;
+
+    ctx->notif.enabled = true;
+
+    return 0;
+}
+
+void ffa_notif_domain_destroy(struct domain *d)
+{
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    if ( ctx->notif.enabled )
+    {
+        ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
+        ctx->notif.enabled = false;
+    }
+}
diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
index dc1059584828..93a03c6bc672 100644
--- a/xen/arch/arm/tee/ffa_partinfo.c
+++ b/xen/arch/arm/tee/ffa_partinfo.c
@@ -306,7 +306,7 @@  static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
     }
 }
 
-bool ffa_partinfo_domain_init(struct domain *d)
+int ffa_partinfo_domain_init(struct domain *d)
 {
     unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
     struct ffa_ctx *ctx = d->arch.tee;
@@ -315,7 +315,7 @@  bool ffa_partinfo_domain_init(struct domain *d)
 
     ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
     if ( !ctx->vm_destroy_bitmap )
-        return false;
+        return -ENOMEM;
 
     for ( n = 0; n < subscr_vm_created_count; n++ )
     {
@@ -330,7 +330,10 @@  bool ffa_partinfo_domain_init(struct domain *d)
     }
     vm_destroy_bitmap_init(ctx, n);
 
-    return n == subscr_vm_created_count;
+    if ( n != subscr_vm_created_count )
+        return -EIO;
+
+    return 0;
 }
 
 bool ffa_partinfo_domain_destroy(struct domain *d)
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..a59c9887774b 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -25,6 +25,7 @@ 
 #define FFA_RET_DENIED                  -6
 #define FFA_RET_RETRY                   -7
 #define FFA_RET_ABORTED                 -8
+#define FFA_RET_NO_DATA                 -9
 
 /* FFA_VERSION helpers */
 #define FFA_VERSION_MAJOR_SHIFT         16U
@@ -175,6 +176,21 @@ 
  */
 #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
 
+/* Flags used in calls to FFA_NOTIFICATION_GET interface  */
+#define FFA_NOTIF_FLAG_BITMAP_SP        BIT(0, U)
+#define FFA_NOTIF_FLAG_BITMAP_VM        BIT(1, U)
+#define FFA_NOTIF_FLAG_BITMAP_SPM       BIT(2, U)
+#define FFA_NOTIF_FLAG_BITMAP_HYP       BIT(3, U)
+
+#define FFA_NOTIF_INFO_GET_MORE_FLAG        BIT(0, U)
+#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT    12
+#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT   7
+#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK    0x1F
+
+/* Feature IDs used with FFA_FEATURES */
+#define FFA_FEATURE_NOTIF_PEND_INTR     0x1U
+#define FFA_FEATURE_SCHEDULE_RECV_INTR  0x2U
+
 /* Function IDs */
 #define FFA_ERROR                       0x84000060U
 #define FFA_SUCCESS_32                  0x84000061U
@@ -213,6 +229,27 @@ 
 #define FFA_MEM_FRAG_TX                 0x8400007BU
 #define FFA_MSG_SEND                    0x8400006EU
 #define FFA_MSG_POLL                    0x8400006AU
+#define FFA_NOTIFICATION_BITMAP_CREATE  0x8400007DU
+#define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
+#define FFA_NOTIFICATION_BIND           0x8400007FU
+#define FFA_NOTIFICATION_UNBIND         0x84000080U
+#define FFA_NOTIFICATION_SET            0x84000081U
+#define FFA_NOTIFICATION_GET            0x84000082U
+#define FFA_NOTIFICATION_INFO_GET_32    0x84000083U
+#define FFA_NOTIFICATION_INFO_GET_64    0xC4000083U
+
+struct ffa_ctx_notif {
+    bool enabled;
+
+    /* Used to serialize access to the rest of this struct */
+    spinlock_t lock;
+
+    /*
+     * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
+     * pending global notifications.
+     */
+    bool secure_pending;
+};
 
 struct ffa_ctx {
     void *rx;
@@ -228,6 +265,7 @@  struct ffa_ctx {
     struct list_head shm_list;
     /* Number of allocated shared memory object */
     unsigned int shm_count;
+    struct ffa_ctx_notif notif;
     /*
      * tx_lock is used to serialize access to tx
      * rx_lock is used to serialize access to rx
@@ -257,7 +295,7 @@  void ffa_handle_mem_share(struct cpu_user_regs *regs);
 int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
 
 bool ffa_partinfo_init(void);
-bool ffa_partinfo_domain_init(struct domain *d);
+int ffa_partinfo_domain_init(struct domain *d);
 bool ffa_partinfo_domain_destroy(struct domain *d);
 int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
                                       uint32_t w4, uint32_t w5, uint32_t *count,
@@ -271,12 +309,28 @@  uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
 uint32_t ffa_handle_rxtx_unmap(void);
 int32_t ffa_handle_rx_release(void);
 
+void ffa_notif_init(void);
+int ffa_notif_domain_init(struct domain *d);
+void ffa_notif_domain_destroy(struct domain *d);
+
+int ffa_handle_notification_bind(struct cpu_user_regs *regs);
+int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
+void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
+void ffa_handle_notification_get(struct cpu_user_regs *regs);
+int ffa_handle_notification_set(struct cpu_user_regs *regs);
+
 static inline uint16_t ffa_get_vm_id(const struct domain *d)
 {
     /* +1 since 0 is reserved for the hypervisor in FF-A */
     return d->domain_id + 1;
 }
 
+static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
+{
+    /* -1 to match ffa_get_vm_id() */
+    return get_domain_by_id(vm_id - 1);
+}
+
 static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
                                 register_t v1, register_t v2, register_t v3,
                                 register_t v4, register_t v5, register_t v6,
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e167e14f8df9..1eac9802aa53 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -505,6 +505,7 @@  typedef uint64_t xen_callback_t;
 #define GUEST_MAX_VCPUS 128
 
 /* Interrupts */
+
 #define GUEST_TIMER_VIRT_PPI    27
 #define GUEST_TIMER_PHYS_S_PPI  29
 #define GUEST_TIMER_PHYS_NS_PPI 30
@@ -515,6 +516,19 @@  typedef uint64_t xen_callback_t;
 #define GUEST_VIRTIO_MMIO_SPI_FIRST   33
 #define GUEST_VIRTIO_MMIO_SPI_LAST    43
 
+/*
+ * SGI is the preferred delivery mechanism of FF-A pending notifications or
+ * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
+ * as they in a non-virtualized system typically are assigned to the secure
+ * world. Here we're free to use SGI 8-15 since they are virtual and have
+ * nothing to do with the secure world.
+ *
+ * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
+ * https://developer.arm.com/documentation/den0094/
+ */
+#define GUEST_FFA_NOTIF_PEND_INTR_ID      8
+#define GUEST_FFA_SCHEDULE_RECV_INTR_ID   9
+
 /* PSCI functions */
 #define PSCI_cpu_suspend 0
 #define PSCI_cpu_off     1