| Message ID | 20230413071424.3273490-10-jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Xen FF-A mediator | expand |
Hi Jens, > -----Original Message----- > Subject: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support > > Adds support for sending a FF-A direct request. Checks that the SP also > supports handling a 32-bit direct request. 64-bit direct requests are > not used by the mediator itself so there is not need to check for that. > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > xen/arch/arm/tee/ffa.c | 112 > +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > index f129879c5b81..f2cce955d981 100644 > --- a/xen/arch/arm/tee/ffa.c > +++ b/xen/arch/arm/tee/ffa.c > @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) > return true; > } > > +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) > +{ > + switch ( resp->a0 ) > + { > + case FFA_ERROR: > + if ( resp->a2 ) > + return resp->a2; > + else > + return FFA_RET_NOT_SUPPORTED; > + case FFA_SUCCESS_32: > + case FFA_SUCCESS_64: > + return FFA_RET_OK; > + default: > + return FFA_RET_NOT_SUPPORTED; > + } > +} > + > +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, > + register_t a3, register_t a4) > +{ > + const struct arm_smccc_1_2_regs arg = { > + .a0 = fid, > + .a1 = a1, > + .a2 = a2, > + .a3 = a3, > + .a4 = a4, > + }; > + struct arm_smccc_1_2_regs resp; > + > + arm_smccc_1_2_smc(&arg, &resp); > + > + return get_ffa_ret_code(&resp); > +} > + > +static int32_t ffa_features(uint32_t id) > +{ > + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > +} > + > +static bool check_mandatory_feature(uint32_t id) > +{ > + int32_t ret = ffa_features(id); > + > + if (ret) Coding style nit: You need spaces before and after "ret", i.e. if ( ret ) With this fixed: Reviewed-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
Hi, On 13/04/2023 08:14, Jens Wiklander wrote: > Adds support for sending a FF-A direct request. Checks that the SP also > supports handling a 32-bit direct request. 64-bit direct requests are > not used by the mediator itself so there is not need to check for that. > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > index f129879c5b81..f2cce955d981 100644 > --- a/xen/arch/arm/tee/ffa.c > +++ b/xen/arch/arm/tee/ffa.c > @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) > return true; > } > > +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) > +{ > + switch ( resp->a0 ) > + { > + case FFA_ERROR: > + if ( resp->a2 ) > + return resp->a2; > + else > + return FFA_RET_NOT_SUPPORTED; > + case FFA_SUCCESS_32: > + case FFA_SUCCESS_64: > + return FFA_RET_OK; > + default: > + return FFA_RET_NOT_SUPPORTED; > + } > +} > + > +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, > + register_t a3, register_t a4) > +{ > + const struct arm_smccc_1_2_regs arg = { > + .a0 = fid, > + .a1 = a1, > + .a2 = a2, > + .a3 = a3, > + .a4 = a4, > + }; > + struct arm_smccc_1_2_regs resp; > + > + arm_smccc_1_2_smc(&arg, &resp); > + > + return get_ffa_ret_code(&resp); > +} > + > +static int32_t ffa_features(uint32_t id) > +{ > + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > +} > + > +static bool check_mandatory_feature(uint32_t id) > +{ > + int32_t ret = ffa_features(id); > + > + if (ret) > + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", > + id, ret); > + > + return !ret; > +} > + > static uint16_t get_vm_id(const struct domain *d) > { > /* +1 since 0 is reserved for the hypervisor in FF-A */ > @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) > set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); > } > > +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) > +{ > + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; > + struct arm_smccc_1_2_regs resp = { }; > + struct domain *d = current->domain; > + uint32_t src_dst; > + uint64_t mask; > + > + if ( smccc_is_conv_64(fid) ) > + mask = GENMASK_ULL(63, 0); > + else > + mask = GENMASK_ULL(31, 0); > + > + src_dst = get_user_reg(regs, 1); > + if ( (src_dst >> 16) != get_vm_id(d) ) > + { > + resp.a0 = FFA_ERROR; > + resp.a2 = FFA_RET_INVALID_PARAMETERS; > + goto out; > + } > + > + arg.a1 = src_dst; > + arg.a2 = get_user_reg(regs, 2) & mask; > + arg.a3 = get_user_reg(regs, 3) & mask; > + arg.a4 = get_user_reg(regs, 4) & mask; > + arg.a5 = get_user_reg(regs, 5) & mask; > + arg.a6 = get_user_reg(regs, 6) & mask; > + arg.a7 = get_user_reg(regs, 7) & mask; > + > + arm_smccc_1_2_smc(&arg, &resp); > + switch ( resp.a0 ) > + { > + case FFA_ERROR: > + case FFA_SUCCESS_32: > + case FFA_SUCCESS_64: > + case FFA_MSG_SEND_DIRECT_RESP_32: > + case FFA_MSG_SEND_DIRECT_RESP_64: > + break; > + default: > + /* Bad fid, report back. */ > + memset(&arg, 0, sizeof(arg)); > + arg.a0 = FFA_ERROR; > + arg.a1 = src_dst; > + arg.a2 = FFA_RET_ABORTED; > + } > + > +out: > + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, > + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); > +} > + > static bool ffa_handle_call(struct cpu_user_regs *regs) > { > uint32_t fid = get_user_reg(regs, 0); > @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) > case FFA_ID_GET: > set_regs_success(regs, get_vm_id(d), 0); > return true; > + case FFA_MSG_SEND_DIRECT_REQ_32: > + case FFA_MSG_SEND_DIRECT_REQ_64: > + handle_msg_send_direct_req(regs, fid); > + return true; > > default: > gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); > @@ -326,6 +431,13 @@ static bool ffa_probe(void) > printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > major_vers, minor_vers); > > + /* > + * TODO save result of checked features and use that information to > + * accept or reject requests from guests. > + */ I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA? > + if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > + return false; > + > ffa_version = vers; > > return true; Cheers,
Hi Julien, > On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 13/04/2023 08:14, Jens Wiklander wrote: >> Adds support for sending a FF-A direct request. Checks that the SP also >> supports handling a 32-bit direct request. 64-bit direct requests are >> not used by the mediator itself so there is not need to check for that. >> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >> --- >> xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 112 insertions(+) >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >> index f129879c5b81..f2cce955d981 100644 >> --- a/xen/arch/arm/tee/ffa.c >> +++ b/xen/arch/arm/tee/ffa.c >> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) >> return true; >> } >> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) >> +{ >> + switch ( resp->a0 ) >> + { >> + case FFA_ERROR: >> + if ( resp->a2 ) >> + return resp->a2; >> + else >> + return FFA_RET_NOT_SUPPORTED; >> + case FFA_SUCCESS_32: >> + case FFA_SUCCESS_64: >> + return FFA_RET_OK; >> + default: >> + return FFA_RET_NOT_SUPPORTED; >> + } >> +} >> + >> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, >> + register_t a3, register_t a4) >> +{ >> + const struct arm_smccc_1_2_regs arg = { >> + .a0 = fid, >> + .a1 = a1, >> + .a2 = a2, >> + .a3 = a3, >> + .a4 = a4, >> + }; >> + struct arm_smccc_1_2_regs resp; >> + >> + arm_smccc_1_2_smc(&arg, &resp); >> + >> + return get_ffa_ret_code(&resp); >> +} >> + >> +static int32_t ffa_features(uint32_t id) >> +{ >> + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); >> +} >> + >> +static bool check_mandatory_feature(uint32_t id) >> +{ >> + int32_t ret = ffa_features(id); >> + >> + if (ret) >> + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", >> + id, ret); >> + >> + return !ret; >> +} >> + >> static uint16_t get_vm_id(const struct domain *d) >> { >> /* +1 since 0 is reserved for the hypervisor in FF-A */ >> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) >> set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); >> } >> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) >> +{ >> + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; >> + struct arm_smccc_1_2_regs resp = { }; >> + struct domain *d = current->domain; >> + uint32_t src_dst; >> + uint64_t mask; >> + >> + if ( smccc_is_conv_64(fid) ) >> + mask = GENMASK_ULL(63, 0); >> + else >> + mask = GENMASK_ULL(31, 0); >> + >> + src_dst = get_user_reg(regs, 1); >> + if ( (src_dst >> 16) != get_vm_id(d) ) >> + { >> + resp.a0 = FFA_ERROR; >> + resp.a2 = FFA_RET_INVALID_PARAMETERS; >> + goto out; >> + } >> + >> + arg.a1 = src_dst; >> + arg.a2 = get_user_reg(regs, 2) & mask; >> + arg.a3 = get_user_reg(regs, 3) & mask; >> + arg.a4 = get_user_reg(regs, 4) & mask; >> + arg.a5 = get_user_reg(regs, 5) & mask; >> + arg.a6 = get_user_reg(regs, 6) & mask; >> + arg.a7 = get_user_reg(regs, 7) & mask; >> + >> + arm_smccc_1_2_smc(&arg, &resp); >> + switch ( resp.a0 ) >> + { >> + case FFA_ERROR: >> + case FFA_SUCCESS_32: >> + case FFA_SUCCESS_64: >> + case FFA_MSG_SEND_DIRECT_RESP_32: >> + case FFA_MSG_SEND_DIRECT_RESP_64: >> + break; >> + default: >> + /* Bad fid, report back. */ >> + memset(&arg, 0, sizeof(arg)); >> + arg.a0 = FFA_ERROR; >> + arg.a1 = src_dst; >> + arg.a2 = FFA_RET_ABORTED; >> + } >> + >> +out: >> + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, >> + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); >> +} >> + >> static bool ffa_handle_call(struct cpu_user_regs *regs) >> { >> uint32_t fid = get_user_reg(regs, 0); >> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) >> case FFA_ID_GET: >> set_regs_success(regs, get_vm_id(d), 0); >> return true; >> + case FFA_MSG_SEND_DIRECT_REQ_32: >> + case FFA_MSG_SEND_DIRECT_REQ_64: >> + handle_msg_send_direct_req(regs, fid); >> + return true; >> default: >> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); >> @@ -326,6 +431,13 @@ static bool ffa_probe(void) >> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >> major_vers, minor_vers); >> + /* >> + * TODO save result of checked features and use that information to >> + * accept or reject requests from guests. >> + */ > > I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA? In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them. Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them. Cheers Bertrand > >> + if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) >> + return false; >> + >> ffa_version = vers; >> return true; > > Cheers, > > -- > Julien Grall
On 13/04/2023 14:20, Bertrand Marquis wrote: > Hi Julien, > >> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 13/04/2023 08:14, Jens Wiklander wrote: >>> Adds support for sending a FF-A direct request. Checks that the SP also >>> supports handling a 32-bit direct request. 64-bit direct requests are >>> not used by the mediator itself so there is not need to check for that. >>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >>> --- >>> xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 112 insertions(+) >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>> index f129879c5b81..f2cce955d981 100644 >>> --- a/xen/arch/arm/tee/ffa.c >>> +++ b/xen/arch/arm/tee/ffa.c >>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) >>> return true; >>> } >>> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) >>> +{ >>> + switch ( resp->a0 ) >>> + { >>> + case FFA_ERROR: >>> + if ( resp->a2 ) >>> + return resp->a2; >>> + else >>> + return FFA_RET_NOT_SUPPORTED; >>> + case FFA_SUCCESS_32: >>> + case FFA_SUCCESS_64: >>> + return FFA_RET_OK; >>> + default: >>> + return FFA_RET_NOT_SUPPORTED; >>> + } >>> +} >>> + >>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, >>> + register_t a3, register_t a4) >>> +{ >>> + const struct arm_smccc_1_2_regs arg = { >>> + .a0 = fid, >>> + .a1 = a1, >>> + .a2 = a2, >>> + .a3 = a3, >>> + .a4 = a4, >>> + }; >>> + struct arm_smccc_1_2_regs resp; >>> + >>> + arm_smccc_1_2_smc(&arg, &resp); >>> + >>> + return get_ffa_ret_code(&resp); >>> +} >>> + >>> +static int32_t ffa_features(uint32_t id) >>> +{ >>> + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); >>> +} >>> + >>> +static bool check_mandatory_feature(uint32_t id) >>> +{ >>> + int32_t ret = ffa_features(id); >>> + >>> + if (ret) >>> + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", >>> + id, ret); >>> + >>> + return !ret; >>> +} >>> + >>> static uint16_t get_vm_id(const struct domain *d) >>> { >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) >>> set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); >>> } >>> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) >>> +{ >>> + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; >>> + struct arm_smccc_1_2_regs resp = { }; >>> + struct domain *d = current->domain; >>> + uint32_t src_dst; >>> + uint64_t mask; >>> + >>> + if ( smccc_is_conv_64(fid) ) >>> + mask = GENMASK_ULL(63, 0); >>> + else >>> + mask = GENMASK_ULL(31, 0); >>> + >>> + src_dst = get_user_reg(regs, 1); >>> + if ( (src_dst >> 16) != get_vm_id(d) ) >>> + { >>> + resp.a0 = FFA_ERROR; >>> + resp.a2 = FFA_RET_INVALID_PARAMETERS; >>> + goto out; >>> + } >>> + >>> + arg.a1 = src_dst; >>> + arg.a2 = get_user_reg(regs, 2) & mask; >>> + arg.a3 = get_user_reg(regs, 3) & mask; >>> + arg.a4 = get_user_reg(regs, 4) & mask; >>> + arg.a5 = get_user_reg(regs, 5) & mask; >>> + arg.a6 = get_user_reg(regs, 6) & mask; >>> + arg.a7 = get_user_reg(regs, 7) & mask; >>> + >>> + arm_smccc_1_2_smc(&arg, &resp); >>> + switch ( resp.a0 ) >>> + { >>> + case FFA_ERROR: >>> + case FFA_SUCCESS_32: >>> + case FFA_SUCCESS_64: >>> + case FFA_MSG_SEND_DIRECT_RESP_32: >>> + case FFA_MSG_SEND_DIRECT_RESP_64: >>> + break; >>> + default: >>> + /* Bad fid, report back. */ >>> + memset(&arg, 0, sizeof(arg)); >>> + arg.a0 = FFA_ERROR; >>> + arg.a1 = src_dst; >>> + arg.a2 = FFA_RET_ABORTED; >>> + } >>> + >>> +out: >>> + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, >>> + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); >>> +} >>> + >>> static bool ffa_handle_call(struct cpu_user_regs *regs) >>> { >>> uint32_t fid = get_user_reg(regs, 0); >>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) >>> case FFA_ID_GET: >>> set_regs_success(regs, get_vm_id(d), 0); >>> return true; >>> + case FFA_MSG_SEND_DIRECT_REQ_32: >>> + case FFA_MSG_SEND_DIRECT_REQ_64: >>> + handle_msg_send_direct_req(regs, fid); >>> + return true; >>> default: >>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); >>> @@ -326,6 +431,13 @@ static bool ffa_probe(void) >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >>> major_vers, minor_vers); >>> + /* >>> + * TODO save result of checked features and use that information to >>> + * accept or reject requests from guests. >>> + */ >> >> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA? > > In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them. > Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them. Thanks. I would suggest the following comment: /* * At the moment domains must supports the same features used by Xen. * TODO: Rework the code to allow domain to use a subset of the features * supported. */ Note that I am using "domains" rather than "guests" because the latter doesn't include dom0. Cheers,
Hi, > On 13 Apr 2023, at 15:27, Julien Grall <julien@xen.org> wrote: > > > > On 13/04/2023 14:20, Bertrand Marquis wrote: >> Hi Julien, >>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 13/04/2023 08:14, Jens Wiklander wrote: >>>> Adds support for sending a FF-A direct request. Checks that the SP also >>>> supports handling a 32-bit direct request. 64-bit direct requests are >>>> not used by the mediator itself so there is not need to check for that. >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >>>> --- >>>> xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 112 insertions(+) >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>>> index f129879c5b81..f2cce955d981 100644 >>>> --- a/xen/arch/arm/tee/ffa.c >>>> +++ b/xen/arch/arm/tee/ffa.c >>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) >>>> return true; >>>> } >>>> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) >>>> +{ >>>> + switch ( resp->a0 ) >>>> + { >>>> + case FFA_ERROR: >>>> + if ( resp->a2 ) >>>> + return resp->a2; >>>> + else >>>> + return FFA_RET_NOT_SUPPORTED; >>>> + case FFA_SUCCESS_32: >>>> + case FFA_SUCCESS_64: >>>> + return FFA_RET_OK; >>>> + default: >>>> + return FFA_RET_NOT_SUPPORTED; >>>> + } >>>> +} >>>> + >>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, >>>> + register_t a3, register_t a4) >>>> +{ >>>> + const struct arm_smccc_1_2_regs arg = { >>>> + .a0 = fid, >>>> + .a1 = a1, >>>> + .a2 = a2, >>>> + .a3 = a3, >>>> + .a4 = a4, >>>> + }; >>>> + struct arm_smccc_1_2_regs resp; >>>> + >>>> + arm_smccc_1_2_smc(&arg, &resp); >>>> + >>>> + return get_ffa_ret_code(&resp); >>>> +} >>>> + >>>> +static int32_t ffa_features(uint32_t id) >>>> +{ >>>> + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); >>>> +} >>>> + >>>> +static bool check_mandatory_feature(uint32_t id) >>>> +{ >>>> + int32_t ret = ffa_features(id); >>>> + >>>> + if (ret) >>>> + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", >>>> + id, ret); >>>> + >>>> + return !ret; >>>> +} >>>> + >>>> static uint16_t get_vm_id(const struct domain *d) >>>> { >>>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) >>>> set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); >>>> } >>>> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) >>>> +{ >>>> + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; >>>> + struct arm_smccc_1_2_regs resp = { }; >>>> + struct domain *d = current->domain; >>>> + uint32_t src_dst; >>>> + uint64_t mask; >>>> + >>>> + if ( smccc_is_conv_64(fid) ) >>>> + mask = GENMASK_ULL(63, 0); >>>> + else >>>> + mask = GENMASK_ULL(31, 0); >>>> + >>>> + src_dst = get_user_reg(regs, 1); >>>> + if ( (src_dst >> 16) != get_vm_id(d) ) >>>> + { >>>> + resp.a0 = FFA_ERROR; >>>> + resp.a2 = FFA_RET_INVALID_PARAMETERS; >>>> + goto out; >>>> + } >>>> + >>>> + arg.a1 = src_dst; >>>> + arg.a2 = get_user_reg(regs, 2) & mask; >>>> + arg.a3 = get_user_reg(regs, 3) & mask; >>>> + arg.a4 = get_user_reg(regs, 4) & mask; >>>> + arg.a5 = get_user_reg(regs, 5) & mask; >>>> + arg.a6 = get_user_reg(regs, 6) & mask; >>>> + arg.a7 = get_user_reg(regs, 7) & mask; >>>> + >>>> + arm_smccc_1_2_smc(&arg, &resp); >>>> + switch ( resp.a0 ) >>>> + { >>>> + case FFA_ERROR: >>>> + case FFA_SUCCESS_32: >>>> + case FFA_SUCCESS_64: >>>> + case FFA_MSG_SEND_DIRECT_RESP_32: >>>> + case FFA_MSG_SEND_DIRECT_RESP_64: >>>> + break; >>>> + default: >>>> + /* Bad fid, report back. */ >>>> + memset(&arg, 0, sizeof(arg)); >>>> + arg.a0 = FFA_ERROR; >>>> + arg.a1 = src_dst; >>>> + arg.a2 = FFA_RET_ABORTED; >>>> + } >>>> + >>>> +out: >>>> + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, >>>> + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); >>>> +} >>>> + >>>> static bool ffa_handle_call(struct cpu_user_regs *regs) >>>> { >>>> uint32_t fid = get_user_reg(regs, 0); >>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) >>>> case FFA_ID_GET: >>>> set_regs_success(regs, get_vm_id(d), 0); >>>> return true; >>>> + case FFA_MSG_SEND_DIRECT_REQ_32: >>>> + case FFA_MSG_SEND_DIRECT_REQ_64: >>>> + handle_msg_send_direct_req(regs, fid); >>>> + return true; >>>> default: >>>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); >>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void) >>>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >>>> major_vers, minor_vers); >>>> + /* >>>> + * TODO save result of checked features and use that information to >>>> + * accept or reject requests from guests. >>>> + */ >>> >>> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA? >> In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them. >> Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them. > > Thanks. I would suggest the following comment: > > /* > * At the moment domains must supports the same features used by Xen. > * TODO: Rework the code to allow domain to use a subset of the features > * supported. > */ > > Note that I am using "domains" rather than "guests" because the latter doesn't include dom0. Makes sense and new comment is nice. Up to Jens to say if he is ok with it. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Henry, On Thu, Apr 13, 2023 at 1:16 PM Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Jens, > > > -----Original Message----- > > Subject: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support > > > > Adds support for sending a FF-A direct request. Checks that the SP also > > supports handling a 32-bit direct request. 64-bit direct requests are > > not used by the mediator itself so there is not need to check for that. > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > > --- > > xen/arch/arm/tee/ffa.c | 112 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 112 insertions(+) > > > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > > index f129879c5b81..f2cce955d981 100644 > > --- a/xen/arch/arm/tee/ffa.c > > +++ b/xen/arch/arm/tee/ffa.c > > @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) > > return true; > > } > > > > +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) > > +{ > > + switch ( resp->a0 ) > > + { > > + case FFA_ERROR: > > + if ( resp->a2 ) > > + return resp->a2; > > + else > > + return FFA_RET_NOT_SUPPORTED; > > + case FFA_SUCCESS_32: > > + case FFA_SUCCESS_64: > > + return FFA_RET_OK; > > + default: > > + return FFA_RET_NOT_SUPPORTED; > > + } > > +} > > + > > +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, > > + register_t a3, register_t a4) > > +{ > > + const struct arm_smccc_1_2_regs arg = { > > + .a0 = fid, > > + .a1 = a1, > > + .a2 = a2, > > + .a3 = a3, > > + .a4 = a4, > > + }; > > + struct arm_smccc_1_2_regs resp; > > + > > + arm_smccc_1_2_smc(&arg, &resp); > > + > > + return get_ffa_ret_code(&resp); > > +} > > + > > +static int32_t ffa_features(uint32_t id) > > +{ > > + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > > +} > > + > > +static bool check_mandatory_feature(uint32_t id) > > +{ > > + int32_t ret = ffa_features(id); > > + > > + if (ret) > > Coding style nit: You need spaces before and after "ret", i.e. > if ( ret ) > > With this fixed: > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> I'll fix it. Thanks, Jens > > Kind regards, > Henry
On Thu, Apr 13, 2023 at 3:44 PM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi, > > > On 13 Apr 2023, at 15:27, Julien Grall <julien@xen.org> wrote: > > > > > > > > On 13/04/2023 14:20, Bertrand Marquis wrote: > >> Hi Julien, > >>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote: > >>> > >>> Hi, > >>> > >>> On 13/04/2023 08:14, Jens Wiklander wrote: > >>>> Adds support for sending a FF-A direct request. Checks that the SP also > >>>> supports handling a 32-bit direct request. 64-bit direct requests are > >>>> not used by the mediator itself so there is not need to check for that. > >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > >>>> --- > >>>> xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 112 insertions(+) > >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >>>> index f129879c5b81..f2cce955d981 100644 > >>>> --- a/xen/arch/arm/tee/ffa.c > >>>> +++ b/xen/arch/arm/tee/ffa.c > >>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) > >>>> return true; > >>>> } > >>>> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) > >>>> +{ > >>>> + switch ( resp->a0 ) > >>>> + { > >>>> + case FFA_ERROR: > >>>> + if ( resp->a2 ) > >>>> + return resp->a2; > >>>> + else > >>>> + return FFA_RET_NOT_SUPPORTED; > >>>> + case FFA_SUCCESS_32: > >>>> + case FFA_SUCCESS_64: > >>>> + return FFA_RET_OK; > >>>> + default: > >>>> + return FFA_RET_NOT_SUPPORTED; > >>>> + } > >>>> +} > >>>> + > >>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, > >>>> + register_t a3, register_t a4) > >>>> +{ > >>>> + const struct arm_smccc_1_2_regs arg = { > >>>> + .a0 = fid, > >>>> + .a1 = a1, > >>>> + .a2 = a2, > >>>> + .a3 = a3, > >>>> + .a4 = a4, > >>>> + }; > >>>> + struct arm_smccc_1_2_regs resp; > >>>> + > >>>> + arm_smccc_1_2_smc(&arg, &resp); > >>>> + > >>>> + return get_ffa_ret_code(&resp); > >>>> +} > >>>> + > >>>> +static int32_t ffa_features(uint32_t id) > >>>> +{ > >>>> + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > >>>> +} > >>>> + > >>>> +static bool check_mandatory_feature(uint32_t id) > >>>> +{ > >>>> + int32_t ret = ffa_features(id); > >>>> + > >>>> + if (ret) > >>>> + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", > >>>> + id, ret); > >>>> + > >>>> + return !ret; > >>>> +} > >>>> + > >>>> static uint16_t get_vm_id(const struct domain *d) > >>>> { > >>>> /* +1 since 0 is reserved for the hypervisor in FF-A */ > >>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) > >>>> set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); > >>>> } > >>>> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) > >>>> +{ > >>>> + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; > >>>> + struct arm_smccc_1_2_regs resp = { }; > >>>> + struct domain *d = current->domain; > >>>> + uint32_t src_dst; > >>>> + uint64_t mask; > >>>> + > >>>> + if ( smccc_is_conv_64(fid) ) > >>>> + mask = GENMASK_ULL(63, 0); > >>>> + else > >>>> + mask = GENMASK_ULL(31, 0); > >>>> + > >>>> + src_dst = get_user_reg(regs, 1); > >>>> + if ( (src_dst >> 16) != get_vm_id(d) ) > >>>> + { > >>>> + resp.a0 = FFA_ERROR; > >>>> + resp.a2 = FFA_RET_INVALID_PARAMETERS; > >>>> + goto out; > >>>> + } > >>>> + > >>>> + arg.a1 = src_dst; > >>>> + arg.a2 = get_user_reg(regs, 2) & mask; > >>>> + arg.a3 = get_user_reg(regs, 3) & mask; > >>>> + arg.a4 = get_user_reg(regs, 4) & mask; > >>>> + arg.a5 = get_user_reg(regs, 5) & mask; > >>>> + arg.a6 = get_user_reg(regs, 6) & mask; > >>>> + arg.a7 = get_user_reg(regs, 7) & mask; > >>>> + > >>>> + arm_smccc_1_2_smc(&arg, &resp); > >>>> + switch ( resp.a0 ) > >>>> + { > >>>> + case FFA_ERROR: > >>>> + case FFA_SUCCESS_32: > >>>> + case FFA_SUCCESS_64: > >>>> + case FFA_MSG_SEND_DIRECT_RESP_32: > >>>> + case FFA_MSG_SEND_DIRECT_RESP_64: > >>>> + break; > >>>> + default: > >>>> + /* Bad fid, report back. */ > >>>> + memset(&arg, 0, sizeof(arg)); > >>>> + arg.a0 = FFA_ERROR; > >>>> + arg.a1 = src_dst; > >>>> + arg.a2 = FFA_RET_ABORTED; > >>>> + } > >>>> + > >>>> +out: > >>>> + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, > >>>> + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); > >>>> +} > >>>> + > >>>> static bool ffa_handle_call(struct cpu_user_regs *regs) > >>>> { > >>>> uint32_t fid = get_user_reg(regs, 0); > >>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) > >>>> case FFA_ID_GET: > >>>> set_regs_success(regs, get_vm_id(d), 0); > >>>> return true; > >>>> + case FFA_MSG_SEND_DIRECT_REQ_32: > >>>> + case FFA_MSG_SEND_DIRECT_REQ_64: > >>>> + handle_msg_send_direct_req(regs, fid); > >>>> + return true; > >>>> default: > >>>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); > >>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void) > >>>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > >>>> major_vers, minor_vers); > >>>> + /* > >>>> + * TODO save result of checked features and use that information to > >>>> + * accept or reject requests from guests. > >>>> + */ > >>> > >>> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA? > >> In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them. > >> Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them. > > > > Thanks. I would suggest the following comment: > > > > /* > > * At the moment domains must supports the same features used by Xen. > > * TODO: Rework the code to allow domain to use a subset of the features > > * supported. > > */ > > > > Note that I am using "domains" rather than "guests" because the latter doesn't include dom0. > > Makes sense and new comment is nice. That's better, I'll update it. Thanks, Jens > > Up to Jens to say if he is ok with it. > > Cheers > Bertrand > > > > > Cheers, > > > > -- > > Julien Grall > >
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c index f129879c5b81..f2cce955d981 100644 --- a/xen/arch/arm/tee/ffa.c +++ b/xen/arch/arm/tee/ffa.c @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers) return true; } +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp) +{ + switch ( resp->a0 ) + { + case FFA_ERROR: + if ( resp->a2 ) + return resp->a2; + else + return FFA_RET_NOT_SUPPORTED; + case FFA_SUCCESS_32: + case FFA_SUCCESS_64: + return FFA_RET_OK; + default: + return FFA_RET_NOT_SUPPORTED; + } +} + +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2, + register_t a3, register_t a4) +{ + const struct arm_smccc_1_2_regs arg = { + .a0 = fid, + .a1 = a1, + .a2 = a2, + .a3 = a3, + .a4 = a4, + }; + struct arm_smccc_1_2_regs resp; + + arm_smccc_1_2_smc(&arg, &resp); + + return get_ffa_ret_code(&resp); +} + +static int32_t ffa_features(uint32_t id) +{ + return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); +} + +static bool check_mandatory_feature(uint32_t id) +{ + int32_t ret = ffa_features(id); + + if (ret) + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n", + id, ret); + + return !ret; +} + static uint16_t get_vm_id(const struct domain *d) { /* +1 since 0 is reserved for the hypervisor in FF-A */ @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs) set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); } +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid) +{ + struct arm_smccc_1_2_regs arg = { .a0 = fid, }; + struct arm_smccc_1_2_regs resp = { }; + struct domain *d = current->domain; + uint32_t src_dst; + uint64_t mask; + + if ( smccc_is_conv_64(fid) ) + mask = GENMASK_ULL(63, 0); + else + mask = GENMASK_ULL(31, 0); + + src_dst = get_user_reg(regs, 1); + if ( (src_dst >> 16) != get_vm_id(d) ) + { + resp.a0 = FFA_ERROR; + resp.a2 = FFA_RET_INVALID_PARAMETERS; + goto out; + } + + arg.a1 = src_dst; + arg.a2 = get_user_reg(regs, 2) & mask; + arg.a3 = get_user_reg(regs, 3) & mask; + arg.a4 = get_user_reg(regs, 4) & mask; + arg.a5 = get_user_reg(regs, 5) & mask; + arg.a6 = get_user_reg(regs, 6) & mask; + arg.a7 = get_user_reg(regs, 7) & mask; + + arm_smccc_1_2_smc(&arg, &resp); + switch ( resp.a0 ) + { + case FFA_ERROR: + case FFA_SUCCESS_32: + case FFA_SUCCESS_64: + case FFA_MSG_SEND_DIRECT_RESP_32: + case FFA_MSG_SEND_DIRECT_RESP_64: + break; + default: + /* Bad fid, report back. */ + memset(&arg, 0, sizeof(arg)); + arg.a0 = FFA_ERROR; + arg.a1 = src_dst; + arg.a2 = FFA_RET_ABORTED; + } + +out: + set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask, + resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask); +} + static bool ffa_handle_call(struct cpu_user_regs *regs) { uint32_t fid = get_user_reg(regs, 0); @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) case FFA_ID_GET: set_regs_success(regs, get_vm_id(d), 0); return true; + case FFA_MSG_SEND_DIRECT_REQ_32: + case FFA_MSG_SEND_DIRECT_REQ_64: + handle_msg_send_direct_req(regs, fid); + return true; default: gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); @@ -326,6 +431,13 @@ static bool ffa_probe(void) printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", major_vers, minor_vers); + /* + * TODO save result of checked features and use that information to + * accept or reject requests from guests. + */ + if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) + return false; + ffa_version = vers; return true;
Adds support for sending a FF-A direct request. Checks that the SP also supports handling a 32-bit direct request. 64-bit direct requests are not used by the mediator itself so there is not need to check for that. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> --- xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)