On 17/01/2019 21:01, Volodymyr Babchuk wrote:
Hi,
Hi Volodymyr,
Julien Grall writes:
HI,
On 1/17/19 7:48 PM, Volodymyr Babchuk wrote:
Julien Grall writes:
Hi Volodymyr,
On 18/12/2018 21:11, Volodymyr Babchuk wrote:
From: Volodymyr Babchuk vlad.babchuk@gmail.com
[...]
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.
In any case, the documentation in Xen should reflect the reality.
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?
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).
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.
Cheers,