FF-A and virtualization #5359
FF-A and virtualization #5359
Conversation
jenswi-linaro
force-pushed
the
virt_ffa
branch
3 times, most recently
from
abdf058 to
96eb116
Compare
|
Can be tested as described in OP-TEE/build#570 |
core/arch/arm/kernel/entry_a64.S
Outdated
| */ | ||
| mov x19, x1 /* Save pagable part */ | ||
| mov x20, x0 /* Save DT address */ |
There was a problem hiding this comment.
Are we dropping the value passed in x0, or is it saved somewhere else?
There was a problem hiding this comment.
We are dropping it since we currently don't look at the SPMC manifest.
If or when we need it we should save it.
There was a problem hiding this comment.
Looking more into this, I think there might be some breakage here.
-
The SPMD by default was always passing the SPMC manifest (
TOS_FW_CONFIG) inx0, at least on FVP. There might be some qemu specific code overriding this (I'm not sure?) but AFAICS the code for this is not platform specific 1 2. -
OP-TEE saves this to
x20at this current line, and later (mis)interprets this as the system dtb (orHW_CONFIGin TF-A's terminology) 3. -
On FVP platform the SPMD passes the Event Log buffer address in the SPMC manifest (
TOS_FW_CONFIG) 4. -
OP-TEE is looking for the Event Log address in
HW_CONFIG, but it's actually passed in the SPMC manifest, but OP-TEE misinterprets the SPMC manifest as theHW_CONFIG. So luckily we end up at the same place, the event log is correctly found, and our use case of getting the event log (on FVP, with OP-TEE as S-EL1 SPMC) is working correctly 5.
This is my understanding currently, does this make any sense?
By dropping x0 we break this use case, so either the SPMD should pass the Event Log in HW_CONFIG or OP-TEE should look for the Event Log in the SPMC manifest, not sure which one is better.
There was a problem hiding this comment.
Thanks for looking into this. I'd like to define a common register usage for OP-TEE with SPMC at S-EL1. This thing with getting it right by mistake should also be resolved. We could if it helps break out all commits needed for FF-A 1.1 and take those in a separate PR.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/15339 should be updated to follow what we agree on. Thanks for the link to populate_next_bl_params_config(), I should use that one on QEMU too.
There was a problem hiding this comment.
I'd like to define a common register usage for OP-TEE with SPMC at S-EL1.
If OP-TEE is the S-EL1 SPMC, I think the current solution on FVP (x0: SPMC manifest, x1: HW_CONFIG) sounds reasonable. Is it feasible for QEMU to align with this? I'm not familiar with that platform.
This thing with getting it right by mistake should also be resolved.
I plan to open a PR soon (next 1-2 weeks) which should address this too.
We could if it helps break out all commits needed for FF-A 1.1 and take those in a separate PR.
Sounds good to me, thanks.
There was a problem hiding this comment.
Could we have some runtime check (even if only in debug mode) so that if what is passed in x0 doesn't look like a manifest and looks like a DT instead, a message is printed pointing to this change?
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
@jenswi-linaro I am re-opening this since I suppose it got stale mainly because higher priority stuff came in. |
|
Thanks. |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
jenswi-linaro
force-pushed
the
virt_ffa
branch
from
96eb116 to
fb5f3a2
Compare
core/arch/arm/kernel/thread_spmc.c
Outdated
| @@ -431,12 +488,14 @@ void spmc_handle_partition_info_get(struct thread_smc_args *args, | |||
| } | |||
|
|
|||
| ret_fid = FFA_SUCCESS_32; | |||
| rxtx->tx_is_mine = false; | |||
| if (!count_only) | |||
| rxtx->tx_is_mine = false; | |||
There was a problem hiding this comment.
I think this should be moved after the out label. From the if (is_nil_uuid... case we always jump to out which means the updating of tx_is_mine is skipped. I'm not sure why this didn't cause any errors before, but it looks like this is the reason for SPMC test failure I was getting.
There was a problem hiding this comment.
Makes sense, thanks for looking into this.
|
Update |
|
|
Squashed, tags applied, and rebased. |
|
Does anyone know what the |
|
I ran the QEMU (v7) test locally and xtest passes, not sure what's wrong with IBART. |
Looks like an out-of-memory condition. I could not reproduce it locally either. I have restarted IBART, we'll see. |
Failed again. That's interesting. |
Yes. I don't know where to start looking. @jbech-linaro, any idea? |
Fixes the following linker errors when CFG_NS_VIRTUALIZATION is enabled:
.../entry_a64.o: in function `clear_bss':
.../entry_a64.S:237:(.text._start+0x8c): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21 against symbol `__nex_bss_start' defined in .bss.mempool_default section in all_objs.o
.../entry_a64.S:238:(.text._start+0x90): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21 against symbol `__nex_bss_end' defined in .bss.mempool_default section in all_objs.o
Use the adr_l macro instead of adr to get the addresses for start and
end of .nex_bss.
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
For the two FF-A functions FFA_MEM_SHARE_64 and FFA_MEM_SHARE_32 update the checks in handle_mem_share() for: - 32-bit vs 64-bit calling convention, that is, when to mask of the upper 32 bits. - that the reported fragment length does not exceed the total length of the memory transaction descriptor. Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds support for FF-A 1.1. Now OP-TEE will need to be able to work with both version 1.0 and 1.1 depending on the other endpoint. The callee supplies its implemented version and OP-TEE chooses the highest common version and returns that. This is done per endpoint so some endpoint may very well use version 1.0 while another uses version 1.1. Two data structures, struct ffa_mem_transaction and struct ffa_partition_info, are affected. Runtime conditionals are used to select which version to use based on the negotiated FF-A version. Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds the helper function virt_get_current_guest_id(). Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds the memory type MEM_AREA_NEX_NSEC_SHM used to map non-secure shared memory in the nexus. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
In case of virtualization registers spmc_init() with boot_final() instead of service_init() to have my_endpoint_id initialized as part of boot initialization instead of delayed initialization when the OP-TEE partition is created. This guarantees that my_endpoint_id holds the correct value when the first FF-A request is received. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds support for virtualization with OP-TEE as SPMC at S-EL1. This if the FF-A counterpart of SMC based ABI with virtualization. Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Add a build with both CFG_CORE_SEL1_SPMC=y and CFG_NS_VIRTUALIZATION=y. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Renames spmc_get_id() to get_my_id() in order to avoid confusion with the function FFA_SPM_ID_GET introduced with FF-A v1.1. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Supports the FFA_SPM_ID_GET function introduced with FF-A v1.1. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
FFA_VERSION using ERET as conduit is not permitted in the FF-A specification. So remove support for it in thread_spmc_msg_recv() but keep it in spmc_sp_msg_handler() for S-EL0 SPs where the conduit is SVC. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Renames nw_rxtx to my_rxtx to be more clear when we have an SPMC at S-EL2 and the rxtx buffer is shared with the SPMC instead of normal world. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds missing initialization of my_rxtx.size with CFG_CORE_SEL2_SPMC=y. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Balint Dobszay <balint.dobszay@arm.com> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
|
Rebased on master. |
|
IBART is happy again. :-) |
No description provided.