Re: [PATCH v4 1/5] xen/arm: Create tee command line parameter

[Julien Grall <julien@xxxxxxx>]

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.marquis@xxxxxxx>
> ---
> 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?
> +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/

> +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
>   
>   #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?
> +        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;
Cheers,

--
Julien Grall