[prev in list] [next in list] [prev in thread] [next in thread] 

List:       xen-devel
Subject:    Re: [XEN PATCH v7 09/20] xen/arm: ffa: add support for FFA_ID_GET
From:       Jens Wiklander <jens.wiklander () linaro ! org>
Date:       2023-02-28 14:18:37
Message-ID: CAHUa44GmSdKwOToQHm3dWQ5stkdy+4Wxm9wdKyQMyTeHgn3kaQ () mail ! gmail ! com
[Download RAW message or body]

Hi,

On Mon, Feb 27, 2023 at 4:00 PM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 27/02/2023 14:48, Bertrand Marquis wrote:
> >> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> 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@linaro.org>
> >> ---
> >> 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.

Makes sense, I'll fix it.

Thanks,
Jens

>
> Cheers,
>
> --
> Julien Grall

[prev in list] [next in list] [prev in thread] [next in thread]