Re: [Xen-devel] [PATCH v2 09/13] optee: add support for arbitrary shared memory

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 10/09/18 19:04, Volodymyr Babchuk wrote:
> On 10.09.18 17:02, Julien Grall wrote:
> > On 03/09/18 17:54, Volodymyr Babchuk wrote:
> > > +    struct {
> > > +        uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE];
> > > +        uint64_t next_page_data;
> > > +    } *pages_data_guest, *pages_data_xen, *pages_data_xen_start;
> > > +    struct shm_buf *shm_buf;
> > > +
> > > +    page_offset = param->u.tmem.buf_ptr & 
> > > (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
> > > +
> > > +    size = ROUNDUP(param->u.tmem.size + page_offset,
> > > +                   OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> > > +
> > > +    num_pages = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> > > +
> > > +    order = get_order_from_bytes(get_pages_list_size(num_pages));
> > > +
> > > +    pages_data_xen_start = alloc_xenheap_pages(order, 0);
> > This could be replaced by a _xmalloc and would avoid to allocate more 
> > memory than necessary when the order is getting bigger.
> Thanks. Would it allocate page-aligned buffer?  This is crucial in this 
> case. I can't find any documentation on it so I don't know which 
> alignment it guarantees.
_xmalloc takes in argument the alignment required for the allocation.

> > > +    if ( !pages_data_xen_start )
> > > +        return false;
> > > +
> > > +    shm_buf = allocate_shm_buf(ctx, param->u.tmem.shm_ref, num_pages);
> > In alocate_shm_buf you are now globally limiting the number of pages ( 
> > (16384) to pin. However, this does not limit per call.
> > With the current limit, you would could call up to 16384 times 
> > lookup_and_pin_guest_ram_addr(...). On Arm, for p2m related operation, 
> > we limit to 512 iterations in one go before checking the preemption.
> > So I think 16384 times is far too much.
> So, in other words, I can translate only 2MB buffer (if 4096KB pages are 
> used), is it right?
2MB for the whole command. So if you have 5 buffer in the command, then 
the sum of the buffer should not be bigger than 2MB.
However, 2MB might be too big considering that you also need to account 
the SMC call. Does buffer can be passed for fast call?
> I think, it will be okay to implement such limitation for this initial
> version of mediator. In the future, it would be possible to do RPC 
> return from XEN (as OP-TEE does) to finish this request later.
> > > +    if ( !shm_buf )
> > > +        goto err_free;
> > > +
> > > +    gaddr = param->u.tmem.buf_ptr & ~(OPTEE_MSG_NONCONTIG_PAGE_SIZE 
> > > - 1);
> > > +    guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL);
> > > +    if ( mfn_eq(guest_mfn, INVALID_MFN) )
> > > +        goto err_free;
> > > +
> > > +    pages_data_guest = map_domain_page(guest_mfn);
> > > +    if ( !pages_data_guest )
> > > +        goto err_free;
> > > +
> > > +    pages_data_xen = pages_data_xen_start;
> > > +    while ( num_pages ) {
> > > +        struct page_info *page;
> > > +        mfn_t entry_mfn = lookup_and_pin_guest_ram_addr(
> > > +            pages_data_guest->pages_list[entries_on_page], &page);
> > > +
> > > +        if ( mfn_eq(entry_mfn, INVALID_MFN) )
> > > +            goto err_unmap;
> > > +
> > > +        shm_buf->pages[shm_buf->page_cnt++] = page;
> > > +        pages_data_xen->pages_list[entries_on_page] = 
> > > mfn_to_maddr(entry_mfn);
> > > +        entries_on_page++;
> > > +
> > > +        if ( entries_on_page == PAGELIST_ENTRIES_PER_PAGE ) {
> > > +            pages_data_xen->next_page_data = 
> > > virt_to_maddr(pages_data_xen + 1);
> > > +            pages_data_xen++;
> > > +            gaddr = pages_data_guest->next_page_data;
> > next_page_data is not a guest address but a machine address. For 
> > anything related to address, the variable should be named accordingly 
> > to avoid confusion.
> Why? In this case it is IPA that comes from the guest.
Because I misread the variables.

> > > +
> > > +    unmap_domain_page(pages_data_guest);
> > > +    unpin_guest_ram_addr(guest_mfn);
> > > +    return true;
> > > +
> > > +err_unmap:
> > > +    unmap_domain_page(pages_data_guest);
> > > +    unpin_guest_ram_addr(guest_mfn);
> > > +    free_shm_buf(ctx, shm_buf->cookie);
> > > +
> > > +err_free:
> > > +    free_xenheap_pages(pages_data_xen_start, order);
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +static bool translate_params(struct domain_ctx *ctx,
> > > +                             struct std_call_ctx *call)
> > > +{
> > > +    unsigned int i;
> > > +    uint32_t attr;
> > > +
> > > +    for ( i = 0; i < call->xen_arg->num_params; i++ ) {
> > Please pay attention to Xen coding style. I haven't pointed out 
> > everywhere, but I would all of them to be fixed in the next version.
> Yes, I'm sorry for that. I simultaneously work  with different projects
> and sometimes it is hard to track coding style. I'll fix all such
> problems.
IIRC your team is working on the checkpatch. It might be worth using it 
to see how it performs on your series.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel