Re: [PATCH] xen/arm: Move TEE mediators in a kconfig submenu

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 21 Jul 2023, at 10:52, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 21/07/2023 09:02, Bertrand Marquis wrote:
>> Rework TEE mediators to put them under a submenu in Kconfig.
>> The submenu is only visible if UNSUPPORTED is activated as all currently
>> existing mediators are UNSUPPORTED.
>> While there rework a bit the configuration so that OP-TEE and FF-A
>> mediators are selecting the generic TEE interface instead of depending
>> on it.
>> Make the TEE option hidden as it is of no interest for anyone to select
>> it without one of the mediators so having them select it instead should
>> be enough.
>> Rework makefile inclusion and selection so that generic TEE is included
>> only when selected and include the tee Makefile all the time making the
>> directory tee self contained.
> The problem is now we will always recurse to the directory even if there is 
> nothing to build. I would rather prefer reducing the build time (even if here 
> it would be minimal) over "self-contained" directory.

Makes sense, I will restore the obj-$(CONFIG_TEE) += tee/ in the Makfile.

> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> I wasn't able to apply this patch cleanly on the latest staging.

I will rebase properly and send a v2.

> 
>> ---
>>  xen/arch/arm/Kconfig      |  7 -------
>>  xen/arch/arm/Makefile     |  2 +-
>>  xen/arch/arm/tee/Kconfig  | 18 ++++++++++++++++--
>>  xen/arch/arm/tee/Makefile |  2 +-
>>  4 files changed, 18 insertions(+), 11 deletions(-)
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 439cc94f3344..fd57a82dd284 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -175,13 +175,6 @@ config ARM64_BTI
>>     Branch Target Identification support.
>>     This feature is not supported in Xen.
>>  -config TEE
>> - bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
>> - default n
>> - help
>> -   This option enables generic TEE mediators support. It allows guests
>> -   to access real TEE via one of TEE mediators implemented in XEN.
>> -
>>  source "arch/arm/tee/Kconfig"
>>    config STATIC_SHM
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7bf07e992046..d47d5c20aa73 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -5,7 +5,7 @@ obj-$(CONFIG_HAS_PCI) += pci/
>>  ifneq ($(CONFIG_NO_PLAT),y)
>>  obj-y += platforms/
>>  endif
>> -obj-$(CONFIG_TEE) += tee/
>> +obj-y += tee/
>>  obj-$(CONFIG_HAS_VPCI) += vpci.o
>>    obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>> index 923f08ba8cb7..cecbf7e12b43 100644
>> --- a/xen/arch/arm/tee/Kconfig
>> +++ b/xen/arch/arm/tee/Kconfig
>> @@ -1,7 +1,17 @@
>> +menu "TEE mediators"
>> + visible if UNSUPPORTED
>> +
>> +config TEE
>> + bool
>> + default n
>> + help
>> +   This option enables generic TEE mediators support. It allows guests
>> +   to access real TEE via one of TEE mediators implemented in XEN.
> 
> We don't typically add an 'help' section for non-select option. In fact, it 
> looks like menuconfig will not show the 'help'.

Yes i kept that one but it can be removed.
Will clean on v2.

> 
>> +
>>  config OPTEE
>> - bool "Enable OP-TEE mediator"
>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
> 
> Given this is under 'TEE mediators', do we still need the 'if UNSUPPORTED'?

As explained to Jan, i used visible if which does not enforce the dependency, 
just a hint for the display.

I thought that it was clearer to keep things like that to make it easier to 
have one of them supported in the future and i added the visible if just so 
that there was not an empty menu.

I could also just use a
if UNSUPPORTED
 ...

endif

to surround everything if that is clearer.
Please tell me and i will push a v2.

> 
>>   default n
>> - depends on TEE
>> + select TEE
> 
> I was sort of hoping we could remove 'select TEE'. But I understand why you 
> did it that way, you have one less selection to do. So I am Ok with that.

The fact that OPTEE or FFA was hidden before TEE was selected was kind of weird 
i thought.

An other solution is to keep the select and leave TEE visible but I do not 
really see the reason to select TEE without one of optee or ffa selected so i 
did it like that.

Cheers
Bertrand

> 
>>   help
>>     Enable the OP-TEE mediator. It allows guests to access
>>     OP-TEE running on your platform. This requires
>> @@ -13,9 +23,13 @@ config FFA
>>   bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>   default n
>>   depends on ARM_64
>> + select TEE
>>   help
>>     This option enables a minimal FF-A mediator. The mediator is
>>     generic as it follows the FF-A specification [1], but it only
>>     implements a small subset of the specification.
>>       [1] https://developer.arm.com/documentation/den0077/latest
>> +
>> +endmenu
>> +
>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>> index 58a1015e40e0..1ef49a271fdb 100644
>> --- a/xen/arch/arm/tee/Makefile
>> +++ b/xen/arch/arm/tee/Makefile
>> @@ -1,3 +1,3 @@
>>  obj-$(CONFIG_FFA) += ffa.o
>> -obj-y += tee.o
>> +obj-$(CONFIG_TEE) += tee.o
>>  obj-$(CONFIG_OPTEE) += optee.o
> 
> Cheers,
> 
> -- 
> Julien Grall