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.

Okay, I'll update this.

+#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.

Okay. I assume you mean something identical to optee_smccc_smc(), that fixes arg7 to OPTEE_SMC_VM_ID.

+
 /**
  * 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.

Yes, so one could use a different DTB, or get the bootloader to update the
conduit method in place, before booting the system into upgrade / recovery.

And as you mention above, there will be trouble if one sets the conduit method
to HVC when booting into compatibility (ie recovery / upgrade) mode, as there is
no hypervisor to handle the HVC on the other side. So the only option for the 
callback is to issue an SMC.

Notice that, we can only switch to the callback discussed earlier after we announce the guest.
All calls prior to that will use whatever method happens to be configured in the DTB. Moreover,
during the execution of optee_probe(), we don't know whether we are running with or without
a hypervisor.

These two combined, introduce the following complexities:

1. When booting into compatibility mode, we cannot explicitly handle the case of a misconfigured
   DTB (ie one that uses HVC).
2. When booting with a hypervisor, if the system is configured to use HVC, we cannot switch to
   our special callback.

Possible ways to mitigate these are:

- Make stripping OPTEE_SMC_SEC_CAP_VIRTUALIZATION a requirement of the OP-TEE mediator in the
  hypervisor. Then, we can use its presence to detect whether we are running on top of a hypervisor
  or not. The downside here is that this deprives the driver side from future use of the virtualization
  capability. In theory this should be okay, as guests should not be aware they are running in a
  virtualized environment anyway.

- Mandate the use of SMC when CONFIG_OPTEE_VIRT_COMPAT is set, whether a hypervisor is present or not.
  Clearly this imposes a hard requirement on the hypervisor's configuration, but personally I don't see
  why would anyone use HVC for this. Calls to the TEE are targeting the Secure Monitor, and as such they
  should be issuing an SMC. Hypervisors can always to trap them, if they wish to do so.

Clearly the most flexible way is to leave it as is, add some documentation, and let adopters debug any
misconfigurations.

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 notice<https://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/>

-- 
Michalis Pappas
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

p: +49 (30) 60 98 54 0 - 0
f: +49 (30) 60 98 54 0 - 99

e: michalis.pappas@opensynergy.com

w: www.opensynergy.com

registered: Amtsgericht Charlottenburg, HRB 108616B

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah