Re: [XEN PATCH v9 02/24] xen/arm: add TEE teardown to arch_domain_teardown()

[Julien Grall <julien@xxxxxxx>]

Hi Jens,

On 05/07/2023 10:34, Jens Wiklander wrote:
> Adds a progress state for tee_domain_teardown() to be called from
> arch_domain_teardown(). tee_domain_teardown() calls the new callback
> domain_teardown() in struct tee_mediator_ops.
>
> An empty domain_teardown() callback is added to the OP-TEE mediator.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Co-developed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
I am a bit confused with the tags ordering. The first signed-off-by 
indicates that Andrew is the author but he co-developped with himself? 
Did you indent to put your signed-off-by first?
> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain.c              | 36 ++++++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/tee/tee.h |  7 ++++++
>   xen/arch/arm/tee/optee.c           |  6 +++++
>   xen/arch/arm/tee/tee.c             |  8 +++++++
>   4 files changed, 57 insertions(+)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 15d9709a97d2..18171decdc66 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -795,6 +795,42 @@ fail:
>   
>   int arch_domain_teardown(struct domain *d)
>   {
> +    int ret = 0;
> +
> +    BUG_ON(!d->is_dying);
> +
> +    /* See domain_teardown() for an explanation of all of this magic. */
> +    switch ( d->teardown.arch_val )
> +    {
> +#define PROGRESS(x)                             \
> +        d->teardown.arch_val = PROG_ ## x;      \
> +        fallthrough;                            \
> +    case PROG_ ## x
> +
> +        enum {
> +            PROG_none,
> +            PROG_tee,
> +            PROG_done,
> +        };
> +
> +    case PROG_none:
> +        BUILD_BUG_ON(PROG_none != 0);
> +
> +    PROGRESS(tee):
> +        ret = tee_domain_teardown(d);
> +        if ( ret )
> +            return ret;
> +        break;
> +
> +    PROGRESS(done):
> +        break;
> +
> +#undef PROGRESS
> +
> +    default:
> +        BUG();
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/arm/include/asm/tee/tee.h b/xen/arch/arm/include/asm/tee/tee.h
> index f483986385c8..da324467e130 100644
> --- a/xen/arch/arm/include/asm/tee/tee.h
> +++ b/xen/arch/arm/include/asm/tee/tee.h
> @@ -34,6 +34,7 @@ struct tee_mediator_ops {
>        * guest and create own structures for the new domain.
>        */
>       int (*domain_init)(struct domain *d);
> +    int (*domain_teardown)(struct domain *d);
>   
>       /*
>        * Called during domain destruction to relinquish resources used
> @@ -62,6 +63,7 @@ struct tee_mediator_desc {
>   
>   bool tee_handle_call(struct cpu_user_regs *regs);
>   int tee_domain_init(struct domain *d, uint16_t tee_type);
> +int tee_domain_teardown(struct domain *d);
>   int tee_relinquish_resources(struct domain *d);
>   uint16_t tee_get_type(void);
>   
> @@ -93,6 +95,11 @@ static inline int tee_relinquish_resources(struct domain *d)
>       return 0;
>   }
>   
> +static inline int tee_domain_teardown(struct domain *d)
> +{
> +    return 0;
> +}
> +
>   static inline uint16_t tee_get_type(void)
>   {
>       return XEN_DOMCTL_CONFIG_TEE_NONE;
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 301d205a36c5..c91bd7d5ac25 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -268,6 +268,11 @@ static int optee_domain_init(struct domain *d)
>       return 0;
>   }
>   
> +static int optee_domain_teardown(struct domain *d)
> +{
> +    return 0;
I think for OP-TEE, we also need to moved the smc call to destroy the VM 
here. I am OK if this is not handled here, but it would be worth 
mentioning in the commit message.
> +}
> +
>   static uint64_t regpair_to_uint64(register_t reg0, register_t reg1)
>   {
>       return ((uint64_t)reg0 << 32) | (uint32_t)reg1;
> @@ -1732,6 +1737,7 @@ static const struct tee_mediator_ops optee_ops =
>   {
>       .probe = optee_probe,
>       .domain_init = optee_domain_init,
> +    .domain_teardown = optee_domain_teardown,
>       .relinquish_resources = optee_relinquish_resources,
>       .handle_call = optee_handle_call,
>   };
> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
> index 3964a8a5cddf..ddd17506a9ff 100644
> --- a/xen/arch/arm/tee/tee.c
> +++ b/xen/arch/arm/tee/tee.c
> @@ -52,6 +52,14 @@ int tee_domain_init(struct domain *d, uint16_t tee_type)
>       return cur_mediator->ops->domain_init(d);
>   }
>   
> +int tee_domain_teardown(struct domain *d)
> +{
> +    if ( !cur_mediator )
> +        return 0;
> +
> +    return cur_mediator->ops->domain_teardown(d);
NIT: I would consider to check if the callback is NULL. This would avoid 
providing dummy helper.
> +}
> +
>   int tee_relinquish_resources(struct domain *d)
>   {
>       if ( !cur_mediator )
Cheers,

--
Julien Grall