Re: [Xen-devel] [PATCH v2 10/13] optee: add support for RPC commands

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi Julien,

On 18.09.18 19:50, Julien Grall wrote:
> Hi Volodymyr,
>
> On 09/11/2018 07:58 PM, Volodymyr Babchuk wrote:
> > On 11.09.18 16:56, Julien Grall wrote:
> > > On 10/09/18 19:14, Volodymyr Babchuk wrote:
> > > > On 10.09.18 18:34, Julien Grall wrote:
> > > > > On 03/09/18 17:54, Volodymyr Babchuk wrote:
> > > > 1. OP-TEE issues RPC "allocate buffer"
> > > > 2. NW returns list of pages
> > > > 3. Mediator translates and stores address in non_contig[x]
> > > > 4. OP-TEE begins to consume this list
> > > > 5. IRQ arrives and OP-TEE forced to break the work
> > > > 6. Mediator receives control back, but it should not free 
> > > > non_contig[x],
> > > >     because it is not sure of OP-TEE finished reading from it
> > > > 7. Xen/guest handles the IRQ and returns control back to OP-TEE
> > > > 8. OP-TEE finishes processing this buffers and asks for another one
> > > > 9. NW returns list of pages for the next buffer
> > > > 10. At this point mediator is sure that OP-TEE finished processing
> > > >      old non_contig[x], so it can free it and allocated another.
> > > Thank you for the description of the protocol. However, it is still 
> > > does not explain why you decided to free MAX_NONCONTIG_ENTRIES - 1. 
> > > Why not 0 or 1 or n?
> > Okay. You can pass up to 4 arguments for TA in command buffer. Any of 
> > that
> > argument can be a shared memory reference, so we need at least 4 items
> > in non_contig array. Sadly, this constant (TEE_NUM_PARAMS) is defined 
> > not optee_msg.h or optee_smc.h, but in tee_api_defines.h, which is 
> > user-space (or TA) part of OP-TEE.
[...]


> > Anyways, we need at least 4 items for arguments. But I defined 
> > MAX_NONCONTIG_ENTRIES as 5, because last item in array is used for 
> > RPC-allocated buffer. I also added comment. I'll copy it there:
> > /* Last entry in non_contig array is used to hold RPC-allocated  
> > buffer */
> > So, first 4 items are used for arguments and last one used for RPC 
> > requests. Right way to define MAX_NONCONTIG_ENTRIES is (TEE_NUM_PARAMS 
> > + 1), but then I should add another header file, which defines 
> > GlobalPlatform TEE core API.
> If I understand correctly your e-mail, a TEE call can never have more 
> than 4 parameters. Right? If so, should not your code mostly use 
> TEE_NUM_PARAMS and not MAX_NONCONTIG_ENTRIES?
It is more complex. GP TEE API defines that there is 4 parameters 
maximum can be passed to TA in single call. OP-TEE can add additional, 
so called "meta" parameters, for own needs. Also, there is OP-TEE calls 
that does not map to GP API calls. So, I need to review this
piece of code. Probably, I'll add some dynamic structure to handle
used lists of pages.

> > > Overall, it feels like to me you want to write more documentation 
> > > about how the mediator is supposed to work.
> > Looks like I need to start on how OP-TEE protocol is supposed to work...
> > I tried to cover this in the commit messages, but looks like it is not 
> > sufficient.
> I appreciate your effort to cover that in the commit message :). I tend 
> to update commit message over revision when there are misunderstanding 
> on the patch.
Thank you for the tip, this is good point. I'll extend commit messages 
to cover topics we discussed.
--
Volodymyr Babchuk

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