| Message ID | 20240422073708.3663529-6-jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FF-A notifications | expand |
Hi Jens, This is not a full review of the code. I will let Bertrand doing it. On 22/04/2024 08:37, Jens Wiklander wrote: > +void ffa_notif_init(void) > +{ > + const struct arm_smccc_1_2_regs arg = { > + .a0 = FFA_FEATURES, > + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, > + }; > + struct arm_smccc_1_2_regs resp; > + unsigned int irq; > + int ret; > + > + arm_smccc_1_2_smc(&arg, &resp); > + if ( resp.a0 != FFA_SUCCESS_32 ) > + return; > + > + irq = resp.a2; > + if ( irq >= NR_GIC_SGI ) > + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); > + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); If I am not mistaken, ffa_notif_init() is only called once on the boot CPU. However, request_irq() needs to be called on every CPU so the callback is registered every where and the interrupt enabled. I know the name of the function is rather confusing. So can you confirm this is what you expected? [...] > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h > index 98236cbf14a3..ef8ffd4526bd 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 > @@ -97,6 +98,18 @@ > */ > #define FFA_MAX_SHM_COUNT 32 > > +/* > + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely > + * unused, but that may change. Are the value below intended for the guests? If so, can they be moved in public/arch-arm.h along with the others guest interrupts? > + * > + * SGI is the preferred delivery mechanism. 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. Do you have a pointer to the specification? [...] Cheers,
Hi Julien, > On 22 Apr 2024, at 13:40, Julien Grall <julien@xen.org> wrote: > > Hi Jens, > > This is not a full review of the code. I will let Bertrand doing it. > > On 22/04/2024 08:37, Jens Wiklander wrote: >> +void ffa_notif_init(void) >> +{ >> + const struct arm_smccc_1_2_regs arg = { >> + .a0 = FFA_FEATURES, >> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, >> + }; >> + struct arm_smccc_1_2_regs resp; >> + unsigned int irq; >> + int ret; >> + >> + arm_smccc_1_2_smc(&arg, &resp); >> + if ( resp.a0 != FFA_SUCCESS_32 ) >> + return; >> + >> + irq = resp.a2; >> + if ( irq >= NR_GIC_SGI ) >> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); >> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); > > If I am not mistaken, ffa_notif_init() is only called once on the boot CPU. However, request_irq() needs to be called on every CPU so the callback is registered every where and the interrupt enabled. > > I know the name of the function is rather confusing. So can you confirm this is what you expected? > > [...] > >> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >> index 98236cbf14a3..ef8ffd4526bd 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 >> @@ -97,6 +98,18 @@ >> */ >> #define FFA_MAX_SHM_COUNT 32 >> +/* >> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely >> + * unused, but that may change. > > Are the value below intended for the guests? If so, can they be moved in public/arch-arm.h along with the others guest interrupts? The values are to be used by the guest but they will discover them through the FFA_FEATURES ABI so I do not think those should belong the public headers. Cheers Bertrand > >> + * >> + * SGI is the preferred delivery mechanism. 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. > > Do you have a pointer to the specification? > > [...] > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 23/04/2024 16:12, Bertrand Marquis wrote: > Hi Julien, > >> On 22 Apr 2024, at 13:40, Julien Grall <julien@xen.org> wrote: >> >> Hi Jens, >> >> This is not a full review of the code. I will let Bertrand doing it. >> >> On 22/04/2024 08:37, Jens Wiklander wrote: >>> +void ffa_notif_init(void) >>> +{ >>> + const struct arm_smccc_1_2_regs arg = { >>> + .a0 = FFA_FEATURES, >>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, >>> + }; >>> + struct arm_smccc_1_2_regs resp; >>> + unsigned int irq; >>> + int ret; >>> + >>> + arm_smccc_1_2_smc(&arg, &resp); >>> + if ( resp.a0 != FFA_SUCCESS_32 ) >>> + return; >>> + >>> + irq = resp.a2; >>> + if ( irq >= NR_GIC_SGI ) >>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); >>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); >> >> If I am not mistaken, ffa_notif_init() is only called once on the boot CPU. However, request_irq() needs to be called on every CPU so the callback is registered every where and the interrupt enabled. >> >> I know the name of the function is rather confusing. So can you confirm this is what you expected? >> >> [...] >> >>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >>> index 98236cbf14a3..ef8ffd4526bd 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 >>> @@ -97,6 +98,18 @@ >>> */ >>> #define FFA_MAX_SHM_COUNT 32 >>> +/* >>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely >>> + * unused, but that may change. >> >> Are the value below intended for the guests? If so, can they be moved in public/arch-arm.h along with the others guest interrupts? > > The values are to be used by the guest but they will discover them through the FFA_FEATURES ABI so I do not think those > should belong the public headers. Sorry I should have been clearer. The values in the public headers are not meant for the guest. They are meant for a common understanding between the toolstack and Xen of the guest layout (memory + interrupts). I know that some of the guests started to rely on it. But this is against our policy: * These are defined for consistency between the tools and the * hypervisor. Guests must not rely on these hardcoded values but * should instead use the FDT. In this case, even if this is only used by Xen (today), I would argue they should defined at the same place because it is easier to understand the layout if it is in one place. Cheers,
Hi Julien, > On 23 Apr 2024, at 17:16, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 23/04/2024 16:12, Bertrand Marquis wrote: >> Hi Julien, >>> On 22 Apr 2024, at 13:40, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Jens, >>> >>> This is not a full review of the code. I will let Bertrand doing it. >>> >>> On 22/04/2024 08:37, Jens Wiklander wrote: >>>> +void ffa_notif_init(void) >>>> +{ >>>> + const struct arm_smccc_1_2_regs arg = { >>>> + .a0 = FFA_FEATURES, >>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, >>>> + }; >>>> + struct arm_smccc_1_2_regs resp; >>>> + unsigned int irq; >>>> + int ret; >>>> + >>>> + arm_smccc_1_2_smc(&arg, &resp); >>>> + if ( resp.a0 != FFA_SUCCESS_32 ) >>>> + return; >>>> + >>>> + irq = resp.a2; >>>> + if ( irq >= NR_GIC_SGI ) >>>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); >>>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); >>> >>> If I am not mistaken, ffa_notif_init() is only called once on the boot CPU. However, request_irq() needs to be called on every CPU so the callback is registered every where and the interrupt enabled. >>> >>> I know the name of the function is rather confusing. So can you confirm this is what you expected? >>> >>> [...] >>> >>>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >>>> index 98236cbf14a3..ef8ffd4526bd 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 >>>> @@ -97,6 +98,18 @@ >>>> */ >>>> #define FFA_MAX_SHM_COUNT 32 >>>> +/* >>>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely >>>> + * unused, but that may change. >>> >>> Are the value below intended for the guests? If so, can they be moved in public/arch-arm.h along with the others guest interrupts? >> The values are to be used by the guest but they will discover them through the FFA_FEATURES ABI so I do not think those >> should belong the public headers. > > Sorry I should have been clearer. The values in the public headers are not meant for the guest. They are meant for a common understanding between the toolstack and Xen of the guest layout (memory + interrupts). > > I know that some of the guests started to rely on it. But this is against our policy: > > * These are defined for consistency between the tools and the > * hypervisor. Guests must not rely on these hardcoded values but > * should instead use the FDT. > > In this case, even if this is only used by Xen (today), I would argue they should defined at the same place because it is easier to understand the layout if it is in one place. I see, that makes sense then to add them there. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Julien, On Mon, Apr 22, 2024 at 1:40 PM Julien Grall <julien@xen.org> wrote: > > Hi Jens, > > This is not a full review of the code. I will let Bertrand doing it. > > On 22/04/2024 08:37, Jens Wiklander wrote: > > +void ffa_notif_init(void) > > +{ > > + const struct arm_smccc_1_2_regs arg = { > > + .a0 = FFA_FEATURES, > > + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, > > + }; > > + struct arm_smccc_1_2_regs resp; > > + unsigned int irq; > > + int ret; > > + > > + arm_smccc_1_2_smc(&arg, &resp); > > + if ( resp.a0 != FFA_SUCCESS_32 ) > > + return; > > + > > + irq = resp.a2; > > + if ( irq >= NR_GIC_SGI ) > > + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); > > + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); > > If I am not mistaken, ffa_notif_init() is only called once on the boot > CPU. However, request_irq() needs to be called on every CPU so the > callback is registered every where and the interrupt enabled. > > I know the name of the function is rather confusing. So can you confirm > this is what you expected? Good catch, no this wasn't what I expected. I'll need to change this. > > [...] > > > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h > > index 98236cbf14a3..ef8ffd4526bd 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 > > @@ -97,6 +98,18 @@ > > */ > > #define FFA_MAX_SHM_COUNT 32 > > > > +/* > > + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely > > + * unused, but that may change. > > Are the value below intended for the guests? If so, can they be moved in > public/arch-arm.h along with the others guest interrupts? Yes, I'll move it. > > > + * > > + * SGI is the preferred delivery mechanism. 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. > > Do you have a pointer to the specification? There's one at the top of arch/arm/tee/ffa.c, https://developer.arm.com/documentation/den0077/e Do you want the link close to the defines when I've moved them to public/arch-arm.h? Or is it perhaps better to give a link to "Arm Base System Architecture v1.0C", https://developer.arm.com/documentation/den0094/ instead? Thanks, Jens
Hi Jens, > On 23 Apr 2024, at 17:26, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Julien, > > On Mon, Apr 22, 2024 at 1:40 PM Julien Grall <julien@xen.org> wrote: >> >> Hi Jens, >> >> This is not a full review of the code. I will let Bertrand doing it. >> >> On 22/04/2024 08:37, Jens Wiklander wrote: >>> +void ffa_notif_init(void) >>> +{ >>> + const struct arm_smccc_1_2_regs arg = { >>> + .a0 = FFA_FEATURES, >>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, >>> + }; >>> + struct arm_smccc_1_2_regs resp; >>> + unsigned int irq; >>> + int ret; >>> + >>> + arm_smccc_1_2_smc(&arg, &resp); >>> + if ( resp.a0 != FFA_SUCCESS_32 ) >>> + return; >>> + >>> + irq = resp.a2; >>> + if ( irq >= NR_GIC_SGI ) >>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); >>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); >> >> If I am not mistaken, ffa_notif_init() is only called once on the boot >> CPU. However, request_irq() needs to be called on every CPU so the >> callback is registered every where and the interrupt enabled. >> >> I know the name of the function is rather confusing. So can you confirm >> this is what you expected? > > Good catch, no this wasn't what I expected. I'll need to change this. > >> >> [...] >> >>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >>> index 98236cbf14a3..ef8ffd4526bd 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 >>> @@ -97,6 +98,18 @@ >>> */ >>> #define FFA_MAX_SHM_COUNT 32 >>> >>> +/* >>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely >>> + * unused, but that may change. >> >> Are the value below intended for the guests? If so, can they be moved in >> public/arch-arm.h along with the others guest interrupts? > > Yes, I'll move it. > >> >>> + * >>> + * SGI is the preferred delivery mechanism. 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. >> >> Do you have a pointer to the specification? > > There's one at the top of arch/arm/tee/ffa.c, > https://developer.arm.com/documentation/den0077/e > Do you want the link close to the defines when I've moved them to > public/arch-arm.h? > Or is it perhaps better to give a link to "Arm Base System > Architecture v1.0C", https://developer.arm.com/documentation/den0094/ > instead? I would say we need the link to Arm Base System Architecture in arch-arm. Regards Bertrand > > Thanks, > Jens
Hi Bertrand & Jens, On 24/04/2024 07:53, Bertrand Marquis wrote: > Hi Jens, > >> On 23 Apr 2024, at 17:26, Jens Wiklander <jens.wiklander@linaro.org> wrote: >> >> Hi Julien, >> >> On Mon, Apr 22, 2024 at 1:40 PM Julien Grall <julien@xen.org> wrote: >>> >>> Hi Jens, >>> >>> This is not a full review of the code. I will let Bertrand doing it. >>> >>> On 22/04/2024 08:37, Jens Wiklander wrote: >>>> +void ffa_notif_init(void) >>>> +{ >>>> + const struct arm_smccc_1_2_regs arg = { >>>> + .a0 = FFA_FEATURES, >>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, >>>> + }; >>>> + struct arm_smccc_1_2_regs resp; >>>> + unsigned int irq; >>>> + int ret; >>>> + >>>> + arm_smccc_1_2_smc(&arg, &resp); >>>> + if ( resp.a0 != FFA_SUCCESS_32 ) >>>> + return; >>>> + >>>> + irq = resp.a2; >>>> + if ( irq >= NR_GIC_SGI ) >>>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); >>>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); >>> >>> If I am not mistaken, ffa_notif_init() is only called once on the boot >>> CPU. However, request_irq() needs to be called on every CPU so the >>> callback is registered every where and the interrupt enabled. >>> >>> I know the name of the function is rather confusing. So can you confirm >>> this is what you expected? >> >> Good catch, no this wasn't what I expected. I'll need to change this. >> >>> >>> [...] >>> >>>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >>>> index 98236cbf14a3..ef8ffd4526bd 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 >>>> @@ -97,6 +98,18 @@ >>>> */ >>>> #define FFA_MAX_SHM_COUNT 32 >>>> >>>> +/* >>>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely >>>> + * unused, but that may change. >>> >>> Are the value below intended for the guests? If so, can they be moved in >>> public/arch-arm.h along with the others guest interrupts? >> >> Yes, I'll move it. >> >>> >>>> + * >>>> + * SGI is the preferred delivery mechanism. 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. >>> >>> Do you have a pointer to the specification? >> >> There's one at the top of arch/arm/tee/ffa.c, >> https://developer.arm.com/documentation/den0077/e >> Do you want the link close to the defines when I've moved them to >> public/arch-arm.h? >> Or is it perhaps better to give a link to "Arm Base System >> Architecture v1.0C", https://developer.arm.com/documentation/den0094/ >> instead? > > I would say we need the link to Arm Base System Architecture in arch-arm. +1 Cheers,
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..aa171c0b61f0 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_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..949587cf4b80 --- /dev/null +++ b/xen/arch/arm/tee/ffa_notif.c @@ -0,0 +1,331 @@ +/* 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], 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); +} + +void ffa_notif_init(void) +{ + const struct arm_smccc_1_2_regs arg = { + .a0 = FFA_FEATURES, + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR, + }; + struct arm_smccc_1_2_regs resp; + unsigned int irq; + int ret; + + arm_smccc_1_2_smc(&arg, &resp); + if ( resp.a0 != FFA_SUCCESS_32 ) + return; + + irq = resp.a2; + if ( irq >= NR_GIC_SGI ) + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); + if ( ret ) + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n", + irq, ret); + notif_enabled = !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..ef8ffd4526bd 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 @@ -97,6 +98,18 @@ */ #define FFA_MAX_SHM_COUNT 32 +/* + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely + * unused, but that may change. + * + * SGI is the preferred delivery mechanism. 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. + */ +#define FFA_NOTIF_PEND_INTR_ID 8 +#define FFA_SCHEDULE_RECV_INTR_ID 9 + /* * The time we wait until trying to tear down a domain again if it was * blocked initially. @@ -175,6 +188,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 +241,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 +277,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 +307,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 +321,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,
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 <jens.wiklander@linaro.org> --- 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/tee/Makefile | 1 + xen/arch/arm/tee/ffa.c | 55 +++++- xen/arch/arm/tee/ffa_notif.c | 331 ++++++++++++++++++++++++++++++++ xen/arch/arm/tee/ffa_partinfo.c | 9 +- xen/arch/arm/tee/ffa_private.h | 68 ++++++- 5 files changed, 457 insertions(+), 7 deletions(-) create mode 100644 xen/arch/arm/tee/ffa_notif.c