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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 21 Jul 2023, at 14:44, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 21/07/2023 13:28, Bertrand Marquis wrote:
>>> 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.
> This seems pretty unlikely to me because the purpose of TEE is to provide a 
> generic interface to the common code. At least, the developer would need to 
> have a very good reason to diverge.
> 
> Also, right now, make will not recurse to the directory tee if CONFIG_TEE=n. 
> So I would rather not have this change.

Ok will fix in v3.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall