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

Sudeep Holla
2020-09-07T09:30:26Z

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