Re: [PATCH 6/9] firmware: arm_ffa: Add initial Arm FFA driver support

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

On Mon, Sep 07, 2020 at 08:55:13AM +0100, Fuad Tabba wrote:
> Hi Sudeep,
> 
> I understand that this is an RFC, but I have a few suggestions about
> how the FF-A interface code might be structured.  See below.
> 
> On Sat, Aug 29, 2020 at 6:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > This just add a basic driver that sets up the transport(e.g. SMCCC),
> > checks the FFA version implemented, get the partition ID for self and
> > sets up the Tx/Rx buffers for communication.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_ffa/Makefile |   3 +-
> >  drivers/firmware/arm_ffa/common.h |  23 +++
> >  drivers/firmware/arm_ffa/driver.c | 288 ++++++++++++++++++++++++++++++
> >  3 files changed, 313 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/firmware/arm_ffa/common.h
> >  create mode 100644 drivers/firmware/arm_ffa/driver.c
> >

[...]

> > +
> > +/**
> > + * FF-A specification mentions explicitly about '4K pages'. This should
> > + * not be confused with the kernel PAGE_SIZE, which is the translation
> > + * granule kernel is configured and may be one among 4K, 16K and 64K.
> > + */
> > +#define FFA_PAGE_SIZE          SZ_4K
> > +/* Keeping RX TX buffer size as 64K for now */
> > +#define RXTX_BUFFER_SIZE       SZ_64K
> 
> The code/definitions above will be reused in other parts that deal
> will FF-A (e.g., support for FF-A in KVM itself), so it might be good
> to have it in a common header.  I was wondering if it might even be a
> good idea to reuse the Hafnium headers here (assuming I understand
> licensing right):
> https://review.trustedfirmware.org/plugins/gitiles/hafnium/hafnium/+/refs/heads/master/inc/vmapi/hf/ffa.h
> 

I know few DTS files have dual license, but I am not sure about the
headers and other source. But I agree on a common header and forgot to
mention that explicitly but I am aware of, that we not only need common
header, but some of the functions may also be reused. I am keeping them
in the driver for now. We can move once we the KVM part also starts
shaping up(before or after one of then gets merged, doesn't matter much)

> > +
> > +static ffa_fn *invoke_ffa_fn;
> > +
> > +static const int ffa_linux_errmap[] = {
> > +       /* better than switch case as long as return value is continuous */
> > +       0,              /* FFA_RET_SUCCESS */
> > +       -EOPNOTSUPP,    /* FFA_RET_NOT_SUPPORTED */
> > +       -EINVAL,        /* FFA_RET_INVALID_PARAMETERS */
> > +       -ENOMEM,        /* FFA_RET_NO_MEMORY */
> > +       -EBUSY,         /* FFA_RET_BUSY */
> > +       -EINTR,         /* FFA_RET_INTERRUPTED */
> > +       -EACCES,        /* FFA_RET_DENIED */
> > +       -EAGAIN,        /* FFA_RET_RETRY */
> > +       -ECANCELED,     /* FFA_RET_ABORTED */
> > +};
> > +
> > +static inline int ffa_to_linux_errno(int errno)
> > +{
> > +       if (errno < FFA_RET_SUCCESS && errno >= FFA_RET_ABORTED)
> > +               return ffa_linux_errmap[-errno];
> > +       return -EINVAL;
> > +}
> 
> Hardcoding the range check to be bound by FFA_RET_ABORTED could cause
> some issues in the future if more error codes are added.  It might be
> safer to check against the number of elements in ffa_linux_errmap.
> 

Makes sense, will see how I can fix that.

-- 
Regards,
Sudeep