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,