Re: [PATCH v3 6/7] firmware: arm_ffa: Setup in-kernel users of FFA partitions

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

On Fri, Dec 11, 2020 at 11:59:40AM +0100, Jens Wiklander wrote:
> One more comment below.
> 
> On Fri, Dec 11, 2020 at 11:45 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sudeep,
> >
> > Some more comments below.
> >
> > On Fri, Dec 4, 2020 at 1:11 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > Parse the FFA nodes from the device-tree and register all the partitions
> > > whose services will be used in the kernel.
> > >
> > > In order to also enable in-kernel users of FFA interface, let us add
> > > simple set of operations for such devices.
> > >
> > > The in-kernel users are registered without the character device interface.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  drivers/firmware/arm_ffa/common.h |   2 +
> > >  drivers/firmware/arm_ffa/driver.c | 186 ++++++++++++++++++++++++++++++
> > >  include/linux/arm_ffa.h           |  36 +++++-
> > >  3 files changed, 223 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h
> > > index d019348bf67d..eb1371c2b2b8 100644
> > > --- a/drivers/firmware/arm_ffa/common.h
> > > +++ b/drivers/firmware/arm_ffa/common.h
> > > @@ -6,6 +6,7 @@
> > >  #ifndef _FFA_COMMON_H
> > >  #define _FFA_COMMON_H
> > >
> > > +#include <linux/arm_ffa.h>
> > >  #include <linux/arm-smccc.h>
> > >  #include <linux/err.h>
> > >
> > > @@ -17,6 +18,7 @@ typedef ffa_res_t
> > >
> > >  int __init arm_ffa_bus_init(void);
> > >  void __exit arm_ffa_bus_exit(void);
> > > +bool ffa_device_is_valid(struct ffa_device *ffa_dev);
> > >
> > >  #ifdef CONFIG_ARM_FFA_SMCCC
> > >  int __init ffa_transport_init(ffa_fn **invoke_ffa_fn);
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index 257b331d781c..3e4ba841dbf8 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -24,9 +24,13 @@
> > >
> > >  #include <linux/arm_ffa.h>
> > >  #include <linux/bitfield.h>
> > > +#include <linux/device.h>
> > >  #include <linux/io.h>
> > > +#include <linux/kernel.h>
> > >  #include <linux/module.h>
> > > +#include <linux/of.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/uuid.h>
> > >
> > >  #include "common.h"
> > >
> > > @@ -179,6 +183,20 @@ static int ffa_version_check(u32 *version)
> > >         return 0;
> > >  }
> > >
> > > +static int ffa_rx_release(void)
> > > +{
> > > +       ffa_res_t ret;
> > > +
> > > +       ret = invoke_ffa_fn(FFA_RX_RELEASE, 0, 0, 0, 0, 0, 0, 0);
> > > +
> > > +       if (ret.a0 == FFA_ERROR)
> > > +               return ffa_to_linux_errno((int)ret.a2);
> > > +
> > > +       /* check for ret.a0 == FFA_RX_RELEASE ? */
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int ffa_rxtx_map(phys_addr_t tx_buf, phys_addr_t rx_buf, u32 pg_cnt)
> > >  {
> > >         ffa_res_t ret;
> > > @@ -203,6 +221,50 @@ static int ffa_rxtx_unmap(u16 vm_id)
> > >         return 0;
> > >  }
> > >
> > > +static int __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > > +                                   struct ffa_partition_info **buffer)
> > > +{
> > > +       int count;
> > > +       ffa_res_t partition_info;
> > > +
> > > +       mutex_lock(&drv_info->rx_lock);
> > > +       partition_info = invoke_ffa_fn(FFA_PARTITION_INFO_GET, uuid0, uuid1,
> > > +                                      uuid2, uuid3, 0, 0, 0);
> > > +
> > > +       if (partition_info.a0 == FFA_ERROR)
> > > +               return ffa_to_linux_errno((int)partition_info.a2);
> > > +
> > > +       count = partition_info.a2;
> > > +
> > > +       if (buffer)
> > > +               memcpy(*buffer, drv_info->rx_buffer, sizeof(*buffer) * count);
> > > +
> > > +       ffa_rx_release();
> > > +
> > > +       mutex_unlock(&drv_info->rx_lock);
> > > +
> > > +       return count;
> > > +}
> > > +
> > > +static int ffa_partition_probe(const char *uuid_str,
> > > +                              struct ffa_partition_info *buffer)
> > > +{
> > > +       int count;
> > > +       uuid_t uuid;
> > > +       u32 uuid0_4[4] = { 0 };
> > > +
> > > +       if (uuid_parse(uuid_str, &uuid)) {
> > > +               pr_err("invalid uuid (%s)\n", uuid_str);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       export_uuid((u8 *)uuid0_4, &uuid);
> > > +       count = __ffa_partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2],
> > > +                                        uuid0_4[3], &buffer);
> Wrong byte order?
> According to section 5.3 of the SMCCC, UUIDs are returned as a single
> 128-bit value using the SMC32 calling convention. This value is mapped
> to argument registers x0-x3 on AArch64 (resp. r0-r3 on AArch32). x0
> for example shall hold bytes 0 to 3, with byte 0 in the low-order
> bits.
>

I need to spend some time to understand the concern here. Initially I agreed
with your analysis and then a quick review make be realise it is all OK.
I need to check if my understanding is correct again. I thought I will
take example and check here itself.

UUID: "fd02c9da-306c-48c7-a49c-bbd827ae86ee"

UUID[0]   UUID[1]  UUID[2]  UUID[3] (referring uuid0_4 above)
dac902fd c7486c30 d8bb9ca4 ee86ae27

It seems correct as per SMCCC convention to me, or am I missing something
obvious ?

--
Regards,
Sudeep