Hi Julien,
> On 26 Mar 2025, at 23:49, Julien Grall <jul...@xen.org> wrote:
>
> Hi Bertrand,
>
> On 24/03/2025 13:52, Bertrand Marquis wrote:
>> Add a new command line parameter "tee=" to be used to explicitly select
>> what tee mediator is to be used by Xen and fail if it does not exist
>> or the probe function for it failed.
>> Without specifying which tee is to be used, Xen will use the first one
>> for which the probe function succeeds which depends on the order of the
>> mediator list which depends on the compiler.
>> Using the command line argument, it is now possible to explicit request
>> a specific TEE mediator and panic on boot if it is not available.
>> Signed-off-by: Bertrand Marquis <bertrand.marq...@arm.com>
>> ---
>> Changes in v4:
>> - None
>> Changes in v3:
>> - Properly classify tee as arm specific (Jan)
>> Changes in v2:
>> - Patch introduced to add a command line selection of the TEE
>> ---
>> docs/misc/xen-command-line.pandoc | 14 ++++++++++++++
>> xen/arch/arm/include/asm/tee/tee.h | 4 ++++
>> xen/arch/arm/tee/tee.c | 31 ++++++++++++++++++++++++++++++
>> 3 files changed, 49 insertions(+)
>> diff --git a/docs/misc/xen-command-line.pandoc
>> b/docs/misc/xen-command-line.pandoc
>> index 89db6e83be66..0c2ff542a138 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2651,6 +2651,20 @@ Specify the per-cpu trace buffer size in pages.
>> Flag to enable TSC deadline as the APIC timer mode.
>> +### tee (arm)
>> +> `= <string>`
>> +
>> +Specify the TEE mediator to be probed and use.
>> +
>> +The default behaviour is to probe all supported TEEs supported by Xen and
>> use
>
>
> typo: I think there is one too many "supported". Maybe keep the one after
> TEEs?
ack
>
>> +the first one successfully probed. When this parameter is passed, Xen will
>> +probe only the TEE mediator passed as argument and boot will fail if this
>> +mediator is not properly probed or if the requested TEE is not supported by
>> +Xen.
>> +
>> +This parameter can be set to `optee` of `ffa` if the corresponding mediators
>
> typo: s/of/or/
ack
>
>> +are compiled in.
>> +
>> ### tevt_mask
>> > `= <integer>`
>> diff --git a/xen/arch/arm/include/asm/tee/tee.h
>> b/xen/arch/arm/include/asm/tee/tee.h
>> index 0169fd746bcd..15d664e28dce 100644
>> --- a/xen/arch/arm/include/asm/tee/tee.h
>> +++ b/xen/arch/arm/include/asm/tee/tee.h
>> @@ -55,6 +55,9 @@ struct tee_mediator_desc {
>> /* Printable name of the TEE. */
>> const char *name;
>> + /* Command line name of the TEE (to be used with tee= cmdline option)
>> */
>> + const char *cmdline_name;
>> +
>> /* Mediator callbacks as described above. */
>> const struct tee_mediator_ops *ops;
>> @@ -77,6 +80,7 @@ void tee_free_domain_ctx(struct domain *d);
>> static const struct tee_mediator_desc __tee_desc_##_name __used \
>> __section(".teemediator.info") = { \
>> .name = _namestr, \
>> + .cmdline_name = #_name, \
>> .ops = _ops, \
>> .tee_type = _type \
>> }
>> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
>> index 3f65e45a7892..066b5ba40f9d 100644
>> --- a/xen/arch/arm/tee/tee.c
>> +++ b/xen/arch/arm/tee/tee.c
>> @@ -19,12 +19,17 @@
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> #include <xen/types.h>
>> +#include <xen/param.h>
>
> Coding style: The includes are order. So this wants to be moved before
> xen/types.h
ack
>
>> #include <asm/tee/tee.h>
>> extern const struct tee_mediator_desc _steemediator[], _eteemediator[];
>> static const struct tee_mediator_desc __read_mostly *cur_mediator;
>> +/* Select the TEE mediator using a name on command line. */
>> +static char __initdata opt_mediator[16] = "";
>> +string_param("tee", opt_mediator);
>> +
>> /*
>> * TODO: Add function to alter Dom0 DTB, so we can properly describe
>> * present TEE.
>> @@ -81,14 +86,40 @@ static int __init tee_init(void)
>> {
>> const struct tee_mediator_desc *desc;
>> + if ( strcmp(opt_mediator, "") )
>
> You are using 'strcmp(opt_mediator, "")' several time in the code. This
> should never changed, so can we introduce a boolean within the function to
> indicate whether opt_mediator is set?
Very good point, I will do that.
>
>> + printk(XENLOG_INFO "TEE Mediator %s selected from command line\n",
>> + opt_mediator);
>> +
>> + /*
>> + * When a specific TEE is selected using the 'tee=' command line
>> + * argument, we panic if the probe fails or if the requested TEE is not
>> + * supported.
>> + */
>> for ( desc = _steemediator; desc != _eteemediator; desc++ )
>> {
>> + if ( strcmp(opt_mediator, "") &&
>> + strncmp(opt_mediator, desc->cmdline_name,
>> sizeof(opt_mediator)) )
> > + continue;> +
>> if ( desc->ops->probe() )
>> {
>> printk(XENLOG_INFO "Using TEE mediator for %s\n", desc->name);
>> cur_mediator = desc;
>> return 0;
>> }
>> + else if ( strcmp(opt_mediator, "") )
>> + {
>> + panic("TEE mediator %s from command line probe failed\n",
>> + opt_mediator);
>> + return -EFAULT;
>> + }
>> + }
>> +
>> + if ( strcmp(opt_mediator, "") )
>> + {
>> + panic("TEE Mediator %s from command line not supported\n",
>> + opt_mediator);
>> + return -EINVAL;
>> }
>> return 0;
Thanks a lot for the review.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
