[XEN,v7,18/20] xen/arm: ffa: support sharing memory

Commit Message

Jens Wiklander Feb. 22, 2023, 3:33 p.m. UTC

Adds support for a guest to share memory with an SP using FFA_MEM_SHARE
and FFA_MEM_RECLAIM. Only small memory regions can be shared using a
single call to FFA_MEM_SHARE are supported.

A memory region that doesn't need to be shared any longer can be
reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
This is checked by the SPMC and not in control of the mediator.

With this commit we have a FF-A version 1.1 [1] mediator able to
communicate with a Secure Partition in secure world using shared memory.
The secure world must use FF-A version 1.1, but the guest is free to use
version 1.0 or version 1.1.

Adds a check that the SP supports the needed FF-A features
FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.

[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander <[e-mail]>
---
 xen/arch/arm/tee/ffa.c | 491 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 491 insertions(+)

Comments

Bertrand Marquis March 13, 2023, 8:49 a.m. UTC | #1

Hi Jens,

> On 22 Feb 2023, at 16:33, Jens Wiklander <[e-mail]> wrote:
> 
> Adds support for a guest to share memory with an SP using FFA_MEM_SHARE
> and FFA_MEM_RECLAIM. Only small memory regions can be shared using a
> single call to FFA_MEM_SHARE are supported.

This sentence needs a bit of rephrasing and to add more details: what is "small".

> 
> A memory region that doesn't need to be shared any longer can be
> reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
> This is checked by the SPMC and not in control of the mediator.

This explanation would make more sense in the following patch adding support
 for Reclaim.

> 
> With this commit we have a FF-A version 1.1 [1] mediator able to
> communicate with a Secure Partition in secure world using shared memory.
> The secure world must use FF-A version 1.1, but the guest is free to use
> version 1.0 or version 1.1.

I do not see anything limiting that in the code.
During init we accept 1.0 or 1.1 versions of the secure world.

> 
> Adds a check that the SP supports the needed FF-A features
> FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.
> 
> [1] https://developer.arm.com/documentation/den0077/latest
> Signed-off-by: Jens Wiklander <[e-mail]>
> ---
> xen/arch/arm/tee/ffa.c | 491 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 491 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 94c90b252454..cdc286caf62c 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -270,6 +270,38 @@ struct ffa_mem_transaction_1_1 {
>     uint8_t reserved[12];
> };
> 
> +/* Calculate offset of struct ffa_mem_access from start of buffer */
> +#define MEM_ACCESS_OFFSET(access_idx) \
> +    ( sizeof(struct ffa_mem_transaction_1_1) + \
> +      ( access_idx ) * sizeof(struct ffa_mem_access) )
> +
> +/* Calculate offset of struct ffa_mem_region from start of buffer */
> +#define REGION_OFFSET(access_count, region_idx) \
> +    ( MEM_ACCESS_OFFSET(access_count) + \
> +      ( region_idx ) * sizeof(struct ffa_mem_region) )
> +
> +/* Calculate offset of struct ffa_address_range from start of buffer */
> +#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
> +    ( REGION_OFFSET(access_count, region_count) + \
> +      ( range_idx ) * sizeof(struct ffa_address_range) )
> +
> +/*
> + * 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
> + * interface and can be changed without changing any ABI.
> + */
> +struct ffa_mem_transaction_x {

I would suggest to s/_x/_int/ in the name here

> +    uint16_t sender_id;
> +    uint8_t mem_reg_attr;
> +    uint8_t flags;
> +    uint8_t mem_access_size;
> +    uint8_t mem_access_count;
> +    uint16_t mem_access_offs;
> +    uint64_t global_handle;
> +    uint64_t tag;
> +};
> +
> /* Endpoint RX/TX descriptor */
> struct ffa_endpoint_rxtx_descriptor_1_0 {
>     uint16_t sender_id;
> @@ -294,8 +326,20 @@ struct ffa_ctx {
>     uint32_t guest_vers;
>     bool tx_is_mine;
>     bool interrupted;
> +    struct list_head shm_list;
> +    unsigned int shm_count;
>     spinlock_t lock;
> };
> +
> +struct ffa_shm_mem {
> +    struct list_head list;
> +    uint16_t sender_id;
> +    uint16_t ep_id;     /* endpoint, the one lending */
> +    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
> +    unsigned int page_count;
> +    struct page_info *pages[];
> +};
> +
> /* Negotiated FF-A version to use with the SPMC */
> static uint32_t ffa_version __ro_after_init;
> 
> @@ -310,6 +354,8 @@ static unsigned int subscr_vm_destroyed_count __read_mostly;
>  *
>  * ffa_page_count is the number of pages used in each of these buffers.
>  *
> + * The TX buffer is protected from concurrent usage with ffa_tx_buffer_lock.
> + *
>  * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
>  * Note that the SPMC is also tracking the ownership of our RX buffer so
>  * for calls which uses our RX buffer to deliver a result we must call
> @@ -319,6 +365,7 @@ static void *ffa_rx __read_mostly;
> static void *ffa_tx __read_mostly;
> static unsigned int ffa_page_count __read_mostly;
> static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> +static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
> 
> static bool ffa_get_version(uint32_t *vers)
> {
> @@ -429,6 +476,42 @@ static int32_t ffa_rx_release(void)
>     return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
> }
> 
> +static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
> +                             register_t addr, uint32_t pg_count,
> +                             uint64_t *handle)
> +{
> +    struct arm_smccc_1_2_regs arg = {
> +        .a0 = FFA_MEM_SHARE_32,
> +        .a1 = tot_len,
> +        .a2 = frag_len,
> +        .a3 = addr,
> +        .a4 = pg_count,
> +    };
> +    struct arm_smccc_1_2_regs resp;
> +
> +    if ( IS_ENABLED(CONFIG_ARM_64) )
> +        arg.a0 = FFA_MEM_SHARE_64;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +
> +    switch ( resp.a0 )
> +    {
> +    case FFA_ERROR:
> +        if ( resp.a2 )
> +            return resp.a2;
> +        else
> +            return FFA_RET_NOT_SUPPORTED;
> +    case FFA_SUCCESS_32:
> +        *handle = regpair_to_uint64(resp.a3, resp.a2);
> +        return FFA_RET_OK;
> +    case FFA_MEM_FRAG_RX:
> +        *handle = regpair_to_uint64(resp.a2, resp.a1);
> +        return resp.a3;

You are using an int32_t type to cast something that is uint32_t from the spec
and here could in fact be a uint64_t.


> +    default:
> +        return FFA_RET_NOT_SUPPORTED;
> +    }
> +}
> +
> static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
>                                       uint8_t msg)
> {
> @@ -757,6 +840,404 @@ out:
>              resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> }
> 
> +/*
> + * Gets all page and assigns them to the supplied shared memory object. If
> + * this function fails then the caller is still expected to call
> + * put_shm_pages() as a cleanup.
> + */
> +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> +                         const struct ffa_address_range *range,
> +                         uint32_t range_count, unsigned int start_page_idx,
> +                         unsigned int *last_page_idx)
> +{
> +    unsigned int pg_idx = start_page_idx;
> +    gfn_t gfn;
> +    unsigned int n;
> +    unsigned int m;
> +    p2m_type_t t;
> +    uint64_t addr;
> +
> +    for ( n = 0; n < range_count; n++ )
> +    {
> +        for ( m = 0; m < range[n].page_count; m++ )
> +        {
> +            if ( pg_idx >= shm->page_count )
> +                return FFA_RET_INVALID_PARAMETERS;
> +
> +            addr = read_atomic(&range[n].address);
> +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
> +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
> +   P2M_ALLOC);
> +            if ( !shm->pages[pg_idx] )
> +                return FFA_RET_DENIED;
> +            pg_idx++;

This increment could be done at the end, why here ?

> +            /* Only normal RAM for now */
> +            if ( !p2m_is_ram(t) )
> +                return FFA_RET_DENIED;
> +        }
> +    }
> +
> +    *last_page_idx = pg_idx;
> +
> +    return FFA_RET_OK;
> +}
> +
> +static void put_shm_pages(struct ffa_shm_mem *shm)
> +{
> +    unsigned int n;
> +
> +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> +    {
> +        put_page(shm->pages[n]);
> +        shm->pages[n] = NULL;

If an error occured during the generation you might have part
of the pages which are NULL already.

So you should do a if (pages[n] != NULL) here

> +    }
> +}
> +
> +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> +                                             unsigned int page_count)
> +{
> +    struct ffa_shm_mem *shm;
> +
> +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
> +        return NULL;

Shouldn't you also filter out for page_count = 0 ?

> +
> +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> +    if ( shm )
> +    {
> +        ctx->shm_count++;
> +        shm->page_count = page_count;
> +    }
> +
> +    return shm;
> +}
> +
> +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> +{
> +    if ( shm ) {
> +        ASSERT(ctx->shm_count > 0);
> +        ctx->shm_count--;
> +        put_shm_pages(shm);
> +        xfree(shm);
> +    }
> +}
> +
> +static void init_range(struct ffa_address_range *addr_range,
> +                       paddr_t pa)
> +{
> +    memset(addr_range, 0, sizeof(*addr_range));
> +    addr_range->address = pa;
> +    addr_range->page_count = 1;
> +}
> +
> +/*
> + * This function uses the ffa_tx buffer to transmit the memory transaction
> + * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
> + * the buffer from concurrent use.
> + */
> +static int share_shm(struct ffa_shm_mem *shm)
> +{
> +    const uint32_t max_frag_len = ffa_page_count * FFA_PAGE_SIZE;
> +    struct ffa_mem_access *mem_access_array;
> +    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;
> +    void *buf = ffa_tx;
> +    uint32_t frag_len;
> +    uint32_t tot_len;
> +    paddr_t last_pa;
> +    unsigned int n;
> +    paddr_t pa;
> +
> +    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
> +    if ( !shm->page_count )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return FFA_RET_INVALID_PARAMETERS;

page_count = 0 should be filtered out before reaching this and this should
only be an assert if you want but no unreachable with a return.

> +    }
> +
> +    descr = buf;
> +    memset(descr, 0, sizeof(*descr));
> +    descr->sender_id = shm->sender_id;
> +    descr->global_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);
> +
> +    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);
> +
> +    region_descr = buf + mem_access_array[0].region_offs;
> +    memset(region_descr, 0, sizeof(*region_descr));
> +    region_descr->total_page_count = shm->page_count;
> +
> +    region_descr->address_range_count = 1;
> +    last_pa = page_to_maddr(shm->pages[0]);
> +    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> +    {
> +        pa = page_to_maddr(shm->pages[n]);
> +        if ( last_pa + FFA_PAGE_SIZE == pa )
> +            continue;
> +        region_descr->address_range_count++;
> +    }
> +
> +    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
> +                                region_descr->address_range_count);
> +    if ( tot_len > max_frag_len )
> +        return FFA_RET_NOT_SUPPORTED;
> +
> +    addr_range = region_descr->address_range_array;
> +    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
> +    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++ )
> +    {
> +        pa = page_to_maddr(shm->pages[n]);
> +        if ( last_pa + FFA_PAGE_SIZE == pa )
> +        {
> +            addr_range->page_count++;
> +            continue;
> +        }
> +
> +        frag_len += sizeof(*addr_range);
> +        addr_range++;
> +        init_range(addr_range, pa);
> +    }
> +
> +    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> +}
> +
> +static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
> +                                struct ffa_mem_transaction_x *trans)
> +{
> +    uint16_t mem_reg_attr;
> +    uint32_t flags;
> +    uint32_t count;
> +    uint32_t offs;
> +    uint32_t size;
> +
> +    if ( ffa_vers >= FFA_VERSION_1_1 )
> +    {
> +        const struct ffa_mem_transaction_1_1 *descr;
> +
> +        if ( blen < sizeof(*descr) )
> +            return FFA_RET_INVALID_PARAMETERS;
> +
> +        descr = buf;
> +        trans->sender_id = descr->sender_id;
> +        mem_reg_attr = descr->mem_reg_attr;
> +        flags = descr->flags;
> +        trans->global_handle = descr->global_handle;
> +        trans->tag = descr->tag;
> +
> +        count = descr->mem_access_count;
> +        size = descr->mem_access_size;
> +        offs = descr->mem_access_offs;
> +    }
> +    else
> +    {
> +        const struct ffa_mem_transaction_1_0 *descr;
> +
> +        if ( blen < sizeof(*descr) )
> +            return FFA_RET_INVALID_PARAMETERS;
> +
> +        descr = buf;
> +        trans->sender_id = descr->sender_id;
> +        mem_reg_attr = descr->mem_reg_attr;
> +        flags = descr->flags;
> +        trans->global_handle = descr->global_handle;
> +        trans->tag = descr->tag;
> +
> +        count = descr->mem_access_count;
> +        size = sizeof(struct ffa_mem_access);
> +        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
> +    }
> +    /*
> +     * Make sure that "descr" which is shared with the guest isn't accessed
> +     * again after this point.
> +     */
> +    barrier();

I am not really following the comment here. You accessed the content of descr
before and it is in the rxtx buffer so why is this needed ?

> +
> +    /*
> +     * We're doing a rough check to see that no information is lost when
> +     * tranfering the values into a struct ffa_mem_transaction_x below. The
> +     * fields in struct ffa_mem_transaction_x are wide enough to hold any
> +     * valid value so being out of range means that something is wrong.
> +     */
> +    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
> +        count > UINT8_MAX || offs > UINT16_MAX )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    /* Check that the endpoint memory access descriptor array fits */
> +    if ( size * count + offs > blen )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    trans->mem_reg_attr = mem_reg_attr;
> +    trans->flags = flags;
> +    trans->mem_access_size = size;
> +    trans->mem_access_count = count;
> +    trans->mem_access_offs = offs;
> +
> +    return 0;
> +}
> +
> +static void handle_mem_share(struct cpu_user_regs *regs)
> +{
> +    uint32_t tot_len = get_user_reg(regs, 1);
> +    uint32_t frag_len = get_user_reg(regs, 2);
> +    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;
> +    struct ffa_mem_transaction_x trans;
> +    struct domain *d = current->domain;
> +    struct ffa_ctx *ctx = d->arch.tee;
> +    struct ffa_shm_mem *shm = NULL;
> +    unsigned int last_page_idx = 0;
> +    register_t handle_hi = 0;
> +    register_t handle_lo = 0;
> +    int ret = FFA_RET_DENIED;
> +    uint32_t range_count;
> +    uint32_t region_offs;
> +
> +    /*
> +     * We're only accepting memory transaction descriptors via the rx/tx
> +     * buffer.

Is this a limitation coming fomr the spec or from the implementation ?

> +     */
> +    if ( addr )
> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_set_ret;
> +    }
> +
> +    /* Check that fragment length doesn't exceed total length */
> +    if ( frag_len > tot_len )
> +    {
> +        ret = FFA_RET_INVALID_PARAMETERS;
> +        goto out_set_ret;
> +    }
> +
> +    /* We currently only support a single fragment */

It would make sense to add some text at the beginning of the files listing
the current limitations of the implementation.

> +    if ( frag_len != tot_len )
> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_set_ret;
> +    }
> +
> +    spin_lock(&ctx->lock);
> +
> +    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
> +        goto out_unlock;
> +
> +    if ( !ffa_page_count )
> +    {
> +        ret = FFA_RET_NO_MEMORY;
> +        goto out_unlock;
> +    }
> +
> +    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
> +    if ( ret )
> +        goto out_unlock;
> +
> +    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_unlock;
> +    }
> +
> +    /* Only supports sharing it with one SP for now */

Also a limitation to list.

> +    if ( trans.mem_access_count != 1 )
> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_unlock;
> +    }
> +
> +    if ( trans.sender_id != get_vm_id(d) )
> +    {
> +        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;
> +

Why are you using atomic operations to access rxtx buffer after here ?

> +    mem_access = ctx->tx + trans.mem_access_offs;
> +    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )

Also a limitation to list.

> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_unlock;
> +    }
> +
> +    region_offs = read_atomic(&mem_access->region_offs);
> +    if ( sizeof(*region_descr) + region_offs > frag_len )
> +    {
> +        ret = FFA_RET_NOT_SUPPORTED;
> +        goto out_unlock;
> +    }
> +
> +    region_descr = ctx->tx + region_offs;
> +    range_count = read_atomic(&region_descr->address_range_count);
> +    page_count = read_atomic(&region_descr->total_page_count);
> +
> +    shm = alloc_ffa_shm_mem(ctx, page_count);
> +    if ( !shm )
> +    {
> +        ret = FFA_RET_NO_MEMORY;
> +        goto out_unlock;
> +    }
> +    shm->sender_id = trans.sender_id;
> +    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
> +
> +    /*
> +     * Check that the Composite memory region descriptor fits.
> +     */
> +    if ( sizeof(*region_descr) + region_offs +
> +         range_count * sizeof(struct ffa_address_range) > frag_len )
> +    {
> +        ret = FFA_RET_INVALID_PARAMETERS;
> +        goto out;
> +    }
> +
> +    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
> +                        0, &last_page_idx);
> +    if ( ret )
> +        goto out;
> +    if ( last_page_idx != shm->page_count )
> +    {
> +        ret = FFA_RET_INVALID_PARAMETERS;
> +        goto out;
> +    }
> +
> +    /* Note that share_shm() uses our tx buffer */
> +    spin_lock(&ffa_tx_buffer_lock);
> +    ret = share_shm(shm);
> +    spin_unlock(&ffa_tx_buffer_lock);
> +    if ( ret )
> +        goto out;
> +
> +    list_add_tail(&shm->list, &ctx->shm_list);
> +
> +    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> +
> +out:
> +    if ( ret )
> +        free_ffa_shm_mem(ctx, shm);
> +out_unlock:
> +    spin_unlock(&ctx->lock);
> +
> +out_set_ret:
> +    if ( ret == 0)
> +            set_regs_success(regs, handle_lo, handle_hi);
> +    else
> +            set_regs_error(regs, ret);
> +}
> +
> static bool ffa_handle_call(struct cpu_user_regs *regs)
> {
>     uint32_t fid = get_user_reg(regs, 0);
> @@ -818,6 +1299,12 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> #endif
>         handle_msg_send_direct_req(regs, fid);
>         return true;
> +    case FFA_MEM_SHARE_32:
> +#ifdef CONFIG_ARM_64
> +    case FFA_MEM_SHARE_64:
> +#endif
> +        handle_mem_share(regs);
> +        return true;
> 
>     default:
>         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -857,6 +1344,8 @@ static int ffa_domain_init(struct domain *d)
>         }
>     }
> 
> +    INIT_LIST_HEAD(&ctx->shm_list);
> +
>     d->arch.tee = ctx;
> 
>     return 0;
> @@ -1012,11 +1501,13 @@ static bool ffa_probe(void)
>          !check_mandatory_feature(FFA_RX_RELEASE) ||
> #ifdef CONFIG_ARM_64
>          !check_mandatory_feature(FFA_RXTX_MAP_64) ||
> +         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
> #endif
> #ifdef CONFIG_ARM_32
>          !check_mandatory_feature(FFA_RXTX_MAP_32) ||
> #endif
>          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
> +         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
>          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>         return false;
> 
> -- 
> 2.34.1


Cheers
Bertrand

Jens Wiklander March 14, 2023, 5:56 p.m. UTC | #2

Hi Bertrand,

On Mon, Mar 13, 2023 at 9:49 AM Bertrand Marquis
<[e-mail]> wrote:
>
> Hi Jens,
>
> > On 22 Feb 2023, at 16:33, Jens Wiklander <[e-mail]> wrote:
> >
> > Adds support for a guest to share memory with an SP using FFA_MEM_SHARE
> > and FFA_MEM_RECLAIM. Only small memory regions can be shared using a
> > single call to FFA_MEM_SHARE are supported.
>
> This sentence needs a bit of rephrasing and to add more details: what is "small".

OK, how about "Only memory regions small enough to be shared with a
single call..."

>
> >
> > A memory region that doesn't need to be shared any longer can be
> > reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
> > This is checked by the SPMC and not in control of the mediator.
>
> This explanation would make more sense in the following patch adding support
>  for Reclaim.

Quite right, I'll move it to the next patch.

>
> >
> > With this commit we have a FF-A version 1.1 [1] mediator able to
> > communicate with a Secure Partition in secure world using shared memory.
> > The secure world must use FF-A version 1.1, but the guest is free to use
> > version 1.0 or version 1.1.
>
> I do not see anything limiting that in the code.
> During init we accept 1.0 or 1.1 versions of the secure world.

Good catch, I'll update to only accept version 1.1 in the secure world.

>
> >
> > Adds a check that the SP supports the needed FF-A features
> > FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander <[e-mail]>
> > ---
> > xen/arch/arm/tee/ffa.c | 491 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 491 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 94c90b252454..cdc286caf62c 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -270,6 +270,38 @@ struct ffa_mem_transaction_1_1 {
> >     uint8_t reserved[12];
> > };
> >
> > +/* Calculate offset of struct ffa_mem_access from start of buffer */
> > +#define MEM_ACCESS_OFFSET(access_idx) \
> > +    ( sizeof(struct ffa_mem_transaction_1_1) + \
> > +      ( access_idx ) * sizeof(struct ffa_mem_access) )
> > +
> > +/* Calculate offset of struct ffa_mem_region from start of buffer */
> > +#define REGION_OFFSET(access_count, region_idx) \
> > +    ( MEM_ACCESS_OFFSET(access_count) + \
> > +      ( region_idx ) * sizeof(struct ffa_mem_region) )
> > +
> > +/* Calculate offset of struct ffa_address_range from start of buffer */
> > +#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
> > +    ( REGION_OFFSET(access_count, region_count) + \
> > +      ( range_idx ) * sizeof(struct ffa_address_range) )
> > +
> > +/*
> > + * 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
> > + * interface and can be changed without changing any ABI.
> > + */
> > +struct ffa_mem_transaction_x {
>
> I would suggest to s/_x/_int/ in the name here

OK, I'll update

>
> > +    uint16_t sender_id;
> > +    uint8_t mem_reg_attr;
> > +    uint8_t flags;
> > +    uint8_t mem_access_size;
> > +    uint8_t mem_access_count;
> > +    uint16_t mem_access_offs;
> > +    uint64_t global_handle;
> > +    uint64_t tag;
> > +};
> > +
> > /* Endpoint RX/TX descriptor */
> > struct ffa_endpoint_rxtx_descriptor_1_0 {
> >     uint16_t sender_id;
> > @@ -294,8 +326,20 @@ struct ffa_ctx {
> >     uint32_t guest_vers;
> >     bool tx_is_mine;
> >     bool interrupted;
> > +    struct list_head shm_list;
> > +    unsigned int shm_count;
> >     spinlock_t lock;
> > };
> > +
> > +struct ffa_shm_mem {
> > +    struct list_head list;
> > +    uint16_t sender_id;
> > +    uint16_t ep_id;     /* endpoint, the one lending */
> > +    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
> > +    unsigned int page_count;
> > +    struct page_info *pages[];
> > +};
> > +
> > /* Negotiated FF-A version to use with the SPMC */
> > static uint32_t ffa_version __ro_after_init;
> >
> > @@ -310,6 +354,8 @@ static unsigned int subscr_vm_destroyed_count __read_mostly;
> >  *
> >  * ffa_page_count is the number of pages used in each of these buffers.
> >  *
> > + * The TX buffer is protected from concurrent usage with ffa_tx_buffer_lock.
> > + *
> >  * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
> >  * Note that the SPMC is also tracking the ownership of our RX buffer so
> >  * for calls which uses our RX buffer to deliver a result we must call
> > @@ -319,6 +365,7 @@ static void *ffa_rx __read_mostly;
> > static void *ffa_tx __read_mostly;
> > static unsigned int ffa_page_count __read_mostly;
> > static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> > +static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
> >
> > static bool ffa_get_version(uint32_t *vers)
> > {
> > @@ -429,6 +476,42 @@ static int32_t ffa_rx_release(void)
> >     return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
> > }
> >
> > +static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
> > +                             register_t addr, uint32_t pg_count,
> > +                             uint64_t *handle)
> > +{
> > +    struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_MEM_SHARE_32,
> > +        .a1 = tot_len,
> > +        .a2 = frag_len,
> > +        .a3 = addr,
> > +        .a4 = pg_count,
> > +    };
> > +    struct arm_smccc_1_2_regs resp;
> > +
> > +    if ( IS_ENABLED(CONFIG_ARM_64) )
> > +        arg.a0 = FFA_MEM_SHARE_64;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +
> > +    switch ( resp.a0 )
> > +    {
> > +    case FFA_ERROR:
> > +        if ( resp.a2 )
> > +            return resp.a2;
> > +        else
> > +            return FFA_RET_NOT_SUPPORTED;
> > +    case FFA_SUCCESS_32:
> > +        *handle = regpair_to_uint64(resp.a3, resp.a2);
> > +        return FFA_RET_OK;
> > +    case FFA_MEM_FRAG_RX:
> > +        *handle = regpair_to_uint64(resp.a2, resp.a1);
> > +        return resp.a3;
>
> You are using an int32_t type to cast something that is uint32_t from the spec
> and here could in fact be a uint64_t.

In practice only much smaller values can be valid, however, I
understand that that's not your point. What's the best recovery if the
SPMC gives an invalid value for FFA_MEM_FRAG_RX? The SPMC has
allocated a memory-sharing state when it returns FFA_MEM_FRAG_RX. The
specification doesn't say how to remove that state apart from either
completing it successfully or if it's terminated earlier by the SPMC.
One option is to do a FFA_MEM_FRAG_TX call with invalid arguments so
that the SPMC can free up the memory-sharing state. Thoughts?

>
>
> > +    default:
> > +        return FFA_RET_NOT_SUPPORTED;
> > +    }
> > +}
> > +
> > static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> >                                       uint8_t msg)
> > {
> > @@ -757,6 +840,404 @@ out:
> >              resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> > }
> >
> > +/*
> > + * Gets all page and assigns them to the supplied shared memory object. If
> > + * this function fails then the caller is still expected to call
> > + * put_shm_pages() as a cleanup.
> > + */
> > +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> > +                         const struct ffa_address_range *range,
> > +                         uint32_t range_count, unsigned int start_page_idx,
> > +                         unsigned int *last_page_idx)
> > +{
> > +    unsigned int pg_idx = start_page_idx;
> > +    gfn_t gfn;
> > +    unsigned int n;
> > +    unsigned int m;
> > +    p2m_type_t t;
> > +    uint64_t addr;
> > +
> > +    for ( n = 0; n < range_count; n++ )
> > +    {
> > +        for ( m = 0; m < range[n].page_count; m++ )
> > +        {
> > +            if ( pg_idx >= shm->page_count )
> > +                return FFA_RET_INVALID_PARAMETERS;
> > +
> > +            addr = read_atomic(&range[n].address);
> > +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
> > +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
> > +   P2M_ALLOC);
> > +            if ( !shm->pages[pg_idx] )
> > +                return FFA_RET_DENIED;
> > +            pg_idx++;
>
> This increment could be done at the end, why here ?

Do you mean after the p2m_is_ram() check? I'll move it there.

>
> > +            /* Only normal RAM for now */
> > +            if ( !p2m_is_ram(t) )
> > +                return FFA_RET_DENIED;
> > +        }
> > +    }
> > +
> > +    *last_page_idx = pg_idx;
> > +
> > +    return FFA_RET_OK;
> > +}
> > +
> > +static void put_shm_pages(struct ffa_shm_mem *shm)
> > +{
> > +    unsigned int n;
> > +
> > +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> > +    {
> > +        put_page(shm->pages[n]);
> > +        shm->pages[n] = NULL;
>
> If an error occured during the generation you might have part
> of the pages which are NULL already.
>
> So you should do a if (pages[n] != NULL) here

I'm doing that above in the head of the loop, the loop is terminated
at the first pages[n] == NULL.

>
> > +    }
> > +}
> > +
> > +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> > +                                             unsigned int page_count)
> > +{
> > +    struct ffa_shm_mem *shm;
> > +
> > +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> > +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
> > +        return NULL;
>
> Shouldn't you also filter out for page_count = 0 ?

Sure, 0 doesn't make sense. But I should probably do it before this
function is called since I suppose we'd like to return something
different from FFA_RET_NO_MEMORY.

>
> > +
> > +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> > +    if ( shm )
> > +    {
> > +        ctx->shm_count++;
> > +        shm->page_count = page_count;
> > +    }
> > +
> > +    return shm;
> > +}
> > +
> > +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> > +{
> > +    if ( shm ) {
> > +        ASSERT(ctx->shm_count > 0);
> > +        ctx->shm_count--;
> > +        put_shm_pages(shm);
> > +        xfree(shm);
> > +    }
> > +}
> > +
> > +static void init_range(struct ffa_address_range *addr_range,
> > +                       paddr_t pa)
> > +{
> > +    memset(addr_range, 0, sizeof(*addr_range));
> > +    addr_range->address = pa;
> > +    addr_range->page_count = 1;
> > +}
> > +
> > +/*
> > + * This function uses the ffa_tx buffer to transmit the memory transaction
> > + * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
> > + * the buffer from concurrent use.
> > + */
> > +static int share_shm(struct ffa_shm_mem *shm)
> > +{
> > +    const uint32_t max_frag_len = ffa_page_count * FFA_PAGE_SIZE;
> > +    struct ffa_mem_access *mem_access_array;
> > +    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;
> > +    void *buf = ffa_tx;
> > +    uint32_t frag_len;
> > +    uint32_t tot_len;
> > +    paddr_t last_pa;
> > +    unsigned int n;
> > +    paddr_t pa;
> > +
> > +    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
> > +    if ( !shm->page_count )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return FFA_RET_INVALID_PARAMETERS;
>
> page_count = 0 should be filtered out before reaching this and this should
> only be an assert if you want but no unreachable with a return.

I'm adding code to filter out page_count = 0. I'm not sure what you
expect here, should I remove the entire check here or what do you
want?

>
> > +    }
> > +
> > +    descr = buf;
> > +    memset(descr, 0, sizeof(*descr));
> > +    descr->sender_id = shm->sender_id;
> > +    descr->global_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);
> > +
> > +    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);
> > +
> > +    region_descr = buf + mem_access_array[0].region_offs;
> > +    memset(region_descr, 0, sizeof(*region_descr));
> > +    region_descr->total_page_count = shm->page_count;
> > +
> > +    region_descr->address_range_count = 1;
> > +    last_pa = page_to_maddr(shm->pages[0]);
> > +    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> > +    {
> > +        pa = page_to_maddr(shm->pages[n]);
> > +        if ( last_pa + FFA_PAGE_SIZE == pa )
> > +            continue;
> > +        region_descr->address_range_count++;
> > +    }
> > +
> > +    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
> > +                                region_descr->address_range_count);
> > +    if ( tot_len > max_frag_len )
> > +        return FFA_RET_NOT_SUPPORTED;
> > +
> > +    addr_range = region_descr->address_range_array;
> > +    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
> > +    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++ )
> > +    {
> > +        pa = page_to_maddr(shm->pages[n]);
> > +        if ( last_pa + FFA_PAGE_SIZE == pa )
> > +        {
> > +            addr_range->page_count++;
> > +            continue;
> > +        }
> > +
> > +        frag_len += sizeof(*addr_range);
> > +        addr_range++;
> > +        init_range(addr_range, pa);
> > +    }
> > +
> > +    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> > +}
> > +
> > +static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
> > +                                struct ffa_mem_transaction_x *trans)
> > +{
> > +    uint16_t mem_reg_attr;
> > +    uint32_t flags;
> > +    uint32_t count;
> > +    uint32_t offs;
> > +    uint32_t size;
> > +
> > +    if ( ffa_vers >= FFA_VERSION_1_1 )
> > +    {
> > +        const struct ffa_mem_transaction_1_1 *descr;
> > +
> > +        if ( blen < sizeof(*descr) )
> > +            return FFA_RET_INVALID_PARAMETERS;
> > +
> > +        descr = buf;
> > +        trans->sender_id = descr->sender_id;
> > +        mem_reg_attr = descr->mem_reg_attr;
> > +        flags = descr->flags;
> > +        trans->global_handle = descr->global_handle;
> > +        trans->tag = descr->tag;
> > +
> > +        count = descr->mem_access_count;
> > +        size = descr->mem_access_size;
> > +        offs = descr->mem_access_offs;
> > +    }
> > +    else
> > +    {
> > +        const struct ffa_mem_transaction_1_0 *descr;
> > +
> > +        if ( blen < sizeof(*descr) )
> > +            return FFA_RET_INVALID_PARAMETERS;
> > +
> > +        descr = buf;
> > +        trans->sender_id = descr->sender_id;
> > +        mem_reg_attr = descr->mem_reg_attr;
> > +        flags = descr->flags;
> > +        trans->global_handle = descr->global_handle;
> > +        trans->tag = descr->tag;
> > +
> > +        count = descr->mem_access_count;
> > +        size = sizeof(struct ffa_mem_access);
> > +        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
> > +    }
> > +    /*
> > +     * Make sure that "descr" which is shared with the guest isn't accessed
> > +     * again after this point.
> > +     */
> > +    barrier();
>
> I am not really following the comment here. You accessed the content of descr
> before and it is in the rxtx buffer so why is this needed ?

I'm making sure that the compiler doesn't optimize and reorders the
reads from memory in funny ways, for instance, reading again after the
ifs just below. The RXTX buffer is shared with the guest so it can
potentially be updated concurrently by another CPU.

>
> > +
> > +    /*
> > +     * We're doing a rough check to see that no information is lost when
> > +     * tranfering the values into a struct ffa_mem_transaction_x below. The
> > +     * fields in struct ffa_mem_transaction_x are wide enough to hold any
> > +     * valid value so being out of range means that something is wrong.
> > +     */
> > +    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
> > +        count > UINT8_MAX || offs > UINT16_MAX )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    /* Check that the endpoint memory access descriptor array fits */
> > +    if ( size * count + offs > blen )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    trans->mem_reg_attr = mem_reg_attr;
> > +    trans->flags = flags;
> > +    trans->mem_access_size = size;
> > +    trans->mem_access_count = count;
> > +    trans->mem_access_offs = offs;
> > +
> > +    return 0;
> > +}
> > +
> > +static void handle_mem_share(struct cpu_user_regs *regs)
> > +{
> > +    uint32_t tot_len = get_user_reg(regs, 1);
> > +    uint32_t frag_len = get_user_reg(regs, 2);
> > +    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;
> > +    struct ffa_mem_transaction_x trans;
> > +    struct domain *d = current->domain;
> > +    struct ffa_ctx *ctx = d->arch.tee;
> > +    struct ffa_shm_mem *shm = NULL;
> > +    unsigned int last_page_idx = 0;
> > +    register_t handle_hi = 0;
> > +    register_t handle_lo = 0;
> > +    int ret = FFA_RET_DENIED;
> > +    uint32_t range_count;
> > +    uint32_t region_offs;
> > +
> > +    /*
> > +     * We're only accepting memory transaction descriptors via the rx/tx
> > +     * buffer.
>
> Is this a limitation coming fomr the spec or from the implementation ?

This is just a limitation in the implementation.

>
> > +     */
> > +    if ( addr )
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_set_ret;
> > +    }
> > +
> > +    /* Check that fragment length doesn't exceed total length */
> > +    if ( frag_len > tot_len )
> > +    {
> > +        ret = FFA_RET_INVALID_PARAMETERS;
> > +        goto out_set_ret;
> > +    }
> > +
> > +    /* We currently only support a single fragment */
>
> It would make sense to add some text at the beginning of the files listing
> the current limitations of the implementation.

That's quite a bit to keep track of, especially since it will change
with each patch. If it's important perhaps we can summarize that in a
final commit instead. By the way, this particular limitation is
removed in a later patch.

>
> > +    if ( frag_len != tot_len )
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_set_ret;
> > +    }
> > +
> > +    spin_lock(&ctx->lock);
> > +
> > +    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
> > +        goto out_unlock;
> > +
> > +    if ( !ffa_page_count )
> > +    {
> > +        ret = FFA_RET_NO_MEMORY;
> > +        goto out_unlock;
> > +    }
> > +
> > +    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
> > +    if ( ret )
> > +        goto out_unlock;
> > +
> > +    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_unlock;
> > +    }
> > +
> > +    /* Only supports sharing it with one SP for now */
>
> Also a limitation to list.

OK

>
> > +    if ( trans.mem_access_count != 1 )
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_unlock;
> > +    }
> > +
> > +    if ( trans.sender_id != get_vm_id(d) )
> > +    {
> > +        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;
> > +
>
> Why are you using atomic operations to access rxtx buffer after here ?

To limit how the compiler can reorder the reads from memory.

>
> > +    mem_access = ctx->tx + trans.mem_access_offs;
> > +    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
>
> Also a limitation to list.

OK

Thanks,
Jens

>
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_unlock;
> > +    }
> > +
> > +    region_offs = read_atomic(&mem_access->region_offs);
> > +    if ( sizeof(*region_descr) + region_offs > frag_len )
> > +    {
> > +        ret = FFA_RET_NOT_SUPPORTED;
> > +        goto out_unlock;
> > +    }
> > +
> > +    region_descr = ctx->tx + region_offs;
> > +    range_count = read_atomic(&region_descr->address_range_count);
> > +    page_count = read_atomic(&region_descr->total_page_count);
> > +
> > +    shm = alloc_ffa_shm_mem(ctx, page_count);
> > +    if ( !shm )
> > +    {
> > +        ret = FFA_RET_NO_MEMORY;
> > +        goto out_unlock;
> > +    }
> > +    shm->sender_id = trans.sender_id;
> > +    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
> > +
> > +    /*
> > +     * Check that the Composite memory region descriptor fits.
> > +     */
> > +    if ( sizeof(*region_descr) + region_offs +
> > +         range_count * sizeof(struct ffa_address_range) > frag_len )
> > +    {
> > +        ret = FFA_RET_INVALID_PARAMETERS;
> > +        goto out;
> > +    }
> > +
> > +    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
> > +                        0, &last_page_idx);
> > +    if ( ret )
> > +        goto out;
> > +    if ( last_page_idx != shm->page_count )
> > +    {
> > +        ret = FFA_RET_INVALID_PARAMETERS;
> > +        goto out;
> > +    }
> > +
> > +    /* Note that share_shm() uses our tx buffer */
> > +    spin_lock(&ffa_tx_buffer_lock);
> > +    ret = share_shm(shm);
> > +    spin_unlock(&ffa_tx_buffer_lock);
> > +    if ( ret )
> > +        goto out;
> > +
> > +    list_add_tail(&shm->list, &ctx->shm_list);
> > +
> > +    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> > +
> > +out:
> > +    if ( ret )
> > +        free_ffa_shm_mem(ctx, shm);
> > +out_unlock:
> > +    spin_unlock(&ctx->lock);
> > +
> > +out_set_ret:
> > +    if ( ret == 0)
> > +            set_regs_success(regs, handle_lo, handle_hi);
> > +    else
> > +            set_regs_error(regs, ret);
> > +}
> > +
> > static bool ffa_handle_call(struct cpu_user_regs *regs)
> > {
> >     uint32_t fid = get_user_reg(regs, 0);
> > @@ -818,6 +1299,12 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > #endif
> >         handle_msg_send_direct_req(regs, fid);
> >         return true;
> > +    case FFA_MEM_SHARE_32:
> > +#ifdef CONFIG_ARM_64
> > +    case FFA_MEM_SHARE_64:
> > +#endif
> > +        handle_mem_share(regs);
> > +        return true;
> >
> >     default:
> >         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -857,6 +1344,8 @@ static int ffa_domain_init(struct domain *d)
> >         }
> >     }
> >
> > +    INIT_LIST_HEAD(&ctx->shm_list);
> > +
> >     d->arch.tee = ctx;
> >
> >     return 0;
> > @@ -1012,11 +1501,13 @@ static bool ffa_probe(void)
> >          !check_mandatory_feature(FFA_RX_RELEASE) ||
> > #ifdef CONFIG_ARM_64
> >          !check_mandatory_feature(FFA_RXTX_MAP_64) ||
> > +         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
> > #endif
> > #ifdef CONFIG_ARM_32
> >          !check_mandatory_feature(FFA_RXTX_MAP_32) ||
> > #endif
> >          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
> > +         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
> >          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> >         return false;
> >
> > --
> > 2.34.1
>
>
> Cheers
> Bertrand
>

Bertrand Marquis March 15, 2023, 1:24 p.m. UTC | #3

Hi Jens,

> On 14 Mar 2023, at 18:56, Jens Wiklander <[e-mail]> wrote:
> 
> Hi Bertrand,
> 
> On Mon, Mar 13, 2023 at 9:49 AM Bertrand Marquis
> <[e-mail]> wrote:
>> 
>> Hi Jens,
>> 
>>> On 22 Feb 2023, at 16:33, Jens Wiklander <[e-mail]> wrote:
>>> 
>>> Adds support for a guest to share memory with an SP using FFA_MEM_SHARE
>>> and FFA_MEM_RECLAIM. Only small memory regions can be shared using a
>>> single call to FFA_MEM_SHARE are supported.
>> 
>> This sentence needs a bit of rephrasing and to add more details: what is "small".
> 
> OK, how about "Only memory regions small enough to be shared with a
> single call..."

Ok

> 
>> 
>>> 
>>> A memory region that doesn't need to be shared any longer can be
>>> reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
>>> This is checked by the SPMC and not in control of the mediator.
>> 
>> This explanation would make more sense in the following patch adding support
>> for Reclaim.
> 
> Quite right, I'll move it to the next patch.
> 
>> 
>>> 
>>> With this commit we have a FF-A version 1.1 [1] mediator able to
>>> communicate with a Secure Partition in secure world using shared memory.
>>> The secure world must use FF-A version 1.1, but the guest is free to use
>>> version 1.0 or version 1.1.
>> 
>> I do not see anything limiting that in the code.
>> During init we accept 1.0 or 1.1 versions of the secure world.
> 
> Good catch, I'll update to only accept version 1.1 in the secure world.
> 
>> 
>>> 
>>> Adds a check that the SP supports the needed FF-A features
>>> FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.
>>> 
>>> [1] https://developer.arm.com/documentation/den0077/latest
>>> Signed-off-by: Jens Wiklander <[e-mail]>
>>> ---
>>> xen/arch/arm/tee/ffa.c | 491 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 491 insertions(+)
>>> 
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index 94c90b252454..cdc286caf62c 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -270,6 +270,38 @@ struct ffa_mem_transaction_1_1 {
>>>    uint8_t reserved[12];
>>> };
>>> 
>>> +/* Calculate offset of struct ffa_mem_access from start of buffer */
>>> +#define MEM_ACCESS_OFFSET(access_idx) \
>>> +    ( sizeof(struct ffa_mem_transaction_1_1) + \
>>> +      ( access_idx ) * sizeof(struct ffa_mem_access) )
>>> +
>>> +/* Calculate offset of struct ffa_mem_region from start of buffer */
>>> +#define REGION_OFFSET(access_count, region_idx) \
>>> +    ( MEM_ACCESS_OFFSET(access_count) + \
>>> +      ( region_idx ) * sizeof(struct ffa_mem_region) )
>>> +
>>> +/* Calculate offset of struct ffa_address_range from start of buffer */
>>> +#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
>>> +    ( REGION_OFFSET(access_count, region_count) + \
>>> +      ( range_idx ) * sizeof(struct ffa_address_range) )
>>> +
>>> +/*
>>> + * 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
>>> + * interface and can be changed without changing any ABI.
>>> + */
>>> +struct ffa_mem_transaction_x {
>> 
>> I would suggest to s/_x/_int/ in the name here
> 
> OK, I'll update
> 
>> 
>>> +    uint16_t sender_id;
>>> +    uint8_t mem_reg_attr;
>>> +    uint8_t flags;
>>> +    uint8_t mem_access_size;
>>> +    uint8_t mem_access_count;
>>> +    uint16_t mem_access_offs;
>>> +    uint64_t global_handle;
>>> +    uint64_t tag;
>>> +};
>>> +
>>> /* Endpoint RX/TX descriptor */
>>> struct ffa_endpoint_rxtx_descriptor_1_0 {
>>>    uint16_t sender_id;
>>> @@ -294,8 +326,20 @@ struct ffa_ctx {
>>>    uint32_t guest_vers;
>>>    bool tx_is_mine;
>>>    bool interrupted;
>>> +    struct list_head shm_list;
>>> +    unsigned int shm_count;
>>>    spinlock_t lock;
>>> };
>>> +
>>> +struct ffa_shm_mem {
>>> +    struct list_head list;
>>> +    uint16_t sender_id;
>>> +    uint16_t ep_id;     /* endpoint, the one lending */
>>> +    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
>>> +    unsigned int page_count;
>>> +    struct page_info *pages[];
>>> +};
>>> +
>>> /* Negotiated FF-A version to use with the SPMC */
>>> static uint32_t ffa_version __ro_after_init;
>>> 
>>> @@ -310,6 +354,8 @@ static unsigned int subscr_vm_destroyed_count __read_mostly;
>>> *
>>> * ffa_page_count is the number of pages used in each of these buffers.
>>> *
>>> + * The TX buffer is protected from concurrent usage with ffa_tx_buffer_lock.
>>> + *
>>> * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
>>> * Note that the SPMC is also tracking the ownership of our RX buffer so
>>> * for calls which uses our RX buffer to deliver a result we must call
>>> @@ -319,6 +365,7 @@ static void *ffa_rx __read_mostly;
>>> static void *ffa_tx __read_mostly;
>>> static unsigned int ffa_page_count __read_mostly;
>>> static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
>>> +static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
>>> 
>>> static bool ffa_get_version(uint32_t *vers)
>>> {
>>> @@ -429,6 +476,42 @@ static int32_t ffa_rx_release(void)
>>>    return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
>>> }
>>> 
>>> +static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
>>> +                             register_t addr, uint32_t pg_count,
>>> +                             uint64_t *handle)
>>> +{
>>> +    struct arm_smccc_1_2_regs arg = {
>>> +        .a0 = FFA_MEM_SHARE_32,
>>> +        .a1 = tot_len,
>>> +        .a2 = frag_len,
>>> +        .a3 = addr,
>>> +        .a4 = pg_count,
>>> +    };
>>> +    struct arm_smccc_1_2_regs resp;
>>> +
>>> +    if ( IS_ENABLED(CONFIG_ARM_64) )
>>> +        arg.a0 = FFA_MEM_SHARE_64;
>>> +
>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>> +
>>> +    switch ( resp.a0 )
>>> +    {
>>> +    case FFA_ERROR:
>>> +        if ( resp.a2 )
>>> +            return resp.a2;
>>> +        else
>>> +            return FFA_RET_NOT_SUPPORTED;
>>> +    case FFA_SUCCESS_32:
>>> +        *handle = regpair_to_uint64(resp.a3, resp.a2);
>>> +        return FFA_RET_OK;
>>> +    case FFA_MEM_FRAG_RX:
>>> +        *handle = regpair_to_uint64(resp.a2, resp.a1);
>>> +        return resp.a3;
>> 
>> You are using an int32_t type to cast something that is uint32_t from the spec
>> and here could in fact be a uint64_t.
> 
> In practice only much smaller values can be valid, however, I
> understand that that's not your point. What's the best recovery if the
> SPMC gives an invalid value for FFA_MEM_FRAG_RX? The SPMC has
> allocated a memory-sharing state when it returns FFA_MEM_FRAG_RX. The
> specification doesn't say how to remove that state apart from either
> completing it successfully or if it's terminated earlier by the SPMC.
> One option is to do a FFA_MEM_FRAG_TX call with invalid arguments so
> that the SPMC can free up the memory-sharing state. Thoughts?

I do not think it is Xen responsability to fix SPMC problems here.
If we detect something of an error we should just return back an error to the client.

If the SPMC is returning an invalid value, we cannot really make much assumptions.

An other solution here would just be to mask out to prevent the implicit cast.

> 
>> 
>> 
>>> +    default:
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +    }
>>> +}
>>> +
>>> static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
>>>                                      uint8_t msg)
>>> {
>>> @@ -757,6 +840,404 @@ out:
>>>             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
>>> }
>>> 
>>> +/*
>>> + * Gets all page and assigns them to the supplied shared memory object. If
>>> + * this function fails then the caller is still expected to call
>>> + * put_shm_pages() as a cleanup.
>>> + */
>>> +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
>>> +                         const struct ffa_address_range *range,
>>> +                         uint32_t range_count, unsigned int start_page_idx,
>>> +                         unsigned int *last_page_idx)
>>> +{
>>> +    unsigned int pg_idx = start_page_idx;
>>> +    gfn_t gfn;
>>> +    unsigned int n;
>>> +    unsigned int m;
>>> +    p2m_type_t t;
>>> +    uint64_t addr;
>>> +
>>> +    for ( n = 0; n < range_count; n++ )
>>> +    {
>>> +        for ( m = 0; m < range[n].page_count; m++ )
>>> +        {
>>> +            if ( pg_idx >= shm->page_count )
>>> +                return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +            addr = read_atomic(&range[n].address);
>>> +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
>>> +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
>>> +   P2M_ALLOC);
>>> +            if ( !shm->pages[pg_idx] )
>>> +                return FFA_RET_DENIED;
>>> +            pg_idx++;
>> 
>> This increment could be done at the end, why here ?
> 
> Do you mean after the p2m_is_ram() check? I'll move it there.

yes that would be more natural i think.

> 
>> 
>>> +            /* Only normal RAM for now */
>>> +            if ( !p2m_is_ram(t) )
>>> +                return FFA_RET_DENIED;
>>> +        }
>>> +    }
>>> +
>>> +    *last_page_idx = pg_idx;
>>> +
>>> +    return FFA_RET_OK;
>>> +}
>>> +
>>> +static void put_shm_pages(struct ffa_shm_mem *shm)
>>> +{
>>> +    unsigned int n;
>>> +
>>> +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
>>> +    {
>>> +        put_page(shm->pages[n]);
>>> +        shm->pages[n] = NULL;
>> 
>> If an error occured during the generation you might have part
>> of the pages which are NULL already.
>> 
>> So you should do a if (pages[n] != NULL) here
> 
> I'm doing that above in the head of the loop, the loop is terminated
> at the first pages[n] == NULL.

Right, sorry i missed that.

> 
>> 
>>> +    }
>>> +}
>>> +
>>> +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
>>> +                                             unsigned int page_count)
>>> +{
>>> +    struct ffa_shm_mem *shm;
>>> +
>>> +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
>>> +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
>>> +        return NULL;
>> 
>> Shouldn't you also filter out for page_count = 0 ?
> 
> Sure, 0 doesn't make sense. But I should probably do it before this
> function is called since I suppose we'd like to return something
> different from FFA_RET_NO_MEMORY.

Very true.

> 
>> 
>>> +
>>> +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
>>> +    if ( shm )
>>> +    {
>>> +        ctx->shm_count++;
>>> +        shm->page_count = page_count;
>>> +    }
>>> +
>>> +    return shm;
>>> +}
>>> +
>>> +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
>>> +{
>>> +    if ( shm ) {
>>> +        ASSERT(ctx->shm_count > 0);
>>> +        ctx->shm_count--;
>>> +        put_shm_pages(shm);
>>> +        xfree(shm);
>>> +    }
>>> +}
>>> +
>>> +static void init_range(struct ffa_address_range *addr_range,
>>> +                       paddr_t pa)
>>> +{
>>> +    memset(addr_range, 0, sizeof(*addr_range));
>>> +    addr_range->address = pa;
>>> +    addr_range->page_count = 1;
>>> +}
>>> +
>>> +/*
>>> + * This function uses the ffa_tx buffer to transmit the memory transaction
>>> + * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
>>> + * the buffer from concurrent use.
>>> + */
>>> +static int share_shm(struct ffa_shm_mem *shm)
>>> +{
>>> +    const uint32_t max_frag_len = ffa_page_count * FFA_PAGE_SIZE;
>>> +    struct ffa_mem_access *mem_access_array;
>>> +    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;
>>> +    void *buf = ffa_tx;
>>> +    uint32_t frag_len;
>>> +    uint32_t tot_len;
>>> +    paddr_t last_pa;
>>> +    unsigned int n;
>>> +    paddr_t pa;
>>> +
>>> +    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
>>> +    if ( !shm->page_count )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return FFA_RET_INVALID_PARAMETERS;
>> 
>> page_count = 0 should be filtered out before reaching this and this should
>> only be an assert if you want but no unreachable with a return.
> 
> I'm adding code to filter out page_count = 0. I'm not sure what you
> expect here, should I remove the entire check here or what do you
> want?

As this is checked before, this could just be an assert and not something with a return.

> 
>> 
>>> +    }
>>> +
>>> +    descr = buf;
>>> +    memset(descr, 0, sizeof(*descr));
>>> +    descr->sender_id = shm->sender_id;
>>> +    descr->global_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);
>>> +
>>> +    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);
>>> +
>>> +    region_descr = buf + mem_access_array[0].region_offs;
>>> +    memset(region_descr, 0, sizeof(*region_descr));
>>> +    region_descr->total_page_count = shm->page_count;
>>> +
>>> +    region_descr->address_range_count = 1;
>>> +    last_pa = page_to_maddr(shm->pages[0]);
>>> +    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
>>> +    {
>>> +        pa = page_to_maddr(shm->pages[n]);
>>> +        if ( last_pa + FFA_PAGE_SIZE == pa )
>>> +            continue;
>>> +        region_descr->address_range_count++;
>>> +    }
>>> +
>>> +    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
>>> +                                region_descr->address_range_count);
>>> +    if ( tot_len > max_frag_len )
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +
>>> +    addr_range = region_descr->address_range_array;
>>> +    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
>>> +    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++ )
>>> +    {
>>> +        pa = page_to_maddr(shm->pages[n]);
>>> +        if ( last_pa + FFA_PAGE_SIZE == pa )
>>> +        {
>>> +            addr_range->page_count++;
>>> +            continue;
>>> +        }
>>> +
>>> +        frag_len += sizeof(*addr_range);
>>> +        addr_range++;
>>> +        init_range(addr_range, pa);
>>> +    }
>>> +
>>> +    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
>>> +}
>>> +
>>> +static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
>>> +                                struct ffa_mem_transaction_x *trans)
>>> +{
>>> +    uint16_t mem_reg_attr;
>>> +    uint32_t flags;
>>> +    uint32_t count;
>>> +    uint32_t offs;
>>> +    uint32_t size;
>>> +
>>> +    if ( ffa_vers >= FFA_VERSION_1_1 )
>>> +    {
>>> +        const struct ffa_mem_transaction_1_1 *descr;
>>> +
>>> +        if ( blen < sizeof(*descr) )
>>> +            return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +        descr = buf;
>>> +        trans->sender_id = descr->sender_id;
>>> +        mem_reg_attr = descr->mem_reg_attr;
>>> +        flags = descr->flags;
>>> +        trans->global_handle = descr->global_handle;
>>> +        trans->tag = descr->tag;
>>> +
>>> +        count = descr->mem_access_count;
>>> +        size = descr->mem_access_size;
>>> +        offs = descr->mem_access_offs;
>>> +    }
>>> +    else
>>> +    {
>>> +        const struct ffa_mem_transaction_1_0 *descr;
>>> +
>>> +        if ( blen < sizeof(*descr) )
>>> +            return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +        descr = buf;
>>> +        trans->sender_id = descr->sender_id;
>>> +        mem_reg_attr = descr->mem_reg_attr;
>>> +        flags = descr->flags;
>>> +        trans->global_handle = descr->global_handle;
>>> +        trans->tag = descr->tag;
>>> +
>>> +        count = descr->mem_access_count;
>>> +        size = sizeof(struct ffa_mem_access);
>>> +        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
>>> +    }
>>> +    /*
>>> +     * Make sure that "descr" which is shared with the guest isn't accessed
>>> +     * again after this point.
>>> +     */
>>> +    barrier();
>> 
>> I am not really following the comment here. You accessed the content of descr
>> before and it is in the rxtx buffer so why is this needed ?
> 
> I'm making sure that the compiler doesn't optimize and reorders the
> reads from memory in funny ways, for instance, reading again after the
> ifs just below. The RXTX buffer is shared with the guest so it can
> potentially be updated concurrently by another CPU.

The client is not suppose to modify the buffer during the call, using atomic
operations here is not really solving any issue if this is modified while we use it right ?

> 
>> 
>>> +
>>> +    /*
>>> +     * We're doing a rough check to see that no information is lost when
>>> +     * tranfering the values into a struct ffa_mem_transaction_x below. The
>>> +     * fields in struct ffa_mem_transaction_x are wide enough to hold any
>>> +     * valid value so being out of range means that something is wrong.
>>> +     */
>>> +    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
>>> +        count > UINT8_MAX || offs > UINT16_MAX )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    /* Check that the endpoint memory access descriptor array fits */
>>> +    if ( size * count + offs > blen )
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    trans->mem_reg_attr = mem_reg_attr;
>>> +    trans->flags = flags;
>>> +    trans->mem_access_size = size;
>>> +    trans->mem_access_count = count;
>>> +    trans->mem_access_offs = offs;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void handle_mem_share(struct cpu_user_regs *regs)
>>> +{
>>> +    uint32_t tot_len = get_user_reg(regs, 1);
>>> +    uint32_t frag_len = get_user_reg(regs, 2);
>>> +    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;
>>> +    struct ffa_mem_transaction_x trans;
>>> +    struct domain *d = current->domain;
>>> +    struct ffa_ctx *ctx = d->arch.tee;
>>> +    struct ffa_shm_mem *shm = NULL;
>>> +    unsigned int last_page_idx = 0;
>>> +    register_t handle_hi = 0;
>>> +    register_t handle_lo = 0;
>>> +    int ret = FFA_RET_DENIED;
>>> +    uint32_t range_count;
>>> +    uint32_t region_offs;
>>> +
>>> +    /*
>>> +     * We're only accepting memory transaction descriptors via the rx/tx
>>> +     * buffer.
>> 
>> Is this a limitation coming fomr the spec or from the implementation ?
> 
> This is just a limitation in the implementation.
> 
>> 
>>> +     */
>>> +    if ( addr )
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_set_ret;
>>> +    }
>>> +
>>> +    /* Check that fragment length doesn't exceed total length */
>>> +    if ( frag_len > tot_len )
>>> +    {
>>> +        ret = FFA_RET_INVALID_PARAMETERS;
>>> +        goto out_set_ret;
>>> +    }
>>> +
>>> +    /* We currently only support a single fragment */
>> 
>> It would make sense to add some text at the beginning of the files listing
>> the current limitations of the implementation.
> 
> That's quite a bit to keep track of, especially since it will change
> with each patch. If it's important perhaps we can summarize that in a
> final commit instead. By the way, this particular limitation is
> removed in a later patch.

I am more thinking at the end of the serie to have one place with the current
state and limitations of the implementation.

We cannot really expect someone to browse all comments to get what is
 supported or not.

> 
>> 
>>> +    if ( frag_len != tot_len )
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_set_ret;
>>> +    }
>>> +
>>> +    spin_lock(&ctx->lock);
>>> +
>>> +    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
>>> +        goto out_unlock;
>>> +
>>> +    if ( !ffa_page_count )
>>> +    {
>>> +        ret = FFA_RET_NO_MEMORY;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
>>> +    if ( ret )
>>> +        goto out_unlock;
>>> +
>>> +    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    /* Only supports sharing it with one SP for now */
>> 
>> Also a limitation to list.
> 
> OK
> 
>> 
>>> +    if ( trans.mem_access_count != 1 )
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    if ( trans.sender_id != get_vm_id(d) )
>>> +    {
>>> +        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;
>>> +
>> 
>> Why are you using atomic operations to access rxtx buffer after here ?
> 
> To limit how the compiler can reorder the reads from memory.

As stated before, don't we assume that the buffer is not modified by the client
 while we use it ?

> 
>> 
>>> +    mem_access = ctx->tx + trans.mem_access_offs;
>>> +    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
>> 
>> Also a limitation to list.
> 
> OK

Cheers
Bertrand

> 
> Thanks,
> Jens
> 
>> 
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    region_offs = read_atomic(&mem_access->region_offs);
>>> +    if ( sizeof(*region_descr) + region_offs > frag_len )
>>> +    {
>>> +        ret = FFA_RET_NOT_SUPPORTED;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    region_descr = ctx->tx + region_offs;
>>> +    range_count = read_atomic(&region_descr->address_range_count);
>>> +    page_count = read_atomic(&region_descr->total_page_count);
>>> +
>>> +    shm = alloc_ffa_shm_mem(ctx, page_count);
>>> +    if ( !shm )
>>> +    {
>>> +        ret = FFA_RET_NO_MEMORY;
>>> +        goto out_unlock;
>>> +    }
>>> +    shm->sender_id = trans.sender_id;
>>> +    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
>>> +
>>> +    /*
>>> +     * Check that the Composite memory region descriptor fits.
>>> +     */
>>> +    if ( sizeof(*region_descr) + region_offs +
>>> +         range_count * sizeof(struct ffa_address_range) > frag_len )
>>> +    {
>>> +        ret = FFA_RET_INVALID_PARAMETERS;
>>> +        goto out;
>>> +    }
>>> +
>>> +    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
>>> +                        0, &last_page_idx);
>>> +    if ( ret )
>>> +        goto out;
>>> +    if ( last_page_idx != shm->page_count )
>>> +    {
>>> +        ret = FFA_RET_INVALID_PARAMETERS;
>>> +        goto out;
>>> +    }
>>> +
>>> +    /* Note that share_shm() uses our tx buffer */
>>> +    spin_lock(&ffa_tx_buffer_lock);
>>> +    ret = share_shm(shm);
>>> +    spin_unlock(&ffa_tx_buffer_lock);
>>> +    if ( ret )
>>> +        goto out;
>>> +
>>> +    list_add_tail(&shm->list, &ctx->shm_list);
>>> +
>>> +    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
>>> +
>>> +out:
>>> +    if ( ret )
>>> +        free_ffa_shm_mem(ctx, shm);
>>> +out_unlock:
>>> +    spin_unlock(&ctx->lock);
>>> +
>>> +out_set_ret:
>>> +    if ( ret == 0)
>>> +            set_regs_success(regs, handle_lo, handle_hi);
>>> +    else
>>> +            set_regs_error(regs, ret);
>>> +}
>>> +
>>> static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> {
>>>    uint32_t fid = get_user_reg(regs, 0);
>>> @@ -818,6 +1299,12 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> #endif
>>>        handle_msg_send_direct_req(regs, fid);
>>>        return true;
>>> +    case FFA_MEM_SHARE_32:
>>> +#ifdef CONFIG_ARM_64
>>> +    case FFA_MEM_SHARE_64:
>>> +#endif
>>> +        handle_mem_share(regs);
>>> +        return true;
>>> 
>>>    default:
>>>        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -857,6 +1344,8 @@ static int ffa_domain_init(struct domain *d)
>>>        }
>>>    }
>>> 
>>> +    INIT_LIST_HEAD(&ctx->shm_list);
>>> +
>>>    d->arch.tee = ctx;
>>> 
>>>    return 0;
>>> @@ -1012,11 +1501,13 @@ static bool ffa_probe(void)
>>>         !check_mandatory_feature(FFA_RX_RELEASE) ||
>>> #ifdef CONFIG_ARM_64
>>>         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
>>> +         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
>>> #endif
>>> #ifdef CONFIG_ARM_32
>>>         !check_mandatory_feature(FFA_RXTX_MAP_32) ||
>>> #endif
>>>         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
>>> +         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
>>>         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>>>        return false;
>>> 
>>> --
>>> 2.34.1
>> 
>> 
>> Cheers
>> Bertrand

Jens Wiklander March 15, 2023, 2:33 p.m. UTC | #4

Hi Bertrand,

On Wed, Mar 15, 2023 at 2:24 PM Bertrand Marquis
<[e-mail]> wrote:
>
> Hi Jens,
>
> > On 14 Mar 2023, at 18:56, Jens Wiklander <[e-mail]> wrote:
> >
> > Hi Bertrand,
> >
> > On Mon, Mar 13, 2023 at 9:49 AM Bertrand Marquis
> > <[e-mail]> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 22 Feb 2023, at 16:33, Jens Wiklander <[e-mail]> wrote:
> >>>
> >>> Adds support for a guest to share memory with an SP using FFA_MEM_SHARE
> >>> and FFA_MEM_RECLAIM. Only small memory regions can be shared using a
> >>> single call to FFA_MEM_SHARE are supported.
> >>
> >> This sentence needs a bit of rephrasing and to add more details: what is "small".
> >
> > OK, how about "Only memory regions small enough to be shared with a
> > single call..."
>
> Ok
>
> >
> >>
> >>>
> >>> A memory region that doesn't need to be shared any longer can be
> >>> reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
> >>> This is checked by the SPMC and not in control of the mediator.
> >>
> >> This explanation would make more sense in the following patch adding support
> >> for Reclaim.
> >
> > Quite right, I'll move it to the next patch.
> >
> >>
> >>>
> >>> With this commit we have a FF-A version 1.1 [1] mediator able to
> >>> communicate with a Secure Partition in secure world using shared memory.
> >>> The secure world must use FF-A version 1.1, but the guest is free to use
> >>> version 1.0 or version 1.1.
> >>
> >> I do not see anything limiting that in the code.
> >> During init we accept 1.0 or 1.1 versions of the secure world.
> >
> > Good catch, I'll update to only accept version 1.1 in the secure world.
> >
> >>
> >>>
> >>> Adds a check that the SP supports the needed FF-A features
> >>> FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.
> >>>
> >>> [1] https://developer.arm.com/documentation/den0077/latest
> >>> Signed-off-by: Jens Wiklander <[e-mail]>
> >>> ---
> >>> xen/arch/arm/tee/ffa.c | 491 +++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 491 insertions(+)
> >>>
> >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >>> index 94c90b252454..cdc286caf62c 100644
> >>> --- a/xen/arch/arm/tee/ffa.c
> >>> +++ b/xen/arch/arm/tee/ffa.c
> >>> @@ -270,6 +270,38 @@ struct ffa_mem_transaction_1_1 {
> >>>    uint8_t reserved[12];
> >>> };
> >>>
> >>> +/* Calculate offset of struct ffa_mem_access from start of buffer */
> >>> +#define MEM_ACCESS_OFFSET(access_idx) \
> >>> +    ( sizeof(struct ffa_mem_transaction_1_1) + \
> >>> +      ( access_idx ) * sizeof(struct ffa_mem_access) )
> >>> +
> >>> +/* Calculate offset of struct ffa_mem_region from start of buffer */
> >>> +#define REGION_OFFSET(access_count, region_idx) \
> >>> +    ( MEM_ACCESS_OFFSET(access_count) + \
> >>> +      ( region_idx ) * sizeof(struct ffa_mem_region) )
> >>> +
> >>> +/* Calculate offset of struct ffa_address_range from start of buffer */
> >>> +#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
> >>> +    ( REGION_OFFSET(access_count, region_count) + \
> >>> +      ( range_idx ) * sizeof(struct ffa_address_range) )
> >>> +
> >>> +/*
> >>> + * 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
> >>> + * interface and can be changed without changing any ABI.
> >>> + */
> >>> +struct ffa_mem_transaction_x {
> >>
> >> I would suggest to s/_x/_int/ in the name here
> >
> > OK, I'll update
> >
> >>
> >>> +    uint16_t sender_id;
> >>> +    uint8_t mem_reg_attr;
> >>> +    uint8_t flags;
> >>> +    uint8_t mem_access_size;
> >>> +    uint8_t mem_access_count;
> >>> +    uint16_t mem_access_offs;
> >>> +    uint64_t global_handle;
> >>> +    uint64_t tag;
> >>> +};
> >>> +
> >>> /* Endpoint RX/TX descriptor */
> >>> struct ffa_endpoint_rxtx_descriptor_1_0 {
> >>>    uint16_t sender_id;
> >>> @@ -294,8 +326,20 @@ struct ffa_ctx {
> >>>    uint32_t guest_vers;
> >>>    bool tx_is_mine;
> >>>    bool interrupted;
> >>> +    struct list_head shm_list;
> >>> +    unsigned int shm_count;
> >>>    spinlock_t lock;
> >>> };
> >>> +
> >>> +struct ffa_shm_mem {
> >>> +    struct list_head list;
> >>> +    uint16_t sender_id;
> >>> +    uint16_t ep_id;     /* endpoint, the one lending */
> >>> +    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
> >>> +    unsigned int page_count;
> >>> +    struct page_info *pages[];
> >>> +};
> >>> +
> >>> /* Negotiated FF-A version to use with the SPMC */
> >>> static uint32_t ffa_version __ro_after_init;
> >>>
> >>> @@ -310,6 +354,8 @@ static unsigned int subscr_vm_destroyed_count __read_mostly;
> >>> *
> >>> * ffa_page_count is the number of pages used in each of these buffers.
> >>> *
> >>> + * The TX buffer is protected from concurrent usage with ffa_tx_buffer_lock.
> >>> + *
> >>> * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
> >>> * Note that the SPMC is also tracking the ownership of our RX buffer so
> >>> * for calls which uses our RX buffer to deliver a result we must call
> >>> @@ -319,6 +365,7 @@ static void *ffa_rx __read_mostly;
> >>> static void *ffa_tx __read_mostly;
> >>> static unsigned int ffa_page_count __read_mostly;
> >>> static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> >>> +static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
> >>>
> >>> static bool ffa_get_version(uint32_t *vers)
> >>> {
> >>> @@ -429,6 +476,42 @@ static int32_t ffa_rx_release(void)
> >>>    return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
> >>> }
> >>>
> >>> +static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
> >>> +                             register_t addr, uint32_t pg_count,
> >>> +                             uint64_t *handle)
> >>> +{
> >>> +    struct arm_smccc_1_2_regs arg = {
> >>> +        .a0 = FFA_MEM_SHARE_32,
> >>> +        .a1 = tot_len,
> >>> +        .a2 = frag_len,
> >>> +        .a3 = addr,
> >>> +        .a4 = pg_count,
> >>> +    };
> >>> +    struct arm_smccc_1_2_regs resp;
> >>> +
> >>> +    if ( IS_ENABLED(CONFIG_ARM_64) )
> >>> +        arg.a0 = FFA_MEM_SHARE_64;
> >>> +
> >>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>> +
> >>> +    switch ( resp.a0 )
> >>> +    {
> >>> +    case FFA_ERROR:
> >>> +        if ( resp.a2 )
> >>> +            return resp.a2;
> >>> +        else
> >>> +            return FFA_RET_NOT_SUPPORTED;
> >>> +    case FFA_SUCCESS_32:
> >>> +        *handle = regpair_to_uint64(resp.a3, resp.a2);
> >>> +        return FFA_RET_OK;
> >>> +    case FFA_MEM_FRAG_RX:
> >>> +        *handle = regpair_to_uint64(resp.a2, resp.a1);
> >>> +        return resp.a3;
> >>
> >> You are using an int32_t type to cast something that is uint32_t from the spec
> >> and here could in fact be a uint64_t.
> >
> > In practice only much smaller values can be valid, however, I
> > understand that that's not your point. What's the best recovery if the
> > SPMC gives an invalid value for FFA_MEM_FRAG_RX? The SPMC has
> > allocated a memory-sharing state when it returns FFA_MEM_FRAG_RX. The
> > specification doesn't say how to remove that state apart from either
> > completing it successfully or if it's terminated earlier by the SPMC.
> > One option is to do a FFA_MEM_FRAG_TX call with invalid arguments so
> > that the SPMC can free up the memory-sharing state. Thoughts?
>
> I do not think it is Xen responsability to fix SPMC problems here.
> If we detect something of an error we should just return back an error to the client.
>
> If the SPMC is returning an invalid value, we cannot really make much assumptions.
>
> An other solution here would just be to mask out to prevent the implicit cast.

OK, I'll do that.

>
> >
> >>
> >>
> >>> +    default:
> >>> +        return FFA_RET_NOT_SUPPORTED;
> >>> +    }
> >>> +}
> >>> +
> >>> static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> >>>                                      uint8_t msg)
> >>> {
> >>> @@ -757,6 +840,404 @@ out:
> >>>             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> >>> }
> >>>
> >>> +/*
> >>> + * Gets all page and assigns them to the supplied shared memory object. If
> >>> + * this function fails then the caller is still expected to call
> >>> + * put_shm_pages() as a cleanup.
> >>> + */
> >>> +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> >>> +                         const struct ffa_address_range *range,
> >>> +                         uint32_t range_count, unsigned int start_page_idx,
> >>> +                         unsigned int *last_page_idx)
> >>> +{
> >>> +    unsigned int pg_idx = start_page_idx;
> >>> +    gfn_t gfn;
> >>> +    unsigned int n;
> >>> +    unsigned int m;
> >>> +    p2m_type_t t;
> >>> +    uint64_t addr;
> >>> +
> >>> +    for ( n = 0; n < range_count; n++ )
> >>> +    {
> >>> +        for ( m = 0; m < range[n].page_count; m++ )
> >>> +        {
> >>> +            if ( pg_idx >= shm->page_count )
> >>> +                return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +            addr = read_atomic(&range[n].address);
> >>> +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
> >>> +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
> >>> +   P2M_ALLOC);
> >>> +            if ( !shm->pages[pg_idx] )
> >>> +                return FFA_RET_DENIED;
> >>> +            pg_idx++;
> >>
> >> This increment could be done at the end, why here ?
> >
> > Do you mean after the p2m_is_ram() check? I'll move it there.
>
> yes that would be more natural i think.
>
> >
> >>
> >>> +            /* Only normal RAM for now */
> >>> +            if ( !p2m_is_ram(t) )
> >>> +                return FFA_RET_DENIED;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    *last_page_idx = pg_idx;
> >>> +
> >>> +    return FFA_RET_OK;
> >>> +}
> >>> +
> >>> +static void put_shm_pages(struct ffa_shm_mem *shm)
> >>> +{
> >>> +    unsigned int n;
> >>> +
> >>> +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> >>> +    {
> >>> +        put_page(shm->pages[n]);
> >>> +        shm->pages[n] = NULL;
> >>
> >> If an error occured during the generation you might have part
> >> of the pages which are NULL already.
> >>
> >> So you should do a if (pages[n] != NULL) here
> >
> > I'm doing that above in the head of the loop, the loop is terminated
> > at the first pages[n] == NULL.
>
> Right, sorry i missed that.
>
> >
> >>
> >>> +    }
> >>> +}
> >>> +
> >>> +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> >>> +                                             unsigned int page_count)
> >>> +{
> >>> +    struct ffa_shm_mem *shm;
> >>> +
> >>> +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> >>> +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
> >>> +        return NULL;
> >>
> >> Shouldn't you also filter out for page_count = 0 ?
> >
> > Sure, 0 doesn't make sense. But I should probably do it before this
> > function is called since I suppose we'd like to return something
> > different from FFA_RET_NO_MEMORY.
>
> Very true.
>
> >
> >>
> >>> +
> >>> +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> >>> +    if ( shm )
> >>> +    {
> >>> +        ctx->shm_count++;
> >>> +        shm->page_count = page_count;
> >>> +    }
> >>> +
> >>> +    return shm;
> >>> +}
> >>> +
> >>> +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> >>> +{
> >>> +    if ( shm ) {
> >>> +        ASSERT(ctx->shm_count > 0);
> >>> +        ctx->shm_count--;
> >>> +        put_shm_pages(shm);
> >>> +        xfree(shm);
> >>> +    }
> >>> +}
> >>> +
> >>> +static void init_range(struct ffa_address_range *addr_range,
> >>> +                       paddr_t pa)
> >>> +{
> >>> +    memset(addr_range, 0, sizeof(*addr_range));
> >>> +    addr_range->address = pa;
> >>> +    addr_range->page_count = 1;
> >>> +}
> >>> +
> >>> +/*
> >>> + * This function uses the ffa_tx buffer to transmit the memory transaction
> >>> + * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
> >>> + * the buffer from concurrent use.
> >>> + */
> >>> +static int share_shm(struct ffa_shm_mem *shm)
> >>> +{
> >>> +    const uint32_t max_frag_len = ffa_page_count * FFA_PAGE_SIZE;
> >>> +    struct ffa_mem_access *mem_access_array;
> >>> +    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;
> >>> +    void *buf = ffa_tx;
> >>> +    uint32_t frag_len;
> >>> +    uint32_t tot_len;
> >>> +    paddr_t last_pa;
> >>> +    unsigned int n;
> >>> +    paddr_t pa;
> >>> +
> >>> +    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
> >>> +    if ( !shm->page_count )
> >>> +    {
> >>> +        ASSERT_UNREACHABLE();
> >>> +        return FFA_RET_INVALID_PARAMETERS;
> >>
> >> page_count = 0 should be filtered out before reaching this and this should
> >> only be an assert if you want but no unreachable with a return.
> >
> > I'm adding code to filter out page_count = 0. I'm not sure what you
> > expect here, should I remove the entire check here or what do you
> > want?
>
> As this is checked before, this could just be an assert and not something with a return.

OK, I'll do that.

>
> >
> >>
> >>> +    }
> >>> +
> >>> +    descr = buf;
> >>> +    memset(descr, 0, sizeof(*descr));
> >>> +    descr->sender_id = shm->sender_id;
> >>> +    descr->global_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);
> >>> +
> >>> +    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);
> >>> +
> >>> +    region_descr = buf + mem_access_array[0].region_offs;
> >>> +    memset(region_descr, 0, sizeof(*region_descr));
> >>> +    region_descr->total_page_count = shm->page_count;
> >>> +
> >>> +    region_descr->address_range_count = 1;
> >>> +    last_pa = page_to_maddr(shm->pages[0]);
> >>> +    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> >>> +    {
> >>> +        pa = page_to_maddr(shm->pages[n]);
> >>> +        if ( last_pa + FFA_PAGE_SIZE == pa )
> >>> +            continue;
> >>> +        region_descr->address_range_count++;
> >>> +    }
> >>> +
> >>> +    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
> >>> +                                region_descr->address_range_count);
> >>> +    if ( tot_len > max_frag_len )
> >>> +        return FFA_RET_NOT_SUPPORTED;
> >>> +
> >>> +    addr_range = region_descr->address_range_array;
> >>> +    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
> >>> +    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++ )
> >>> +    {
> >>> +        pa = page_to_maddr(shm->pages[n]);
> >>> +        if ( last_pa + FFA_PAGE_SIZE == pa )
> >>> +        {
> >>> +            addr_range->page_count++;
> >>> +            continue;
> >>> +        }
> >>> +
> >>> +        frag_len += sizeof(*addr_range);
> >>> +        addr_range++;
> >>> +        init_range(addr_range, pa);
> >>> +    }
> >>> +
> >>> +    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> >>> +}
> >>> +
> >>> +static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
> >>> +                                struct ffa_mem_transaction_x *trans)
> >>> +{
> >>> +    uint16_t mem_reg_attr;
> >>> +    uint32_t flags;
> >>> +    uint32_t count;
> >>> +    uint32_t offs;
> >>> +    uint32_t size;
> >>> +
> >>> +    if ( ffa_vers >= FFA_VERSION_1_1 )
> >>> +    {
> >>> +        const struct ffa_mem_transaction_1_1 *descr;
> >>> +
> >>> +        if ( blen < sizeof(*descr) )
> >>> +            return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +        descr = buf;
> >>> +        trans->sender_id = descr->sender_id;
> >>> +        mem_reg_attr = descr->mem_reg_attr;
> >>> +        flags = descr->flags;
> >>> +        trans->global_handle = descr->global_handle;
> >>> +        trans->tag = descr->tag;
> >>> +
> >>> +        count = descr->mem_access_count;
> >>> +        size = descr->mem_access_size;
> >>> +        offs = descr->mem_access_offs;
> >>> +    }
> >>> +    else
> >>> +    {
> >>> +        const struct ffa_mem_transaction_1_0 *descr;
> >>> +
> >>> +        if ( blen < sizeof(*descr) )
> >>> +            return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +        descr = buf;
> >>> +        trans->sender_id = descr->sender_id;
> >>> +        mem_reg_attr = descr->mem_reg_attr;
> >>> +        flags = descr->flags;
> >>> +        trans->global_handle = descr->global_handle;
> >>> +        trans->tag = descr->tag;
> >>> +
> >>> +        count = descr->mem_access_count;
> >>> +        size = sizeof(struct ffa_mem_access);
> >>> +        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
> >>> +    }
> >>> +    /*
> >>> +     * Make sure that "descr" which is shared with the guest isn't accessed
> >>> +     * again after this point.
> >>> +     */
> >>> +    barrier();
> >>
> >> I am not really following the comment here. You accessed the content of descr
> >> before and it is in the rxtx buffer so why is this needed ?
> >
> > I'm making sure that the compiler doesn't optimize and reorders the
> > reads from memory in funny ways, for instance, reading again after the
> > ifs just below. The RXTX buffer is shared with the guest so it can
> > potentially be updated concurrently by another CPU.
>
> The client is not suppose to modify the buffer during the call, using atomic
> operations here is not really solving any issue if this is modified while we use it right ?

We are guarding against a TOCTOU (time-of-check to time-of-use) type
of attack or bug.

>
> >
> >>
> >>> +
> >>> +    /*
> >>> +     * We're doing a rough check to see that no information is lost when
> >>> +     * tranfering the values into a struct ffa_mem_transaction_x below. The
> >>> +     * fields in struct ffa_mem_transaction_x are wide enough to hold any
> >>> +     * valid value so being out of range means that something is wrong.
> >>> +     */
> >>> +    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
> >>> +        count > UINT8_MAX || offs > UINT16_MAX )
> >>> +        return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +    /* Check that the endpoint memory access descriptor array fits */
> >>> +    if ( size * count + offs > blen )
> >>> +        return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +    trans->mem_reg_attr = mem_reg_attr;
> >>> +    trans->flags = flags;
> >>> +    trans->mem_access_size = size;
> >>> +    trans->mem_access_count = count;
> >>> +    trans->mem_access_offs = offs;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static void handle_mem_share(struct cpu_user_regs *regs)
> >>> +{
> >>> +    uint32_t tot_len = get_user_reg(regs, 1);
> >>> +    uint32_t frag_len = get_user_reg(regs, 2);
> >>> +    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;
> >>> +    struct ffa_mem_transaction_x trans;
> >>> +    struct domain *d = current->domain;
> >>> +    struct ffa_ctx *ctx = d->arch.tee;
> >>> +    struct ffa_shm_mem *shm = NULL;
> >>> +    unsigned int last_page_idx = 0;
> >>> +    register_t handle_hi = 0;
> >>> +    register_t handle_lo = 0;
> >>> +    int ret = FFA_RET_DENIED;
> >>> +    uint32_t range_count;
> >>> +    uint32_t region_offs;
> >>> +
> >>> +    /*
> >>> +     * We're only accepting memory transaction descriptors via the rx/tx
> >>> +     * buffer.
> >>
> >> Is this a limitation coming fomr the spec or from the implementation ?
> >
> > This is just a limitation in the implementation.
> >
> >>
> >>> +     */
> >>> +    if ( addr )
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_set_ret;
> >>> +    }
> >>> +
> >>> +    /* Check that fragment length doesn't exceed total length */
> >>> +    if ( frag_len > tot_len )
> >>> +    {
> >>> +        ret = FFA_RET_INVALID_PARAMETERS;
> >>> +        goto out_set_ret;
> >>> +    }
> >>> +
> >>> +    /* We currently only support a single fragment */
> >>
> >> It would make sense to add some text at the beginning of the files listing
> >> the current limitations of the implementation.
> >
> > That's quite a bit to keep track of, especially since it will change
> > with each patch. If it's important perhaps we can summarize that in a
> > final commit instead. By the way, this particular limitation is
> > removed in a later patch.
>
> I am more thinking at the end of the serie to have one place with the current
> state and limitations of the implementation.
>
> We cannot really expect someone to browse all comments to get what is
>  supported or not.

OK

>
> >
> >>
> >>> +    if ( frag_len != tot_len )
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_set_ret;
> >>> +    }
> >>> +
> >>> +    spin_lock(&ctx->lock);
> >>> +
> >>> +    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
> >>> +        goto out_unlock;
> >>> +
> >>> +    if ( !ffa_page_count )
> >>> +    {
> >>> +        ret = FFA_RET_NO_MEMORY;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +
> >>> +    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
> >>> +    if ( ret )
> >>> +        goto out_unlock;
> >>> +
> >>> +    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +
> >>> +    /* Only supports sharing it with one SP for now */
> >>
> >> Also a limitation to list.
> >
> > OK
> >
> >>
> >>> +    if ( trans.mem_access_count != 1 )
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +
> >>> +    if ( trans.sender_id != get_vm_id(d) )
> >>> +    {
> >>> +        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;
> >>> +
> >>
> >> Why are you using atomic operations to access rxtx buffer after here ?
> >
> > To limit how the compiler can reorder the reads from memory.
>
> As stated before, don't we assume that the buffer is not modified by the client
>  while we use it ?

No, as I'm saying above we're guarding against it.

Thanks,
Jens

>
> >
> >>
> >>> +    mem_access = ctx->tx + trans.mem_access_offs;
> >>> +    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
> >>
> >> Also a limitation to list.
> >
> > OK
>
> Cheers
> Bertrand
>
> >
> > Thanks,
> > Jens
> >
> >>
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +
> >>> +    region_offs = read_atomic(&mem_access->region_offs);
> >>> +    if ( sizeof(*region_descr) + region_offs > frag_len )
> >>> +    {
> >>> +        ret = FFA_RET_NOT_SUPPORTED;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +
> >>> +    region_descr = ctx->tx + region_offs;
> >>> +    range_count = read_atomic(&region_descr->address_range_count);
> >>> +    page_count = read_atomic(&region_descr->total_page_count);
> >>> +
> >>> +    shm = alloc_ffa_shm_mem(ctx, page_count);
> >>> +    if ( !shm )
> >>> +    {
> >>> +        ret = FFA_RET_NO_MEMORY;
> >>> +        goto out_unlock;
> >>> +    }
> >>> +    shm->sender_id = trans.sender_id;
> >>> +    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
> >>> +
> >>> +    /*
> >>> +     * Check that the Composite memory region descriptor fits.
> >>> +     */
> >>> +    if ( sizeof(*region_descr) + region_offs +
> >>> +         range_count * sizeof(struct ffa_address_range) > frag_len )
> >>> +    {
> >>> +        ret = FFA_RET_INVALID_PARAMETERS;
> >>> +        goto out;
> >>> +    }
> >>> +
> >>> +    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
> >>> +                        0, &last_page_idx);
> >>> +    if ( ret )
> >>> +        goto out;
> >>> +    if ( last_page_idx != shm->page_count )
> >>> +    {
> >>> +        ret = FFA_RET_INVALID_PARAMETERS;
> >>> +        goto out;
> >>> +    }
> >>> +
> >>> +    /* Note that share_shm() uses our tx buffer */
> >>> +    spin_lock(&ffa_tx_buffer_lock);
> >>> +    ret = share_shm(shm);
> >>> +    spin_unlock(&ffa_tx_buffer_lock);
> >>> +    if ( ret )
> >>> +        goto out;
> >>> +
> >>> +    list_add_tail(&shm->list, &ctx->shm_list);
> >>> +
> >>> +    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> >>> +
> >>> +out:
> >>> +    if ( ret )
> >>> +        free_ffa_shm_mem(ctx, shm);
> >>> +out_unlock:
> >>> +    spin_unlock(&ctx->lock);
> >>> +
> >>> +out_set_ret:
> >>> +    if ( ret == 0)
> >>> +            set_regs_success(regs, handle_lo, handle_hi);
> >>> +    else
> >>> +            set_regs_error(regs, ret);
> >>> +}
> >>> +
> >>> static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>> {
> >>>    uint32_t fid = get_user_reg(regs, 0);
> >>> @@ -818,6 +1299,12 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>> #endif
> >>>        handle_msg_send_direct_req(regs, fid);
> >>>        return true;
> >>> +    case FFA_MEM_SHARE_32:
> >>> +#ifdef CONFIG_ARM_64
> >>> +    case FFA_MEM_SHARE_64:
> >>> +#endif
> >>> +        handle_mem_share(regs);
> >>> +        return true;
> >>>
> >>>    default:
> >>>        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> >>> @@ -857,6 +1344,8 @@ static int ffa_domain_init(struct domain *d)
> >>>        }
> >>>    }
> >>>
> >>> +    INIT_LIST_HEAD(&ctx->shm_list);
> >>> +
> >>>    d->arch.tee = ctx;
> >>>
> >>>    return 0;
> >>> @@ -1012,11 +1501,13 @@ static bool ffa_probe(void)
> >>>         !check_mandatory_feature(FFA_RX_RELEASE) ||
> >>> #ifdef CONFIG_ARM_64
> >>>         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
> >>> +         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
> >>> #endif
> >>> #ifdef CONFIG_ARM_32
> >>>         !check_mandatory_feature(FFA_RXTX_MAP_32) ||
> >>> #endif
> >>>         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
> >>> +         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
> >>>         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> >>>        return false;
> >>>
> >>> --
> >>> 2.34.1
> >>
> >>
> >> Cheers
> >> Bertrand
>
>

Patch

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 94c90b252454..cdc286caf62c 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -270,6 +270,38 @@  struct ffa_mem_transaction_1_1 {
     uint8_t reserved[12];
 };
 
+/* Calculate offset of struct ffa_mem_access from start of buffer */
+#define MEM_ACCESS_OFFSET(access_idx) \
+    ( sizeof(struct ffa_mem_transaction_1_1) + \
+      ( access_idx ) * sizeof(struct ffa_mem_access) )
+
+/* Calculate offset of struct ffa_mem_region from start of buffer */
+#define REGION_OFFSET(access_count, region_idx) \
+    ( MEM_ACCESS_OFFSET(access_count) + \
+      ( region_idx ) * sizeof(struct ffa_mem_region) )
+
+/* Calculate offset of struct ffa_address_range from start of buffer */
+#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
+    ( REGION_OFFSET(access_count, region_count) + \
+      ( range_idx ) * sizeof(struct ffa_address_range) )
+
+/*
+ * 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
+ * interface and can be changed without changing any ABI.
+ */
+struct ffa_mem_transaction_x {
+    uint16_t sender_id;
+    uint8_t mem_reg_attr;
+    uint8_t flags;
+    uint8_t mem_access_size;
+    uint8_t mem_access_count;
+    uint16_t mem_access_offs;
+    uint64_t global_handle;
+    uint64_t tag;
+};
+
 /* Endpoint RX/TX descriptor */
 struct ffa_endpoint_rxtx_descriptor_1_0 {
     uint16_t sender_id;
@@ -294,8 +326,20 @@  struct ffa_ctx {
     uint32_t guest_vers;
     bool tx_is_mine;
     bool interrupted;
+    struct list_head shm_list;
+    unsigned int shm_count;
     spinlock_t lock;
 };
+
+struct ffa_shm_mem {
+    struct list_head list;
+    uint16_t sender_id;
+    uint16_t ep_id;     /* endpoint, the one lending */
+    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
+    unsigned int page_count;
+    struct page_info *pages[];
+};
+
 /* Negotiated FF-A version to use with the SPMC */
 static uint32_t ffa_version __ro_after_init;
 
@@ -310,6 +354,8 @@  static unsigned int subscr_vm_destroyed_count __read_mostly;
  *
  * ffa_page_count is the number of pages used in each of these buffers.
  *
+ * The TX buffer is protected from concurrent usage with ffa_tx_buffer_lock.
+ *
  * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
  * Note that the SPMC is also tracking the ownership of our RX buffer so
  * for calls which uses our RX buffer to deliver a result we must call
@@ -319,6 +365,7 @@  static void *ffa_rx __read_mostly;
 static void *ffa_tx __read_mostly;
 static unsigned int ffa_page_count __read_mostly;
 static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
+static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
 
 static bool ffa_get_version(uint32_t *vers)
 {
@@ -429,6 +476,42 @@  static int32_t ffa_rx_release(void)
     return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
 }
 
+static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
+                             register_t addr, uint32_t pg_count,
+                             uint64_t *handle)
+{
+    struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_MEM_SHARE_32,
+        .a1 = tot_len,
+        .a2 = frag_len,
+        .a3 = addr,
+        .a4 = pg_count,
+    };
+    struct arm_smccc_1_2_regs resp;
+
+    if ( IS_ENABLED(CONFIG_ARM_64) )
+        arg.a0 = FFA_MEM_SHARE_64;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+
+    switch ( resp.a0 )
+    {
+    case FFA_ERROR:
+        if ( resp.a2 )
+            return resp.a2;
+        else
+            return FFA_RET_NOT_SUPPORTED;
+    case FFA_SUCCESS_32:
+        *handle = regpair_to_uint64(resp.a3, resp.a2);
+        return FFA_RET_OK;
+    case FFA_MEM_FRAG_RX:
+        *handle = regpair_to_uint64(resp.a2, resp.a1);
+        return resp.a3;
+    default:
+        return FFA_RET_NOT_SUPPORTED;
+    }
+}
+
 static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
                                       uint8_t msg)
 {
@@ -757,6 +840,404 @@  out:
              resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
 }
 
+/*
+ * Gets all page and assigns them to the supplied shared memory object. If
+ * this function fails then the caller is still expected to call
+ * put_shm_pages() as a cleanup.
+ */
+static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
+                         const struct ffa_address_range *range,
+                         uint32_t range_count, unsigned int start_page_idx,
+                         unsigned int *last_page_idx)
+{
+    unsigned int pg_idx = start_page_idx;
+    gfn_t gfn;
+    unsigned int n;
+    unsigned int m;
+    p2m_type_t t;
+    uint64_t addr;
+
+    for ( n = 0; n < range_count; n++ )
+    {
+        for ( m = 0; m < range[n].page_count; m++ )
+        {
+            if ( pg_idx >= shm->page_count )
+                return FFA_RET_INVALID_PARAMETERS;
+
+            addr = read_atomic(&range[n].address);
+            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
+            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
+						   P2M_ALLOC);
+            if ( !shm->pages[pg_idx] )
+                return FFA_RET_DENIED;
+            pg_idx++;
+            /* Only normal RAM for now */
+            if ( !p2m_is_ram(t) )
+                return FFA_RET_DENIED;
+        }
+    }
+
+    *last_page_idx = pg_idx;
+
+    return FFA_RET_OK;
+}
+
+static void put_shm_pages(struct ffa_shm_mem *shm)
+{
+    unsigned int n;
+
+    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
+    {
+        put_page(shm->pages[n]);
+        shm->pages[n] = NULL;
+    }
+}
+
+static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
+                                             unsigned int page_count)
+{
+    struct ffa_shm_mem *shm;
+
+    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
+         ctx->shm_count >= FFA_MAX_SHM_COUNT )
+        return NULL;
+
+    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
+    if ( shm )
+    {
+        ctx->shm_count++;
+        shm->page_count = page_count;
+    }
+
+    return shm;
+}
+
+static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
+{
+    if ( shm ) {
+        ASSERT(ctx->shm_count > 0);
+        ctx->shm_count--;
+        put_shm_pages(shm);
+        xfree(shm);
+    }
+}
+
+static void init_range(struct ffa_address_range *addr_range,
+                       paddr_t pa)
+{
+    memset(addr_range, 0, sizeof(*addr_range));
+    addr_range->address = pa;
+    addr_range->page_count = 1;
+}
+
+/*
+ * This function uses the ffa_tx buffer to transmit the memory transaction
+ * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
+ * the buffer from concurrent use.
+ */
+static int share_shm(struct ffa_shm_mem *shm)
+{
+    const uint32_t max_frag_len = ffa_page_count * FFA_PAGE_SIZE;
+    struct ffa_mem_access *mem_access_array;
+    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;
+    void *buf = ffa_tx;
+    uint32_t frag_len;
+    uint32_t tot_len;
+    paddr_t last_pa;
+    unsigned int n;
+    paddr_t pa;
+
+    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
+    if ( !shm->page_count )
+    {
+        ASSERT_UNREACHABLE();
+        return FFA_RET_INVALID_PARAMETERS;
+    }
+
+    descr = buf;
+    memset(descr, 0, sizeof(*descr));
+    descr->sender_id = shm->sender_id;
+    descr->global_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);
+
+    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);
+
+    region_descr = buf + mem_access_array[0].region_offs;
+    memset(region_descr, 0, sizeof(*region_descr));
+    region_descr->total_page_count = shm->page_count;
+
+    region_descr->address_range_count = 1;
+    last_pa = page_to_maddr(shm->pages[0]);
+    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+    {
+        pa = page_to_maddr(shm->pages[n]);
+        if ( last_pa + FFA_PAGE_SIZE == pa )
+            continue;
+        region_descr->address_range_count++;
+    }
+
+    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
+                                region_descr->address_range_count);
+    if ( tot_len > max_frag_len )
+        return FFA_RET_NOT_SUPPORTED;
+
+    addr_range = region_descr->address_range_array;
+    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
+    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++ )
+    {
+        pa = page_to_maddr(shm->pages[n]);
+        if ( last_pa + FFA_PAGE_SIZE == pa )
+        {
+            addr_range->page_count++;
+            continue;
+        }
+
+        frag_len += sizeof(*addr_range);
+        addr_range++;
+        init_range(addr_range, pa);
+    }
+
+    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
+}
+
+static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
+                                struct ffa_mem_transaction_x *trans)
+{
+    uint16_t mem_reg_attr;
+    uint32_t flags;
+    uint32_t count;
+    uint32_t offs;
+    uint32_t size;
+
+    if ( ffa_vers >= FFA_VERSION_1_1 )
+    {
+        const struct ffa_mem_transaction_1_1 *descr;
+
+        if ( blen < sizeof(*descr) )
+            return FFA_RET_INVALID_PARAMETERS;
+
+        descr = buf;
+        trans->sender_id = descr->sender_id;
+        mem_reg_attr = descr->mem_reg_attr;
+        flags = descr->flags;
+        trans->global_handle = descr->global_handle;
+        trans->tag = descr->tag;
+
+        count = descr->mem_access_count;
+        size = descr->mem_access_size;
+        offs = descr->mem_access_offs;
+    }
+    else
+    {
+        const struct ffa_mem_transaction_1_0 *descr;
+
+        if ( blen < sizeof(*descr) )
+            return FFA_RET_INVALID_PARAMETERS;
+
+        descr = buf;
+        trans->sender_id = descr->sender_id;
+        mem_reg_attr = descr->mem_reg_attr;
+        flags = descr->flags;
+        trans->global_handle = descr->global_handle;
+        trans->tag = descr->tag;
+
+        count = descr->mem_access_count;
+        size = sizeof(struct ffa_mem_access);
+        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
+    }
+    /*
+     * Make sure that "descr" which is shared with the guest isn't accessed
+     * again after this point.
+     */
+    barrier();
+
+    /*
+     * We're doing a rough check to see that no information is lost when
+     * tranfering the values into a struct ffa_mem_transaction_x below. The
+     * fields in struct ffa_mem_transaction_x are wide enough to hold any
+     * valid value so being out of range means that something is wrong.
+     */
+    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
+        count > UINT8_MAX || offs > UINT16_MAX )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    /* Check that the endpoint memory access descriptor array fits */
+    if ( size * count + offs > blen )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    trans->mem_reg_attr = mem_reg_attr;
+    trans->flags = flags;
+    trans->mem_access_size = size;
+    trans->mem_access_count = count;
+    trans->mem_access_offs = offs;
+
+    return 0;
+}
+
+static void handle_mem_share(struct cpu_user_regs *regs)
+{
+    uint32_t tot_len = get_user_reg(regs, 1);
+    uint32_t frag_len = get_user_reg(regs, 2);
+    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;
+    struct ffa_mem_transaction_x trans;
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    struct ffa_shm_mem *shm = NULL;
+    unsigned int last_page_idx = 0;
+    register_t handle_hi = 0;
+    register_t handle_lo = 0;
+    int ret = FFA_RET_DENIED;
+    uint32_t range_count;
+    uint32_t region_offs;
+
+    /*
+     * We're only accepting memory transaction descriptors via the rx/tx
+     * buffer.
+     */
+    if ( addr )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_set_ret;
+    }
+
+    /* Check that fragment length doesn't exceed total length */
+    if ( frag_len > tot_len )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_set_ret;
+    }
+
+    /* We currently only support a single fragment */
+    if ( frag_len != tot_len )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_set_ret;
+    }
+
+    spin_lock(&ctx->lock);
+
+    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
+        goto out_unlock;
+
+    if ( !ffa_page_count )
+    {
+        ret = FFA_RET_NO_MEMORY;
+        goto out_unlock;
+    }
+
+    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
+    if ( ret )
+        goto out_unlock;
+
+    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    /* Only supports sharing it with one SP for now */
+    if ( trans.mem_access_count != 1 )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    if ( trans.sender_id != get_vm_id(d) )
+    {
+        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 = ctx->tx + trans.mem_access_offs;
+    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    region_offs = read_atomic(&mem_access->region_offs);
+    if ( sizeof(*region_descr) + region_offs > frag_len )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    region_descr = ctx->tx + region_offs;
+    range_count = read_atomic(&region_descr->address_range_count);
+    page_count = read_atomic(&region_descr->total_page_count);
+
+    shm = alloc_ffa_shm_mem(ctx, page_count);
+    if ( !shm )
+    {
+        ret = FFA_RET_NO_MEMORY;
+        goto out_unlock;
+    }
+    shm->sender_id = trans.sender_id;
+    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
+
+    /*
+     * Check that the Composite memory region descriptor fits.
+     */
+    if ( sizeof(*region_descr) + region_offs +
+         range_count * sizeof(struct ffa_address_range) > frag_len )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+
+    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
+                        0, &last_page_idx);
+    if ( ret )
+        goto out;
+    if ( last_page_idx != shm->page_count )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+
+    /* Note that share_shm() uses our tx buffer */
+    spin_lock(&ffa_tx_buffer_lock);
+    ret = share_shm(shm);
+    spin_unlock(&ffa_tx_buffer_lock);
+    if ( ret )
+        goto out;
+
+    list_add_tail(&shm->list, &ctx->shm_list);
+
+    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
+
+out:
+    if ( ret )
+        free_ffa_shm_mem(ctx, shm);
+out_unlock:
+    spin_unlock(&ctx->lock);
+
+out_set_ret:
+    if ( ret == 0)
+            set_regs_success(regs, handle_lo, handle_hi);
+    else
+            set_regs_error(regs, ret);
+}
+
 static bool ffa_handle_call(struct cpu_user_regs *regs)
 {
     uint32_t fid = get_user_reg(regs, 0);
@@ -818,6 +1299,12 @@  static bool ffa_handle_call(struct cpu_user_regs *regs)
 #endif
         handle_msg_send_direct_req(regs, fid);
         return true;
+    case FFA_MEM_SHARE_32:
+#ifdef CONFIG_ARM_64
+    case FFA_MEM_SHARE_64:
+#endif
+        handle_mem_share(regs);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -857,6 +1344,8 @@  static int ffa_domain_init(struct domain *d)
         }
     }
 
+    INIT_LIST_HEAD(&ctx->shm_list);
+
     d->arch.tee = ctx;
 
     return 0;
@@ -1012,11 +1501,13 @@  static bool ffa_probe(void)
          !check_mandatory_feature(FFA_RX_RELEASE) ||
 #ifdef CONFIG_ARM_64
          !check_mandatory_feature(FFA_RXTX_MAP_64) ||
+         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
 #endif
 #ifdef CONFIG_ARM_32
          !check_mandatory_feature(FFA_RXTX_MAP_32) ||
 #endif
          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
+         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
         return false;