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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 10 Apr 2024, at 17:45, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> On Tue, Apr 9, 2024 at 5:36 PM 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);
>> +        else
>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> +        break;
> 
> With the recently posted kernel patch
> https://lore.kernel.org/all/20240410-ffa_npi_support-v1-3-1a5223391bd1@xxxxxxx/
> we need to decide which feature interrupt to return since the kernel
> will only install a handle for the first it finds. Right now I propose
> to to not report FFA_FEATURE_SCHEDULE_RECV_INTR. When the time comes
> to use a secondary scheduler we'll need to report the SRI instead.


We just had a meeting with Sudeep to discuss that matter and he agreed that
he would register the interrupt handler for all the interrupts available so that
we can keep a model where we will generate SRIs only to a secondary scheduler
and NPI for notification interrupts (so that the VM does not do a INFO_GET when
not required).

We will have to report both as any VM could act as secondary scheduler for SPs
in theory (we might need at some point a parameter for that) but for now those
should only be generated to Dom0 if there are pending notifications for SPs.

Cheers
Bertrand