Re: [XEN PATCH v7 09/20] xen/arm: ffa: add support for FFA_ID_GET

[Julien Grall <julien@xxxxxxx>]

Hi,

On 27/02/2023 14:48, Bertrand Marquis wrote:
> > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Adds support for the FF-A function FFA_ID_GET to return the ID of the
> > calling client.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> > xen/arch/arm/tee/ffa.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 8b0b80ce1ff5..463fd7730573 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -167,6 +167,12 @@ static bool ffa_get_version(uint32_t *vers)
> >      return true;
> > }
> >
> > +static uint16_t get_vm_id(const struct domain *d)
> > +{
> > +    /* +1 since 0 is reserved for the hypervisor in FF-A */
> > +    return d->domain_id + 1;
> > +}
> > +
> > static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
> >                       register_t v2, register_t v3, register_t v4, register_t 
> > v5,
> >                       register_t v6, register_t v7)
> > @@ -181,6 +187,12 @@ static void set_regs(struct cpu_user_regs *regs, 
> > register_t v0, register_t v1,
> >          set_user_reg(regs, 7, v7);
> > }
> >
> > +static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
> > +                             uint32_t w3)
> > +{
> > +    set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
> > +}
> > +
> > static void handle_version(struct cpu_user_regs *regs)
> > {
> >      struct domain *d = current->domain;
> > @@ -210,6 +222,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >      case FFA_VERSION:
> >          handle_version(regs);
> >          return true;
> > +    case FFA_ID_GET:
> > +        set_regs_success(regs, get_vm_id(d), 0);
> > +        return true;
> >
> >      default:
> >          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -221,7 +236,11 @@ static int ffa_domain_init(struct domain *d)
> > {
> >      struct ffa_ctx *ctx;
> >
> > -    if ( !ffa_version )
> > +     /*
> > +      * We can't use that last possible domain ID or get_vm_id() would cause
> > +      * an overflow.
> > +      */
> > +    if ( !ffa_version || d->domain_id == UINT16_MAX)
> >          return -ENODEV;
> In reality the overflow could only happen if this is called by the IDLE domain 
> right now.
> Anyway this could change and this is making the code more robust at no real 
> cost.
>
> I would just suggest here to return a different error code like ERANGE for 
> example to
> prevent missing ENODEV with other cases not related to FFA not being available.
+1. I would also like to suggest to use >= rather than == in case we 
decide to support more than 16-bit domid.
Cheers,

--
Julien Grall