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

[Sudeep Holla <sudeep.holla@arm.com>]

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@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@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 ?

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.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel