Hi Volodymyr,
On 3/15/19 7:00 PM, Volodymyr Babchuk wrote:
- */
+#include <xen/device_tree.h> +#include <xen/sched.h>
+#include <asm/smccc.h> +#include <asm/tee/tee.h> +#include <asm/tee/optee_msg.h> +#include <asm/tee/optee_smc.h>
+#define OPTEE_ENABLED ((void*)0x1)
Please don't do that. Introduce a dummy structure instead and fill it up when needed.
It will be removed in the patch #5. But probably yes, I can create empty optee_domain structure in this patch.
I know it will be removed in patch #5. This is exactly why I don't want to see such things at the first place.
+/* Client ID 0 is reserved for hypervisor itself */
s/for/for the/
+#define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1)
+static bool optee_probe(void) +{
- struct dt_device_node *node;
- struct arm_smccc_res resp;
- /* Check for entry in dtb */
- node = dt_find_compatible_node(NULL, NULL, "linaro,optee-tz");
- if ( !node )
return false;- /* Check UID */
- arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
- if ( (uint32_t)resp.a0 != OPTEE_MSG_UID_0 ||
(uint32_t)resp.a1 != OPTEE_MSG_UID_1 ||(uint32_t)resp.a2 != OPTEE_MSG_UID_2 ||(uint32_t)resp.a3 != OPTEE_MSG_UID_3 )return false;- return true;
+}
+static int optee_domain_init(struct domain *d) +{
- struct arm_smccc_res resp;
- /*
* Inform OP-TEE about a new guest.* This is a "Fast" call in terms of OP-TEE. This basically* means that it can't be preempted, because there is no* thread allocated for it in OP-TEE. It is close to atomic* context in linux kernel: E.g. no blocking calls can be issued.This does not really make sense to describe Linux here. Can't you just make the wording OS agnostic?
It was just as an example. But okay, I'll remove mention of Linux.
Xen developer may not know the Linux internals.
* Also, interrupts are disabled.** a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()*/- arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
&resp);- if ( resp.a0 != OPTEE_SMC_RETURN_OK )
- {
gprintk(XENLOG_WARNING, "Unable to create OPTEE client: rc = 0x%X\n",gprintk will print the current vCPU and not the domain created. This is not very useful to know the current domain. So it would be better to use:
printk(XENLOG_G_WARNING, "%pd: Unable to create OPTEE client: rc ...", d);
Good point, thank you.
(uint32_t)resp.a0);return -ENODEV;- }
- d->arch.tee = OPTEE_ENABLED;
- return 0;
+}
+static void forward_call(struct cpu_user_regs *regs) +{
- struct arm_smccc_res resp;
- arm_smccc_smc(get_user_reg(regs, 0),
get_user_reg(regs, 1),get_user_reg(regs, 2),get_user_reg(regs, 3),get_user_reg(regs, 4),get_user_reg(regs, 5),get_user_reg(regs, 6),OPTEE_CLIENT_ID(current->domain),&resp);- set_user_reg(regs, 0, resp.a0);
- set_user_reg(regs, 1, resp.a1);
- set_user_reg(regs, 2, resp.a2);
- set_user_reg(regs, 3, resp.a3);
- set_user_reg(regs, 4, 0);
- set_user_reg(regs, 5, 0);
- set_user_reg(regs, 6, 0);
- set_user_reg(regs, 7, 0);
+}
+static int optee_relinquish_resources(struct domain *d) +{
- return 0;
+}
+static void optee_domain_destroy(struct domain *d) +{
- struct arm_smccc_res resp;
- if ( !d->arch.tee )
return;- /*
* Inform OP-TEE that domain is shutting down. This is* also a fast SMC call, like OPTEE_SMC_VM_CREATED, so* it is also non-preemptible.* At this time all domain VCPUs should be stopped. OP-TEE* relies on this.** a7 should be 0, so we can't skip last 6 parameters od arm_smccc_smc()NIT: s/od/of/
*/- arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
&resp);Your split between domain_destroy and relinquish_resources looks wrong. If you relinquish resources before telling OP-TEE then you are at risk that OP-TEE will use those resources.
Instead you should first tell OP-TEE the domain is shutting down, then release the resources.
Both this calls are supposed to be called after all guest's VCPUs are stopped, so OP-TEE will be not able to use any resources allocated to the domain, because OP-TEE is schedule by the Normal World. I assume, this is true for any other TEE.
I don't know enough OP-TEE in general to be able to say how it works. However, you can only be sure the buffers will not be used after VM_DESTROYED is called. Before, this may be the case, but this a pretty fragile assumption. If it does not hold, then you may end up to corrupt the hypervisor (or another guest).
It is not a position I would like to be in. The more that it should not require too much work to re-order the code.
Cheers,