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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Julien,

On Wed, Jul 12, 2023 at 11:53 AM Julien Grall <julien@xxxxxxx> wrote:
>
> 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?

Sorry, my mistake, I swapped the two lines. I'll fix it in the next version.

>
> > 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.

Fair enough, I'll mention that 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.

Yes, that's an advantage, but we'd treat this callback differently
from others. I'd prefer to keep this as it is if you don't mind.

Thanks,
Jens

>
> > +}
> > +
> >   int tee_relinquish_resources(struct domain *d)
> >   {
> >       if ( !cur_mediator )
>
> Cheers,
>
> --
> Julien Grall