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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 26 Mar 2025, at 23:49, Julien Grall <julien@xxxxxxx> 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.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?

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