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