| 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 |
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,
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
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
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
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,
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
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
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
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
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
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
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 --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
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(-)