|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v10 20/24] xen/arm: ffa: support sharing large memory ranges
Hi Bertrand,
On Wed, Jul 19, 2023 at 11:37 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 17 Jul 2023, at 09:21, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Adds support for sharing large memory ranges transmitted in fragments
> > using FFA_MEM_FRAG_TX.
> >
> > The implementation is the bare minimum to be able to communicate with
> > OP-TEE running as an SPMC at S-EL1.
> >
> > Adds a check that the SP supports the needed FF-A feature
> > FFA_MEM_FRAG_TX.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> > xen/arch/arm/tee/ffa.c | 253 ++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 240 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index c623c51168b9..ac23b9edc74c 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -359,6 +359,8 @@ struct ffa_ctx {
> > */
> > uint16_t create_signal_count;
> > bool rx_is_free;
> > + /* Currently used fragment states, struct mem_frag_state */
> > + struct list_head frag_list;
> > /* Used shared memory objects, struct ffa_shm_mem */
> > struct list_head shm_list;
> > /* Number of allocated shared memory object */
> > @@ -375,6 +377,18 @@ struct ffa_shm_mem {
> > struct page_info *pages[];
> > };
> >
> > +struct mem_frag_state {
> > + struct list_head list;
> > + struct ffa_shm_mem *shm;
> > + uint32_t range_count;
> > + unsigned int current_page_idx;
> > + unsigned int frag_offset;
> > + unsigned int range_offset;
> > + const uint8_t *buf;
> > + unsigned int buf_size;
> > + struct ffa_address_range range;
> > +};
>
> Please add some comments inside this structure as
> from reading the code it is not quite clear what is done.
OK
>
> > +
> > /* Negotiated FF-A version to use with the SPMC */
> > static uint32_t __ro_after_init ffa_version;
> >
> > @@ -538,6 +552,36 @@ static int32_t ffa_mem_share(uint32_t tot_len,
> > uint32_t frag_len,
> > }
> > }
> >
> > +static int32_t ffa_mem_frag_tx(uint64_t handle, uint32_t frag_len,
> > + uint16_t sender_id)
> > +{
> > + struct arm_smccc_1_2_regs arg = {
> > + .a0 = FFA_MEM_FRAG_TX,
> > + .a1 = handle & UINT32_MAX,
> > + .a2 = handle >> 32,
> > + .a3 = frag_len,
> > + .a4 = (uint32_t)sender_id << 16,
> > + };
> > + struct arm_smccc_1_2_regs resp;
> > +
> > + 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:
> > + return FFA_RET_OK;
> > + case FFA_MEM_FRAG_RX:
> > + return resp.a3;
> > + default:
> > + return FFA_RET_NOT_SUPPORTED;
> > + }
> > +}
> > +
> > static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
> > uint32_t flags)
> > {
> > @@ -627,6 +671,14 @@ static void set_regs_success(struct cpu_user_regs
> > *regs, uint32_t w2,
> > set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
> > }
> >
> > +static void set_regs_frag_rx(struct cpu_user_regs *regs, uint32_t
> > handle_lo,
> > + uint32_t handle_hi, uint32_t frag_offset,
> > + uint16_t sender_id)
> > +{
> > + set_regs(regs, FFA_MEM_FRAG_RX, handle_lo, handle_hi, frag_offset,
> > + (uint32_t)sender_id << 16, 0, 0, 0);
> > +}
> > +
> > static void handle_version(struct cpu_user_regs *regs)
> > {
> > struct domain *d = current->domain;
> > @@ -999,6 +1051,8 @@ static int share_shm(struct ffa_shm_mem *shm)
> > paddr_t last_pa;
> > unsigned int n;
> > paddr_t pa;
> > + bool first;
> > + int ret;
> >
> > ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
> > ASSERT(shm->page_count);
> > @@ -1034,13 +1088,23 @@ static int share_shm(struct ffa_shm_mem *shm)
> >
> > 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;
> >
> > + /*
> > + * Sharing memory with secure world may have to be done with multiple
> > + * calls depending on how many address ranges will be needed. If we're
> > + * sharing physically contiguous memory we will only need one range but
> > + * we will also need to deal with the worst case where all physical
> > + * pages are non-contiguous. For the first batch of address ranges we
> > + * call ffa_mem_share() and for all that follows ffa_mem_frag_tx().
> > + *
> > + * We use frag_len to keep track of how far into the transmit buffer we
> > + * have gone.
> > + */
> > 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);
> > + first = true;
> > for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> > {
> > pa = page_to_maddr(shm->pages[n]);
> > @@ -1050,12 +1114,34 @@ static int share_shm(struct ffa_shm_mem *shm)
> > continue;
> > }
> >
> > - frag_len += sizeof(*addr_range);
> > - addr_range++;
> > + if ( frag_len == max_frag_len )
>
> This test seem a bit dangerous as there is nothing ensuring that frag_len
> will end
> up aligned to a page.
flag_len is always a multiple of 16
>
> I would suggest here to do frag_len + sizeof(*addr_range) > max_frag_len to
> check
> if we can fit or not an extra address range in the area.
If we have that check instead, then I'm not sure what we should do
with the part that doesn't belong there.
>
>
> > + {
> > + if ( first )
> > + {
> > + ret = ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> > + first = false;
> > + }
> > + else
> > + {
> > + ret = ffa_mem_frag_tx(shm->handle, frag_len,
> > shm->sender_id);
> > + }
> > + if ( ret <= 0 )
> > + return ret;
> > + frag_len = sizeof(*addr_range);
> > + addr_range = buf;
> > + }
> > + else
> > + {
> > + frag_len += sizeof(*addr_range);
> > + addr_range++;
> > + }
> > init_range(addr_range, pa);
> > }
> >
> > - return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> > + if ( first )
> > + return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> > + else
> > + return ffa_mem_frag_tx(shm->handle, frag_len, shm->sender_id);
> > }
> >
> > static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t
> > blen,
> > @@ -1132,8 +1218,53 @@ static int read_mem_transaction(uint32_t ffa_vers,
> > const void *buf, size_t blen,
> > return 0;
> > }
> >
> > +static int add_mem_share_frag(struct mem_frag_state *s, unsigned int offs,
> > + unsigned int frag_len)
> > +{
> > + struct domain *d = current->domain;
> > + unsigned int o = offs;
> > + unsigned int l;
> > + int ret;
> > +
> > + if ( frag_len < o )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + /* Fill up the first struct ffa_address_range */
>
> The magic here after requires some explanation, could you add more details in
> the comment ?
OK
>
> > + l = min_t(unsigned int, frag_len - o, sizeof(s->range) -
> > s->range_offset);
> > + memcpy((uint8_t *)&s->range + s->range_offset, s->buf + o, l);
> > + s->range_offset += l;
> > + o += l;
> > + if ( s->range_offset != sizeof(s->range) )
> > + goto out;
>
> Shouldn't we test this before doing the memcpy ?
> Is this an error case ?
No, it's an unusually small fragment but not necessarily an error.
> What is the expected frag_offset value here ?
That depends, usually, I'd expect it to be a multiple of 4k, but the
caller can use different fragment lengths.
>
>
> > + s->range_offset = 0;
> > +
> > + while ( true )
> > + {
> > + ret = get_shm_pages(d, s->shm, &s->range, 1, s->current_page_idx,
> > + &s->current_page_idx);
> > + if ( ret )
> > + return ret;
> > + if ( s->range_count == 1 )
> > + return 0;
> > + s->range_count--;
> > + if ( frag_len - o < sizeof(s->range) )
> > + break;
> > + memcpy(&s->range, s->buf + o, sizeof(s->range));
> > + o += sizeof(s->range);
> > + }
> > +
> > + /* Collect any remaining bytes for the next struct ffa_address_range */
> > + s->range_offset = frag_len - o;
> > + memcpy(&s->range, s->buf + o, frag_len - o);
> > +out:
> > + s->frag_offset += frag_len;
> > +
> > + return s->frag_offset;
> > +}
>
> Overall the processing in this function is not quite clear so we either need
> to add comments to explain it more or find a better way to implement to make
> it a bit clearer.
>
> The implementation for fragmented memory sharing requests here is very
> complex and I am not quite feeling confident that it does not contains bugs.
Yeah, I had to debug it a few time before I got it to work. This is
one of the more complicated parts of the specification.
>
> As this is not something required to have optee support, I would suggest to
> discard this part for now in the support.
>
> What do you think ?
Sure, we can skip it for now. I'll remove it from the next version of
the patch set.
Thanks,
Jens
>
> Cheers
> Bertrand
>
> > +
> > static void handle_mem_share(struct cpu_user_regs *regs)
> > {
> > + static uint64_t next_handle = FFA_HANDLE_HYP_FLAG;
> > 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);
> > @@ -1168,13 +1299,6 @@ static void handle_mem_share(struct cpu_user_regs
> > *regs)
> > 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 )
> > @@ -1240,6 +1364,36 @@ static void handle_mem_share(struct cpu_user_regs
> > *regs)
> > shm->sender_id = trans.sender_id;
> > shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
> >
> > + if ( frag_len != tot_len )
> > + {
> > + struct mem_frag_state *s = xzalloc(struct mem_frag_state);
> > +
> > + if ( !s )
> > + {
> > + ret = FFA_RET_NO_MEMORY;
> > + goto out;
> > + }
> > + s->shm = shm;
> > + s->range_count = range_count;
> > + s->buf = ctx->tx;
> > + s->buf_size = FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE;
> > + ret = add_mem_share_frag(s, sizeof(*region_descr) + region_offs,
> > + frag_len);
> > + if ( ret <= 0 )
> > + {
> > + xfree(s);
> > + if ( ret < 0 )
> > + goto out;
> > + }
> > + else
> > + {
> > + shm->handle = next_handle++;
> > + uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> > + list_add_tail(&s->list, &ctx->frag_list);
> > + }
> > + goto out_unlock;
> > + }
> > +
> > /*
> > * Check that the Composite memory region descriptor fits.
> > */
> > @@ -1278,7 +1432,75 @@ out_unlock:
> > spin_unlock(&ctx->lock);
> >
> > out_set_ret:
> > - if ( ret == 0)
> > + if ( ret > 0 )
> > + set_regs_frag_rx(regs, handle_lo, handle_hi, ret,
> > trans.sender_id);
> > + else if ( ret == 0)
> > + set_regs_success(regs, handle_lo, handle_hi);
> > + else
> > + set_regs_error(regs, ret);
> > +}
> > +
> > +static struct mem_frag_state *find_frag_state(struct ffa_ctx *ctx,
> > + uint64_t handle)
> > +{
> > + struct mem_frag_state *s;
> > +
> > + list_for_each_entry(s, &ctx->frag_list, list)
> > + if ( s->shm->handle == handle )
> > + return s;
> > +
> > + return NULL;
> > +}
> > +
> > +static void handle_mem_frag_tx(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + struct ffa_ctx *ctx = d->arch.tee;
> > + uint32_t frag_len = get_user_reg(regs, 3);
> > + uint32_t handle_lo = get_user_reg(regs, 1);
> > + uint32_t handle_hi = get_user_reg(regs, 2);
> > + uint64_t handle = regpair_to_uint64(handle_hi, handle_lo);
> > + struct mem_frag_state *s;
> > + uint16_t sender_id = 0;
> > + int ret;
> > +
> > + spin_lock(&ctx->lock);
> > + s = find_frag_state(ctx, handle);
> > + if ( !s )
> > + {
> > + ret = FFA_RET_INVALID_PARAMETERS;
> > + goto out;
> > + }
> > + sender_id = s->shm->sender_id;
> > +
> > + if ( frag_len > s->buf_size )
> > + {
> > + ret = FFA_RET_INVALID_PARAMETERS;
> > + goto out_free_s;
> > + }
> > +
> > + ret = add_mem_share_frag(s, 0, frag_len);
> > + if ( ret < 0 )
> > + goto out_free_s;
> > +
> > + /* Note that share_shm() uses our tx buffer */
> > + spin_lock(&ffa_tx_buffer_lock);
> > + ret = share_shm(s->shm);
> > + spin_unlock(&ffa_tx_buffer_lock);
> > + if ( ret < 0 )
> > + goto out_free_s;
> > + list_add_tail(&s->shm->list, &ctx->shm_list);
> > +out_free_s:
> > + if ( ret < 0 )
> > + free_ffa_shm_mem(ctx, s->shm);
> > + list_del(&s->list);
> > + xfree(s);
> > +out:
> > + spin_unlock(&ctx->lock);
> > +
> > + if ( ret > 0 )
> > + set_regs_frag_rx(regs, handle_lo, handle_hi, ret, sender_id);
> > + else if ( ret == 0)
> > set_regs_success(regs, handle_lo, handle_hi);
> > else
> > set_regs_error(regs, ret);
> > @@ -1391,6 +1613,9 @@ static bool ffa_handle_call(struct cpu_user_regs
> > *regs)
> > else
> > set_regs_success(regs, 0, 0);
> > return true;
> > + case FFA_MEM_FRAG_TX:
> > + handle_mem_frag_tx(regs);
> > + return true;
> >
> > default:
> > gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -1432,6 +1657,7 @@ static int ffa_domain_init(struct domain *d)
> > }
> > ctx->create_signal_count = n;
> >
> > + INIT_LIST_HEAD(&ctx->frag_list);
> > INIT_LIST_HEAD(&ctx->shm_list);
> >
> > return 0;
> > @@ -1625,6 +1851,7 @@ static bool ffa_probe(void)
> > !check_mandatory_feature(FFA_MEM_SHARE_64) ||
> > !check_mandatory_feature(FFA_RXTX_UNMAP) ||
> > !check_mandatory_feature(FFA_MEM_SHARE_32) ||
> > + !check_mandatory_feature(FFA_MEM_FRAG_TX) ||
> > !check_mandatory_feature(FFA_MEM_RECLAIM) ||
> > !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> > return false;
> > --
> > 2.34.1
> >
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |