| Message ID | 82e4e0c3ac1614822fddd90336c22e6fad5b485e.1677079672.git.jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen FF-A mediator | expand |
Hi Jens, > 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. Cheers Bertrand > > ctx = xzalloc(struct ffa_ctx); > -- > 2.34.1 >
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. Cheers,
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
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; ctx = xzalloc(struct ffa_ctx);
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(-)