Re: [Xen-devel] [PATCH v3 07/11] optee: add support for RPC SHM buffers

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall writes:

[...]
>>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>>> index dc90e2ed8e..771148e940 100644
>>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>>> @@ -30,6 +30,12 @@
>>>>>>      * OP-TEE spawns a thread for every standard call.
>>>>>>      */
>>>>>>     #define MAX_STD_CALLS   16
>>>>>> +/*
>>>>>> + * Maximal number of pre-allocated SHM buffers. OP-TEE generally asks
>>>>>> + * for one SHM buffer per thread, so this also corresponds to OP-TEE
>>>>>> + * option CFG_NUM_THREADS
>>>>>> + */
>>>>>
>>>>> Same as patch #6 regarding CFG_NUM_THREADS.
>>>> Right now OP-TEE will not allocate more than one buffer per OP-TEE
>>>> thread. And I can see no reason why it would change. So, basically I can
>>>> remove this MAX_RPC_SHMS at all and use MAX_STD_CALLS instead. But then
>>>> it will be not so obvious, why I compare number of SHM buffers with
>>>> number of std calls. Thus, I think it is good to have separate
>>>> define and comment.
>>>
>>> I am not against have the 2 defines, what I was pointed out with the
>>> documentation on top is incorrect as patch #6.
>> I'm sorry, I don't quite understand. In patch #6 your concern was that
>> CFG_NUM_THREADS depends on platform, right?
>
> My first concern was the documentation does not reflect the reality
> because CFG_NUM_THREADS is not always equal to 16.

> Ideally we should be able to know the number of threads supported. But
> that could be a follow-up patch (or potentially ignored) if nothing
> bad can happen when Xen handles more thread than OP-TEE does.
No, it is valid situation. Opposite (when OP-TEE can handle more threads
than Xen) also can be handled in a right way.

> In any case, the documentation in Xen should reflect the reality.
Ah, okay, got it.

>>
>>> If you happen to make MAX_STD_CALLS dynamic, then this should also be
>>> dynamic.
>> Of course.
>>
>> [...]
>>>>>>     +static void handle_rpc_func_alloc(struct optee_domain *ctx,
>>>>>> +                                  struct cpu_user_regs *regs)
>>>>>> +{
>>>>>> +    paddr_t ptr = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);
>>>>>> +
>>>>>> +    if ( ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) )
>>>>>> +        gprintk(XENLOG_WARNING, "Domain returned invalid RPC command 
>>>>>> buffer\n");
>>>>>
>>>>> Should not you bail-out in that case? Also, I would turn it to a gdprintk.
>>>> OP-TEE does own checks and that check will fail also. Then OP-TEE will
>>>> issue request to free this SHM.
>>>
>>> I think it is better if we go on the safe-side. I.e if we know there
>>> would be an error (like here), then you need to return an error in
>>> from Xen rather than calling OP-TEE. Otherwise, you may end up to
>>> nasty problem.
>> Actually, I don't see how I can do this. This is an RPC response. I
>> can't return error to RPC response. All I can do is to mangle RPC
>> response in a such way, that OP-TEE surely will treat it as an error and
>> act accordingly.
>
> I am not sure to understand what you mean here. Surely if the address
> is not aligned, then OP-TEE will return an error too, right? So can't
> you emulate OP-TEE behavior in that case?
No, OP-TEE will not return an error. OP-TEE will handle it as an error.
For OP-TEE any RPC looks like a function call. So it excepts some return
value - buffer pointer in this case. If it gets invalid pointer, it
issues another RPC to free this buffer and then propagates error back to
caller.

So, if I'll return error back to the guest (or rather issue RPC to free
the buffer), OP-TEE will be still blocked, waiting for a pointer from
the guest. Of course I can go further end emulate a new buffer
allocation RPC but from Xen side in a hope that guest will provide valid
buffer this time. But, what if not? And why I should decide instead of
OP-TEE?

>
>>
>>> Also, I think you want a comment (maybe in the commit message)
>>> explaining that OPTEE_MSG_NONCONTIG_PAGE_SIZE will always be equal to
>>> PAGE_SIZE.
>> It is always equal to 4096 bytes. But, AFAIK, XEN can work with other
>> PAGE_SIZEs, isn't? Actually, linux driver support other page sizes, it
>> just splits those pages into 4k chunks. The same can be done in XEN, but
>> I don't want to introduce this logic right now. The patches are complex
>> enough. Right now we have
>> BUILD_BUG_ON(PAGE_SIZE != OPTEE_MSG_NONCONTIG_PAGE_SIZE) and this is
>> enough, I hope.
>
> Xen only support 4KB page, but I wouldn't bet that in the future on
> Arm as we will require 64KB page for some features (i.e 52-bit
> support).
So, I was mistaken. For some reason I though that Xen can handle other
page sizes as well.

> I wasn't asking you to handle a different PAGE_SIZE, just to clarify
> in a comment on top that you expect the both defined to be
> valid. Another BUILD_BUG_ON is not necessary assuming you add the one
> in patch #6.
Okay, I'll add such comment.


--
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel