Re: [Tee-dev] [PATCH 2/2] tee: optee: allow native OS to interact with virt-enabled OP-TEE

Hi Michalis,

On Tue, Oct 29, 2019 at 11:04:03AM +0100, Michalis Pappas wrote:

This patch introduces a new configuration parameter that allows the optee driver to announce itself as a guest (CONFIG_OPTEE_VIRT_COMPAT) and tag subsequent requests with a predefined vm id. A second configuration option (CONFIG_OPTEE_VIRT_COMPAT_VM_ID) allows specifying the vm id the driver will use.

This feature is useful for setups where a privileged guest needs to interact with OP-TEE OS, without the presence of a hypervisor (eg system recovery).

Enabling this option has no impact on OP-TEE OS builds that are configured without virtualization support.

Signed-off-by: Michalis Pappas mpp@opensynergy.com

drivers/tee/optee/Kconfig | 23 +++++++++++++++++++++ drivers/tee/optee/core.c | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+)

diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig index 4728b591afd1..031dbe0ff467 100644 --- a/drivers/tee/optee/Kconfig +++ b/drivers/tee/optee/Kconfig @@ -20,3 +20,26 @@ config OPTEE_BENCHMARK help This enables benchmarking feature in the OP-TEE Trusted Execution Environment (TEE) driver.

+config OPTEE_VIRT_COMPAT

•   bool "Virtualization compatibility"
  

•   default n
  

•   depends on OPTEE
  

•   help
  

•     Makes the optee driver to announce itself as a guest, and tag
  

•     requests with a predefined vm id.
  

•     This task is normally performed by the hypervisor. Enabling this
  

•     option allows interoperability between a system running natively
  

•     (ie without a hypervisor), and a virtualization-enabled build of
  

•     OP-TEE OS.
  

•     If unsure, say N.
  

+config OPTEE_VIRT_COMPAT_VM_ID

•   int "Virtualization Compatibility VM ID"
  

•   default 1
  

•   depends on OPTEE_VIRT_COMPAT
  

•   help
  

•     This sets the value of vm id that the driver will use when presenting
  

•     itself as a guest to a virtualization-enabled build of OP-TEE OS.
  

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 7c6de0bc3823..2237e35f49af 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -33,8 +33,15 @@

#define DRIVER_NAME "optee"

+#define OPTEE_SMC_HV_ID 0x00

0 will do.

+#define OPTEE_SMC_VM_ID CONFIG_OPTEE_VIRT_COMPAT_VM_ID

#define OPTEE_SHM_NUM_PRIV_PAGES CONFIG_OPTEE_SHM_NUM_PRIV_PAGES

+#if defined(CONFIG_OPTEE_VIRT_COMPAT)

•   static unsigned long smc_vm_id = OPTEE_SMC_HV_ID;
  

+#endif

I'd rather not have this as a global variable. I understand that it will make this patch a bit more complicated. Perhaps optee->invoke_fn can be assigned a special callback function instead.

/**

• optee_from_msg_param() - convert from OPTEE_MSG parameters to

•                     struct tee_param
  

@@ -532,6 +539,9 @@ static void optee_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a6, unsigned long a7, struct arm_smccc_res *res) { +#if defined(CONFIG_OPTEE_VIRT_COMPAT)

•   a7 = smc_vm_id;
  

+#endif arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res); }

@@ -541,9 +551,30 @@ static void optee_smccc_hvc(unsigned long a0, unsigned long a1, unsigned long a6, unsigned long a7, struct arm_smccc_res *res) { +#if defined(CONFIG_OPTEE_VIRT_COMPAT)

•   a7 = smc_vm_id;
  

+#endif arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res); }

+#if defined(CONFIG_OPTEE_VIRT_COMPAT) +static void announce_guest_created(optee_invoke_fn *invoke_fn) +{

•   struct arm_smccc_res res;
  

•   invoke_fn(OPTEE_SMC_VM_CREATED, OPTEE_SMC_VM_ID, 0, 0, 0, 0, 0,
  

•             OPTEE_SMC_HV_ID, &res);
  

+}

+static void announce_guest_destroyed(optee_invoke_fn *invoke_fn) +{

•   struct arm_smccc_res res;
  

•   invoke_fn(OPTEE_SMC_VM_DESTROYED, OPTEE_SMC_VM_ID, 0, 0, 0, 0, 0,
  

•             OPTEE_SMC_HV_ID, &res);
  

+} +#endif

static optee_invoke_fn *get_invoke_func(struct device_node *np) { const char *method; @@ -648,6 +679,15 @@ static struct optee *optee_probe(struct device_node *np) optee->memremaped_shm = memremaped_shm; optee->pool = pool;

+#if defined(CONFIG_OPTEE_VIRT_COMPAT)

•   if (sec_caps & OPTEE_SMC_SEC_CAP_VIRTUALIZATION) {
  

•           pr_info("virtualization compatibility is enabled\n");
  

•           announce_guest_created(invoke_fn);
  

•           /* All requests from now on should be tagged with vm id */
  

•           smc_vm_id = OPTEE_SMC_VM_ID;
  

•   }
  

+#endif

How is DTB handled in the recovery case? A special recovery DTB? If not there could be trouble with "method" set to "hvc". I think we need a comment in the code explaining how this should work too.

Cheers, Jens

    optee_enable_shm_cache(optee);

    if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)

@@ -686,6 +726,9 @@ static void optee_remove(struct optee *optee) */ optee_disable_shm_cache(optee);

+#if defined(CONFIG_OPTEE_VIRT_COMPAT)

•   announce_guest_destroyed(optee->invoke_fn);
  

+#endif /* * The two devices has to be unregistered before we can free the * other resources. -- 2.17.1

Please mind our privacy noticehttps://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/ pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/