On Tue, Aug 01, 2023 at 05:10:08PM +0100, Abdellatif El Khlifi wrote: > Hi guys, > > On Tue, Aug 01, 2023 at 11:00:57AM -0400, Tom Rini wrote: > > > > > > > > > ... > > > > > > > > > Changelog: > > > > > > > > > =============== > > > > > > > > > > > > > > > > > > v17: > > > > > > > > > > > > > > > > > > * show a debug message rather than an error when FF-A is not > > > > > > > > > detected > > > > > > > > [snip] > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > > > index c5835e6ef6..8fbadb9201 100644 > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > @@ -55,13 +55,53 @@ config EFI_VARIABLE_FILE_STORE > > > > > > > > > stored as file /ubootefi.var on the EFI system > > > > > > > > > partition. > > > > > > > > > > > > > > > > > > config EFI_MM_COMM_TEE > > > > > > > > > - bool "UEFI variables storage service via OP-TEE" > > > > > > > > > - depends on OPTEE > > > > > > > > > + bool "UEFI variables storage service via the trusted > > > > > > > > > world" > > > > > > > > > + depends on OPTEE && ARM_FFA_TRANSPORT > > > > > > > > > > > > > > > > You didn't get my changes in here however. If you can do > > > > > > > > EFI_MM_COMM_TEE > > > > > > > > without ARM_FFA_TRANSPORT (as lx2160ardb_tfa_stmm_defconfig > > > > > > > > does) then > > > > > > > > you don't make this option depend on . If FF-A is only > > > > > > > > for use here, you make FF-A depend on this, and the FF-A > > > > > > > > specific > > > > > > > > variable depend on ARM_FFA_TRANSPORT. > > > > > > > > > > > > > > Abdellatif hinted at what's going on here. When I added this > > > > > > > Kconfig > > > > > > > option to lx2160 FF-A wasn't implemented yet. > > > > > > > > > > > > The defconfig has existed since May 2020, which is when you added > > > > > > EFI_MM_COMM_TEE itself too. So I think it's that no one did the > > > > > > check I > > > > > > did until now and saw this series was disabling what was on the > > > > > > other > > > > > > platform. > > > > > > > > > > > > > Since FF-A isn't a new > > > > > > > communication mechanism but builds upon the existing SMCs to > > > > > > > build an > > > > > > > easier API, I asked Abdellatif to hide this complexity. > > > > > > > We had two options, either make Kconfig options for either FF-A > > > > > > > or the > > > > > > > traditional SMCs and remove the dependencies, or piggyback on > > > > > > > FF-As > > > > > > > discovery mechanism and make the choice at runtime. The latter > > > > > > > has a > > > > > > > small impact on code size, but imho makes developers' life a lot > > > > > > > easier. > > > > > > > > > > > > I'm not sure how much you can do a run-time option here since you're > > > > > > setting a bunch of default values for FF-A to 0 in Kconfig. If > > > > > > we're > > > > > > supposed to be able to get them at run time, we shouldn't need a > > > > > > Kconfig > > > > > > option at all. I'm also not sure how valid a use case it is where > > > > > > we > > > > > > won't know at build time what the rest of the firmware stack > > > > > > supports > > > > > > here. > > > > > > > > > > > > > > > > That's a fair point. FF-A in theory has APIs to discover memory. > > > > > Abdellatif, why do we need the Kconfigs for shared memory on FF-A? > > > > > > > > The statically carved out MM shared buffer address, size and offset > > > > cannot be discovered by FF-A ABIs. > > > > The MM communication driver in U-Boot could allocate the buffer and > > > > share it with the MM SP but > > > > we do not implement that support currently in either U-Boot or UEFI. > > > > > > Ok, that's a bit unfortunate, but Tom is right. Having the FF-A > > > addresses show up is as confusing as having Kconfig options for > > > discrete options. The whole point of my suggestion was to make users' > > > lives easier. Apologies for the confusion but can you bring back the > > > ifdefs? Looking at the patch this should be minimal just use > > > ifdef ARM_FFA_TRANSPORT and ifndef ARM_FFA_TRANSPORT. > > > > > > Tom you prefer that as well? > > > > Pending an answer to Jens' feedback, yes, going back to #ifdef's is > > fine, especially since default values of 0 are nonsense in this case > > (and as Heinrich's patch re SYS_MALLOC_LEN shows, dangerous since 0 != > > 0x0 once we do string comparisons). > > > > I'd like to give some context why it's important for Corstone-1000 platform > that the DT passed to the kernel matches the official kernel DT.
Note that we've set aside the "should this be in DT or not" question. > There is a SystemReady IR 2.0 test checking the DT. It compares the DT > passed by U-Boot with a reference DT (the kernel DT) . The test fails if there > is a mismatch. So, if we add a DT node in U-Boot and the node is not > upstreamed > to the kernel DT, the DT test will fail. This is overall good and progress. > To be approved by the kernel DT maintainers, the node should have a use case > in the kernel which is not the case. This is, I believe / hope wrong. It needs to be in the dt-schema repository, not strictly "the kernel". For example, bootph-all (etc) are in dt-schema and so can be in the upstream kernel but are not used in the kernel itself. > There is a solution for this which is deleting the node we don't want to pass > to > the kernel using delete-node in the U-Boot DT. Something like this will likely be needed, in the end, at least for some cases. But the goal is that everything gets in to dt-schema. [snip] > With this we can get rid of the configs and the #defines: > FFA_SHARED_MM_BUF_ADDR, > FFA_SHARED_MM_BUF_OFFSET and FFA_SHARED_MM_BUF_SIZE. > > Also, we will avoid setting 0 as default values for the address, size and > offset. We just need to not have default values offered. The symbols just need to depend on FFA so that they aren't asked when not used. > 2/ the FF-A specific code in efi_variable_tee.c will try to find the > mm-comms-buf > reserved memory node. When found, it reads the buffer address, size and > offset. > > 3/ adding #ifdef CONFIG_ARM_FFA_TRANSPORT in lib/efi_loader/efi_variable_tee.c > for the FF-A specific code. > > 4/ make EFI_MM_COMM_TEE depends on OPTEE only > > What do you think guys ? Yes, we need to do 3 and 4. -- Tom
signature.asc
Description: PGP signature
