Re: [PATCH 04/12] xen/arm: ffa: Add FF-A 1.2 endpoint memory access descriptors

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Feb 9, 2026 at 3:50 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 9 Feb 2026, at 11:57, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
> > <bertrand.marquis@xxxxxxx> wrote:
> >>
> >> FF-A 1.2 extends the endpoint memory access descriptor (EMAD) from
> >> 16 to 32 bytes, adding implementation-defined (IMPDEF) fields and
> >> reserved space. The MEM_SHARE path currently assumes the 1.1 EMAD
> >> size and rejects the 1.2 layout.
> >>
> >> Add FF-A 1.2 EMAD support to MEM_SHARE:
> >> - define ffa_mem_access_1_2 and store IMPDEF payload in ffa_shm_mem
> >> - emit 1.2 EMADs to the SPMC for FF-A 1.2 guests, forwarding IMPDEF
> >> - refactor header parsing into read_mem_transaction() for 1.0/1.1+
> >> - detect EMAD format by mem_access_size to allow 1.1 on 1.2 guests
> >>
> >> Functional impact: MEM_SHARE supports FF-A 1.2 EMADs.
> >>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >> ---
> >> xen/arch/arm/tee/ffa_shm.c | 108 +++++++++++++++++++++++++++++--------
> >> 1 file changed, 86 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> >> index 4c0b45cde6ee..905a64e3db01 100644
> >> --- a/xen/arch/arm/tee/ffa_shm.c
> >> +++ b/xen/arch/arm/tee/ffa_shm.c
> >> @@ -30,6 +30,14 @@ struct ffa_mem_access {
> >>     uint64_t reserved;
> >> };
> >>
> >> +/* Endpoint memory access descriptor (FF-A 1.2) */
> >> +struct ffa_mem_access_1_2 {
> >> +    struct ffa_mem_access_perm access_perm;
> >> +    uint32_t region_offs;
> >> +    uint8_t impdef[16];
> >> +    uint8_t reserved[8];
> >> +};
> >> +
> >> /* Lend, donate or share memory transaction descriptor */
> >> struct ffa_mem_transaction_1_0 {
> >>     uint16_t sender_id;
> >> @@ -73,7 +81,7 @@ struct ffa_mem_transaction_1_1 {
> >> /*
> >>  * The parts needed from struct ffa_mem_transaction_1_0 or struct
> >>  * ffa_mem_transaction_1_1, used to provide an abstraction of difference in
> >> - * data structures between version 1.0 and 1.1. This is just an internal
> >> + * data structures between version 1.0 and 1.2. This is just an internal
> >>  * interface and can be changed without changing any ABI.
> >>  */
> >> struct ffa_mem_transaction_int {
> >> @@ -92,6 +100,8 @@ struct ffa_shm_mem {
> >>     uint16_t sender_id;
> >>     uint16_t ep_id;     /* endpoint, the one lending */
> >>     uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
> >> +    /* Endpoint memory access descriptor IMPDEF value (FF-A 1.2). */
> >> +    uint64_t impdef[2];
> >>     unsigned int page_count;
> >>     struct page_info *pages[];
> >> };
> >> @@ -297,17 +307,21 @@ static void init_range(struct ffa_address_range 
> >> *addr_range,
> >>  * This function uses the ffa_spmc tx buffer to transmit the memory 
> >> transaction
> >>  * descriptor.
> >>  */
> >> -static int share_shm(struct ffa_shm_mem *shm)
> >> +static int share_shm(struct ffa_shm_mem *shm, uint32_t ffa_vers)
> >> {
> >>     const uint32_t max_frag_len = FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE;
> >>     struct ffa_mem_access *mem_access_array;
> >> +    struct ffa_mem_access_1_2 *mem_access_array_1_2;
> >>     struct ffa_mem_transaction_1_1 *descr;
> >>     struct ffa_address_range *addr_range;
> >>     struct ffa_mem_region *region_descr;
> >> -    const unsigned int region_count = 1;
> >>     uint32_t frag_len;
> >>     uint32_t tot_len;
> >> +    uint32_t mem_access_size;
> >> +    uint32_t mem_access_offs;
> >> +    uint32_t region_offs;
> >>     paddr_t last_pa;
> >> +    uint32_t range_count;
> >>     unsigned int n;
> >>     paddr_t pa;
> >>     int32_t ret;
> >> @@ -326,16 +340,35 @@ static int share_shm(struct ffa_shm_mem *shm)
> >>     descr->handle = shm->handle;
> >>     descr->mem_reg_attr = FFA_NORMAL_MEM_REG_ATTR;
> >>     descr->mem_access_count = 1;
> >> -    descr->mem_access_size = sizeof(*mem_access_array);
> >> -    descr->mem_access_offs = MEM_ACCESS_OFFSET(0);
> >> +    if ( ffa_vers >= FFA_VERSION_1_2 )
> >> +        mem_access_size = sizeof(struct ffa_mem_access_1_2);
> >> +    else
> >> +        mem_access_size = sizeof(struct ffa_mem_access);
> >> +    mem_access_offs = sizeof(struct ffa_mem_transaction_1_1);
> >> +    region_offs = mem_access_offs + mem_access_size;
> >> +    descr->mem_access_size = mem_access_size;
> >> +    descr->mem_access_offs = mem_access_offs;
> >>
> >> -    mem_access_array = buf + descr->mem_access_offs;
> >> -    memset(mem_access_array, 0, sizeof(*mem_access_array));
> >> -    mem_access_array[0].access_perm.endpoint_id = shm->ep_id;
> >> -    mem_access_array[0].access_perm.perm = FFA_MEM_ACC_RW;
> >> -    mem_access_array[0].region_offs = 
> >> REGION_OFFSET(descr->mem_access_count, 0);
> >> +    if ( ffa_vers >= FFA_VERSION_1_2 )
> >> +    {
> >> +        mem_access_array_1_2 = buf + mem_access_offs;
> >> +        memset(mem_access_array_1_2, 0, sizeof(*mem_access_array_1_2));
> >> +        mem_access_array_1_2[0].access_perm.endpoint_id = shm->ep_id;
> >> +        mem_access_array_1_2[0].access_perm.perm = FFA_MEM_ACC_RW;
> >> +        mem_access_array_1_2[0].region_offs = region_offs;
> >> +        memcpy(mem_access_array_1_2[0].impdef, shm->impdef,
> >> +               sizeof(mem_access_array_1_2[0].impdef));
> >> +    }
> >> +    else
> >> +    {
> >> +        mem_access_array = buf + mem_access_offs;
> >> +        memset(mem_access_array, 0, sizeof(*mem_access_array));
> >> +        mem_access_array[0].access_perm.endpoint_id = shm->ep_id;
> >> +        mem_access_array[0].access_perm.perm = FFA_MEM_ACC_RW;
> >> +        mem_access_array[0].region_offs = region_offs;
> >> +    }
> >>
> >> -    region_descr = buf + mem_access_array[0].region_offs;
> >> +    region_descr = buf + region_offs;
> >>     memset(region_descr, 0, sizeof(*region_descr));
> >>     region_descr->total_page_count = shm->page_count;
> >>
> >> @@ -349,8 +382,9 @@ static int share_shm(struct ffa_shm_mem *shm)
> >>         region_descr->address_range_count++;
> >>     }
> >>
> >> -    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
> >> -                                region_descr->address_range_count);
> >
> > Please remove the unused ADDR_RANGE_OFFSET() macro and friends, as
> > they're no longer accurate.
>
> Very true, will remove them in v2.
>
> >
> >> +    range_count = region_descr->address_range_count;
> >> +    tot_len = region_offs + sizeof(*region_descr) +
> >> +              range_count * sizeof(struct ffa_address_range);
> >>     if ( tot_len > max_frag_len )
> >>     {
> >>         ret = FFA_RET_NOT_SUPPORTED;
> >> @@ -358,7 +392,7 @@ static int share_shm(struct ffa_shm_mem *shm)
> >>     }
> >>
> >>     addr_range = region_descr->address_range_array;
> >> -    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 
> >> 1);
> >> +    frag_len = region_offs + sizeof(*region_descr) + sizeof(*addr_range);
> >>     last_pa = page_to_maddr(shm->pages[0]);
> >>     init_range(addr_range, last_pa);
> >>     for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> >> @@ -448,6 +482,12 @@ static int read_mem_transaction(uint32_t ffa_vers, 
> >> const void *buf, size_t blen,
> >>     if ( size * count + offs > blen )
> >>         return FFA_RET_INVALID_PARAMETERS;
> >>
> >> +    if ( size < sizeof(struct ffa_mem_access) )
> >> +        return FFA_RET_INVALID_PARAMETERS;
> >
> > Implicitly, size should also be a multiple of 16. Don't you agree?
>
> The spec is giving some constraints on the offset but there is nothing 
> explicit for the
> per mem access size. As we have no 64bit fields in it, I am not really seeing 
> any
> implicit multiple of 16.

I was perhaps assuming too much. The offset is required to be a
multiple of 16, so it would make sense for all elements in the array
to be at a 16-byte aligned offset.

>
> I am checking the offset because it is enforced by the spec but for the size 
> i would
> rather not put something as who knows what might be added in the future.

It's not important right now, since we only accept a single EMAD. But
once we accept more than one, the second EMAD might be accessed from
an unaligned address if we're not careful.

>
> >
> >> +
> >> +    if ( offs & 0xF )
> >> +        return FFA_RET_INVALID_PARAMETERS;
> >> +
> >>     trans->mem_reg_attr = mem_reg_attr;
> >>     trans->flags = flags;
> >>     trans->mem_access_size = size;
> >> @@ -464,7 +504,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     uint64_t addr = get_user_reg(regs, 3);
> >>     uint32_t page_count = get_user_reg(regs, 4);
> >>     const struct ffa_mem_region *region_descr;
> >> -    const struct ffa_mem_access *mem_access;
> >> +    const struct ffa_mem_access_1_2 *mem_access;
> >>     struct ffa_mem_transaction_int trans;
> >>     struct domain *d = current->domain;
> >>     struct ffa_ctx *ctx = d->arch.tee;
> >> @@ -474,9 +514,12 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     register_t handle_hi = 0;
> >>     register_t handle_lo = 0;
> >>     int ret = FFA_RET_DENIED;
> >> +    uint32_t ffa_vers;
> >>     uint32_t range_count;
> >>     uint32_t region_offs;
> >>     uint16_t dst_id;
> >> +    uint8_t perm;
> >> +    uint64_t impdef[2];
> >>
> >>     if ( !ffa_fw_supports_fid(FFA_MEM_SHARE_64) )
> >>     {
> >> @@ -515,8 +558,8 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     if ( frag_len > tx_size )
> >>         goto out_unlock;
> >>
> >> -    ret = read_mem_transaction(ACCESS_ONCE(ctx->guest_vers), tx_buf,
> >> -                               frag_len, &trans);
> >> +    ffa_vers = ACCESS_ONCE(ctx->guest_vers);
> >> +    ret = read_mem_transaction(ffa_vers, tx_buf, frag_len, &trans);
> >>     if ( ret )
> >>         goto out_unlock;
> >>
> >> @@ -545,13 +588,35 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>         goto out_unlock;
> >>     }
> >>
> >> +    if ( trans.mem_access_size < sizeof(struct ffa_mem_access) )
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out_unlock;
> >> +    }
> >> +
> >>     /* Check that it fits in the supplied data */
> >>     if ( trans.mem_access_offs + trans.mem_access_size > frag_len )
> >>         goto out_unlock;
> >>
> >>     mem_access = tx_buf + trans.mem_access_offs;
> >> -
> >>     dst_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
> >> +    perm = ACCESS_ONCE(mem_access->access_perm.perm);
> >> +    region_offs = ACCESS_ONCE(mem_access->region_offs);
> >> +
> >> +    /*
> >> +     * FF-A 1.2 introduced an extended mem_access descriptor with impdef
> >> +     * fields, but guests can still use the 1.1 format if they don't need
> >> +     * implementation-defined data. Detect which format is used based on
> >> +     * the mem_access_size field rather than the negotiated FF-A version.
> >> +     */
> >> +    if ( trans.mem_access_size >= sizeof(struct ffa_mem_access_1_2) )
> >> +        memcpy(impdef, mem_access->impdef, sizeof(impdef));
> >> +    else
> >> +    {
> >> +        impdef[0] = 0;
> >> +        impdef[1] = 0;
> >> +    }
> >> +
> >>     if ( !FFA_ID_IS_SECURE(dst_id) )
> >>     {
> >>         /* we do not support sharing with VMs */
> >> @@ -559,13 +624,11 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>         goto out_unlock;
> >>     }
> >>
> >> -    if ( ACCESS_ONCE(mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
> >> +    if ( perm != FFA_MEM_ACC_RW )
> >>     {
> >>         ret = FFA_RET_NOT_SUPPORTED;
> >>         goto out_unlock;
> >>     }
> >> -
> >> -    region_offs = ACCESS_ONCE(mem_access->region_offs);
> >>     if ( sizeof(*region_descr) + region_offs > frag_len )
> >>     {
> >>         ret = FFA_RET_NOT_SUPPORTED;
> >> @@ -590,6 +653,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     }
> >>     shm->sender_id = trans.sender_id;
> >>     shm->ep_id = dst_id;
> >> +    memcpy(shm->impdef, impdef, sizeof(shm->impdef));
> >>
> >>     /*
> >>      * Check that the Composite memory region descriptor fits.
> >> @@ -605,7 +669,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> >>     if ( ret )
> >>         goto out;
> >>
> >> -    ret = share_shm(shm);
> >> +    ret = share_shm(shm, ffa_vers);
> >
> > Shouldn't we rather use ffa_fw_version?
>
> Definitely yes.
>
> In fact i have done this in a follow up patch and i need to export the fw 
> version to be able to
> do that but I will bring that forward and do it here, that makes a lot more 
> sense.

Sounds good.

Cheers,
Jens

>
> Cheers
> Bertrand
>
> >
> > Cheers,
> > Jens
> >
> >>     if ( ret )
> >>         goto out;
> >>
> >> --
> >> 2.50.1 (Apple Git-155)
>
>