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