FF-A Notifications - #6394
Conversation
|
As I mentioned here [1], it would be good to unify notifications feature to use secure world donated SGI and get rid of platform specific SPI or PPI for SMC ABI too. |
stm32mp platforms will continue to use a PPI over SMC ABI. |
Although I don't see ST platforms enabling notifications in upstream OP-TEE but any particular reason for this choice? I suppose there would be free SGI available there too. |
|
stm32mp13 does, since not that long. |
| SHIFT_U64(mask_aff2, 32) | SHIFT_U64(mask_aff3, 48); | ||
|
|
||
| if (cpu_mask & ITR_CPU_MASK_TO_THIS_CPU) | ||
| target_list = BIT32(mpidr & 0xf); |
There was a problem hiding this comment.
Shouldn't the whole bitfield of affinity 0 be covered? I see no limitations on the number of CPU per cluster
target_list = BIT32(mpidr & 0xff)?
More than 4 core per cluster is quite unusual though.
There was a problem hiding this comment.
The target list bitfield is only 16 bits wide so we need to mask the cpu_id to max 15 or we'll overflow.
etienne-lms
left a comment
There was a problem hiding this comment.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"qemu_armv8a: enable testing of notifications using the console",
"core: separate async notification implementation",
"core: notif: remove interrupt assumptions",
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: mobj_ffa.c: add reassuring comment in mobj_ffa_unregister_by_cookie()",
"qemu_armv8a: add GIC v3 redistributor base address",
"core: ffa: special treatment for FFA_ERROR",
For commit "core: gic: add gic_init_donate_sgi_to_ns()":
The commit message states "Secure world currently doesn't use any SGIs at all" which may not be true with #6440.
Commit "core: ffa: add notifications with SPMC at S-EL1" is tough, I'm not done with it, event for a shallow review :)
| #define GICD_CTLR_ENABLEGRP1 (1 << 1) | ||
|
|
||
| #define GICD_PIDR2_ARCHREV_SHIFT (4) | ||
| #define GICD_PIDR2_ARCHREV_MASK (0xF) |
There was a problem hiding this comment.
could remove parentheses (really need to be consistent with the useless ones above?)
Maybe add an inline comment stating on GICv2 the GICD register offset and bit mapping is the same despite the register is named ICPIDR2. Suggestion:
/* GICD IDR2 name differs on GICv3 and GICv2 but uses same offset and bit map */
There was a problem hiding this comment.
I noticed that the offset is different for GICv2 (0xfe8 vs 0xffe8). I'll update.
There was a problem hiding this comment.
Thanks you to have cross checked that! I've misread both to specs as I failed to see they differ for an 'f'.
My apologies.
There was a problem hiding this comment.
No worries, it was my mistake. Thanks for reviewing.
| struct gic_data *gd = &gic_data; | ||
| vaddr_t gicc_base = 0; | ||
| vaddr_t gicd_base = 0; | ||
| uint32_t vers = 0; |
| #else | ||
| uint32_t mask_id = it & 0xf; | ||
| uint32_t mask_group = group & 0x1; | ||
| uint32_t mask_group = group; |
There was a problem hiding this comment.
why this change on GICv2 implemenation?
| #define GICD_CTLR_ARE_NS BIT32(5) | ||
|
|
||
| #define GICD_PIDR2_ARCHREV_SHIFT (4) | ||
| #define GICD_PIDR2_ARCHREV_MASK (0xF) |
There was a problem hiding this comment.
already define by a previous commit (see lines 72-73).
|
|
||
| /* GIC configuration is initialized from TF-A when embedded */ | ||
| #ifndef CFG_WITH_ARM_TRUSTED_FW | ||
| #if defined(CFG_WITH_ARM_TRUSTED_FW) |
There was a problem hiding this comment.
could you move the above inline comment inside this #ifdef block?
|
|
||
| /* GIC configuration is initialized from TF-A when embedded */ | ||
| #ifndef CFG_WITH_ARM_TRUSTED_FW | ||
| #if defined(CFG_WITH_ARM_TRUSTED_FW) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| void thread_spmc_set_async_notif_intid(int intid); | ||
| #else | ||
| static inline void __noreturn | ||
| thread_spmc_set_async_notif_intid(int intid __unused) |
There was a problem hiding this comment.
nitpicking: would fit in a single line with s/intid/id/.
There was a problem hiding this comment.
Agree, but I prefer using the same name for the argument.
| * w3: Error code, 0 on success | ||
| * w4-w7: Not used (MBZ) | ||
| */ | ||
| #define OPTEE_FFA_ENABLE_ASYNC_NOTIF OPTEE_FFA_BLOCKING_CALL(5) |
There was a problem hiding this comment.
I'm not sure what happened with ID 4, but this has to be kept in synch with the kernel patches so changing this now might be more trouble than it's worth.
| switch (args->a1 & FFA_FEATURES_FEATURE_ID_MASK) { | ||
| case FFA_FEATURE_SCHEDULE_RECV_INTR: | ||
| if (!spmc_notif_is_ready) | ||
| goto err; |
There was a problem hiding this comment.
I think it would be better to have explicit return values here:
case FFA_FEATURE_SCHEDULE_RECV_INTR:
if (spmc_notif_is_ready) {
ret_fid = FFA_SUCCESS_32;
ret_w2 = notif_intid;
} else {
ret_fid = FFA_ERROR;
ret_w2 = FFA_NOT_SUPPORTED;
}
goto out;There was a problem hiding this comment.
This turned out a bit more complicated than I liked. I'll refactor this function to see if you prefer that.
|
Update |
etienne-lms
left a comment
There was a problem hiding this comment.
comments for commit "core: ffa: add notifications with SPMC at S-EL1"
| * w5: Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below, | ||
| * unused bits MBZ. | ||
| * w6-w7: Not used (MBZ) | ||
| * w5: bitfield of OP-TEE capabilities OPTEE_FFA_SEC_CAP_* |
| cpu_spin_unlock_xrestore(&spmc_notif_lock, old_itr_status); | ||
| } | ||
|
|
||
| spmc_set_args(args, ret_fid, 0, ret_val, 0, 0, 0); |
There was a problem hiding this comment.
generic comment: at many places is FFA_PARAM_MBZ is used instead of 0.
|
|
||
| old_itr_status = cpu_spin_lock_xsave(&spmc_notif_lock); | ||
|
|
||
| if (!notif_async_is_started() && notif_vm_id_valid && |
There was a problem hiding this comment.
From the FF-A spec, return code should be DENIED "if the Notification bitmap is not registered or is
registered but not in a masked and non-pending state.".
There was a problem hiding this comment.
Thanks, I'll update.
| if (args->a5 || args->a6 || args->a7) | ||
| goto out; | ||
| if (args->a2) { | ||
| /* We only deal with global notifications for now. */ |
There was a problem hiding this comment.
nitpicking: remove period for inline comments consistency
| goto out; | ||
| } | ||
|
|
||
| /* DST as in the destination of the eventual notification */ |
|
|
||
| /* | ||
| * Note we're only supporting physical OS kernel in normal world | ||
| * with Global Notifications at that. |
| return TEE_SUCCESS; | ||
| } | ||
| #else /* !defined(CFG_CORE_SEL1_SPMC) */ | ||
|
|
| else | ||
| # CFG_CORE_ASYNC_NOTIF is defined by the platform to enable support | ||
| # for sending asynchronous notifications to normal world. Note that an | ||
| # for sending asynchronous notifications to normal world. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressing comments. This PR needs to be rebased. OK if I squash in the review updates and rebase? |
|
ok for rebasing (sorry for the delayed answer) |
|
Squashed and rebased. |
|
Tags applied and commit message of "core: gic: add gic_init_donate_sgi_to_ns()" with regards to used SGIs in the secure world. |
|
I've picked up the kernel counterpart of this, so the ABI is frozen. |
|
This fails at IBART at the same way as @etienne-lms 's #6361 .
|
|
Rebased on master |
etienne-lms
left a comment
There was a problem hiding this comment.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: gic: check gic version" and
"core: gic: use redistributor CPU interface" (with minor suggestion).
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"plat-vexpress: donate a secure SGI to normal world".
Question for commit "core: gic: add gic_init_donate_sgi_to_ns()" otherwise it looks good to me.
|
|
||
| /* Offsets from gic.gicr_base[core_pos] */ | ||
| #define GICR_V3_PCPUBASE_SIZE (2 * 64 * 1024) | ||
| #define GICR_SGIBASE_OFFSET (64 * 1024) |
There was a problem hiding this comment.
the spec uses SGI_base worinf, maybe rename to GICR_SGI_BASE_OFFSET?
There was a problem hiding this comment.
Sure, I'll update.
| write_icc_igrpen1(1); | ||
| #else | ||
| io_write32(gd->gicc_base + GICC_PMR, 0x80); | ||
| io_write32(gd->gicc_base + GICC_PMR, 0x80); |
There was a problem hiding this comment.
Before this change, when CFG_WITH_ARM_TRUSTED_FW is enable,
for GICv3 we called write_icc_pmr(0x80); + write_icc_igrpen1(1);
for GICv2 we called write_icc_igrpen1(1); + io_write32(gd->gicc_base + GICC_CTLR, ...)
Now its not the case. Is it on purpose?
There was a problem hiding this comment.
Yes, that's on purpose. Should I do that in a separate commit or perhaps mention it in the commit?
There was a problem hiding this comment.
Either a specific commit or a comment in the commit message would help.
There was a problem hiding this comment.
It's actually no change in behaviour since gic_cpu_init() is normally not called with CFG_WITH_ARM_TRUSTED_FW=y, but with donate SGI it will be needed. I'll make a separate commit and update all the platforms.
There was a problem hiding this comment.
There are many platforms that enable CFG_WITH_ARM_TRUSTED_FW and call gic_cpu_init() from boot_secondary_init_intc().
There was a problem hiding this comment.
Seems tricky to update all platforms in one go. How about deprecating gic_cpu_init() and add a new gic_init_per_cpu()? gic_cpu_init() is kept unchanged with regards to PMR and igrpen1/GICC_CTLR and gic_init_per_cpu() is expected by be by all secondary CPUs regardless of CFG_WITH_ARM_TRUSTED_FW. I'll submit separate PRs to tidy up the various platforms after this PR.
There was a problem hiding this comment.
Sounds a good comprise. We should notify platform maintainers to update their implementation so that we can get rid of gic_cpu_init() afterward.
|
You can rebase to solve confict and IBART test failure. |
When asynchronous notifications are enabled the console driver in qemu_armv8a is configured as a top half and bottom half driver allowing basic testing of the notification framework. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Separates the implementation of sending asynchronous notifications from the part managing reception of events. This makes room for an alternative implementation based on FF-A. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Removes the assumption in notif_register_driver() that OP-TEE is using interrupts to notify the normal world. The coming FF-A based implementation will use FFA_NOTIFICATION_SET for when notifying using an SPMC at S-EL2 or EL3. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
…okie() Adds a reassuring comment in mobj_ffa_unregister_by_cookie() to explain why it may fail if the cookie hasn't been used yet. Updates the error message to include inactive_refs. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Adds a check to see that the version of the GIC matches the expected version. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
GICv3 has a redistributor CPU interface that until now hasn't been used. To prepare for coming patches that need to access the redistributor initialize a list with each CPU specific redistributor address. A new function gic_init_v3() is added with a parameter for the redistributor base address. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Adds and configures the GIC v3 redistributor base address. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
|
Rebased to resolve the conflict. Tags applied. |
|
Added "core: deprecate gic_cpu_init()" and "plat-vexpress: use gic_init_per_cpu()". |
|
Fixing the compile error. |
etienne-lms
left a comment
There was a problem hiding this comment.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: deprecate gic_cpu_init()" (with typo fixed. Feel free to discard the suggestion)
and "plat-vexpress: use gic_init_per_cpu()".
| * then used by the other functions. | ||
| * The two gic_init() and gic_init_v3() functions initializes the struct | ||
| * gic_data which is then used by the other functions. These two functions | ||
| * also initlized the GIC and are only supposed to be called from the |
There was a problem hiding this comment.
s/initlized/initialized/
| void gic_cpu_init(void) | ||
| { | ||
| struct gic_data *gd = &gic_data; | ||
|
|
There was a problem hiding this comment.
maybe add IMSG("gic_cpu_init() is deprecated, please use gic_init_per_cpu()")
f0ebf67 to
86efdbd
Compare
|
Comments addressed and tag applied. |
Deprecates gic_cpu_init() in favor of the new function gic_init_per_cpu(). gic_cpu_init() is only supposed to be called by secondary CPUs in non-TF-A configurations while gic_init_per_cpu() should be called by all secondary CPUs. gic_init_per_cpu() itself takes CFG_WITH_ARM_TRUSTED_FW into account instead of having each platform doing that. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Calls gic_init_per_cpu() instead of the now deprecated gic_cpu_init(). Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
|
Fixing the checkpatch warning. |
|
Two commits remain to be reviewed:
|
etienne-lms
left a comment
There was a problem hiding this comment.
For commit "core: gic: add gic_init_donate_sgi_to_ns()":
Should core ensures there is no secure interrupt handler registered for donated SGIs, and counter part that secure interrupt handler cannot be registered for a donated SGI?
Typo in commit message:
"In case another non-secure SGI is needed secure world can donate one of its ..."
| /* Ignore matching bits */ | ||
| if (!(BIT32(n) & (grp0 ^ gd->per_cpu_group_status))) | ||
| continue; | ||
| need_sync = true; |
There was a problem hiding this comment.
maybe add this instruction below the below inline comment.
etienne-lms
left a comment
There was a problem hiding this comment.
Comments for commit "core: ffa: add notifications with SPMC at S-EL1".
| ifeq ($(filter y, $(CFG_CORE_SEL1_SPMC) $(CFG_CORE_SEL2_SPMC) \ | ||
| $(CFG_CORE_EL3_SPMC)),y) | ||
| # FF-A case, handled via the FF-A ABI | ||
| CFG_CORE_ASYNC_NOTIF ?= y |
There was a problem hiding this comment.
instad of the $(call force,...) in arm.mk and _CFG_CORE_ASYNC_NOTIF_DEFAULT_IMPL ?= $(CFG_CORE_ASYNC_NOTIF) in core.mk directives that are a bit confusing, I would suggest here:
ifeq ($(filter y, $(CFG_CORE_SEL1_SPMC) $(CFG_CORE_SEL2_SPMC) \
$(CFG_CORE_EL3_SPMC)),y)
# FF-A case, handled via the FF-A ABI
CFG_CORE_ASYNC_NOTIF ?= y
+$(call force,_CFG_CORE_ASYNC_NOTIF_DEFAULT_IMPL,n)`
else
# CFG_CORE_ASYNC_NOTIF is defined by the platform to enable support
# for sending asynchronous notifications to normal world.
# Interrupt ID must be configurged by the platform too. Currently is only
# CFG_CORE_ASYNC_NOTIF_GIC_INTID defined.
CFG_CORE_ASYNC_NOTIF ?= n
+$(call force,_CFG_CORE_ASYNC_NOTIF_DEFAULT_IMPL,$(CFG_CORE_ASYNC_NOTIF))`
endifThere was a problem hiding this comment.
Much better, thanks.
| */ | ||
|
|
||
| #include <bitstring.h> | ||
| #include <drivers/gic.h> |
There was a problem hiding this comment.
replace with #include <kernel/interrupt.h>
add inclusion of assert.h and trace.h.
Perhaps, but we should then also prevent registering a handler for a non-secure SGI. Can we take this later in another PR? |
|
Force push to update the commit message of "core: gic: add gic_init_donate_sgi_to_ns", the rest of the changes went into "[review]" commits. |
Sure. Build errors: s/ |
There was a problem hiding this comment.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"core: gic: add gic_init_donate_sgi_to_ns()" (edited: with comment addressed :)
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"core: ffa: add notifications with SPMC at S-EL1".
| * SGI-n differs from primary CPU configuration, | ||
| * let's sync up. | ||
| */ | ||
| need_sync = true; |
There was a problem hiding this comment.
I forgot to mention my comment also applied to gicv3_sync_sgi_config() line 225.
|
I'll squash and apply tags once this has passed CI. |
Adds gic_init_donate_sgi_to_ns() which changes a secure SGI to become non-secure. Secure world currently has a few SGIs to spare while normal world typically have reserved each of the non-secure SGIs for different purposes. In case another non-secure SGI is needed secure world can donate one of its unused SGIs. This configuration will then deviate from the standard GIC configuration where SGI ID0-ID7 are non-secure and ID8-ID15 are secure. Platforms using gic_init_donate_sgi_to_ns() should also use gic_init_per_cpu() instead of the deprecated gic_cpu_init(). Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
If FFA_ERROR is received print the error code. If the FFA_ERROR is from the SPMC panic, else return back FFA_ERROR(FFA_NOT_SUPPORTED). Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Adds support for asynchronous notifications via FF-A with SPMC at S-EL1. The OP-TEE FF-A ABI is extended to report support for asynchronous notifications during OPTEE_FFA_EXCHANGE_CAPABILITIES. The SPMC at S-EL1 is extended to provide the FF-A notifications ABI to a normal world VM. The notifications depends on having a non-secure SGI interrupt ID available to notify normal world that a notification is pending. Notifications becomes available once platform code has called thread_spmc_set_async_notif_intid() with a designed SGI ID. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
With OP-TEE acting as SPMC in S-EL1 donate a secure SGI to normal world to be used to signal asynchronous notifications for FF-A. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
|
Squashed and tags applied. All commits are reviewed and should be ready to merge. |
| paddr_t gicr_base_pa); | ||
| static inline void gic_init(paddr_t gicc_base_pa, paddr_t gicd_base_pa) | ||
| { | ||
| gic_init_v3(gicc_base_pa, gicd_base_pa, 0); |
There was a problem hiding this comment.
How does this work? Any platform with CFG_ARM_GICV3 set will panic with a 0 for gicr_base_pa. I'm guessing you will need to convert all those plats to use gic_init_v3(), but none are converted so all will panic..
There was a problem hiding this comment.
I'm sorry, I missed that. Would it work to only check for the redistributor address if affinity routing is enabled? Or should we rather let it be optional and only panic or fail if a function that needs it is called?
No description provided.