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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 21 Jul 2023, at 12:00, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 21/07/2023 10:23, 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.
> 
> Is this a left over?

Oups yes, please remove.

> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v2:
>> - only included tee subdirectory in makefile if CONFIG_TEE is selected
>>   (reverts to state before patch)
>> - remove help in hidden TEE config
>> - rebase on top of staging
>> ---
>>  xen/arch/arm/Kconfig      |  7 -------
>>  xen/arch/arm/tee/Kconfig  | 17 ++++++++++++++---
>>  xen/arch/arm/tee/Makefile |  2 +-
>>  3 files changed, 15 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/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>> index db3ea78faaaa..c5b0f88d7522 100644
>> --- a/xen/arch/arm/tee/Kconfig
>> +++ b/xen/arch/arm/tee/Kconfig
>> @@ -1,7 +1,14 @@
>> +menu "TEE mediators"
>> + visible if UNSUPPORTED
>> +
>> +config TEE
>> + bool
>> + default n
>> +
>>  config OPTEE
>> - bool "Enable OP-TEE mediator"
>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
>>   default n
>> - depends on TEE
>> + select TEE
>>   help
>>     Enable the OP-TEE mediator. It allows guests to access
>>     OP-TEE running on your platform. This requires
>> @@ -12,10 +19,14 @@ config OPTEE
>>  config FFA
>>   bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>   default n
>> - depends on ARM_64 && TEE
>> + 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
> 
> Same here? The rest LGTM and I would be happy to do the changes on commit.

This one is not, I think it is more logic to keep that one like this so that if 
someone
creates a TEE not depending on this it could still put it here without 
compiling tee.o
for no reason.

Now there is still a discussion around visibility and if UNSUPPORTED that we 
need
to settle (even though current behaviour is right and as i expected, the effect 
of the
"visibility" is not what I expected).

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall