[PATCH 4/4] firmware: arm_ffa: Add support for FFA_MSG_SEND2

[Jens Wiklander]

On Tue, Apr 16, 2024 at 10:48 AM Sudeep Holla <sudeep.holla at arm.com> wrote:
>
> On Tue, Apr 16, 2024 at 09:41:51AM +0200, Jens Wiklander wrote:
> > On Mon, Apr 15, 2024 at 6:05 PM Sudeep Holla <sudeep.holla at arm.com> wrote:
> > >
> > > The FFA_MSG_SEND2 can be  used to transmit a partition message from
> > > the Tx buffer of the sender(the driver in this case) endpoint to the Rx
> > > buffer of the receiver endpoint.
> > >
> > > An invocation of the FFA_MSG_SEND2 transfers the ownership to the
> >
> > ownership of the TX buffer to the
> >
> > > receiver endpoint(or any intermediate consumer). Completion of an
> > > FFA_MSG_SEND2 invocation transfers the ownership back to the sender
> >
> > ownership of the buffer back
> >
> > > endpoint.
> > >
> > > The framework defines the FFA_MSG_SEND2 interface to transmit a partition
> > > message from the Tx buffer of the sender to the Rx buffer of a receiver
> > > and inform the scheduler that the receiver must be run.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla at arm.com>
> > > ---
> > >  drivers/firmware/arm_ffa/driver.c | 40 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/arm_ffa.h           |  9 +++++++++
> > >  2 files changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index d5087e4f6d35..6c2602f7e7cc 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -344,6 +344,34 @@ static int ffa_msg_send_direct_req(u16 src_id, u16 dst_id, bool mode_32bit,
> > >         return -EINVAL;
> > >  }
> > >
> > > +static int ffa_msg_send2(u16 src_id, u16 dst_id, void *buf, size_t sz)
> > > +{
> > > +       u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
> > > +       struct ffa_indirect_msg_hdr *msg;
> > > +       ffa_value_t ret;
> > > +
> > > +       mutex_lock(&drv_info->tx_lock);
> > > +
> > > +       msg = drv_info->tx_buffer;
> > > +       msg->flags = 0;
> > > +       msg->res0 = 0;
> > > +       msg->offset = sizeof(*msg);
> > > +       msg->send_recv_id = src_dst_ids;
> > > +       msg->size = sz;
> > > +       memcpy(msg + msg->offset, buf, sz);
> > > +
> > > +       /* flags = 0, sender VMID = 0 works for both physical/virtual NS */
> > > +       invoke_ffa_fn((ffa_value_t){
> > > +                     .a0 = FFA_MSG_SEND2, .a1 = 0, .a2 = 0
> > > +                     }, &ret);
> > > +
> > > +       if (ret.a0 == FFA_ERROR)
> > > +               return ffa_to_linux_errno((int)ret.a2);
> > > +
> > > +       mutex_lock(&drv_info->tx_lock);
> >
> > mutex_unlock(), before the potential return above?
> >
>
> Ah, my bad. Thanks for the catch.
>
> > > +       return 0;
> > > +}
> > > +
> > >  static int ffa_mem_first_frag(u32 func_id, phys_addr_t buf, u32 buf_sz,
> > >                               u32 frag_len, u32 len, u64 *handle)
> > >  {
> > > @@ -888,6 +916,17 @@ static int ffa_sync_send_receive(struct ffa_device *dev,
> > >                                        dev->mode_32bit, data);
> > >  }
> > >
> > > +#define ffa_partition_supports_indirect_msg(dev)       \
> > > +       ffa_partition_check_property(dev, FFA_PARTITION_INDIRECT_MSG)
> > > +
> > > +static int ffa_indirect_msg_send(struct ffa_device *dev, void *buf, size_t sz)
> > > +{
> > > +       if (!ffa_partition_supports_direct_recv(dev))
> >
> > ffa_partition_supports_indirect_msg(), but I'm not sure we should do
> > this check at all. The client could do this in advance. Unexpected
> > FFA_MSG_SEND2 calls are caught in other layers.
> >
>
> Good point. I was not sure if it makes sense to add on each message but
> I wasn't sure if we can defer this to the client. But based on what you
> say, it should be OK do defer it to the client.
>
> So the next question I have is: should we populate properties in the
> ffa_device so that client can use the same. I started with that but then
> didn't want to expose the info to the client.
>
> I can move the properties to the struct ffa_device and keep these macro
> arm_ffa.h for clients to use if they wish. Does that make sense ?

Yes, that sounds good.

>
> Thanks for taking look at the patches. I will skip responding on other
> 2 patches as I have asked all my questions as part of this patch and they
> apply to those 2 as well.

OK

Cheers,
Jens

>
> --
> Regards,
> Sudeep