Re: [PATCH 7/9] firmware: arm_ffa: Add v1.1 get_partition_info support

[Jens Wiklander <jens.wiklander@linaro.org>]

On Thu, Sep 1, 2022 at 10:58 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Sep 01, 2022 at 10:43:58AM +0200, Jens Wiklander wrote:
> > On Tue, Aug 30, 2022 at 12:07 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > FF-A v1.1 adds support to discovery the UUIDs of the partitions that was
> > > missing in v1.0 and which the driver workarounds by using UUIDs supplied
> > > by the ffa_drivers.
> > >
> > > Add the v1.1 get_partition_info support and disable the workaround if
> > > the detected FF-A version is greater than v1.0.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  drivers/firmware/arm_ffa/driver.c | 39 ++++++++++++++++++++++++-------
> > >  include/linux/arm_ffa.h           |  1 +
> > >  2 files changed, 32 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index 5c8484b05c50..6822241168d6 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -264,18 +264,24 @@ static int ffa_rxtx_unmap(u16 vm_id)
> > >         return 0;
> > >  }
> > >
> > > +#define PARTITION_INFO_GET_RETURN_COUNT_ONLY   BIT(0)
> > > +
> > >  /* buffer must be sizeof(struct ffa_partition_info) * num_partitions */
> > >  static int
> > >  __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > >                          struct ffa_partition_info *buffer, int num_partitions)
> > >  {
> > > -       int count;
> > > +       int idx, count, flags = 0, size;
> > >         ffa_value_t partition_info;
> > >
> > > +       if (!buffer || !num_partitions) /* Just get the count for now */
> > > +               flags = PARTITION_INFO_GET_RETURN_COUNT_ONLY;
> > > +
> > >         mutex_lock(&drv_info->rx_lock);
> > >         invoke_ffa_fn((ffa_value_t){
> > >                       .a0 = FFA_PARTITION_INFO_GET,
> > >                       .a1 = uuid0, .a2 = uuid1, .a3 = uuid2, .a4 = uuid3,
> > > +                     .a5 = flags,
> > >                       }, &partition_info);
> > >
> > >         if (partition_info.a0 == FFA_ERROR) {
> > > @@ -285,8 +291,15 @@ __ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
> > >
> > >         count = partition_info.a2;
> > >
> > > +       if (drv_info->version > FFA_VERSION_1_0)
> > > +               size = partition_info.a3;
> >
> > What happens if this value is larger than sizeof(struct ffa_partition_info)?
> >
>
> Right, I had the check for both 0 size and size > struct size. I removed
> both at once instead of just dropping 0 size. I assume 0 size is non
> compliant or do you prefer to keep that check as well.

The purpose with this size field is to be able to read the known part
of each element in the array.
So I believe that memcpy() below should copy MIN(size, sizeof(struct
ffa_partition_info)), assuming that we update struct
ffa_partition_info if we need to when bumping our reported FF-A
version.

I'm not sure if we need more checks for compliance or not.

Cheers,
Jens

>
> Thanks a lot for the reviews.
>
> --
> Regards,
> Sudeep