QEMU enablement for Trusted Services #688
Conversation
|
I'm going to ask you the same thing as I asked the author of #686 before he closed that PR. It would be nice with a bit of background to this. Do you represent a company? Are you going to maintain this? Do you expect to contribute more? |
|
Hi! I was working at Arm recently on the TS project. I left, so I am making these contributions privately. I am available to maintain it and make other contributions. I thought it would be great to be able to use the project on QEMU too, instead of the proprietary FVP. |
|
Got it, thanks for contributing. |
|
|
qemu-psa-sp.mk
Outdated
|
|
||
| # TS SP configurations | ||
| DEFAULT_SP_CONFIG ?= default-opteesp | ||
| SP_BLOCK_STORAGE_CONFIG ?= $(DEFAULT_SP_CONFIG) |
qemu-psa-sp.mk
Outdated
| OUT_PATH ?= $(ROOT)/out | ||
|
|
||
| QEMU_VIRTFS_HOST_DIR = $(ROOT) | ||
| QEMU_VIRTFS_AUTOMOUNT = y |
qemu-psa-sp.mk
Outdated
|
|
||
| QEMU_VIRTFS_HOST_DIR = $(ROOT) | ||
| QEMU_VIRTFS_AUTOMOUNT = y | ||
| UBOOT ?= n |
qemu-psa-sp.mk
Outdated
|
|
||
| # The boot order of the SPs is determined by the order of calls here. This is | ||
| # due to the SPMC not (yet) supporting the boot order field of the SP manifest. | ||
| ifeq ($(SPMC_TESTS),n) |
There was a problem hiding this comment.
ifneq ($(SPMC_TESTS),y)?
There was a problem hiding this comment.
The same pattern is used for the build file for FVP (fvp-psa-sp.mk), so it might be better to keep it for easier maintenance.
qemu-psa-sp.mk
Outdated
| endif | ||
|
|
||
| # Linux user space applications | ||
| ifeq ($(SPMC_TESTS),n) |
There was a problem hiding this comment.
ifneq ($(SPMC_TESTS),y)?
There was a problem hiding this comment.
The same pattern is used for the build file for FVP (fvp-psa-sp.mk), so it might be better to keep it for easier maintenance.
There was a problem hiding this comment.
Testing against y is better IMO. I wouldn't mind a subsequent commit to fix all occurrences.
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed pull request at any time. |
|
@stormset do you plan to address the review comments? Thanks. |
Hi! I'm really sorry for the delay. I noticed another issue with the PR, namely the handling of the reserved memory location (for the carveout) in the device tree. I have a solution that dumps out the device tree (which is generated by QEMU when the 'virt' platform is used) and passes the modified one to QEMU. Looking at this patch, I don't see any better solution. I still haven't tested it properly, but I will do it this week and also address the other comments. |
Thanks for the update. Given what I could read in the qemu-devel thread, I think your approach of dumping the QEMU-generated DT and modifying it is sensible, however it should be done at runtime so that the base DT file does not need to be maintained as QEMU evolves. A similar suggestion was made in another PR by the way (#695 (comment)). |
stormset
force-pushed
the
qemu-ts
branch
2 times, most recently
from
8ed872c
to
733b67d
Compare
|
Addressed the mentioned issue, also rebased. |
There was a problem hiding this comment.
Hi @stormset,
One minor comment, otherwise LGTM.
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
| # purposes. | ||
|
|
||
| CARVEOUT_ENTRY = $(ROOT)/build/qemu_v8/mm_communicate_carveout.dtsi | ||
| $(QEMU_DTB_PATH): $(CARVEOUT_ENTRY) dumpdtb | linux |
There was a problem hiding this comment.
I think dumptdb should be replaced by the actual file path (here and in place of the dumptdb: target in qemu_v8.mk), otherwise make will rebuild the same thing each time it is invoked although nothing changed. You may still keep a dumptdb: path/to/dtb target for convenience.
There was a problem hiding this comment.
The reason I want it to be always regenerated is that the arguments (QEMU_ARGS) passed to QEMU will have an effect on the generated device tree.
There was a problem hiding this comment.
Ah, makes sense. Ideally you could generate a file with the value of $(QEMU_ARGS) which would change only when those args change (via some clever make macro), then use that as the true dependency. But that's perhaps for another PR (see for instance check-conf-h and mv-if-changed in the OP-TEE OS makefiles).
Could you at least add a comment here? "# Note: the DTB generated by dumpdtb depends on $(QEMU_ARGS)" or something like that. Thanks!
There was a problem hiding this comment.
Done. Also created a commit using the mentioned macros. Will create a PR after this gets merged.
There was a problem hiding this comment.
Done. Also created a commit using the mentioned macros. Will create a PR after this gets merged.
Excellent!
There was a problem hiding this comment.
#695 also generates a DTB for Qemu. Maybe there are some factorization to prepare.
There was a problem hiding this comment.
It seems to use a hard-coded device tree, but probably needs to be changed.
| QEMU_EXTRA_ARGS ?= \ | ||
| -cpu cortex-a57 \ | ||
| -semihosting-config enable=on,target=native,userspace=on \ | ||
| -d trace:'__*' |
There was a problem hiding this comment.
You might not want this trace flag always? Not sure.
There was a problem hiding this comment.
I can remove it, when I was using edk2 it printed out a lot of trace messages, with this pattern I was able to disable it, but with u-boot it doesn't do it anymore, so can be removed.
There was a problem hiding this comment.
Commit message header line:
qemu-psa-sp: Armv8-A QEMU enablement for Trusted Services
Could you add a short description of how to build for qemu_armv8a with trusted service, e.g. Build with "make -f qemu-pas-sp.mk".
It would be nice to have also the list of the available config switches: SPMC_TESTS, MEASURED_BOOT, MEASURED_BOOT_FTPM, TS_SMM_GATEWAY, TS_UEFI_TESTS.
| # Use GICv2 or GICv3 | ||
| GICV3 ?= y | ||
| # Use TF-A's Trusted Board Boot mechanism? | ||
| TF_A_TRUSTED_BOARD_BOOT ?= y |
There was a problem hiding this comment.
This switch seems unused. Is there even an altternate to TF-A?
There was a problem hiding this comment.
It is used in qemu_v8.mk, I am ot sure what you are referring to.
There was a problem hiding this comment.
Right, sorry. discard my comment.
qemu-psa-sp.mk
Outdated
|
|
||
| OPTEE_OS_COMMON_EXTRA_FLAGS += \ | ||
| CFG_SECURE_PARTITION=y \ | ||
| CFG_CORE_SEL1_SPMC=y \ |
There was a problem hiding this comment.
depends on SPMC_AT_EL?
There was a problem hiding this comment.
Removed these SPMC deployment dependent arguments as they are already handled in qemu_v8.mk depending on the value of SPMC_AT_EL.
|
|
||
| ifeq ($(QEMU_USE_CUSTOM_DTB),y) | ||
| QEMU_DTB_ARG ?= -dtb $(QEMU_DTB_PATH) | ||
| endif |
There was a problem hiding this comment.
can replace this change with:
ifeq ($(QEMU_USE_CUSTOM_DTB),y)
QEMU_EXTRA_ARGS += -dtb $(QEMU_DTB_PATH)
endifThere was a problem hiding this comment.
When QEMU receives both dtb and dumpdtb arguments it wants to boot with that DTB, so it won't dump anything, but QEMU_EXTRA_ARGS needs to be passed to QEMU when dumping DTB as it has an effect on the DTB.
| # purposes. | ||
|
|
||
| CARVEOUT_ENTRY = $(ROOT)/build/qemu_v8/mm_communicate_carveout.dtsi | ||
| $(QEMU_DTB_PATH): $(CARVEOUT_ENTRY) dumpdtb | linux |
There was a problem hiding this comment.
#695 also generates a DTB for Qemu. Maybe there are some factorization to prepare.
Add new build files to configure OP-TEE and TF-A for the S-EL1 SPMC scenario on QEMU platform for Trusted Services. Signed-off-by: Gabor Ambrus <agabor.ambrus@gmail.com>
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed pull request at any time. |
Add new build files to configure OP-TEE and TF-A for the S-EL1 SPMC scenario on QEMU platform for Trusted Services.