On 06.11.19 18:38, Jens Wiklander wrote:

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.commailto: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 noticehttps://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/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/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.commailto:michalis.pappas@opensynergy.com

w: www.opensynergy.comhttp://www.opensynergy.com

registered: Amtsgericht Charlottenburg, HRB 108616B

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

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/