diff mbox series

xen/arm: Move TEE mediators in a kconfig submenu

Message ID a44f74559f52d1fa90a3f77390e7d121c9cd848e.1689926422.git.bertrand.marquis@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Move TEE mediators in a kconfig submenu | expand

Commit Message

Bertrand Marquis July 21, 2023, 8:02 a.m. UTC
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.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 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(-)

Comments

Julien Grall July 21, 2023, 8:52 a.m. UTC | #1
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.

> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>

I wasn't able to apply this patch cleanly on the latest staging.

> ---
>   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'.

> +
>   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'?

>   	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.

>   	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,
Jan Beulich July 21, 2023, 8:52 a.m. UTC | #2
On 21.07.2023 10:02, Bertrand Marquis wrote:
> --- a/xen/arch/arm/tee/Kconfig
> +++ b/xen/arch/arm/tee/Kconfig
> @@ -1,7 +1,17 @@
> +menu "TEE mediators"
> +	visible if UNSUPPORTED

With this ...

> +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.
> +
>  config OPTEE
> -	bool "Enable OP-TEE mediator"
> +	bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED

... you shouldn't need the "if" here, and ...

>  	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
> @@ -13,9 +23,13 @@ config FFA
>  	bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED

... you could drop the one here. I think.

Jan
Bertrand Marquis July 21, 2023, 9:02 a.m. UTC | #3
Hi Jan,

> On 21 Jul 2023, at 10:52, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 21.07.2023 10:02, Bertrand Marquis wrote:
>> --- a/xen/arch/arm/tee/Kconfig
>> +++ b/xen/arch/arm/tee/Kconfig
>> @@ -1,7 +1,17 @@
>> +menu "TEE mediators"
>> + visible if UNSUPPORTED
> 
> With this ...
> 
>> +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.
>> +
>> config OPTEE
>> - bool "Enable OP-TEE mediator"
>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
> 
> ... you shouldn't need the "if" here, and ...
> 
>> 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
>> @@ -13,9 +23,13 @@ config FFA
>> bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
> 
> ... you could drop the one here. I think.

visible if is only for the GUI/ncurse display but the if is required
to make sure that a .config file cannot set CONFIG_TEE or
CONFIG_FFA if. UNSUPPORTED is not selected.

Bertrand
Bertrand Marquis July 21, 2023, 9:07 a.m. UTC | #4
Hi Julien,

> On 21 Jul 2023, at 10:52, Julien Grall <julien@xen.org> 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@arm.com>
> 
> 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
Julien Grall July 21, 2023, 9:09 a.m. UTC | #5
Hi Bertrand,

On 21/07/2023 10:07, Bertrand Marquis wrote:
>> On 21 Jul 2023, at 10:52, Julien Grall <julien@xen.org> wrote:
>>> +
>>>   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.

Indeed.

> 
> 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.

I am happy with the existing version.

Cheers,
Jan Beulich July 21, 2023, 10:45 a.m. UTC | #6
On 21.07.2023 11:02, Bertrand Marquis wrote:
>> On 21 Jul 2023, at 10:52, Jan Beulich <jbeulich@suse.com> wrote:
>> On 21.07.2023 10:02, Bertrand Marquis wrote:
>>> --- a/xen/arch/arm/tee/Kconfig
>>> +++ b/xen/arch/arm/tee/Kconfig
>>> @@ -1,7 +1,17 @@
>>> +menu "TEE mediators"
>>> + visible if UNSUPPORTED
>>
>> With this ...
>>
>>> +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.
>>> +
>>> config OPTEE
>>> - bool "Enable OP-TEE mediator"
>>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
>>
>> ... you shouldn't need the "if" here, and ...
>>
>>> 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
>>> @@ -13,9 +23,13 @@ config FFA
>>> bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>
>> ... you could drop the one here. I think.
> 
> visible if is only for the GUI/ncurse display but the if is required
> to make sure that a .config file cannot set CONFIG_TEE or
> CONFIG_FFA if. UNSUPPORTED is not selected.

Is what you describe "depends on"? "if" controls merely prompt
visibility aiui.

Jan
Bertrand Marquis July 21, 2023, 12:27 p.m. UTC | #7
Hi Jan,

> On 21 Jul 2023, at 12:45, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 21.07.2023 11:02, Bertrand Marquis wrote:
>>> On 21 Jul 2023, at 10:52, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 21.07.2023 10:02, Bertrand Marquis wrote:
>>>> --- a/xen/arch/arm/tee/Kconfig
>>>> +++ b/xen/arch/arm/tee/Kconfig
>>>> @@ -1,7 +1,17 @@
>>>> +menu "TEE mediators"
>>>> + visible if UNSUPPORTED
>>> 
>>> With this ...
>>> 
>>>> +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.
>>>> +
>>>> config OPTEE
>>>> - bool "Enable OP-TEE mediator"
>>>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
>>> 
>>> ... you shouldn't need the "if" here, and ...
>>> 
>>>> 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
>>>> @@ -13,9 +23,13 @@ config FFA
>>>> bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>> 
>>> ... you could drop the one here. I think.
>> 
>> visible if is only for the GUI/ncurse display but the if is required
>> to make sure that a .config file cannot set CONFIG_TEE or
>> CONFIG_FFA if. UNSUPPORTED is not selected.
> 
> Is what you describe "depends on"? "if" controls merely prompt
> visibility aiui.

So you think that having  CONFIG_FFA without CONFIG_UNSUPPORTED
would be a valid configuration and the if is only here for the gui ?

I tested that with the following procedure:
- use menuconfig, select UNSUPPORTED and FFA
- edit .config and disable UNSUPPORTED but keep FFA
- build
- CONFIG_FFA is removed from .config

Now what puzzles me is that i did the same but removing the if UNSUPPORTED
for TEE and FFA and i have exactly the same behaviour.

So it seems that "if UNSUPPORTED" and visibility all behave in the same way
as depends on which i was not expecting.

So what should i keep or remove here ?

Cheers
Bertrand

> 
> Jan
Jan Beulich July 21, 2023, 1:08 p.m. UTC | #8
On 21.07.2023 14:27, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 21 Jul 2023, at 12:45, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 21.07.2023 11:02, Bertrand Marquis wrote:
>>>> On 21 Jul 2023, at 10:52, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 21.07.2023 10:02, Bertrand Marquis wrote:
>>>>> --- a/xen/arch/arm/tee/Kconfig
>>>>> +++ b/xen/arch/arm/tee/Kconfig
>>>>> @@ -1,7 +1,17 @@
>>>>> +menu "TEE mediators"
>>>>> + visible if UNSUPPORTED
>>>>
>>>> With this ...
>>>>
>>>>> +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.
>>>>> +
>>>>> config OPTEE
>>>>> - bool "Enable OP-TEE mediator"
>>>>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
>>>>
>>>> ... you shouldn't need the "if" here, and ...
>>>>
>>>>> 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
>>>>> @@ -13,9 +23,13 @@ config FFA
>>>>> bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>>>
>>>> ... you could drop the one here. I think.
>>>
>>> visible if is only for the GUI/ncurse display but the if is required
>>> to make sure that a .config file cannot set CONFIG_TEE or
>>> CONFIG_FFA if. UNSUPPORTED is not selected.
>>
>> Is what you describe "depends on"? "if" controls merely prompt
>> visibility aiui.
> 
> So you think that having  CONFIG_FFA without CONFIG_UNSUPPORTED
> would be a valid configuration and the if is only here for the gui ?
> 
> I tested that with the following procedure:
> - use menuconfig, select UNSUPPORTED and FFA
> - edit .config and disable UNSUPPORTED but keep FFA
> - build
> - CONFIG_FFA is removed from .config
> 
> Now what puzzles me is that i did the same but removing the if UNSUPPORTED
> for TEE and FFA and i have exactly the same behaviour.
> 
> So it seems that "if UNSUPPORTED" and visibility all behave in the same way
> as depends on which i was not expecting.

Hmm, maybe that's a bug in our variant of kconfig (we didn't sync
for quite some time)?

> So what should i keep or remove here ?

My understanding so far was that "visibility" merely hides all prompts
underneath (but then I use the command line version of the tool, not
menuconfig), so it largely is shorthand for adding "if" to all enclosed
prompts. Therefore I think all the "if UNSUPPORTED" are redundant and
could be dropped. But then I'm also working from the understanding that
"depends on" would behave somewhat differently ...

Jan
Bertrand Marquis July 21, 2023, 2:07 p.m. UTC | #9
Hi Jan,

> On 21 Jul 2023, at 15:08, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 21.07.2023 14:27, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 21 Jul 2023, at 12:45, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 21.07.2023 11:02, Bertrand Marquis wrote:
>>>>> On 21 Jul 2023, at 10:52, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 21.07.2023 10:02, Bertrand Marquis wrote:
>>>>>> --- a/xen/arch/arm/tee/Kconfig
>>>>>> +++ b/xen/arch/arm/tee/Kconfig
>>>>>> @@ -1,7 +1,17 @@
>>>>>> +menu "TEE mediators"
>>>>>> + visible if UNSUPPORTED
>>>>> 
>>>>> With this ...
>>>>> 
>>>>>> +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.
>>>>>> +
>>>>>> config OPTEE
>>>>>> - bool "Enable OP-TEE mediator"
>>>>>> + bool "Enable OP-TEE mediator (UNSUPPORTED)" if UNSUPPORTED
>>>>> 
>>>>> ... you shouldn't need the "if" here, and ...
>>>>> 
>>>>>> 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
>>>>>> @@ -13,9 +23,13 @@ config FFA
>>>>>> bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
>>>>> 
>>>>> ... you could drop the one here. I think.
>>>> 
>>>> visible if is only for the GUI/ncurse display but the if is required
>>>> to make sure that a .config file cannot set CONFIG_TEE or
>>>> CONFIG_FFA if. UNSUPPORTED is not selected.
>>> 
>>> Is what you describe "depends on"? "if" controls merely prompt
>>> visibility aiui.
>> 
>> So you think that having  CONFIG_FFA without CONFIG_UNSUPPORTED
>> would be a valid configuration and the if is only here for the gui ?
>> 
>> I tested that with the following procedure:
>> - use menuconfig, select UNSUPPORTED and FFA
>> - edit .config and disable UNSUPPORTED but keep FFA
>> - build
>> - CONFIG_FFA is removed from .config
>> 
>> Now what puzzles me is that i did the same but removing the if UNSUPPORTED
>> for TEE and FFA and i have exactly the same behaviour.
>> 
>> So it seems that "if UNSUPPORTED" and visibility all behave in the same way
>> as depends on which i was not expecting.
> 
> Hmm, maybe that's a bug in our variant of kconfig (we didn't sync
> for quite some time)?

It could be but if it is the case and we update we might end up with
people getting UNSUPPORTED features in where they would not
have been before (because the build is cleaning up .config).

> 
>> So what should i keep or remove here ?
> 
> My understanding so far was that "visibility" merely hides all prompts
> underneath (but then I use the command line version of the tool, not
> menuconfig), so it largely is shorthand for adding "if" to all enclosed
> prompts. Therefore I think all the "if UNSUPPORTED" are redundant and
> could be dropped. But then I'm also working from the understanding that
> "depends on" would behave somewhat differently ...

If that is ok with you I would rather keep them so that making one of them
SUPPORTED one day will not end up in wrongly making the other one
supported to. The visible if i added was more to "beautify" a bit when
unsupported is not selected so that we do not have an empty menu.

Cheers
Bertrand


> 
> Jan
Jan Beulich July 21, 2023, 2:24 p.m. UTC | #10
On 21.07.2023 16:07, Bertrand Marquis wrote:
>> On 21 Jul 2023, at 15:08, Jan Beulich <jbeulich@suse.com> wrote:
>> On 21.07.2023 14:27, Bertrand Marquis wrote:
>>> So what should i keep or remove here ?
>>
>> My understanding so far was that "visibility" merely hides all prompts
>> underneath (but then I use the command line version of the tool, not
>> menuconfig), so it largely is shorthand for adding "if" to all enclosed
>> prompts. Therefore I think all the "if UNSUPPORTED" are redundant and
>> could be dropped. But then I'm also working from the understanding that
>> "depends on" would behave somewhat differently ...
> 
> If that is ok with you I would rather keep them so that making one of them
> SUPPORTED one day will not end up in wrongly making the other one
> supported to. The visible if i added was more to "beautify" a bit when
> unsupported is not selected so that we do not have an empty menu.

You're the maintainer, so you judge what is best. If I was maintainer, the
primary thing I would ask for is that there be no redundancy. IOW here
either no "if"s or no "visibility".

Jan
Bertrand Marquis July 21, 2023, 2:34 p.m. UTC | #11
Hi Jan,

> On 21 Jul 2023, at 16:24, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 21.07.2023 16:07, Bertrand Marquis wrote:
>>> On 21 Jul 2023, at 15:08, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 21.07.2023 14:27, Bertrand Marquis wrote:
>>>> So what should i keep or remove here ?
>>> 
>>> My understanding so far was that "visibility" merely hides all prompts
>>> underneath (but then I use the command line version of the tool, not
>>> menuconfig), so it largely is shorthand for adding "if" to all enclosed
>>> prompts. Therefore I think all the "if UNSUPPORTED" are redundant and
>>> could be dropped. But then I'm also working from the understanding that
>>> "depends on" would behave somewhat differently ...
>> 
>> If that is ok with you I would rather keep them so that making one of them
>> SUPPORTED one day will not end up in wrongly making the other one
>> supported to. The visible if i added was more to "beautify" a bit when
>> unsupported is not selected so that we do not have an empty menu.
> 
> You're the maintainer, so you judge what is best. If I was maintainer, the
> primary thing I would ask for is that there be no redundancy. IOW here
> either no "if"s or no "visibility".

In this case i do think that the "if UNSUPPORTED" per entry is important
so that it clear per config entry which ones are unsupported.

So if other arm maintainers agree with your point, i would remove the
"visibility" and keep an empty menu.
But my vote is to keep both.

@julien and Stefano: Any view on that ?

Cheers
Bertrand

> 
> Jan
Julien Grall July 25, 2023, 2:30 p.m. UTC | #12
Hi,

On 21/07/2023 15:34, Bertrand Marquis wrote:
>> On 21 Jul 2023, at 16:24, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 21.07.2023 16:07, Bertrand Marquis wrote:
>>>> On 21 Jul 2023, at 15:08, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 21.07.2023 14:27, Bertrand Marquis wrote:
>>>>> So what should i keep or remove here ?
>>>>
>>>> My understanding so far was that "visibility" merely hides all prompts
>>>> underneath (but then I use the command line version of the tool, not
>>>> menuconfig), so it largely is shorthand for adding "if" to all enclosed
>>>> prompts. Therefore I think all the "if UNSUPPORTED" are redundant and
>>>> could be dropped. But then I'm also working from the understanding that
>>>> "depends on" would behave somewhat differently ...
>>>
>>> If that is ok with you I would rather keep them so that making one of them
>>> SUPPORTED one day will not end up in wrongly making the other one
>>> supported to. The visible if i added was more to "beautify" a bit when
>>> unsupported is not selected so that we do not have an empty menu.
>>
>> You're the maintainer, so you judge what is best. If I was maintainer, the
>> primary thing I would ask for is that there be no redundancy. IOW here
>> either no "if"s or no "visibility".
> 
> In this case i do think that the "if UNSUPPORTED" per entry is important
> so that it clear per config entry which ones are unsupported.
> 
> So if other arm maintainers agree with your point, i would remove the
> "visibility" and keep an empty menu.
> But my vote is to keep both.
> 
> @julien and Stefano: Any view on that ?

I agree with keeping both.

Cheers,
diff mbox series

Patch

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.
+
 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
@@ -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