[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v7 10/20] xen/arm: ffa: add direct request support
Hi, On Wed, Mar 1, 2023 at 2:06 PM Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote: > > HI Jens, > > > On 1 Mar 2023, at 11:55, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > Hi Bertrand, > > > > On Mon, Feb 27, 2023 at 4:28 PM Bertrand Marquis > > <Bertrand.Marquis@xxxxxxx> wrote: > >> > >> Hi Jens, > >> > >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@xxxxxxxxxx> > >>> 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@xxxxxxxxxx> > >>> --- > >>> xen/arch/arm/tee/ffa.c | 119 +++++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 119 insertions(+) > >>> > >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >>> index 463fd7730573..a5d8a12635b6 100644 > >>> --- a/xen/arch/arm/tee/ffa.c > >>> +++ b/xen/arch/arm/tee/ffa.c > >>> @@ -142,6 +142,7 @@ > >>> > >>> struct ffa_ctx { > >>> uint32_t guest_vers; > >>> + bool interrupted; > >> > >> This is added and set here for one special error code but is never used. > >> I would suggest to introduce this when there will be an action based on it. > > > > I'm sorry, I forgot about completing this. I'll add code to deal with > > FFA_INTERRUPT. This will be tricky to test though since we don't use > > FFA_INTERRUPT like this with OP-TEE. The Hypervisor is required by the > > FF-A standard to support it so I better add something. > > You can do that in a different patch then and just remove this from this > patch ? OK, I'll do that. > > > > >> > >>> }; > >>> > >>> /* Negotiated FF-A version to use with the SPMC */ > >>> @@ -167,6 +168,55 @@ 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) > >>> +{ > >>> + uint32_t ret = ffa_features(id); > >>> + > >>> + if (ret) > >>> + printk(XENLOG_ERR "ffa: mandatory feature id %#x missing\n", id); > >> > >> It might be useful here to actually print the error code. > >> Are we sure that all errors actually mean not supported ? > > > > Yes, that's what the standard says. > > The error code might still be useful in the print. OK, I'll add it. > > > > >> > >>> + > >>> + return !ret; > >>> +} > >>> + > >>> static uint16_t get_vm_id(const struct domain *d) > >>> { > >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ > >>> @@ -208,6 +258,66 @@ 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; > >>> + struct ffa_ctx *ctx = d->arch.tee; > >>> + 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; > >>> + > >>> + while ( true ) > >>> + { > >>> + arm_smccc_1_2_smc(&arg, &resp); > >>> + > >>> + switch ( resp.a0 ) > >>> + { > >>> + case FFA_INTERRUPT: > >>> + ctx->interrupted = true; > >>> + goto out; > >>> + 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: > >>> + goto out; > >>> + default: > >>> + /* Bad fid, report back. */ > >>> + memset(&arg, 0, sizeof(arg)); > >>> + arg.a0 = FFA_ERROR; > >>> + arg.a1 = src_dst; > >>> + arg.a2 = FFA_RET_NOT_SUPPORTED; > >>> + continue; > >> > >> There is a potential infinite loop here and i do not understand > >> why this needs to be done. > >> Here if something is returning a value that you do not understand > >> you send back an ERROR to it. I do not find in the spec where this > >> is supposed to be done. > >> Can you explain a bit here ? > > > > This should normally not happen, but the SP/SPMC is responding with a > > request that we don't know what to do with. The standard doesn't say > > how to handle that as far as I understand. However, returning back to > > the VM at this point with an error may leave the SP/SPMC in a strange > > state. So I think it's better to report back to the SP/SPMC that the > > request isn't understood and hopefully it can at least return back > > with an error in a sane state. > > > > I'll add something to the comment. > > Please also make sure that the code is not looping infinitely on this. I'll remove the loop as agreed in the other mail. > > > > >> > >>> + } > >>> + } > >>> + > >>> +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); > >>> @@ -225,6 +335,12 @@ 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: > >>> +#ifdef CONFIG_ARM_64 > >>> + case FFA_MSG_SEND_DIRECT_REQ_64: > >>> +#endif > >>> + handle_msg_send_direct_req(regs, fid); > >>> + return true; > >>> > >>> default: > >>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); > >>> @@ -310,6 +426,9 @@ static bool ffa_probe(void) > >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > >>> major_vers, minor_vers); > >>> > >>> + if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > >>> + return false; > >> > >> One could not need this feature and here this will make everything > >> unavailable instead. > >> Why not just reporting back the unsupported error to clients using > >> unsupported interfaces ? > > > > One could perhaps argue that this check should be moved to a later > > patch in this series. Perhaps there's some future configuration that > > might make sense without this feature, but for now, it doesn't make > > sense to initialize without it. > > I am starting to wonder if we should not at boot scan for available features, > save them > somewhere and then accept/reject calls depending on the supported features. > > Maybe just add a TODO here so that we remember that this is something that > could be > checked/modified one day. That would also give an insight if someone has such > a usecase > one day. I'll add a comment. Cheers, Jens > > Cheers > Bertrand > > > > > Thanks, > > Jens > > > >> > >> Cheers > >> Bertrand > >> > >>> + > >>> ffa_version = vers; > >>> > >>> return true; > >>> -- > >>> 2.34.1 > >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |