FW: [PATCH] firmware: arm_ffa: Add support for IMPDEF value in the memory access descriptor

[Sudeep Holla]

On Tue, Sep 16, 2025 at 01:14:53PM +0800, Lixiang Mao wrote:
> Let me reply this by my OSS mail username.
> 

For some reason this got marked spam and landed in junk emails.
Sorry for the delay in finding/responding.

> The comments was inline.
> 
> > -----Original Message-----
> > From: Sudeep Holla<sudeep.holla at arm.com> Sent: Monday, September 15,
> > 2025 11:01 PM
> > To:linux-arm-kernel at lists.infradead.org
> > Cc: Sudeep Holla<sudeep.holla at arm.com>; Lixiang Mao<liximao at qti.qualcomm.com>
> > Subject: [PATCH] firmware: arm_ffa: Add support for IMPDEF value in the memory access descriptor
> > 
> > WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> > 
> > FF-A v1.2 introduced 16 byte IMPLEMENTATION DEFINED value in the endpoint memory access descriptor to allow any sender could to specify an its any custom value for each receiver. Also this value must be specified by the receiver when retrieving the memory region. The sender must ensure it informs the receiver of this value via an IMPLEMENTATION DEFINED mechanism such as a partition message.
> > 
> > So the FF-A driver can use the message interfaces to communicate the value and set the same in the ffa_mem_region_attributes structures when using the memory interfaces.
> > 
> > The driver ensure that the size of the endpoint memory access descriptors is set correctly based on the FF-A version.
> > 
> > Fixes: 9fac08d9d985 ("firmware: arm_ffa: Upgrade FF-A version to v1.2 in the driver")
> > Reported-by: Lixiang Mao<liximao at qti.qualcomm.com>
> > Signed-off-by: Sudeep Holla<sudeep.holla at arm.com>
> > ---
> >   drivers/firmware/arm_ffa/driver.c | 46 ++++++++++++++++++++++++++-----
> >   include/linux/arm_ffa.h           |  1 +
> >   2 files changed, 40 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index cf6e44429ecd..2ea30ce93d78 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -654,6 +654,41 @@ static u16 ffa_memory_attributes_get(u32 func_id)
> >          return FFA_MEM_NORMAL | FFA_MEM_WRITE_BACK | FFA_MEM_INNER_SHAREABLE;  }
> > 
> > +static u32 ffa_emad_size_get(u32 version) {
> > +       struct ffa_mem_region_attributes *ep_mem_access;
> > +       u32 sz;
> > +
> > +       if (version <= FFA_VERSION_1_1)
> > +               sz = sizeof(*ep_mem_access) - sizeof(ep_mem_access->impdef_val);
> > +       else
> > +               sz = sizeof(*ep_mem_access);
> > +
> > +       return sz;
> > +}
> > +
> > +static void ffa_emad_impdef_value_init(u32 version, void *dst, void
> > +*src) {
> > +       struct ffa_mem_region_attributes *ep_mem_access;
> > +
> > +       if (version > FFA_VERSION_1_1)
> > +               memcpy(dst, src, sizeof(ep_mem_access->impdef_val));
> > +       else
> > +               memset(dst, 0, sizeof(ep_mem_access->impdef_val));
> > +}
> > +
> > +static void
> > +ffa_mem_region_additional_setup(u32 version, struct ffa_mem_region
> > +*mem_region) {
> > +       if (version <= FFA_VERSION_1_0) {
> > +               mem_region->ep_mem_size = 0;
> > +       } else {
> > +               mem_region->ep_mem_size = ffa_emad_size_get(version);
> > +               mem_region->ep_mem_offset = sizeof(*mem_region);
> > +               memset(mem_region->reserved, 0, 12);
> > +       }
> > +}
> > +
> >   static int
> >   ffa_setup_and_transmit(u32 func_id, void *buffer, u32 max_fragsize,
> >                         struct ffa_mem_ops_args *args) @@ -683,16 +718,13 @@ ffa_setup_and_transmit(u32 func_id, void *buffer, u32 max_fragsize,
> >                  ep_mem_access->composite_off = composite_offset;
> >                  ep_mem_access->flag = 0;
> >                  ep_mem_access->reserved = 0;
> > +               ffa_emad_impdef_value_init(drv_info->version,
> > +                                          ep_mem_access->impdef_val,
> > +                                          args->attrs[idx].impdef_val);
> 
> Lixiang: We shouldn't reach next emad by increasing ep_mem_access if the
> negotiated version is V1.1. For V1.1 emad, we should increase 16 bytes for
> every iteration.

Agreed, I missed that.

> 
> >          }
> >          mem_region->handle = 0;
> >          mem_region->ep_count = args->nattrs;
> > -       if (drv_info->version <= FFA_VERSION_1_0) {
> > -               mem_region->ep_mem_size = 0;
> > -       } else {
> > -               mem_region->ep_mem_size = sizeof(*ep_mem_access);
> > -               mem_region->ep_mem_offset = sizeof(*mem_region);
> > -               memset(mem_region->reserved, 0, 12);
> > -       }
> > +       ffa_mem_region_additional_setup(drv_info->version, mem_region);
> > 
> >          composite = buffer + composite_offset;
> >          composite->total_pg_cnt = ffa_get_num_pages_sg(args->sg); diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h index 0a0132c6ae3e..a1b0ee11c4a6 100644
> > --- a/include/linux/arm_ffa.h
> > +++ b/include/linux/arm_ffa.h
> > @@ -347,6 +347,7 @@ struct ffa_mem_region_attributes {
> >           * an `struct ffa_mem_region_addr_range`.
> >           */
> >          u32 composite_off;
> > +       u8 impdef_val[16];
> Lixiang:If we add the implementation defined value here, we must take care
> when use sizeof(struct ffa_mem_region_attributes). At least we should make a
> replacement of the one in function ffa_mem_desc_offset. Change sizeof(struct
> ffa_mem_region_attributes) to ffa_emad_size_get().


Yes, will quickly rework and repost it. I still need your help to validate
as I don't have a setup to check this imdef_value usage.

-- 
Regards,
Sudeep