[Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
This is follow-up to our conversation during community call. You asked me to send OP-TEE mediator as a patch, so we can discuss it in the mailing list. So, there it is. I squashed two patches into one and skipped patches that we already discussed. So, this is basically all what is needed to support OP-TEE in XEN. There are some TODOs left all over the code. But, I don't expect that TODOs implementation would significantly increase codebase. Currently mediator parses requests to perform addresses translation and that's all what is should be done to allow guests to work with OP-TEE. This become possible because I completely revisited virtualization support in OP-TEE. I have found way to enforce complete isolation between different guest states. This lifts many questions like usage quotas, RPC routing, sudden guest death, data isolation, etc. I'm aware that I didn't addressed all comments from previous discussion. Sorry for this. I'm currently busy with OP-TEE, and I think proper mediator implementation will be possible only after I'll stabilize OP-TEE part. So I don't ask anybody to do thorough review. I just want to share my status and discuss this code a whole. --- Add OP-TEE mediator as an example how TEE mediator framework works. OP-TEE mediator support address translation for DomUs. It tracks execution of STD calls, correctly handles memory-related RPC requests, tracks buffer allocated for RPCs. With this patch OP-TEE successfully passes own tests, while client is running in DomU. Currently it lacks some code for exceptional cases, because this patch was used mostly to debug virtualization in OP-TEE. Nevertheless, it provides all features needed for OP-TEE mediation. WARNING: This is a development patch, it does not cover all corner cases, so, please don't use it in production. It was tested on RCAR Salvator-M3 board. Signed-off-by: Volodymyr Babchuk --- xen/arch/arm/tee/Kconfig | 4 + xen/arch/arm/tee/Makefile| 1 + xen/arch/arm/tee/optee.c | 765 +++ xen/arch/arm/tee/optee_smc.h | 50 +++ 4 files changed, 820 insertions(+) create mode 100644 xen/arch/arm/tee/optee.c diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig index e69de29..7c6b5c6 100644 --- a/xen/arch/arm/tee/Kconfig +++ b/xen/arch/arm/tee/Kconfig @@ -0,0 +1,4 @@ +config ARM_OPTEE + bool "Enable OP-TEE mediator" + default n + depends on ARM_TEE diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile index c54d479..9d93b42 100644 --- a/xen/arch/arm/tee/Makefile +++ b/xen/arch/arm/tee/Makefile @@ -1 +1,2 @@ obj-y += tee.o +obj-$(CONFIG_ARM_OPTEE) += optee.o diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c new file mode 100644 index 000..59c3600 --- /dev/null +++ b/xen/arch/arm/tee/optee.c @@ -0,0 +1,765 @@ +/* + * xen/arch/arm/tee/optee.c + * + * OP-TEE mediator + * + * Volodymyr Babchuk + * Copyright (c) 2017 EPAM Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include + +#include +#include + +#include "optee_msg.h" +#include "optee_smc.h" + +/* + * Global TODO: + * 1. Create per-domain context, where call and shm will be stored + * 2. Pin pages shared between OP-TEE and guest + */ +/* + * OP-TEE violates SMCCC when it defines own UID. So we need + * to place bytes in correct order. + */ +#define OPTEE_UID (xen_uuid_t){{ \ +(uint8_t)(OPTEE_MSG_UID_0 >> 0), (uint8_t)(OPTEE_MSG_UID_0 >> 8), \ +(uint8_t)(OPTEE_MSG_UID_0 >> 16), (uint8_t)(OPTEE_MSG_UID_0 >> 24), \ +(uint8_t)(OPTEE_MSG_UID_1 >> 0), (uint8_t)(OPTEE_MSG_UID_1 >> 8), \ +(uint8_t)(OPTEE_MSG_UID_1 >> 16), (uint8_t)(OPTEE_MSG_UID_1 >> 24), \ +(uint8_t)(OPTEE_MSG_UID_2 >> 0), (uint8_t)(OPTEE_MSG_UID_2 >> 8), \ +(uint8_t)(OPTEE_MSG_UID_2 >> 16), (uint8_t)(OPTEE_MSG_UID_2 >> 24), \ +(uint8_t)(OPTEE_MSG_UID_3 >> 0), (uint8_t)(OPTEE_MSG_UID_3 >> 8), \ +(uint8_t)(OPTEE_MSG_UID_3 >> 16), (uint8_t)(OPTEE_MSG_UID_3 >> 24), \ +}} + +#define MAX_NONCONTIG_ENTRIES 8 + +/* + * Call context. OP-TEE can issue mulitple RPC returns during one call. + * We need to preserve context during them. + */ +struct std_call_ctx { +struct list_head list; +struct optee_msg_arg *guest_arg; +struct optee_msg
Re: [Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
Hi Stefano, On Fri, Dec 01, 2017 at 02:58:57PM -0800, Stefano Stabellini wrote: > On Mon, 27 Nov 2017, Volodymyr Babchuk wrote: > > This is follow-up to our conversation during community call. > > You asked me to send OP-TEE mediator as a patch, so we can > > discuss it in the mailing list. So, there it is. I squashed > > two patches into one and skipped patches that we already > > discussed. > > > > So, this is basically all what is needed to support OP-TEE in XEN. > > There are some TODOs left all over the code. But, I don't > > expect that TODOs implementation would significantly > > increase codebase. Currently mediator parses requests to perform > > addresses translation and that's all what is should be done > > to allow guests to work with OP-TEE. > > > > This become possible because I completely revisited virtualization > > support in OP-TEE. I have found way to enforce complete isolation > > between different guest states. This lifts many questions like usage > > quotas, RPC routing, sudden guest death, data isolation, etc. > > > > I'm aware that I didn't addressed all comments from previous > > discussion. Sorry for this. I'm currently busy with OP-TEE, > > and I think proper mediator implementation will be possible > > only after I'll stabilize OP-TEE part. > > > > So I don't ask anybody to do thorough review. I just want to > > share my status and discuss this code a whole. > > Thank you for sharing! Actually, I think it is not too bad as a starting > point. > > I'll also try to summarize some key concept we have been discussing > about OP-TEE support in Xen. > > > = Xen cannot protect the system from a broken/insecure OP-TEE = > > OP-TEE runs at a higher privilege level than Xen, thus, we can't really > expect Xen to protect the system from a broken OP-TEE. Also, Xen cannot > really protect OP-TEE from a malicious caller. Yes, this is right. > What we can and should do is to protect Xen, the OP-TEE mediator in Xen > specifically, from malicious attackers. > > In other words, we are not responsible if a call, forwarded to OP-TEE by > Xen, ends up crashing OP-TEE, therefore taking down the system. > > However, we have to pay special care to avoid callers to crash or take > over the mediator in Xen. We also have to pay attention so that a caller > won't be able to exhaust Xen resources or DOS Xen (allocate too much > memory, infinite loops in Xen, etc). This brings me to the next topic. Yes, I see where are you going. > > = Error checking / DOS protection = > > We need powerful checks on arguments passed by the caller and evaluated > by the mediator. > > For example, we cannot expect the guest to actually pass arguments in > the format expected by translate_params. ctx->xen_arg could be > gibberish. Yes. The same arguments stands also for OP-TEE itself. OP-TEE checks validity of arguments and mediator should do the same. Actaully, I implemented this checks in mediator. > From the resource allocation point of view, it looks like every > handle_std_call allocates a new context; every copy_std_request > allocates a new Xen page. It would be easy to exhaust Xen resources. > Maybe we need a max concurrent request limit or max page allocation per > domain or something of the kind. This is a very good point. Thanks. Yes, it is currently missing. Is there any mechanism in XEN to provide quotas? I think, this mediator is not the single entity that allocates memory to handle guest calls? Also, this problem is somewhat handled from OP-TEE site: it have limited number of threads, so it can't handle many STD call simultaneously. But I wouldn't rely on OP-TEE there, of course. > > > = Locks and Lists = > > The current lock and list is global. Do you think it should actually be > global or per-domain? I do realize that only dom0 is allowed to make > calls now so the question for the moment is not too useful. Agree lists (and corresponding locks) should be domain-bound. This is in my todo list, which is included in this patch. > > = Xen command forwarding = > > In the code below, it looks like Xen is forwarding everything to OP-TEE. > Are there some commands Xen should avoid forwarding? Should we have a > whitelist or a blacklist? My code implements whitelists (at least, I hope so :-) ). It forwards only known requests. If it does not know type of the request, it returns error back to a caller. > > = Long running OP-TEE commands and interruptions = > > I have been told that some OP-TEE RPC commands might take long to > complete. Is that right? Like for example one of the > OPTEE_MSG_RPC_CMD_*? > > I
Re: [Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
Hi Julien, On Mon, Dec 04, 2017 at 02:30:32PM +, Julien Grall wrote: > Hi, > > I am going to answer both e-mails (Stefano and Volodymyr) at once. > > On 01/12/17 22:58, Stefano Stabellini wrote: > >On Mon, 27 Nov 2017, Volodymyr Babchuk wrote: > >>This is follow-up to our conversation during community call. > >>You asked me to send OP-TEE mediator as a patch, so we can > >>discuss it in the mailing list. So, there it is. I squashed > >>two patches into one and skipped patches that we already > >>discussed. > >> > >>So, this is basically all what is needed to support OP-TEE in XEN. > >>There are some TODOs left all over the code. But, I don't > >>expect that TODOs implementation would significantly > >>increase codebase. Currently mediator parses requests to perform > >>addresses translation and that's all what is should be done > >>to allow guests to work with OP-TEE. > >> > >>This become possible because I completely revisited virtualization > >>support in OP-TEE. I have found way to enforce complete isolation > >>between different guest states. This lifts many questions like usage > >>quotas, RPC routing, sudden guest death, data isolation, etc. > > I disagree here. You still have to handle sudden guest death in Xen and > release any memory allocated in the hypervisor for that guests. I was speaking from OP-TEE point-of-view. From mediator side, there is callback optee_domain_destroy() with comment "TODO: Clean call contexts and SHMs associated with domain". This callback will be called during domain description and it will free any resources that mediator allocated on behalf of the guest. > >> > >>I'm aware that I didn't addressed all comments from previous > >>discussion. Sorry for this. I'm currently busy with OP-TEE, > >>and I think proper mediator implementation will be possible > >>only after I'll stabilize OP-TEE part. > >> > >>So I don't ask anybody to do thorough review. I just want to > >>share my status and discuss this code a whole. > > > >Thank you for sharing! Actually, I think it is not too bad as a starting > >point. > > > >I'll also try to summarize some key concept we have been discussing > >about OP-TEE support in Xen. > > > > > >= Xen cannot protect the system from a broken/insecure OP-TEE = > > > >OP-TEE runs at a higher privilege level than Xen, thus, we can't really > >expect Xen to protect the system from a broken OP-TEE. Also, Xen cannot > >really protect OP-TEE from a malicious caller. > > > >What we can and should do is to protect Xen, the OP-TEE mediator in Xen > >specifically, from malicious attackers. > > > >In other words, we are not responsible if a call, forwarded to OP-TEE by > >Xen, ends up crashing OP-TEE, therefore taking down the system. > > > >However, we have to pay special care to avoid callers to crash or take > >over the mediator in Xen. We also have to pay attention so that a caller > >won't be able to exhaust Xen resources or DOS Xen (allocate too much > >memory, infinite loops in Xen, etc). This brings me to the next topic. > > > > > >= Error checking / DOS protection = > > > >We need powerful checks on arguments passed by the caller and evaluated > >by the mediator. > > > >For example, we cannot expect the guest to actually pass arguments in > >the format expected by translate_params. ctx->xen_arg could be > >gibberish. > > > > From the resource allocation point of view, it looks like every > >handle_std_call allocates a new context; every copy_std_request > >allocates a new Xen page. It would be easy to exhaust Xen resources. > >Maybe we need a max concurrent request limit or max page allocation per > >domain or something of the kind. > > > > > >= Locks and Lists = > > > >The current lock and list is global. Do you think it should actually be > >global or per-domain? I do realize that only dom0 is allowed to make > >calls now so the question for the moment is not too useful. > > Hmmm, the commit message says: "With this patch OP-TEE successfully passes > own tests, while client is running in DomU.". As said above, we need to make > sure that Xen will release memory allocated for SMC requests when a domain > dies. So have a list/lock per domain would make more sense (easier to go > through). You are totaly right. I can't say why I did in a way I did :-) I think, it was easier approach during initial implementation. But I cer
Re: [Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
Hi Julien, On Mon, Dec 04, 2017 at 04:27:14PM +, Julien Grall wrote: [...] > >>= Error checking / DOS protection = > >> > >>We need powerful checks on arguments passed by the caller and evaluated > >>by the mediator. > >> > >>For example, we cannot expect the guest to actually pass arguments in > >>the format expected by translate_params. ctx->xen_arg could be > >>gibberish. > >Yes. The same arguments stands also for OP-TEE itself. OP-TEE checks > >validity of arguments and mediator should do the same. Actaully, I > >implemented this checks in mediator. > > > >> From the resource allocation point of view, it looks like every > >>handle_std_call allocates a new context; every copy_std_request > >>allocates a new Xen page. It would be easy to exhaust Xen resources. > >>Maybe we need a max concurrent request limit or max page allocation per > >>domain or something of the kind. > >This is a very good point. Thanks. Yes, it is currently missing. > >Is there any mechanism in XEN to provide quotas? I think, this mediator > >is not the single entity that allocates memory to handle guest calls? > > Most of the time, the memory is either accounted to the guest or only a > small amount of memory is allocated for a known period of time (the time of > an hypercall for instance). Aha, so in my case, I will need to implement own quota mechanism. I think something like "max_pages", initialized with value from xenpolicy will be fine. What do you think? > > > >Also, this problem is somewhat handled from OP-TEE site: it have limited > >number of threads, so it can't handle many STD call simultaneously. But > >I wouldn't rely on OP-TEE there, of course. > > Does it mean OP-TEE will deny the call if it can't handle it? Or will it be > put on hold? OP-TEE will return OPTEE_SMC_RETURN_ETHREAD_LIMIT. Current behavior of optee driver is to wait on a wait queue until another thread will complete its call. So, from OP-TEE OS side - it is a call denial. But from OP-TEE client point of view this is a hold, thanks to mentioned behavior of driver. > [..] > > >> > >>= Page pinning = > >> > >>Guest pages passed to OP-TEE need to be pinned (otherwise Xen doesn't > >>guarantee they'll be available). I think the right function to call for > >>that is get_page_from_gfn or get_page. > >Yes, I need to pin pages. I have this in my TODO list. Question is how > >to do this in a proper way. Julien has objections against get_page() > >as I can see. > > If you saw my objection about get_page(), you also saw my suggestions on how > to do proper pinning in Xen. Yes, I'm sorry, I missed second part of your email. -- Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
Hi Stefano, On 05.12.17 00:06, Stefano Stabellini wrote: On Mon, 4 Dec 2017, Volodymyr Babchuk wrote: = Xen command forwarding = In the code below, it looks like Xen is forwarding everything to OP-TEE. Are there some commands Xen should avoid forwarding? Should we have a whitelist or a blacklist? My code implements whitelists (at least, I hope so :-) ). It forwards only known requests. If it does not know type of the request, it returns error back to a caller. Actually, see below: +static bool optee_handle_smc(struct cpu_user_regs *regs) +{ + +switch ( get_user_reg(regs, 0) ) +{ +case OPTEE_SMC_GET_SHM_CONFIG: +return handle_get_shm_config(regs); +case OPTEE_SMC_EXCHANGE_CAPABILITIES: +return handle_exchange_capabilities(regs); +case OPTEE_SMC_CALL_WITH_ARG: +return handle_std_call(regs); +case OPTEE_SMC_CALL_RETURN_FROM_RPC: +return handle_rpc(regs); +default: +return forward_call(regs); +} +return false; +} In the unknown ("default") case the smc is still forwarded. Am I missing anything? No, you are right. It is I who missed to complete this piece of code. There should be a list of all known OPTEE_SMC_* commands, plus "return false" in "default" case. WBR, -- Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 16/31] arm: add SMC wrapper that is compatible with SMCCC
Hi Stefano, On Mon, Dec 04, 2017 at 06:30:13PM -0800, Stefano Stabellini wrote: > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote: > > From: Volodymyr Babchuk > > > > Existing SMC wrapper call_smc() allows only 4 parameters and > > returns only one value. This is enough for existing > > use in PSCI code, but TEE mediator will need a call that is > > fully compatible with ARM SMCCC. > > This patch adds this call for both arm32 and arm64. > > > > There was similar patch by Edgar E. Iglesias ([1]), but looks > > like it is abandoned. > > > > [1] > > https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg00636.html > > > > CC: "Edgar E. Iglesias" > > > > Signed-off-by: Volodymyr Babchuk > > CC: Stefano Stabellini > > CC: Julien Grall > > --- > > xen/arch/arm/arm32/Makefile | 1 + > > xen/arch/arm/arm32/smc.S| 32 > > xen/arch/arm/arm64/Makefile | 1 + > > xen/arch/arm/arm64/smc.S| 29 + > > xen/include/asm-arm/processor.h | 4 > > 5 files changed, 67 insertions(+) > > create mode 100644 xen/arch/arm/arm32/smc.S > > create mode 100644 xen/arch/arm/arm64/smc.S > > > > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile > > index 0ac254f..a2362f3 100644 > > --- a/xen/arch/arm/arm32/Makefile > > +++ b/xen/arch/arm/arm32/Makefile > > @@ -8,6 +8,7 @@ obj-y += insn.o > > obj-$(CONFIG_LIVEPATCH) += livepatch.o > > obj-y += proc-v7.o proc-caxx.o > > obj-y += smpboot.o > > +obj-y += smc.o > > obj-y += traps.o > > obj-y += vfp.o > > > > diff --git a/xen/arch/arm/arm32/smc.S b/xen/arch/arm/arm32/smc.S > > new file mode 100644 > > index 000..1cc9528 > > --- /dev/null > > +++ b/xen/arch/arm/arm32/smc.S > > @@ -0,0 +1,32 @@ > > +/* > > + * xen/arch/arm/arm32/smc.S > > + * > > + * Wrapper for Secure Monitors Calls > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include > > + > > +/* > > + * void call_smccc_smc(register_t a0, register_t a1, register_t a2, > > + * register_t a3, register_t a4, register_t a5, > > + * register_t a6, register_t a7, register_t res[4]) > > + */ > > +ENTRY(call_smccc_smc) > > +mov r12, sp > > +push{r4-r7} > > +ldm r12, {r4-r7} > > +smc #0 > > +pop {r4-r7} > > +ldr r12, [sp, #(4 * 4)] > > I haven't run this, but shouldn't it be: > > ldr r12, [sp, #20] > > ? > I took this code from linux (arch/arm/kernel/arm-smccc.h). But, why #20? There are 5 parameters on the stack: a4-a7 and res: a4: [sp] a5: [sp, #4] a6: [sp, #8] a7: [sp, #12] res: [sp, #16] We need to save returnred values to res. So it looks right. Unless I'm terribly wrong :) > > +stm r12, {r0-r3} > > +bx lr > > diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile > > index 149b6b3..7831dc1 100644 > > --- a/xen/arch/arm/arm64/Makefile > > +++ b/xen/arch/arm/arm64/Makefile > > @@ -8,5 +8,6 @@ obj-y += entry.o > > obj-y += insn.o > > obj-$(CONFIG_LIVEPATCH) += livepatch.o > > obj-y += smpboot.o > > +obj-y += smc.o > > obj-y += traps.o > > obj-y += vfp.o > > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S > > new file mode 100644 > > index 000..aa44fba > > --- /dev/null > > +++ b/xen/arch/arm/arm64/smc.S > > @@ -0,0 +1,29 @@ > > +/* > > + * xen/arch/arm/arm64/smc.S > > + * > > + * Wrapper for Secure Monitors Calls > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > +
Re: [Xen-devel] [RFC PATCH 16/31] arm: add SMC wrapper that is compatible with SMCCC
Hi Julien, On 05.12.17 16:58, Julien Grall wrote: Hi Oleksandr, On 09/11/17 17:10, Oleksandr Tyshchenko wrote: From: Volodymyr Babchuk Existing SMC wrapper call_smc() allows only 4 parameters and returns only one value. This is enough for existing use in PSCI code, but TEE mediator will need a call that is fully compatible with ARM SMCCC. This patch adds this call for both arm32 and arm64. There was similar patch by Edgar E. Iglesias ([1]), but looks like it is abandoned. [1] https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg00636.html CC: "Edgar E. Iglesias" Signed-off-by: Volodymyr Babchuk CC: Stefano Stabellini CC: Julien Grall This patch was sent by Volodymyr a month ago (see [2]) and I had comments on it. I would appreciate if you address them. I can address your comments and send it as a separate patch to the ML. Will it be fine? WBR -- Volodymyr ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: setup: Relocate the Device-Tree later on in the boot
Hi Julien, Julien Grall writes: > At the moment, the Device-Tree is relocated into xenheap while setting > up the memory subsystem. This is actually not necessary because the > early mapping is still present and we don't require the virtual address > to be stable until unflatting the Device-Tree. > > So the relocation can safely be moved after the memory subsystem is > fully setup. This has the nice advantage to make the relocation common > and let the xenheap allocator decides where to put it. > > Lastly, the device-tree is not going to be used for ACPI system. So > there are no need to relocate it and can just be discarded. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk This certainly is looking better now. > --- > xen/arch/arm/setup.c | 58 > > 1 file changed, 22 insertions(+), 36 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 1b303bde34..ebbfad94e4 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -399,6 +399,19 @@ void __init discard_initial_modules(void) > remove_early_mappings(); > } > > +/* Relocate the FDT in Xen heap */ > +static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size) > +{ > +void *fdt = xmalloc_bytes(dtb_size); > + > +if ( !fdt ) > +panic("Unable to allocate memory for relocating the Device-Tree.\n"); > + > +copy_from_paddr(fdt, dtb_paddr, dtb_size); > + > +return fdt; > +} > + > #ifdef CONFIG_ARM_32 > /* > * Returns the end address of the highest region in the range s..e > @@ -572,16 +585,13 @@ static void __init init_pdx(void) > } > > #ifdef CONFIG_ARM_32 > -static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) > +static void __init setup_mm(void) > { > paddr_t ram_start, ram_end, ram_size; > paddr_t s, e; > unsigned long ram_pages; > unsigned long heap_pages, xenheap_pages, domheap_pages; > -unsigned long dtb_pages; > -unsigned long boot_mfn_start, boot_mfn_end; > int i; > -void *fdt; > const uint32_t ctr = READ_CP32(CTR); > > if ( !bootinfo.mem.nr_banks ) > @@ -655,21 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, > size_t dtb_size) > > setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages); > > -/* > - * Need a single mapped page for populating bootmem_region_list > - * and enough mapped pages for copying the DTB. > - */ > -dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT; > -boot_mfn_start = mfn_x(xenheap_mfn_end) - dtb_pages - 1; > -boot_mfn_end = mfn_x(xenheap_mfn_end); > - > -init_boot_pages(pfn_to_paddr(boot_mfn_start), > pfn_to_paddr(boot_mfn_end)); > - > -/* Copy the DTB. */ > -fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1))); > -copy_from_paddr(fdt, dtb_paddr, dtb_size); > -device_tree_flattened = fdt; > - > /* Add non-xenheap memory */ > for ( i = 0; i < bootinfo.mem.nr_banks; i++ ) > { > @@ -713,20 +708,17 @@ static void __init setup_mm(unsigned long dtb_paddr, > size_t dtb_size) > setup_frametable_mappings(ram_start, ram_end); > max_page = PFN_DOWN(ram_end); > > -/* Add xenheap memory that was not already added to the boot > - allocator. */ > +/* Add xenheap memory that was not already added to the boot allocator. > */ > init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start), > - pfn_to_paddr(boot_mfn_start)); > + mfn_to_maddr(xenheap_mfn_end)); > } > #else /* CONFIG_ARM_64 */ > -static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) > +static void __init setup_mm(void) > { > paddr_t ram_start = ~0; > paddr_t ram_end = 0; > paddr_t ram_size = 0; > int bank; > -unsigned long dtb_pages; > -void *fdt; > > init_pdx(); > > @@ -770,16 +762,6 @@ static void __init setup_mm(unsigned long dtb_paddr, > size_t dtb_size) > xenheap_mfn_start = maddr_to_mfn(ram_start); > xenheap_mfn_end = maddr_to_mfn(ram_end); > > -/* > - * Need enough mapped pages for copying the DTB. > - */ > -dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT; > - > -/* Copy the DTB. */ > -fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1))); > -copy_from_paddr(fdt, dtb_paddr, dtb_size); > -device_tree_flattened = fdt; > - > setup_frametable_mappings(ram_start, ram_end); > max_page = PFN_DOWN(ram_end); > } > @@ -838,7 +820,7 @@ void __init start
Re: [Xen-devel] [PATCH] xen/arm: bootfd: Fix indentation in process_multiboot_node()
Julien Grall writes: > One line in process_multiboot_node() is using hard tab rather than soft > tab. So fix it! > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk So, hunting them one-by-one is the preferred way? I'm asking this because there are more of them: % find xen/arch/arm -type f -name "*.[ch]" -exec grep -lP "\t" {} \; xen/arch/arm/traps.c xen/arch/arm/acpi/boot.c xen/arch/arm/arm32/lib/assembler.h xen/arch/arm/arm64/livepatch.c xen/arch/arm/arm64/lib/find_next_bit.c xen/arch/arm/arm64/insn.c xen/arch/arm/alternative.c xen/arch/arm/bootfdt.c (apparently, I don't have this patch applied) > --- > xen/arch/arm/bootfdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 258b057f00..623173bc7f 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -264,7 +264,7 @@ static void __init process_multiboot_node(const void > *fdt, int node, > case 1: kind = BOOTMOD_RAMDISK; break; > default: break; > } > - if ( kind_guess > 1 && has_xsm_magic(start) ) > + if ( kind_guess > 1 && has_xsm_magic(start) ) > kind = BOOTMOD_XSM; > } -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC 4/9] arm64: utilize time accounting
Hi Andrii, Andrii Anisov writes: > From: Andrii Anisov > > Call time accounting hooks from appropriate transition points > of the ARM64 code. I'd prefer more elaborate commit message. For example - what are appropriate transition points? I mean - how you chose ones? > Signed-off-by: Andrii Anisov > --- > xen/arch/arm/arm64/entry.S | 39 --- > xen/arch/arm/domain.c | 2 ++ > 2 files changed, 38 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 2d9a271..6fb2fa9 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -143,12 +143,21 @@ > > .endm > > -.macro exit, hyp, compat > +.macro exit, hyp, compat, tacc=1 > > .if \hyp == 0 /* Guest mode */ > > + .if \tacc == 1 There is a hard tab, instead of 8 spaces. Also, while it is easy to guess what 'hyp' and 'compat' mean, it is hard to tell what 'tacc' stands for. I think, you need either better name for this or a comment, which explains all parameters. > + > +mov x0, #1 > +bl tacc_hyp > + > + .endif The same about hard tabs. Probably, there are more of them in this patch. > + > bl leave_hypervisor_tail /* Disables interrupts on return */ > > + mov x0, #1 > + bl tacc_guest > exit_guest \compat > > .endif > @@ -205,9 +214,15 @@ hyp_sync: > > hyp_irq: > entry hyp=1 > +mov x0,#5 Space is missing before #5 > +bl tacc_irq_enter > msr daifclr, #4 > mov x0, sp > bl do_trap_irq > + > +mov x0,#5 Space is missing before #5 > +bl tacc_irq_exit > + > exithyp=1 > > guest_sync: > @@ -291,6 +306,9 @@ guest_sync_slowpath: > * to save them. > */ > entry hyp=0, compat=0, save_x0_x1=0 > + There are trailing whitespaces. I sure that 'git diff' highlights such mistakes... > +mov x0,#1 Space is missing before #1 > +bl tacc_gsync > /* > * The vSError will be checked while > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > * is not set. If a vSError took place, the initial exception will be > @@ -307,6 +325,10 @@ guest_sync_slowpath: > > guest_irq: > entry hyp=0, compat=0 > + > +mov x0,#6 Space is missing before #6 > +bl tacc_irq_enter > + > /* > * The vSError will be checked while > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > * is not set. If a vSError took place, the initial exception will be > @@ -319,6 +341,8 @@ guest_irq: > mov x0, sp > bl do_trap_irq > 1: > + mov x0,#6 Space is missing before #6, also looks like there is a hard tab character. > +bl tacc_irq_exit > exithyp=0, compat=0 > > guest_fiq_invalid: > @@ -334,6 +358,9 @@ guest_error: > > guest_sync_compat: > entry hyp=0, compat=1 > + > +mov x0,#2 Space is missing before #2. > +bl tacc_gsync > /* > * The vSError will be checked while > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > * is not set. If a vSError took place, the initial exception will be > @@ -350,6 +377,10 @@ guest_sync_compat: > > guest_irq_compat: > entry hyp=0, compat=1 > + > +mov x0,#7 Space is missing before #7. > +bl tacc_irq_enter > + > /* > * The vSError will be checked while > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > * is not set. If a vSError took place, the initial exception will be > @@ -362,6 +393,8 @@ guest_irq_compat: > mov x0, sp > bl do_trap_irq > 1: > +mov x0,#7 Space is missing before #7... > +bl tacc_irq_exit > exithyp=0, compat=1 > > guest_fiq_invalid_compat: > @@ -376,9 +409,9 @@ guest_error_compat: > exithyp=0, compat=1 > > ENTRY(return_to_new_vcpu32) > -exithyp=0, compat=1 > +exithyp=0, compat=1, tacc=0 > ENTRY(return_to_new_vcpu64) > -exithyp=0, compat=0 > +exithyp=0, compat=0, tacc=0 > > return_from_trap: > msr daifset, #2 /* Mask interrupts */ > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index a9c4113..53ef630 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -51,11 +51,13 @@ static void do_idle(void) > process_pe
Re: [Xen-devel] [RFC 1/9] schedule: Introduce per-pcpu time accounting
ext_saved(struct vcpu *prev) > { > /* Clear running flag /after/ writing context to memory. */ > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index e3601c1..04a8724 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1002,6 +1002,33 @@ extern void dump_runq(unsigned char key); > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi); > > +enum TACC_STATES { If I remember correct, enum names should in lower case > +TACC_HYP = 0, > + TACC_GUEST = 1, > +TACC_IDLE = 2, > +TACC_IRQ = 3, > +TACC_GSYNC = 4, > +TACC_STATES_MAX > +}; > + > +struct tacc > +{ > +s_time_t state_time[TACC_STATES_MAX]; > +s_time_t state_entry_time; > +int state; enum, maybe? > + > +s_time_t guest_time; > + > +s_time_t irq_enter_time; > +s_time_t irq_time; > +int irq_cnt; > +}; > + > +DECLARE_PER_CPU(struct tacc, tacc); > + > +void tacc_hyp(int place); > +void tacc_idle(int place); What about functions from sched.c? Should they be declared there? > + > #endif /* __SCHED_H__ */ > > /* -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/5] xen/arm: optee: handle share buffer translation error
Julien Grall writes: > Hi Volodymyr, > > On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >> There is a case possible, when OP-TEE asks guest to allocate shared >> buffer, but Xen for some reason can't translate buffer's addresses. In >> this situation we should do two things: >> >> 1. Tell guest to free allocated buffer, so there will be no memory >> leak for guest. >> >> 2. Tell OP-TEE that buffer allocation failed. >> >> To ask guest to free allocated buffer we should perform the same >> thing, as OP-TEE does - issue RPC request. This is done by filling >> request buffer (luckily we can reuse the same buffer, that OP-TEE used >> to issue original request) and then return to guest with special >> return code. >> >> Then we need to handle next call from guest in a special way: as RPC >> was issued by Xen, not by OP-TEE, it should be handled by Xen. >> Basically, this is the mechanism to preempt OP-TEE mediator. >> >> The same mechanism can be used in the future to preempt mediator >> during translation large (>512 pages) shared buffers. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> xen/arch/arm/tee/optee.c | 167 +++ >> 1 file changed, 136 insertions(+), 31 deletions(-) >> >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> index 3ce6e7fa55..4eebc60b62 100644 >> --- a/xen/arch/arm/tee/optee.c >> +++ b/xen/arch/arm/tee/optee.c >> @@ -96,6 +96,11 @@ >> OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ >> OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) >> +enum optee_call_state { >> +OPTEEM_CALL_NORMAL = 0, > > enum always start counting at 0. Also, looking at the code, it does > not seem you need to know the value. Right? Yep. This is a bad habit. Will remove. > >> +OPTEEM_CALL_XEN_RPC, > > I am a bit confused, the enum is called optee_call_state but all the > enum are prefixed with OPTEEM_CALL_. Why the discrepancy? Because I'm bad at naming things :) OPTEEM_CALL_STATE_XEN_RPC looks too long. But you are right, so I'll rename the enum values. Unless, you have a better idea for this. > >> +}; >> + >> static unsigned int __read_mostly max_optee_threads; >> /* >> @@ -112,6 +117,9 @@ struct optee_std_call { >> paddr_t guest_arg_ipa; >> int optee_thread_id; >> int rpc_op; >> +/* Saved buffer type for the last buffer allocate request */ > > Looking at the code, it feels to me you are saving the buffer type for > the current command and not the last. Did I miss anything? Yes, right. Will rename. >> +unsigned int rpc_buffer_type; >> +enum optee_call_state state; >> uint64_t rpc_data_cookie; >> bool in_flight; >> register_t rpc_params[2]; >> @@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct >> optee_domain *ctx) >> call->optee_thread_id = -1; >> call->in_flight = true; >> +call->state = OPTEEM_CALL_NORMAL; >> spin_lock(&ctx->lock); >> list_add_tail(&call->list, &ctx->call_list); >> @@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx, >> ret = -ERESTART; >> } >> +/* Save the buffer type in case we will want to free it >> */ >> +if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC ) >> +call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a; >> + >> unmap_domain_page(shm_rpc->xen_arg); >> } >> @@ -1239,18 +1252,102 @@ err: >> return; >> } >> +/* >> + * Prepare RPC request to free shared buffer in the same way, as >> + * OP-TEE does this. >> + * >> + * Return values: >> + * true - successfully prepared RPC request >> + * false - there was an error >> + */ >> +static bool issue_rpc_cmd_free(struct optee_domain *ctx, >> + struct cpu_user_regs *regs, >> + struct optee_std_call *call, >> + struct shm_rpc *shm_rpc, >> + uint64_t cookie) >> +{ >> +register_t r1, r2; >> + >> +/* In case if guest will forget to update it with meaningful value */ >> +shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC; >> +shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE; >> +shm_rpc->xen_arg->num_params = 1; >>
Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status
Julien Grall writes: > On 8/23/19 8:20 PM, Volodymyr Babchuk wrote: >> >> Hi Julien, > > Hi, > > Apologies for the delay. It is okay. I myself was busy a bit. > >> >> Julien Grall writes: >> >>> Hi, >>> >>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >>>> As all TODOs and potential security issues are resolved now, >>>> remove experimental status from OP-TEE mediator. >>> >>> Looking at SUPPORT.MD, I think OP-TEE without this series would be >>> considered as "Experimental". >> Right. >> >>> >>> With this series applied, I still think we should keep the Kconfig >>> behind EXPERT but mark it as "Technical Preview" for at least a >>> release. This would encourage people to test and report any potential >>> issues with OP-TEE. >>> >>> We can re-discuss about the state in a few months for future release. >>> >>> BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen. >> Fair enough. In the next version I'll replace this patch with patch to >> SUPPORT.md. Or it is better to push separate patch for the documentation? > > I think the patch in SUPPORT.MD should go regardless of the state of > the rest. It is fine to keep it in this series. Okay. By the way, I skimmed thru SUPPORT.MD and I'm not sure what is the best place to describe mediator. So I could use some advice there. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size
Julien Grall writes: > Hi Volodymyr, > > On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >> We want to limit number of calls to lookup_and_pin_guest_ram_addr() >> per one request. There are two ways to do this: either preempt >> translate_noncontig() or to limit size of one shared buffer size. >> >> It is quite hard to preempt translate_noncontig(), because it is deep >> nested. So we chose second option. We will allow 512 pages per one >> shared buffer. This does not interfere with GP standard, as it >> requires that size limit for shared buffer should be at lest 512kB. > > Do you mean "least" instead of "lest"? Yes > If so, why 512 pages (i.e 1MB) > is plenty enough for most of the use cases? What does "xtest" consist > on? Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB is enough for the most cases, because OP-TEE itself have a very limited resources. But this value is chosen arbitrary. > >> Also, with this limitation OP-TEE still passes own "xtest" test suite, >> so this is okay for now. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> xen/arch/arm/tee/optee.c | 30 ++ >> 1 file changed, 18 insertions(+), 12 deletions(-) >> >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> index ec5402e89b..f4fa8a7758 100644 >> --- a/xen/arch/arm/tee/optee.c >> +++ b/xen/arch/arm/tee/optee.c >> @@ -72,6 +72,17 @@ >>*/ >> #define MAX_TOTAL_SMH_BUF_PG16384 >> +/* >> + * Arbitrary value that limits maximum shared buffer size. It is >> + * merely coincidence that it equals to both default OP-TEE SHM buffer >> + * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that >> + * this define limits number of pages. But user buffer can be not >> + * aligned to a page boundary. So it is possible that user would not >> + * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with >> + * OP-TEE. >> + */ >> +#define MAX_SHM_BUFFER_PG 512 >> + >> #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR >> #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ >> OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ >> @@ -697,15 +708,17 @@ static int translate_noncontig(struct optee_domain >> *ctx, >> size = ROUNDUP(param->u.tmem.size + offset, >> OPTEE_MSG_NONCONTIG_PAGE_SIZE); >> pg_count = DIV_ROUND_UP(size, >> OPTEE_MSG_NONCONTIG_PAGE_SIZE); >> +if ( pg_count > MAX_SHM_BUFFER_PG ) >> +return -ENOMEM; >> + >> order = get_order_from_bytes(get_pages_list_size(pg_count)); >> /* >> - * In the worst case we will want to allocate 33 pages, which is >> - * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at >> - * most 64 pages allocated. This buffer will be freed right after >> - * the end of the call and there can be no more than >> + * In the worst case we will want to allocate 2 pages, which is >> + * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed >> + * right after the end of the call and there can be no more than >>* max_optee_threads calls simultaneously. So in the worst case >> - * guest can trick us to allocate 64 * max_optee_threads pages in >> + * guest can trick us to allocate 2 * max_optee_threads pages in >>* total. >>*/ >> xen_pgs = alloc_domheap_pages(current->domain, order, 0); >> @@ -747,13 +760,6 @@ static int translate_noncontig(struct optee_domain *ctx, >> xen_data = __map_domain_page(xen_pgs); >> } >> -/* >> - * TODO: That function can pin up to 64MB of guest memory by >> - * calling lookup_and_pin_guest_ram_addr() 16384 times >> - * (assuming that PAGE_SIZE equals to 4096). >> - * This should be addressed before declaring OP-TEE security >> - * supported. >> - */ >> BUILD_BUG_ON(PAGE_SIZE != 4096); > > Without the comment, the BUILD_BUG_ON() looks random. So either you > want to have a different version of the comment or you want to move > the BUILD_BUG_ON() to somewhere else. It is still before get_domain_ram_page() call. But for clarity I can add comment like "Only 4k pages are supported right now". >> page = >> get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); >> if ( !page ) >> > > Cheers, -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers
Julien Grall writes: > Hi Volodymyr, > > On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >> Now we have limit for one shared buffer size, so we can be sure that >> one call to free_optee_shm_buf() will not free all >> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for >> hypercall_preempt_check() in the loop inside >> optee_relinquish_resources() and this will ensure that we are not >> missing preemption. > > I am not sure to understand the correlation between the two > sentences. Even if previously the guest could pin up to > MAX_TOTAL_SHM_BUF_PG in one call, a well-behaved guest would result to > do multiple calls and therefore preemption would have been useful. Looks like now I don't understand you. I'm talking about shared buffers. We have limited shared buffer to some reasonable size. There is bad- or well-behaving guests in this context, because guest can't share one big buffer in multiple calls. In other worlds, if guest *needs* to share 512MB buffer with OP-TEE, it will be forced to do this in one call. But we are forbidding big buffers right now. optee_relinquish_resources() is called during domain destruction. At this time we can have a number of still living shared buffers, each of one is no bigger than 512 pages. Thanks to this, we can call hypercall_preempt_check() only in optee_relinquish_resources(), but not in free_optee_shm_buf(). If we will allow guest to register bigger buffer, than we will be forced to check for preemption in free_optee_shm_buf() as well. This is what I meant in the commit message. > So I think the commit message needs some rewording. Probably yes... >> >> Signed-off-by: Volodymyr Babchuk >> --- >> xen/arch/arm/tee/optee.c | 10 +- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> index f4fa8a7758..a84ffa3089 100644 >> --- a/xen/arch/arm/tee/optee.c >> +++ b/xen/arch/arm/tee/optee.c >> @@ -634,14 +634,14 @@ static int optee_relinquish_resources(struct domain *d) >> if ( hypercall_preempt_check() ) >> return -ERESTART; >> -/* >> - * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of >> - * them will be put in this loop. It is worth considering to >> - * check for preemption inside the loop. >> - */ >> list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp, >> &ctx->optee_shm_buf_list, list ) >> +{ >> +if ( hypercall_preempt_check() ) > > So on the first iteration, you will check twice preemption (one before > the loop and just entering). hypercall_preempt_check(). The function > is not entirely free on Arm because of the implementation of > vgic_vcpu_pending_irq(). So preventing pointless call would be nice. > > In this case, the hypercall_preempt_check() before the loop could be > dropped. Yes, you are right. > >> +return -ERESTART; >> + >> free_optee_shm_buf(ctx, optee_shm_buf->cookie); >> +} >> if ( hypercall_preempt_check() ) >> return -ERESTART; >> > > Cheers, -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > On 9/11/19 7:48 PM, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> Hi Volodymyr, >>> >>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr() >>>> per one request. There are two ways to do this: either preempt >>>> translate_noncontig() or to limit size of one shared buffer size. >>>> >>>> It is quite hard to preempt translate_noncontig(), because it is deep >>>> nested. So we chose second option. We will allow 512 pages per one >>>> shared buffer. This does not interfere with GP standard, as it >>>> requires that size limit for shared buffer should be at lest 512kB. >>> >>> Do you mean "least" instead of "lest"? >> Yes >> >>> If so, why 512 pages (i.e 1MB) >>> is plenty enough for most of the use cases? What does "xtest" consist >>> on? >> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB >> is enough for the most cases, because OP-TEE itself have a very limited >> resources. But this value is chosen arbitrary. > > Could we potentially reduce to let say 512KB (or maybe lower) if xtest > only allocate 32KB? Potentially - yes. But only to 512KB if we want to be compatible with the Global Platform specification. Why are you asking, though? -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers
Julien Grall writes: > Hi Volodymyr, > > On 9/11/19 7:53 PM, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> Hi Volodymyr, >>> >>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >>>> Now we have limit for one shared buffer size, so we can be sure that >>>> one call to free_optee_shm_buf() will not free all >>>> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for >>>> hypercall_preempt_check() in the loop inside >>>> optee_relinquish_resources() and this will ensure that we are not >>>> missing preemption. >>> >>> I am not sure to understand the correlation between the two >>> sentences. Even if previously the guest could pin up to >>> MAX_TOTAL_SHM_BUF_PG in one call, a well-behaved guest would result to >>> do multiple calls and therefore preemption would have been useful. >> Looks like now I don't understand you. >> >> I'm talking about shared buffers. We have limited shared buffer to some >> reasonable size. There is bad- or well-behaving guests in this context, >> because guest can't share one big buffer in multiple calls. In other >> worlds, if guest *needs* to share 512MB buffer with OP-TEE, it will be >> forced to do this in one call. But we are forbidding big buffers right >> now. >> >> optee_relinquish_resources() is called during domain destruction. At >> this time we can have a number of still living shared buffers, each of >> one is no bigger than 512 pages. Thanks to this, we can call >> hypercall_preempt_check() only in optee_relinquish_resources(), but not >> in free_optee_shm_buf(). > > I understand what you mean, however my point is that this patch does > not dependent of the previous patch. Even if this patch goes alone, > you will improve well-behaved guest. For ill-behaved guest, the > problem will stay the same so no change. > Ah, I see now. Okay, I'll rework the commit description. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size
Hi Julien, Julien Grall writes: > Hi, > > On 9/12/19 8:45 PM, Volodymyr Babchuk wrote: >> >> Hi Julien, >> >> Julien Grall writes: >> >>> Hi Volodymyr, >>> >>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote: >>>> >>>> Julien Grall writes: >>>> >>>>> Hi Volodymyr, >>>>> >>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr() >>>>>> per one request. There are two ways to do this: either preempt >>>>>> translate_noncontig() or to limit size of one shared buffer size. >>>>>> >>>>>> It is quite hard to preempt translate_noncontig(), because it is deep >>>>>> nested. So we chose second option. We will allow 512 pages per one >>>>>> shared buffer. This does not interfere with GP standard, as it >>>>>> requires that size limit for shared buffer should be at lest 512kB. >>>>> >>>>> Do you mean "least" instead of "lest"? >>>> Yes >>>> >>>>> If so, why 512 pages (i.e 1MB) I have missed that earlier. But 512 pages is 2MB, actually. >>>>> is plenty enough for most of the use cases? What does "xtest" consist >>>>> on? >>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB >>>> is enough for the most cases, because OP-TEE itself have a very limited >>>> resources. But this value is chosen arbitrary. >>> >>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest >>> only allocate 32KB? >> Potentially - yes. But only to 512KB if we want to be compatible with >> the Global Platform specification. Why are you asking, though? > > Does the Global Platform specification limit to 512KB? Or is it a minimum? GP Spec says, that platform should allow *at lest* 512KB. Upper limit is not set. > Because, the smaller the buffer is, the less time it will take to > process in the worst case. Also, if we can have a reason for the size > (you seem to suggest the spec define a size...) then it is much better > than a random value. I have no strong arguments here, but I want to allow the biggest size possible. It seems, that 512 pages is the accepted limit in hypervisor code (at least, in p2m.c), so I chose this value. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > On 9/16/19 4:26 PM, Volodymyr Babchuk wrote: >> >> Hi Julien, >> >> Julien Grall writes: >> >>> Hi, >>> >>> On 9/12/19 8:45 PM, Volodymyr Babchuk wrote: >>>> >>>> Hi Julien, >>>> >>>> Julien Grall writes: >>>> >>>>> Hi Volodymyr, >>>>> >>>>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote: >>>>>> >>>>>> Julien Grall writes: >>>>>> >>>>>>> Hi Volodymyr, >>>>>>> >>>>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote: >>>>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr() >>>>>>>> per one request. There are two ways to do this: either preempt >>>>>>>> translate_noncontig() or to limit size of one shared buffer size. >>>>>>>> >>>>>>>> It is quite hard to preempt translate_noncontig(), because it is deep >>>>>>>> nested. So we chose second option. We will allow 512 pages per one >>>>>>>> shared buffer. This does not interfere with GP standard, as it >>>>>>>> requires that size limit for shared buffer should be at lest 512kB. >>>>>>> >>>>>>> Do you mean "least" instead of "lest"? >>>>>> Yes >>>>>> >>>>>>> If so, why 512 pages (i.e 1MB) >> I have missed that earlier. But 512 pages is 2MB, actually. >> >>>>>>> is plenty enough for most of the use cases? What does "xtest" consist >>>>>>> on? >>>>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB >>>>>> is enough for the most cases, because OP-TEE itself have a very limited >>>>>> resources. But this value is chosen arbitrary. >>>>> >>>>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest >>>>> only allocate 32KB? >>>> Potentially - yes. But only to 512KB if we want to be compatible with >>>> the Global Platform specification. Why are you asking, though? >>> >>> Does the Global Platform specification limit to 512KB? Or is it a minimum? >> GP Spec says, that platform should allow *at lest* 512KB. Upper limit is >> not set. >> >>> Because, the smaller the buffer is, the less time it will take to >>> process in the worst case. Also, if we can have a reason for the size >>> (you seem to suggest the spec define a size...) then it is much better >>> than a random value. >> I have no strong arguments here, but I want to allow the biggest size >> possible. It seems, that 512 pages is the accepted limit in hypervisor >> code (at least, in p2m.c), so I chose this value. > > If GP specific request at least 512KB, then any portable code should > be able to deal with 512KB, right? So why would you allow more? What > are the cons/pros? Yes, any portable code should work with 512KB. I want to allow bigger buffers for two reasons: on OP-TEE issues tracking people often ask how to increase various memory limits across OP-TEE. So, apparently people sometimes wants bigger buffers. Second reasons is the non-portable things like Secure Data Path. For SDP one wants to pass media (like audio and video) data to and from OP-TEE. Media requires big buffers. Anyways, I can see that 512 pages are established limit in the p2m code. So, why do you want OP-TEE mediator to have smaller limit? I want to be straight there: 512KB will likely work for most of the users. But there are always users who want more. So I would like to set largest plausible limit just in case. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] xen/arm: livepatch: Prevent CPUs to fetch stale instructions after livepatching
Hi Julien, Julien Grall writes: > During livepatch, a single CPU will take care of applying the patch and > all the others will wait for the action to complete. They will then once > execute arch_livepatch_post_action() to flush the pipeline. > > Per B2.2.5 "Concurrent modification and execution of instructions" in > DDI 0487E.a, flushing the instruction cache is not enough to ensure new > instructions are seen. All the PEs should also do an isb() to > synchronize the fetched instruction stream. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk > --- > xen/arch/arm/livepatch.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c > index 279d52cc6c..00c5e2bc45 100644 > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -88,7 +88,8 @@ void arch_livepatch_revert(const struct livepatch_func > *func) > > void arch_livepatch_post_action(void) > { > -/* arch_livepatch_revive has nuked the instruction cache. */ > +/* Discard any stale instructions that may have been fetched. */ > +isb(); > } > > void arch_livepatch_mask(void) -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 3/6] xen/arm: optee: limit number of shared buffers
We want to limit number of shared buffers that guest can register in OP-TEE. Every such buffer consumes XEN resources and we don't want guest to exhaust XEN. So we choose arbitrary limit for shared buffers. Signed-off-by: Volodymyr Babchuk --- xen/arch/arm/tee/optee.c | 30 +++--- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 55d11b91a9..88be959819 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -85,6 +85,14 @@ */ #define MAX_SHM_BUFFER_PG 129 +/* + * Limits the number of shared buffers that guest can have at once. + * This is to prevent case, when guests tricks XEN into exhausting + * own memory by allocating zillions of one-byte buffers. Value is + * chosen arbitrary. + */ +#define MAX_SHM_BUFFER_COUNT 16 + #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ @@ -146,6 +154,7 @@ struct optee_domain { struct list_head optee_shm_buf_list; atomic_t call_count; atomic_t optee_shm_buf_pages; +atomic_t optee_shm_buf_count; spinlock_t lock; }; @@ -233,6 +242,7 @@ static int optee_domain_init(struct domain *d) INIT_LIST_HEAD(&ctx->optee_shm_buf_list); atomic_set(&ctx->call_count, 0); atomic_set(&ctx->optee_shm_buf_pages, 0); +atomic_set(&ctx->optee_shm_buf_count, 0); spin_lock_init(&ctx->lock); d->arch.tee = ctx; @@ -481,23 +491,26 @@ static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx, struct optee_shm_buf *optee_shm_buf, *optee_shm_buf_tmp; int old, new; int err_code; +int count; + +count = atomic_add_unless(&ctx->optee_shm_buf_count, 1, + MAX_SHM_BUFFER_COUNT); +if ( count == MAX_SHM_BUFFER_COUNT ) +return ERR_PTR(-ENOMEM); do { old = atomic_read(&ctx->optee_shm_buf_pages); new = old + pages_cnt; if ( new >= MAX_TOTAL_SMH_BUF_PG ) -return ERR_PTR(-ENOMEM); +{ +err_code = -ENOMEM; +goto err_dec_cnt; +} } while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages, old, new)) ); -/* - * TODO: Guest can try to register many small buffers, thus, forcing - * XEN to allocate context for every buffer. Probably we need to - * limit not only total number of pages pinned but also number - * of buffer objects. - */ optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) + pages_cnt * sizeof(struct page *)); if ( !optee_shm_buf ) @@ -533,6 +546,8 @@ static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx, err: xfree(optee_shm_buf); atomic_sub(pages_cnt, &ctx->optee_shm_buf_pages); +err_dec_cnt: +atomic_dec(&ctx->optee_shm_buf_count); return ERR_PTR(err_code); } @@ -575,6 +590,7 @@ static void free_optee_shm_buf(struct optee_domain *ctx, uint64_t cookie) free_pg_list(optee_shm_buf); atomic_sub(optee_shm_buf->page_cnt, &ctx->optee_shm_buf_pages); +atomic_dec(&ctx->optee_shm_buf_count); xfree(optee_shm_buf); } -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error
There is a case possible, when OP-TEE asks guest to allocate shared buffer, but Xen for some reason can't translate buffer's addresses. In this situation we should do two things: 1. Tell guest to free allocated buffer, so there will be no memory leak for guest. 2. Tell OP-TEE that buffer allocation failed. To ask guest to free allocated buffer we should perform the same thing, as OP-TEE does - issue RPC request. This is done by filling request buffer (luckily we can reuse the same buffer, that OP-TEE used to issue original request) and then return to guest with special return code. Then we need to handle next call from guest in a special way: as RPC was issued by Xen, not by OP-TEE, it should be handled by Xen. Basically, this is the mechanism to preempt OP-TEE mediator. The same mechanism can be used in the future to preempt mediator during translation large (>512 pages) shared buffers. Signed-off-by: Volodymyr Babchuk --- Changes from v1: - Renamed OPTEEM_CALL_* to OPTEE_CALL_* - Fixed comments - Added ASSERT() in handle_xen_rpc_return() --- xen/arch/arm/tee/optee.c | 172 --- 1 file changed, 141 insertions(+), 31 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 88be959819..0a3205f9e8 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -98,6 +98,11 @@ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) +enum optee_call_state { +OPTEE_CALL_NORMAL, +OPTEE_CALL_XEN_RPC, +}; + static unsigned int __read_mostly max_optee_threads; /* @@ -114,6 +119,9 @@ struct optee_std_call { paddr_t guest_arg_ipa; int optee_thread_id; int rpc_op; +/* Saved buffer type for the current buffer allocate request */ +unsigned int rpc_buffer_type; +enum optee_call_state state; uint64_t rpc_data_cookie; bool in_flight; register_t rpc_params[2]; @@ -301,6 +309,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx) call->optee_thread_id = -1; call->in_flight = true; +call->state = OPTEE_CALL_NORMAL; spin_lock(&ctx->lock); list_add_tail(&call->list, &ctx->call_list); @@ -1086,6 +1095,10 @@ static int handle_rpc_return(struct optee_domain *ctx, ret = -ERESTART; } +/* Save the buffer type in case we will want to free it */ +if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC ) +call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a; + unmap_domain_page(shm_rpc->xen_arg); } @@ -1250,18 +1263,107 @@ err: return; } +/* + * Prepare RPC request to free shared buffer in the same way, as + * OP-TEE does this. + * + * Return values: + * true - successfully prepared RPC request + * false - there was an error + */ +static bool issue_rpc_cmd_free(struct optee_domain *ctx, + struct cpu_user_regs *regs, + struct optee_std_call *call, + struct shm_rpc *shm_rpc, + uint64_t cookie) +{ +register_t r1, r2; + +/* In case if guest will forget to update it with meaningful value */ +shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC; +shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE; +shm_rpc->xen_arg->num_params = 1; +shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; +shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type; +shm_rpc->xen_arg->params[0].u.value.b = cookie; + +if ( access_guest_memory_by_ipa(current->domain, +gfn_to_gaddr(shm_rpc->gfn), +shm_rpc->xen_arg, +OPTEE_MSG_GET_ARG_SIZE(1), +true) ) +{ +/* + * Well, this is quite bad. We have error in the error + * path. This can happen only if guest behaves badly, so all + * we can do is to return error to OP-TEE and leave guest's + * memory leaked. We already have freed all resources + * allocated for this buffer, but guest will never receive + * OPTEE_RPC_CMD_SHM_FREE request, so it will not know that it + * can release allocated buffer. + */ +shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC; +shm_rpc->xen_arg->num_params = 0; + +return false; +} + +uint64_to_regpair(&r1, &r2, shm_rpc->cookie); + +call->state = OPTEE_CALL_XEN_RPC; +call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD; +call->rpc_params[0] = r1; +call->rpc_params[1] = r2; +call->optee_thread_id = get_user_reg(regs, 3); + +set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD); +set_user_reg(re
[Xen-devel] [PATCH v2 1/6] xen/arm: optee: impose limit on shared buffer size
We want to limit number of calls to lookup_and_pin_guest_ram_addr() per one request. There are two ways to do this: either preempt translate_noncontig() or limit size of one shared buffer size. It is quite hard to preempt translate_noncontig(), because it is deep nested. So we chose the second option. We will allow 129 pages per one shared buffer. This corresponds to the GP standard, as it requires that size limit for shared buffer should be at least 512kB. One extra page (129th) is needed to cope with the fact that user's buffer is not necessary aligned with page boundary. Also, with this limitation OP-TEE still passes own "xtest" test suite, so this is okay for now. Signed-off-by: Volodymyr Babchuk --- Changes from v1: - Added comment before BUILD_BUG_ON(PAGE_SIZE != 4096); - Fixed typo in the commit message - Decreased MAX_SHM_BUFFER_PG to 129 --- xen/arch/arm/tee/optee.c | 44 +--- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index ec5402e89b..d64e9c3b85 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -72,6 +72,19 @@ */ #define MAX_TOTAL_SMH_BUF_PG16384 +/* + * Limit for shared buffer size. Please note that this define limits + * number of pages. But user buffer can be not aligned to a page + * boundary. So it is possible that user would not be able to share + * exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with OP-TEE. + * + * Global Platform specification for TEE requires that any TEE + * implementation should allow to share buffers with size of at least + * 512KB, which equals to 128 4KB pages. Due to align issue mentioned + * above, we need to increase this value to 129. + */ +#define MAX_SHM_BUFFER_PG 129 + #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ @@ -697,16 +710,29 @@ static int translate_noncontig(struct optee_domain *ctx, size = ROUNDUP(param->u.tmem.size + offset, OPTEE_MSG_NONCONTIG_PAGE_SIZE); pg_count = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE); +if ( pg_count > MAX_SHM_BUFFER_PG ) +return -ENOMEM; + order = get_order_from_bytes(get_pages_list_size(pg_count)); /* - * In the worst case we will want to allocate 33 pages, which is - * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at - * most 64 pages allocated. This buffer will be freed right after - * the end of the call and there can be no more than + * In the worst case we will want to allocate 1 page, which is + * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed + * right after the end of the call and there can be no more than * max_optee_threads calls simultaneously. So in the worst case - * guest can trick us to allocate 64 * max_optee_threads pages in + * guest can trick us to allocate 1 * max_optee_threads pages in * total. + * + * It may seem strange to have such complex calculations if we + * always will allocate exactly one page. Those calculations exist + * in the first place because earlier there were bigger limit for + * shared buffer size, so there were cases, when we needed more + * that one page there. Right now this is not true, but this code + * remains for two reasons: + * - Users can change MAX_SHM_BUFFER_PG to a higher value, in which + * case they will need this code. + * - There is a plan to implement preemption in the code below, which + * will allow use to increase default MAX_SHM_BUFFER_PG value. */ xen_pgs = alloc_domheap_pages(current->domain, order, 0); if ( !xen_pgs ) @@ -747,13 +773,7 @@ static int translate_noncontig(struct optee_domain *ctx, xen_data = __map_domain_page(xen_pgs); } -/* - * TODO: That function can pin up to 64MB of guest memory by - * calling lookup_and_pin_guest_ram_addr() 16384 times - * (assuming that PAGE_SIZE equals to 4096). - * This should be addressed before declaring OP-TEE security - * supported. - */ +/* Only 4k pages are supported right now */ BUILD_BUG_ON(PAGE_SIZE != 4096); page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); if ( !page ) -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/6] xen/arm: optee: check for preemption while freeing shared buffers
We can check for hypercall_preempt_check() in the loop inside optee_relinquish_resources() to increase hypervisor responsiveness in case if preemption is required. Signed-off-by: Volodymyr Babchuk --- Changes from v1: - Removed extra hypercall_preempt_check() - Updated the commit message --- xen/arch/arm/tee/optee.c | 13 + 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index d64e9c3b85..55d11b91a9 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -633,17 +633,14 @@ static int optee_relinquish_resources(struct domain *d) list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list ) free_shm_rpc(ctx, shm_rpc->cookie); -if ( hypercall_preempt_check() ) -return -ERESTART; - -/* - * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of - * them will be put in this loop. It is worth considering to - * check for preemption inside the loop. - */ list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp, &ctx->optee_shm_buf_list, list ) +{ +if ( hypercall_preempt_check() ) +return -ERESTART; + free_optee_shm_buf(ctx, optee_shm_buf->cookie); +} if ( hypercall_preempt_check() ) return -ERESTART; -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 5/6] SUPPORT.md: Describe OP-TEE mediator
With the latest patches to the mediator, it can be considered as Technological Preview feature. Signed-off-by: Volodymyr Babchuk --- Note for commiter: Obviously this patch should be merged after all other patches in this series. --- SUPPORT.md | 4 1 file changed, 4 insertions(+) diff --git a/SUPPORT.md b/SUPPORT.md index 375473a456..8d50a72dcb 100644 --- a/SUPPORT.md +++ b/SUPPORT.md @@ -660,6 +660,10 @@ No support for QEMU backends in a 16K or 64K domain. Status: Supported +### ARM: OP-TEE Mediator + +Status: Tech Preview + ## Virtual Hardware, QEMU This section describes supported devices available in HVM mode using a -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 6/6] xen/arm: optee: update description in Kconfig
OP-TEE mediator now is "Tech Preview" state, and we want to update it's description in Kconfig accordingly. Signed-off-by: Volodymyr Babchuk --- Note to commiter: this patch depends on first 4 patches in the series. --- xen/arch/arm/tee/Kconfig | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig index b4b6aa2610..a4a598191e 100644 --- a/xen/arch/arm/tee/Kconfig +++ b/xen/arch/arm/tee/Kconfig @@ -3,7 +3,11 @@ config OPTEE default n depends on TEE help - Enable experimental OP-TEE mediator. It allows guests to access - OP-TEE running on your platform. This requires virtualization-enabled - OP-TEE present. You can learn more about virtualization for OP-TEE - at https://optee.readthedocs.io/architecture/virtualization.html + Enable the OP-TEE mediator. It allows guests to access + OP-TEE running on your platform. This requires + virtualization-enabled OP-TEE present. You can learn more + about virtualization for OP-TEE at + https://optee.readthedocs.io/architecture/virtualization.html + + Right now OP-TEE mediator is "Tech Preview" state, so it is + not good idea to use it in production. -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 0/6] arch/arm: optee: fix TODOs and change status to "Tech Preview"
Hello, This is the second version for maturing the OP-TEE mediator. Changes also can be pulled from [2]. Changes from v1: - Added patch that updates SUPPORT.md - Instead of removing "experimental" status I changed it to "Tech Preview" - Other changes are described in the corresponding patches Cover letter for v1: This patch series fixes various unfinished items in the OP-TEE mediator. Mostly this is about limiting resources that guest can consume. This includes both memory and time - how many buffers guest can share with OP-TEE (this uses Xen memory) and when mediator should preempt itself, to make sure that guest does not stress scheduling. Apart from this, there were one case, when mediator's actions might lead to memory leak in a good-behaving guest. To fix this issue I had to extend mediator logic, so now it can issue RPC requests to guest in the same way, as OP-TEE does this. This is useful feature, because it allows to preempt mediator during long operations. So, in the future it will be possible to remove shared buffer size limitation, because mediator can preempt self during buffer translation. This patch series can be pulled from [1]. [1] https://github.com/lorc/xen/tree/optee3_v1 [2] https://github.com/lorc/xen/tree/optee3_v2 Volodymyr Babchuk (6): xen/arm: optee: impose limit on shared buffer size xen/arm: optee: check for preemption while freeing shared buffers xen/arm: optee: limit number of shared buffers xen/arm: optee: handle shared buffer translation error SUPPORT.md: Describe OP-TEE mediator xen/arm: optee: update description in Kconfig SUPPORT.md | 4 + xen/arch/arm/tee/Kconfig | 12 +- xen/arch/arm/tee/optee.c | 259 ++- 3 files changed, 213 insertions(+), 62 deletions(-) -- 2.22.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error
Hi Julien, Julien Grall writes: > Hi, > > On 18/09/2019 19:51, Volodymyr Babchuk wrote: >> +/* Handles return from Xen-issued RPC */ >> +static void handle_xen_rpc_return(struct optee_domain *ctx, >> + struct cpu_user_regs *regs, >> + struct optee_std_call *call, >> + struct shm_rpc *shm_rpc) >> +{ >> +call->state = OPTEE_CALL_NORMAL; >> + >> +/* >> + * Right now we have only one reason to be there - we asked guest >> + * to free shared buffer and it did it. Now we can tell OP-TEE >> + * that buffer allocation failed. We are not storing exact command >> + * type, only type of RPC return. So, this is the only check we >> + * can perform there. >> + */ >> +ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD); > > As I pointed out in v1, ASSERT() is probably the less desirable > solution here as this is an error path. Looks like I misunderstood you :( > Can you explain why you chose that over the 3 solutions I suggested? I think I need some clarification there. ASSERT() is adopted way to tell about invariant. Clearly, this is programming error if ASSERT() fails. But I understand, that ASSERT() is available only in debug build. So, in release it will never fire. As this is very unlikely error path, bug there can live forever. Okay, so ASSERT() is the least desirable way. Warning is not enough, as we are already in incorrect state. So, best way is to crash guest, it this correct? I'll do this in the next version then. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error
Julien Grall writes: > On 24/09/2019 12:37, Volodymyr Babchuk wrote: >> >> Hi Julien, >> >> Julien Grall writes: >> >>> Hi, >>> >>> On 18/09/2019 19:51, Volodymyr Babchuk wrote: >>>> +/* Handles return from Xen-issued RPC */ >>>> +static void handle_xen_rpc_return(struct optee_domain *ctx, >>>> + struct cpu_user_regs *regs, >>>> + struct optee_std_call *call, >>>> + struct shm_rpc *shm_rpc) >>>> +{ >>>> +call->state = OPTEE_CALL_NORMAL; >>>> + >>>> +/* >>>> + * Right now we have only one reason to be there - we asked guest >>>> + * to free shared buffer and it did it. Now we can tell OP-TEE >>>> + * that buffer allocation failed. We are not storing exact command >>>> + * type, only type of RPC return. So, this is the only check we >>>> + * can perform there. >>>> + */ >>>> +ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD); >>> >>> As I pointed out in v1, ASSERT() is probably the less desirable >>> solution here as this is an error path. >> Looks like I misunderstood you :( >> >>> Can you explain why you chose that over the 3 solutions I suggested? >> I think I need some clarification there. ASSERT() is adopted way to tell >> about invariant. Clearly, this is programming error if ASSERT() fails. > > That's correct. > >> >> But I understand, that ASSERT() is available only in debug build. So, in >> release it will never fire. As this is very unlikely error path, bug >> there can live forever. Okay, so ASSERT() is the least desirable way. > > This is my concern, ASSERT() are fine in path that can be exercised > quite well. By experience, error path as not so well tested, so any > verbosity in non-debug build is always helpful. Yep, I see. >> >> Warning is not enough, as we are already in incorrect state. > Incorrect state for who? The guest? Yes, for the current call of the current guest. State of other calls and other guests should not be affected. But it is possible that our view of OP-TEE state for that guest is not correct also. >> >> So, best way is to crash guest, it this correct? I'll do this in the >> next version then. > > A rule of thumb is > - BUG_ON can be replaced by domain_crash() as this is a fatal error > you can't recover (the scope depends on the function call). This seems correct. > > - ASSERT() can be replaced by WARN_ON() as the former will be a NOP > in non-debug build. In both case, the error is not fatal and continue > will not result So it means the error is not fatal. I can't agree with this in general. But maybe this makes sense for Xen. As I said, generally ASSERT() is used to, well, assert that condition is true for any correct state of a program. So it cant' be replaced with WARN_ON(). If we detected invalid state we should either try to correct it (if know how) or to immediately stop the program. But I can see, why this behavior is not desired for hypervisor. Especially in production use. > You used ASSERT() in your code, so it feels to me that WARN_ON() would > be a suitable replacement. Well, honestly I believe that it is better to crash guest to prevent any additional harm. Look, we already detected that something is wrong with mediator state for certain guest. We can pretend that all is fine and try to continue the call. But who knows which consequences it will have? > However, if the state inconsistency is for Xen, then domain_crash() is > the best. If this is for the guest, then this is not really our > business and it may be best to let him continue as it could provide > more debug (domain_crash() will only dump the stack and registers). I'm looking at this from different point: we promised to provide some service for a guest and screwed up. It is not guest's fault. Now we know that we can't provide reliable service for a guest anymore. From safety point of view we should shut down the service. (But this is job for another patch) For now, we at least should crash the guest. This is the safest way. What do you think? -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] xen/arm: Implement workaround for Cortex A-57 and Cortex A72 AT speculate
Julien Grall writes: > Both Cortex-A57 (erratum 1319537) and Cortex-A72 (erratum 1319367) can > end with corrupt TLBs if they speculate an AT instruction while S1/S2 > system registers in inconsistent state. > > The workaround is the same as for Cortex A-76 implemented by commit > a18be06aca "xen/arm: Implement workaround for Cortex-A76 erratum 1165522", > so it is only necessary to plumb in the cpuerrata framework. > > Signed-off-by: Julien Grall With a few nits: Reviewed-by: Volodymyr Babchuk > --- > docs/misc/arm/silicon-errata.txt | 2 ++ > xen/arch/arm/cpuerrata.c | 10 ++ > 2 files changed, 12 insertions(+) > > diff --git a/docs/misc/arm/silicon-errata.txt > b/docs/misc/arm/silicon-errata.txt > index 6cd1366f15..cf193a6d4d 100644 > --- a/docs/misc/arm/silicon-errata.txt > +++ b/docs/misc/arm/silicon-errata.txt > @@ -48,5 +48,7 @@ stable hypervisors. > | ARM| Cortex-A57 | #852523 | N/A > | > | ARM| Cortex-A57 | #832075 | ARM64_ERRATUM_832075 > | > | ARM| Cortex-A57 | #834220 | ARM64_ERRATUM_834220 > | > +| ARM| Cortex-A57 | #1319537| N/A > +| ARM| Cortex-A72 | #1319367| N/A Looks like you missed the last |. Also, is this "ARM" or "Arm"? I believe you asked me to user the latter. > | ARM| Cortex-A76 | #1165522| N/A > | > | ARM| MMU-500 | #842869 | N/A > | > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > index 6f483b2d8d..da72b02442 100644 > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -481,6 +481,16 @@ static const struct arm_cpu_capabilities arm_errata[] = { > .capability = ARM64_WORKAROUND_AT_SPECULATE, > MIDR_RANGE(MIDR_CORTEX_A76, 0, 2 << MIDR_VARIANT_SHIFT), > }, > +{ > +.desc = "ARM erratum 1319537", > +.capability = ARM64_WORKAROUND_AT_SPECULATE, > +MIDR_ALL_VERSIONS(MIDR_CORTEX_A72), > +}, > +{ > + .desc = "ARM erratum 1319367", > +.capability = ARM64_WORKAROUND_AT_SPECULATE, > +MIDR_ALL_VERSIONS(MIDR_CORTEX_A57), > +}, > {}, > }; -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RESEND][PATCH for-4.13] xen/arm: mm: Clear boot pagetables before bringing-up each secondary CPU
Julien Grall writes: > At the moment, boot pagetables are only cleared once at boot. This means > when booting CPU2 (and onwards) then boot pagetables will not be > cleared. > > To keep the interface exactly the same for all secondary CPU, the boot > pagetables are now cleared before bringing-up each secondary CPU. > > Signed-off-by: Julien Grall Taking into account fixed remark below: Reviewed-by: Volodymyr Babchuk > --- > xen/arch/arm/mm.c | 27 ++- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 1129dc28c8..e14ee76ff8 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -704,8 +704,20 @@ void __init setup_pagetables(unsigned long > boot_phys_offset) > > switch_ttbr(ttbr); > > -/* Clear the copy of the boot pagetables. Each secondary CPU > - * rebuilds these itself (see head.S) */ > +xen_pt_enforce_wnx(); > + > +#ifdef CONFIG_ARM_32 > +per_cpu(xen_pgtable, 0) = cpu0_pgtable; > +per_cpu(xen_dommap, 0) = cpu0_dommap; > +#endif > +} > + > +static void clear_boot_pagetables(void) > +{ > +/* > + * Clear the copy of the boot pagetables. Each secondary CPU > + * rebuilds these itself (see head.S) Missing full stop. > + */ > clear_table(boot_pgtable); > #ifdef CONFIG_ARM_64 > clear_table(boot_first); > @@ -713,18 +725,13 @@ void __init setup_pagetables(unsigned long > boot_phys_offset) > #endif > clear_table(boot_second); > clear_table(boot_third); > - > -xen_pt_enforce_wnx(); > - > -#ifdef CONFIG_ARM_32 > -per_cpu(xen_pgtable, 0) = cpu0_pgtable; > -per_cpu(xen_dommap, 0) = cpu0_dommap; > -#endif > } > > #ifdef CONFIG_ARM_64 > int init_secondary_pagetables(int cpu) > { > +clear_boot_pagetables(); > + > /* Set init_ttbr for this CPU coming up. All CPus share a single setof > * pagetables, but rewrite it each time for consistency with 32 bit. */ > init_ttbr = (uintptr_t) xen_pgtable + phys_offset; > @@ -767,6 +774,8 @@ int init_secondary_pagetables(int cpu) > per_cpu(xen_pgtable, cpu) = first; > per_cpu(xen_dommap, cpu) = domheap; > > +clear_boot_pagetables(); > + > /* Set init_ttbr for this CPU coming up */ > init_ttbr = __pa(first); > clean_dcache(init_ttbr); -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v2 2/2] docs: Replace all instance of ARM by Arm
s kinds of domains (in this order: > @@ -1804,7 +1804,7 @@ accidentally leaking secrets by releasing pages without > proper sanitization. > > Set the serial transmit buffer size. > > -### serrors (ARM) > +### serrors (Arm) > > `= diverse | forward | panic` > > > Default: `diverse` > @@ -2184,8 +2184,8 @@ production systems (see > http://xenbits.xen.org/xsa/advisory-163.html)! > > > Default: `trap` > > -WFI is the ARM instruction to "wait for interrupt". WFE is similar and > -means "wait for event". This option, which is ARM specific, changes the > +WFI is the Arm instruction to "wait for interrupt". WFE is similar and > +means "wait for event". This option, which is Arm specific, changes the > way guest WFI and WFE are implemented in Xen. By default, Xen traps both > instructions. In the case of WFI, Xen blocks the guest vcpu; in the case > of WFE, Xen yield the guest vcpu. When setting vwfi to `native`, Xen > diff --git a/docs/process/xen-release-management.pandoc > b/docs/process/xen-release-management.pandoc > index d6abc90a02..96207c93f0 100644 > --- a/docs/process/xen-release-management.pandoc > +++ b/docs/process/xen-release-management.pandoc > @@ -416,7 +416,7 @@ J: XEN-28 > > === x86 === > > -=== ARM === > +=== Arm === > > == Completed == > > diff --git a/docs/specs/libxc-migration-stream.pandoc > b/docs/specs/libxc-migration-stream.pandoc > index 97dacb6e30..ddd7d1eb2f 100644 > --- a/docs/specs/libxc-migration-stream.pandoc > +++ b/docs/specs/libxc-migration-stream.pandoc > @@ -30,7 +30,7 @@ image used in Xen 4.4 and earlier (the _legacy format_). > > A new format that addresses the above is required. > > -ARM does not yet have have a domain save image format specified and > +Arm does not yet have have a domain save image format specified and > the format described in this specification should be suitable. > > Not Yet Included > @@ -41,7 +41,7 @@ included in a future draft. > > * Page data compression. > > -* ARM > +* Arm > > > Overview > @@ -162,7 +162,7 @@ type0x: Reserved. > > 0x0003: x86 PVH. > > -0x0004: ARM. > +0x0004: Arm. > > 0x0005 - 0x: Reserved. > > diff --git a/docs/specs/libxl-migration-stream.pandoc > b/docs/specs/libxl-migration-stream.pandoc > index 3766f37f4f..d407abd817 100644 > --- a/docs/specs/libxl-migration-stream.pandoc > +++ b/docs/specs/libxl-migration-stream.pandoc > @@ -43,7 +43,7 @@ Not Yet Included > The following features are not yet fully specified and will be > included in a future draft. > > -* ARM > +* Arm > > > Overview -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v2 2/2] docs: Replace all instance of ARM by Arm
Julien Grall writes: > Hi, > > On 9/24/19 3:56 PM, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >>> The documentation is using a mix of ARM (old) and Arm (new). To stay >>> consistent, use only the new name. >>> >> Honestly, this rename confuses me. I think, this commit is the good >> place to clarify a couple of questions. > > It did confuse a lot when the rename was made by Arm... What I want to > avoid is mixing the both in the documentation as this makes things > more difficult to follow. > > There are probably more to tidy up, but then you have to start somewhere. Thank you for explanation. Just in case: I have nothing against this particular patch. I just wanted to understand how to use this names correctly. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 0/3] arch/arm: optee: fix TODOs and change status to "Tech Preview"
This is the third version of maturing the OP-TEE mediator patches. Changes also can be pulled from [3]. Changes from v2: - The following 3 patches were commited: xen/arm: optee: impose limit on shared buffer size xen/arm: optee: check for preemption while freeing shared buffers xen/arm: optee: limit number of shared buffers - Other changes are described in the corresponding patches Changes from v1: - Added patch that updates SUPPORT.md - Instead of removing "experimental" status I changed it to "Tech Preview" - Other changes are described in the corresponding patches Cover letter for v1: This patch series fixes various unfinished items in the OP-TEE mediator. Mostly this is about limiting resources that guest can consume. This includes both memory and time - how many buffers guest can share with OP-TEE (this uses Xen memory) and when mediator should preempt itself, to make sure that guest does not stress scheduling. Apart from this, there were one case, when mediator's actions might lead to memory leak in a good-behaving guest. To fix this issue I had to extend mediator logic, so now it can issue RPC requests to guest in the same way, as OP-TEE does this. This is useful feature, because it allows to preempt mediator during long operations. So, in the future it will be possible to remove shared buffer size limitation, because mediator can preempt self during buffer translation. This patch series can be pulled from [1]. [1] https://github.com/lorc/xen/tree/optee3_v1 [2] https://github.com/lorc/xen/tree/optee3_v2 [3] https://github.com/lorc/xen/tree/optee3_v3 Volodymyr Babchuk (3): xen/arm: optee: handle shared buffer translation error SUPPORT.md: Describe OP-TEE mediator xen/arm: optee: update description in Kconfig SUPPORT.md | 4 + xen/arch/arm/tee/Kconfig | 9 +- xen/arch/arm/tee/optee.c | 173 --- 3 files changed, 151 insertions(+), 35 deletions(-) -- 2.23.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 1/3] xen/arm: optee: handle shared buffer translation error
There is a case possible, when OP-TEE asks guest to allocate shared buffer, but Xen for some reason can't translate buffer's addresses. In this situation we should do two things: 1. Tell guest to free allocated buffer, so there will be no memory leak for guest. 2. Tell OP-TEE that buffer allocation failed. To ask guest to free allocated buffer we should perform the same thing, as OP-TEE does - issue RPC request. This is done by filling request buffer (luckily we can reuse the same buffer, that OP-TEE used to issue original request) and then return to guest with special return code. Then we need to handle next call from guest in a special way: as RPC was issued by Xen, not by OP-TEE, it should be handled by Xen. Basically, this is the mechanism to preempt OP-TEE mediator. The same mechanism can be used in the future to preempt mediator during translation large (>512 pages) shared buffers. Signed-off-by: Volodymyr Babchuk --- Changes from v1: - Renamed OPTEEM_CALL_* to OPTEE_CALL_* - Fixed comments - Added ASSERT() in handle_xen_rpc_return() Changes from v2: - ASSERT() in handle_xen_rpc_return() is replaced with domain_crash() --- xen/arch/arm/tee/optee.c | 173 --- 1 file changed, 142 insertions(+), 31 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 350af87d90..6a035355db 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -98,6 +98,11 @@ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) +enum optee_call_state { +OPTEE_CALL_NORMAL, +OPTEE_CALL_XEN_RPC, +}; + static unsigned int __read_mostly max_optee_threads; /* @@ -114,6 +119,9 @@ struct optee_std_call { paddr_t guest_arg_ipa; int optee_thread_id; int rpc_op; +/* Saved buffer type for the current buffer allocate request */ +unsigned int rpc_buffer_type; +enum optee_call_state state; uint64_t rpc_data_cookie; bool in_flight; register_t rpc_params[2]; @@ -301,6 +309,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx) call->optee_thread_id = -1; call->in_flight = true; +call->state = OPTEE_CALL_NORMAL; spin_lock(&ctx->lock); list_add_tail(&call->list, &ctx->call_list); @@ -1086,6 +1095,10 @@ static int handle_rpc_return(struct optee_domain *ctx, ret = -ERESTART; } +/* Save the buffer type in case we will want to free it */ +if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC ) +call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a; + unmap_domain_page(shm_rpc->xen_arg); } @@ -1250,18 +1263,108 @@ err: return; } +/* + * Prepare RPC request to free shared buffer in the same way, as + * OP-TEE does this. + * + * Return values: + * true - successfully prepared RPC request + * false - there was an error + */ +static bool issue_rpc_cmd_free(struct optee_domain *ctx, + struct cpu_user_regs *regs, + struct optee_std_call *call, + struct shm_rpc *shm_rpc, + uint64_t cookie) +{ +register_t r1, r2; + +/* In case if guest will forget to update it with meaningful value */ +shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC; +shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE; +shm_rpc->xen_arg->num_params = 1; +shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; +shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type; +shm_rpc->xen_arg->params[0].u.value.b = cookie; + +if ( access_guest_memory_by_ipa(current->domain, +gfn_to_gaddr(shm_rpc->gfn), +shm_rpc->xen_arg, +OPTEE_MSG_GET_ARG_SIZE(1), +true) ) +{ +/* + * Well, this is quite bad. We have error in the error + * path. This can happen only if guest behaves badly, so all + * we can do is to return error to OP-TEE and leave guest's + * memory leaked. We already have freed all resources + * allocated for this buffer, but guest will never receive + * OPTEE_RPC_CMD_SHM_FREE request, so it will not know that it + * can release allocated buffer. + */ +shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC; +shm_rpc->xen_arg->num_params = 0; + +return false; +} + +uint64_to_regpair(&r1, &r2, shm_rpc->cookie); + +call->state = OPTEE_CALL_XEN_RPC; +call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD; +call->rpc_params[0] = r1; +call->rpc_params[1] = r2; +call->optee_thread_id = get_user_
[Xen-devel] [PATCH v3 2/3] SUPPORT.md: Describe OP-TEE mediator
With the latest patches to the mediator, it can be considered as Technological Preview feature. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- Note for commiter: Obviously this patch should be merged after all other patches in this series. Changes from v2: - ARM->Arm - Added a-b tag by Julien --- SUPPORT.md | 4 1 file changed, 4 insertions(+) diff --git a/SUPPORT.md b/SUPPORT.md index 375473a456..a733d74464 100644 --- a/SUPPORT.md +++ b/SUPPORT.md @@ -660,6 +660,10 @@ No support for QEMU backends in a 16K or 64K domain. Status: Supported +### Arm: OP-TEE Mediator + +Status: Tech Preview + ## Virtual Hardware, QEMU This section describes supported devices available in HVM mode using a -- 2.23.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 3/3] xen/arm: optee: update description in Kconfig
OP-TEE mediator now is "Tech Preview" state, and we want to update it's description in Kconfig accordingly. Signed-off-by: Volodymyr Babchuk --- Note to commiter: this patch depends on first 4 patches in the series. --- xen/arch/arm/tee/Kconfig | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig index b4b6aa2610..392169b255 100644 --- a/xen/arch/arm/tee/Kconfig +++ b/xen/arch/arm/tee/Kconfig @@ -3,7 +3,8 @@ config OPTEE default n depends on TEE help - Enable experimental OP-TEE mediator. It allows guests to access - OP-TEE running on your platform. This requires virtualization-enabled - OP-TEE present. You can learn more about virtualization for OP-TEE - at https://optee.readthedocs.io/architecture/virtualization.html + Enable the OP-TEE mediator. It allows guests to access + OP-TEE running on your platform. This requires + virtualization-enabled OP-TEE present. You can learn more + about virtualization for OP-TEE at + https://optee.readthedocs.io/architecture/virtualization.html -- 2.23.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 01/10] xen/arm64: entry: Introduce a macro to generate guest vector and use it
Hello Julien, Julien Grall writes: > Most of the guest vectors are using the same pattern. This makes fairly > tedious to alter the pattern and risk introducing mistakes when updating > each path. > > A new macro is introduced to generate the guest vectors and now use it > in the one that use the open-code version. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 02/10] xen/arm64: head: Check if an SError is pending when receiving a vSError
Julien Grall writes: > At the moment, when we receive an SError exception from the guest, we > don't check if there are any other pending. For hardening the code, we > should ensure any pending SError are accounted to the guest before > executing any code with SError unmasked. > > The recently introduced macro 'guest_vector' could used to generate the > two vectors and therefore take advantage of any change required in the > future. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path
syncing back the VGIC state. > - * > - * TODO: Investigate whether this is necessary to do on every > - * trap and how it can be optimised. > - */ > -vtimer_update_irqs(v); > -vcpu_update_evtchn_irq(v); > +/* > + * We need to update the state of our emulated devices using level > + * triggered interrupts before syncing back the VGIC state. > + * > + * TODO: Investigate whether this is necessary to do on every > + * trap and how it can be optimised. > + */ > +vtimer_update_irqs(v); > +vcpu_update_evtchn_irq(v); > #endif > > -vgic_sync_from_lrs(v); > -} > +vgic_sync_from_lrs(v); > } > > void do_trap_guest_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > -enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > case HSR_EC_WFI_WFE: > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > -enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > #ifdef CONFIG_ARM_64 > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > > void do_trap_hyp_serror(struct cpu_user_regs *regs) > { > -enter_hypervisor_head(regs); > - > __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > } > > void do_trap_guest_serror(struct cpu_user_regs *regs) > { > -enter_hypervisor_head(regs); > - > __do_trap_serror(regs, true); > } > > void do_trap_irq(struct cpu_user_regs *regs) > { > -enter_hypervisor_head(regs); > gic_interrupt(regs, 0); > } > > void do_trap_fiq(struct cpu_user_regs *regs) > { > -enter_hypervisor_head(regs); > gic_interrupt(regs, 1); > } > > @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void) > local_irq_disable(); > } > > -void leave_hypervisor_tail(void) > +/* > + * Actions that needs to be done before entering the guest. This is the > + * last thing executed before the guest context is fully restored. > + * > + * The function will return with interrupts disabled. > + */ > +void leave_hypervisor_to_guest(void) > { > local_irq_disable(); -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 05/10] xen/arm: alternative: Remove unused parameter for alternative_if_not_cap
Julien Grall writes: > The macro alternative_if_not_cap is taking two parameters. The second > parameter is never used and it is hard to see how this can be used > correctly as it is only protecting the alternative section magic. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk > --- > xen/include/asm-arm/alternative.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/xen/include/asm-arm/alternative.h > b/xen/include/asm-arm/alternative.h > index dedb6dd001..2830a6da2d 100644 > --- a/xen/include/asm-arm/alternative.h > +++ b/xen/include/asm-arm/alternative.h > @@ -116,13 +116,11 @@ int apply_alternatives(const struct alt_instr *start, > const struct alt_instr *en > * The code that follows this macro will be assembled and linked as > * normal. There are no restrictions on this code. > */ > -.macro alternative_if_not cap, enable = 1 > - .if \enable > +.macro alternative_if_not cap > .pushsection .altinstructions, "a" > altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f > .popsection > 661: > - .endif > .endm > > /* -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 06/10] xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h
Julien Grall writes: > At the moment, ARCH_PATCH_INSN_SIZE is defined in the header > livepatch.h. However, this is also used in the alternative code. > > Rather than including livepatch.h just for using the define, move it in > the header insn.h which seems more suitable. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly
Julien Grall writes: > A follow-up patch will require to include insn.h from assembly code. So > wee need to protect any C-specific definition to avoid compilation > error when used in assembly code. > > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk > --- > xen/include/asm-arm/insn.h | 8 > 1 file changed, 8 insertions(+) > > diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h > index 19277212e1..00391f83f9 100644 > --- a/xen/include/asm-arm/insn.h > +++ b/xen/include/asm-arm/insn.h > @@ -1,8 +1,14 @@ > #ifndef __ARCH_ARM_INSN > #define __ARCH_ARM_INSN > > +#ifndef __ASSEMBLY__ > + > #include > > +/* > + * At the moment, arch-specific headers contain only definition for C > + * code. > + */ > #if defined(CONFIG_ARM_64) > # include > #elif defined(CONFIG_ARM_32) > @@ -11,6 +17,8 @@ > # error "unknown ARM variant" > #endif > > +#endif /* __ASSEMBLY__ */ > + > /* On ARM32,64 instructions are always 4 bytes long. */ > #define ARCH_PATCH_INSN_SIZE 4 -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Julien, Julien Grall writes: > At the moment, SSBD workaround is re-enabled for Xen after interrupts > are unmasked. This means we may end up to execute some part of the > hypervisor if an interrupt is received before the workaround is > re-enabled. > > As the rest of enter_hypervisor_from_guest() does not require to have > interrupts masked, the function is now split in two parts: > 1) enter_hypervisor_from_guest_noirq() called with interrupts >masked. I'm okay with this approach, but I don't like name for enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one thing - mitigates SSBD. So, maybe more appropriate name will be something like "mitigate_ssbd()" ? > 2) enter_hypervisor_from_guest() called with interrupts unmasked. > > Note that while enter_hypervisor_from_guest_noirq() does not use the > on-stack context registers, it is still passed as parameter to match the > rest of the C functions called from the entry path. As I pointed in the previous email, enter_hypervisor_from_guest() does not use on-stack registers as well. > Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests") > Reported-by: Andrii Anisov > Signed-off-by: Julien Grall > > --- > > Note the Arm32 code has not been changed yet. I am also open on turn > both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from() > to functions not taking any parameters. That would be appropriate in my opinion. > --- > xen/arch/arm/arm64/entry.S | 2 ++ > xen/arch/arm/traps.c | 16 +--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 9eafae516b..458d12f188 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -173,6 +173,8 @@ > ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > "nop; nop", > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > +mov x0, sp > +bl enter_hypervisor_from_guest_noirq > msr daifclr, \iflags > mov x0, sp > bl enter_hypervisor_from_guest > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 20ba34ec91..5848dd8399 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v) > } > > /* > - * Actions that needs to be done after exiting the guest and before any > - * request from it is handled. > + * Actions that needs to be done after exiting the guest and before the > + * interrupts are unmasked. > */ > -void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs) > { > struct vcpu *v = current; > > /* If the guest has disabled the workaround, bring it back on. */ > if ( needs_ssbd_flip(v) ) > arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > +} > + > +/* > + * Actions that needs to be done after exiting the guest and before any > + * request from it is handled. Depending on the exception trap, this may > + * be called with interrupts unmasked. > + */ > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > +{ > +struct vcpu *v = current; > > /* > * If we pended a virtual abort, preserve it until it gets cleared. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if
Julien Grall writes: > Using alternative_if makes the code a bit more streamlined. > > Take the opportunity to use the new auto-nop infrastructure to avoid > counting the number of nop in the else part for arch/arm/arm64/entry.S > > Signed-off-by: Julien Grall > > --- > This is pretty much a matter of taste, but at least for arm64 this > allows us to use the auto-nop infrastructure. So the arm32 is more > to keep inline with arm64. > --- > xen/arch/arm/arm32/entry.S | 9 ++--- > xen/arch/arm/arm64/entry.S | 8 +--- > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S > index 0b4cd19abd..1428cd3583 100644 > --- a/xen/arch/arm/arm32/entry.S > +++ b/xen/arch/arm/arm32/entry.S > @@ -65,9 +65,12 @@ save_guest_regs: > * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu > * feature, the checking of pending SErrors will be skipped. > */ > -ALTERNATIVE("nop", > -"b skip_check", > -SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > +alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > +nop > +alternative_else > + b skip_check > +alternative_endif > + for the arm32 code you can have my r-b: Reviewed-By: Volodymyr Babchuk > /* > * Start to check pending virtual abort in the gap of Guest -> HYP > * world switch. > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 458d12f188..91cf6ee6f4 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -170,9 +170,11 @@ > * is not set. If a vSError took place, the initial exception will be > * skipped. Exit ASAP > */ > -ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > -"nop; nop", > -SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > +alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > +bl check_pending_vserror > +cbnzx0, 1f > +alternative_else_nop_endif > + You asked other people to do not introduce new code in one patch and rewrite it in the following patch. But there you are doing exactly the same. I believe, it is possible to move all "alternative" patches to the very beginning of the patch series and only then introduce macro guest_vector. > mov x0, sp > bl enter_hypervisor_from_guest_noirq > msr daifclr, \iflags -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path
Julien Grall writes: > On 27/09/2019 12:45, Volodymyr Babchuk wrote: >> >> Julien, > > Hi... > >> Julien Grall writes: >> >>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are >>> used to deal with actions to be done before/after any guest request is >>> handled. >>> >>> While they are meant to work in pair, the former is called for most of >>> the traps, including traps from the same exception level (i.e. >>> hypervisor) whilst the latter will only be called when returning to the >>> guest. >>> >>> As pointed out, the enter_hypervisor_head() is not called from all the >>> traps, so this makes potentially difficult to extend it for the dealing >>> with same exception level. >>> >>> Furthermore, some assembly only path will require to call >>> enter_hypervisor_tail(). So the function is now directly call by >>> assembly in for guest vector only. This means that the check whether we >>> are called in a guest trap can now be removed. >>> >>> Take the opportunity to rename enter_hypervisor_tail() and >>> leave_hypervisor_tail() to something more meaningful and document them. >>> This should help everyone to understand the purpose of the two >>> functions. >>> >>> Signed-off-by: Julien Grall >>> >>> --- >>> >>> I haven't done the 32-bits part yet. I wanted to gather feedback before >>> looking in details how to integrate that with Arm32. >> I'm looking at patches one by one and it is looking okay so far. >> >> >>> --- >>> xen/arch/arm/arm64/entry.S | 4 ++- >>> xen/arch/arm/traps.c | 71 >>> ++ >>> 2 files changed, 37 insertions(+), 38 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >>> index 40d9f3ec8c..9eafae516b 100644 >>> --- a/xen/arch/arm/arm64/entry.S >>> +++ b/xen/arch/arm/arm64/entry.S >>> @@ -147,7 +147,7 @@ >>> >>> .if \hyp == 0 /* Guest mode */ >>> >>> -bl leave_hypervisor_tail /* Disables interrupts on return */ >>> +bl leave_hypervisor_to_guest /* Disables interrupts on return >>> */ >>> >>> exit_guest \compat >>> >>> @@ -175,6 +175,8 @@ >>> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>> msr daifclr, \iflags >>> mov x0, sp >> Looks like this mov can be removed (see commend below). >> >>> +bl enter_hypervisor_from_guest >>> +mov x0, sp >>> bl do_trap_\trap >>> 1: >>> exithyp=0, compat=\compat >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index a3b961bd06..20ba34ec91 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v) >>>cpu_require_ssbd_mitigation(); >>> } >>> >>> -static void enter_hypervisor_head(struct cpu_user_regs *regs) >>> +/* >>> + * Actions that needs to be done after exiting the guest and before any >>> + * request from it is handled. >> Maybe it is me only, but the phrasing is confusing. I had to read it two >> times before I get it. What about "Actions that needs to be done when >> raising exception level"? Or maybe "Actions that needs to be done when >> switching from guest to hypervisor mode" ? > > Is it a suggestion to replace the full sentence or just the first > before (i.e. before 'and')? This is a suggestion for the first part. >> >>> + */ >>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) >> With the guest_mode(regs) check removal , this function does not use regs >> anymore. > > I have nearly done it while working on the series, but then I thought > that it would be better keep all the functions called from the entry > path in assembly similar. This can save one assembly instruction in the entry path. But I'm not sure if it is worth it. So it is up to you. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Julien Grall writes: > On 27/09/2019 12:56, Volodymyr Babchuk wrote: >> >> Julien, > > Hi... > >> >> Julien Grall writes: >> >>> At the moment, SSBD workaround is re-enabled for Xen after interrupts >>> are unmasked. This means we may end up to execute some part of the >>> hypervisor if an interrupt is received before the workaround is >>> re-enabled. >>> >>> As the rest of enter_hypervisor_from_guest() does not require to have >>> interrupts masked, the function is now split in two parts: >>> 1) enter_hypervisor_from_guest_noirq() called with interrupts >>> masked. >> I'm okay with this approach, but I don't like name for >> enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one >> thing - mitigates SSBD. So, maybe more appropriate name will be >> something like "mitigate_ssbd()" ? > > If I wanted to call it mitigate_ssbd() I would have implemented > completely differently. The reason it is like that is because we may > need more code to be added here in the future (I have Andrii's series > in mind). So I would rather avoid a further renaming later on and some > rework. Fair enough > > Regarding the name, this is a split of > enter_hypervisor_from_guest(). Hence, why the first path is the > same. The noirq merely help the user to know what to expect. This is > better of yet an __ version. Feel free to suggest a better suffix. I'm bad at naming things :) I understand that is two halves of one function. But func_name_noirq() pattern is widely used for other case: when we have func_name_noirq() function and some func_name() that disables interrupts like this: void func_name() { disable_irqs(); func_name_noirq(); enable_irqs(); } I like principle of least surprise, so it is better to use some other naming pattern there. maybe something like enter_hypervisor_from_guest_pt1() and enter_hypervisor_from_guest_pt2()? Or maybe, we should not split the function at all? Instead, we enable interrupts right in the middle of it. > >> >>> 2) enter_hypervisor_from_guest() called with interrupts unmasked. >>> >>> Note that while enter_hypervisor_from_guest_noirq() does not use the >>> on-stack context registers, it is still passed as parameter to match the >>> rest of the C functions called from the entry path. >> As I pointed in the previous email, enter_hypervisor_from_guest() does >> not use on-stack registers as well. > > I am well aware of this, hence my comment here in the commit message > ;). The reason it is like that is because I wanted to keep the > prototype the same for all functions called from the entry path (this > includes do_trap_*). Let's continue those discussion in the other thread. [...] -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if
Julien Grall writes: > Hi, > > On 27/09/2019 13:11, Volodymyr Babchuk wrote: >> >> >> Julien Grall writes: >> >>> Using alternative_if makes the code a bit more streamlined. >>> >>> Take the opportunity to use the new auto-nop infrastructure to avoid >>> counting the number of nop in the else part for arch/arm/arm64/entry.S >>> >>> Signed-off-by: Julien Grall >>> >>> --- >>> This is pretty much a matter of taste, but at least for arm64 this >>> allows us to use the auto-nop infrastructure. So the arm32 is more >>> to keep inline with arm64. >>> --- >>> xen/arch/arm/arm32/entry.S | 9 ++--- >>> xen/arch/arm/arm64/entry.S | 8 +--- >>> 2 files changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S >>> index 0b4cd19abd..1428cd3583 100644 >>> --- a/xen/arch/arm/arm32/entry.S >>> +++ b/xen/arch/arm/arm32/entry.S >>> @@ -65,9 +65,12 @@ save_guest_regs: >>>* If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the >>> cpu >>>* feature, the checking of pending SErrors will be skipped. >>>*/ >>> -ALTERNATIVE("nop", >>> -"b skip_check", >>> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>> +alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >>> +nop >>> +alternative_else >>> +b skip_check >>> +alternative_endif >>> + >> for the arm32 code you can have my r-b: >> Reviewed-By: Volodymyr Babchuk >> >>> /* >>>* Start to check pending virtual abort in the gap of Guest -> HYP >>>* world switch. >>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >>> index 458d12f188..91cf6ee6f4 100644 >>> --- a/xen/arch/arm/arm64/entry.S >>> +++ b/xen/arch/arm/arm64/entry.S >>> @@ -170,9 +170,11 @@ >>>* is not set. If a vSError took place, the initial exception >>> will be >>>* skipped. Exit ASAP >>>*/ >>> -ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", >>> -"nop; nop", >>> -SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>> +alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >>> +bl check_pending_vserror >>> +cbnzx0, 1f >>> +alternative_else_nop_endif >>> + >> You asked other people to do not introduce new code in one patch and >> rewrite it in the following patch. But there you are doing exactly the >> same. > > This is a fairly borderline comment knowing that I usually don't > request clean-up and code consolidation in the same patch. I understand this. Also I understand why are you asking for clean-up. No one likes to review the same code twice. Anyways, I not wanted to be offensive. Sorry for that. >> I believe, it is possible to move all "alternative" patches to the >> very beginning of the patch series and only then introduce macro >> guest_vector. > > For a first, the first patch is definitely not new code. This is code > consolidation and therefore I don't tend to mix the two for > clarity. So this should have been a patch before the first patch. > > Secondly, the first 4 patches are candidate for backport. The rest of > the series would be good to backport but I am not aware of a critical > issue in previous Xen release to strongly push for it. I see. Yes, I'm always forgetting about backporting :( So, for the rest of the patch: Reviewed-by: Volodymyr Babchuk -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path
Julien Grall writes: > Hi, > > On 27/09/2019 13:27, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> On 27/09/2019 12:45, Volodymyr Babchuk wrote: >>>> >>>> Julien, >>> >>> Hi... >>> >>>> Julien Grall writes: >>>> >>>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are >>>>> used to deal with actions to be done before/after any guest request is >>>>> handled. >>>>> >>>>> While they are meant to work in pair, the former is called for most of >>>>> the traps, including traps from the same exception level (i.e. >>>>> hypervisor) whilst the latter will only be called when returning to the >>>>> guest. >>>>> >>>>> As pointed out, the enter_hypervisor_head() is not called from all the >>>>> traps, so this makes potentially difficult to extend it for the dealing >>>>> with same exception level. >>>>> >>>>> Furthermore, some assembly only path will require to call >>>>> enter_hypervisor_tail(). So the function is now directly call by >>>>> assembly in for guest vector only. This means that the check whether we >>>>> are called in a guest trap can now be removed. >>>>> >>>>> Take the opportunity to rename enter_hypervisor_tail() and >>>>> leave_hypervisor_tail() to something more meaningful and document them. >>>>> This should help everyone to understand the purpose of the two >>>>> functions. >>>>> >>>>> Signed-off-by: Julien Grall >>>>> >>>>> --- >>>>> >>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before >>>>> looking in details how to integrate that with Arm32. >>>> I'm looking at patches one by one and it is looking okay so far. >>>> >>>> >>>>> --- >>>>>xen/arch/arm/arm64/entry.S | 4 ++- >>>>>xen/arch/arm/traps.c | 71 >>>>> ++ >>>>>2 files changed, 37 insertions(+), 38 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >>>>> index 40d9f3ec8c..9eafae516b 100644 >>>>> --- a/xen/arch/arm/arm64/entry.S >>>>> +++ b/xen/arch/arm/arm64/entry.S >>>>> @@ -147,7 +147,7 @@ >>>>> >>>>>.if \hyp == 0 /* Guest mode */ >>>>> >>>>> -bl leave_hypervisor_tail /* Disables interrupts on return */ >>>>> +bl leave_hypervisor_to_guest /* Disables interrupts on >>>>> return */ >>>>> >>>>>exit_guest \compat >>>>> >>>>> @@ -175,6 +175,8 @@ >>>>>SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>>>>msr daifclr, \iflags >>>>>mov x0, sp >>>> Looks like this mov can be removed (see commend below). >>>> >>>>> +bl enter_hypervisor_from_guest >>>>> +mov x0, sp >>>>>bl do_trap_\trap >>>>>1: >>>>>exithyp=0, compat=\compat >>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>>>> index a3b961bd06..20ba34ec91 100644 >>>>> --- a/xen/arch/arm/traps.c >>>>> +++ b/xen/arch/arm/traps.c >>>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v) >>>>> cpu_require_ssbd_mitigation(); >>>>>} >>>>> >>>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs) >>>>> +/* >>>>> + * Actions that needs to be done after exiting the guest and before any >>>>> + * request from it is handled. >>>> Maybe it is me only, but the phrasing is confusing. I had to read it two >>>> times before I get it. What about "Actions that needs to be done when >>>> raising exception level"? Or maybe "Actions that needs to be done when >>>> switching from guest to hypervisor mode" ? >>> >>> Is it a suggestion to replace the full sentence or just the first >>> before (i.e. before 'and')? >> This is a suggestion for the first part. > > How about: > > "Actions that needs to be done after entering the hypervisor from the > guest and before we handle any request." Sound perfect. [...] -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Hi, Julien Grall writes: > Hi, > > On 27/09/2019 13:39, Volodymyr Babchuk wrote: >> Julien Grall writes: >>> On 27/09/2019 12:56, Volodymyr Babchuk wrote: >>>> Julien Grall writes: >>>> >>>>> At the moment, SSBD workaround is re-enabled for Xen after interrupts >>>>> are unmasked. This means we may end up to execute some part of the >>>>> hypervisor if an interrupt is received before the workaround is >>>>> re-enabled. >>>>> >>>>> As the rest of enter_hypervisor_from_guest() does not require to have >>>>> interrupts masked, the function is now split in two parts: >>>>> 1) enter_hypervisor_from_guest_noirq() called with interrupts >>>>> masked. >>>> I'm okay with this approach, but I don't like name for >>>> enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one >>>> thing - mitigates SSBD. So, maybe more appropriate name will be >>>> something like "mitigate_ssbd()" ? >>> >>> If I wanted to call it mitigate_ssbd() I would have implemented >>> completely differently. The reason it is like that is because we may >>> need more code to be added here in the future (I have Andrii's series >>> in mind). So I would rather avoid a further renaming later on and some >>> rework. >> Fair enough >> >>> >>> Regarding the name, this is a split of >>> enter_hypervisor_from_guest(). Hence, why the first path is the >>> same. The noirq merely help the user to know what to expect. This is >>> better of yet an __ version. Feel free to suggest a better suffix. >> I'm bad at naming things :) > > Me too ;). > >> >> I understand that is two halves of one function. But func_name_noirq() >> pattern is widely used for other case: when we have func_name_noirq() >> function and some func_name() that disables interrupts like this: >> >> void func_name() >> { >> disable_irqs(); >> func_name_noirq(); >> enable_irqs(); >> } >> >> I like principle of least surprise, so it is better to use some other >> naming pattern there. > > I can't find any function suffixed with _noirq in Xen. So I don't > think this would be a major issue here. Yes, there are no such functions in Xen. But it may confuse developers who come from another projects. >> >> maybe something like enter_hypervisor_from_guest_pt1() and >> enter_hypervisor_from_guest_pt2()? > Hmmm, it reminds me uni when we had to limit function size to 20 lines :). > > I chose _noirq because the other name I had in mind was quite > verbose. I was thinking: > enter_hypervisor_from_guest_before_interrupts(). A was thinking about something like this too. What about enter_hypervisor_from_guest_preirq()? I think that "_pre" better shows the relation to enter_hypervisor_from_guest() > >> >> Or maybe, we should not split the function at all? Instead, we enable >> interrupts right in the middle of it. > > I thought about this but I didn't much like the resulting code. > > The instruction to unmask interrupts requires to take an immediate > (indicates which interrupts to unmask). As not all the traps require > to unmask the same interrupts, we would end up to have to a bunch of > if in the code to select the right unmasking. Ah, yes, this is the problem. We can provide callback to enter_hypervisor_from_guest(). Or switch() instead of multiple ifs. Maybe in some helper function. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Julien Grall writes: > On 27/09/2019 14:33, Volodymyr Babchuk wrote: >> Julien Grall writes: >>> On 27/09/2019 13:39, Volodymyr Babchuk wrote: >>>> Julien Grall writes: >>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote: >>>>>> Julien Grall writes: >>>>>> >>>>>>> At the moment, SSBD workaround is re-enabled for Xen after interrupts >>>>>>> are unmasked. This means we may end up to execute some part of the >>>>>>> hypervisor if an interrupt is received before the workaround is >>>>>>> re-enabled. >>>>>>> >>>>>>> As the rest of enter_hypervisor_from_guest() does not require to have >>>>>>> interrupts masked, the function is now split in two parts: >>>>>>>1) enter_hypervisor_from_guest_noirq() called with interrupts >>>>>>> masked. >>>>>> I'm okay with this approach, but I don't like name for >>>>>> enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one >>>>>> thing - mitigates SSBD. So, maybe more appropriate name will be >>>>>> something like "mitigate_ssbd()" ? >>>>> >>>>> If I wanted to call it mitigate_ssbd() I would have implemented >>>>> completely differently. The reason it is like that is because we may >>>>> need more code to be added here in the future (I have Andrii's series >>>>> in mind). So I would rather avoid a further renaming later on and some >>>>> rework. >>>> Fair enough >>>> >>>>> >>>>> Regarding the name, this is a split of >>>>> enter_hypervisor_from_guest(). Hence, why the first path is the >>>>> same. The noirq merely help the user to know what to expect. This is >>>>> better of yet an __ version. Feel free to suggest a better suffix. >>>> I'm bad at naming things :) >>> >>> Me too ;). >>> >>>> >>>> I understand that is two halves of one function. But func_name_noirq() >>>> pattern is widely used for other case: when we have func_name_noirq() >>>> function and some func_name() that disables interrupts like this: >>>> >>>> void func_name() >>>> { >>>> disable_irqs(); >>>> func_name_noirq(); >>>> enable_irqs(); >>>> } >>>> >>>> I like principle of least surprise, so it is better to use some other >>>> naming pattern there. >>> >>> I can't find any function suffixed with _noirq in Xen. So I don't >>> think this would be a major issue here. >> Yes, there are no such functions in Xen. But it may confuse developers >> who come from another projects. > > Well, each projects have their own style. So there are always some > adaptations needed to move to a new project. What matters is the > documentation clarifies what is the exact use. But... > >> >>>> >>>> maybe something like enter_hypervisor_from_guest_pt1() and >>>> enter_hypervisor_from_guest_pt2()? >>> Hmmm, it reminds me uni when we had to limit function size to 20 lines :). >>> >>> I chose _noirq because the other name I had in mind was quite >>> verbose. I was thinking: >>> enter_hypervisor_from_guest_before_interrupts(). >> A was thinking about something like this too. >> What about enter_hypervisor_from_guest_preirq()? > > ... this would be indeed better. >> >> I think that "_pre" better shows the relation to >> enter_hypervisor_from_guest() >> >>> >>>> >>>> Or maybe, we should not split the function at all? Instead, we enable >>>> interrupts right in the middle of it. >>> >>> I thought about this but I didn't much like the resulting code. >>> >>> The instruction to unmask interrupts requires to take an immediate >>> (indicates which interrupts to unmask). As not all the traps require >>> to unmask the same interrupts, we would end up to have to a bunch of >>> if in the code to select the right unmasking. >> Ah, yes, this is the problem. We can provide callback to >> enter_hypervisor_from_guest(). > > I am not sure what you mean by this. Do you mean a callback that will > unmask the interrupts? Yes. You can pass function pointer to enter_hypervisor_from_guest(). To a function, that will unmask the interrupts. I'm sure that guest_vector macro can generate it for you. Something like this: .macro guest_vector compat, iflags, trap, save_x0_x1=1 entry hyp=0, compat=\compat, save_x0_x1=\save_x0_x1 /* * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT * is not set. If a vSError took place, the initial exception will be * skipped. Exit ASAP */ ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", "nop; nop", SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) ldr x0, =1f bl enter_hypervisor_from_guest mov x0, sp bl do_trap_\trap b 1f 2: msr daifclr, \iflags ret 1: exithyp=0, compat=\compat .endm -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 10/10] xen/arm64: entry: Ensure the guest state is synced when receiving a vSError
Hi Julien, Julien Grall writes: > At the moment, when a SError is received while checking for a pending > one, we will skip the handling the initial exception. > > This includes call to exit_from_guest{, _noirq} that is used to Did you mean enter_hypervisor_from_guest{, _noirq} there? Otherwise, I'm confused a lot. > synchronize part of the guest state with the internal representation. > However, we still call leave_hypervisor_tail() which is used for preempting > the guest and synchronizing back part of the guest state. > > exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so > skipping if former may result to a loss of some part of guest state. > An example is the new vGIC which will save the state of the LRS on exit > from the guest and rewrite all of them on entry to the guest. > > For now, calling leave_hypervisor_tail() is not necessary when injecting > a vSError to the guest. But as the path is spread accross multiple file, > it is hard to enforce that for the future (someone we may want to crash the > domain). Therefore it is best to call exit_from_guest{, _noirq} in the > vSError path as well. > > Note that the return value of check_pending_vserror is now set in x19 > instead of x0. This is because we want to keep the value across call to > C-function and x0, unlike x19, will not be saved by the callee. > > Signed-off-by: Julien Grall > > --- > > I am not aware of any issues other than with the new vGIC. But I > haven't looked hard enough so I think it would be worth to try to fix it > for Xen 4.13. > --- > xen/arch/arm/arm64/entry.S | 21 ++--- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 91cf6ee6f4..f5350247e1 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -168,11 +168,13 @@ > /* > * The vSError will be checked while > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > * is not set. If a vSError took place, the initial exception will be > - * skipped. Exit ASAP > + * skipped. > + * > + * However, we still need to call exit_from_guest{,_noirq} as the > + * return path to the guest may rely on state saved by them. > */ > alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > bl check_pending_vserror > -cbnzx0, 1f > alternative_else_nop_endif > > mov x0, sp > @@ -180,6 +182,11 @@ > msr daifclr, \iflags > mov x0, sp > bl enter_hypervisor_from_guest > + > +alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > +cbnzx19, 1f > +alternative_else_nop_endif > + > mov x0, sp > bl do_trap_\trap > 1: > @@ -383,9 +390,9 @@ return_from_trap: > /* > * This function is used to check pending virtual SError in the gap of > * EL1 -> EL2 world switch. > - * The x0 register will be used to indicate the results of detection. > - * x0 -- Non-zero indicates a pending virtual SError took place. > - * x0 -- Zero indicates no pending virtual SError took place. > + * The register x19 will be used to indicate the results of detection. > + * x19 -- Non-zero indicates a pending virtual SError took place. > + * x19 -- Zero indicates no pending virtual SError took place. > */ > check_pending_vserror: > /* > @@ -432,9 +439,9 @@ abort_guest_exit_end: > > /* > * Not equal, the pending SError exception took place, set > - * x0 to non-zero. > + * x19 to non-zero. > */ > -csetx0, ne > +csetx19, ne > > ret -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 08/10] xen/arm: alternative: add auto-nop infrastructure
Julien Grall writes: > From: Mark Rutland > > In some cases, one side of an alternative sequence is simply a number of > NOPs used to balance the other side. Keeping track of this manually is > tedious, and the presence of large chains of NOPs makes the code more > painful to read than necessary. > > To ameliorate matters, this patch adds a new alternative_else_nop_endif, > which automatically balances an alternative sequence with a trivial NOP > sled. > > In many cases, we would like a NOP-sled in the default case, and > instructions patched in in the presence of a feature. To enable the NOPs > to be generated automatically for this case, this patch also adds a new > alternative_if, and updates alternative_else and alternative_endif to > work with either alternative_if or alternative_endif. > > The alternative infrastructure was originally ported from Linux. So this > is pretty much a straight backport from commit 792d47379f4d "arm64: > alternative: add auto-nop infrastructure". The only difference is the > nops macro added as not yet existing in Xen. > > Signed-off-by: Mark Rutland > [will: use new nops macro to generate nop sequences] > Signed-off-by: Will Deacon > [julien: Add nops and port to Xen] > Signed-off-by: Julien Grall Reviewed-by: Volodymyr Babchuk -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Julien Grall writes: > Hi, > > On 27/09/2019 15:21, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> On 27/09/2019 14:33, Volodymyr Babchuk wrote: >>>> Julien Grall writes: >>>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote: >>>>>> Julien Grall writes: >>>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote: >>>>>>>> Julien Grall writes: >>>>>> Or maybe, we should not split the function at all? Instead, we enable >>>>>> interrupts right in the middle of it. >>>>> >>>>> I thought about this but I didn't much like the resulting code. >>>>> >>>>> The instruction to unmask interrupts requires to take an immediate >>>>> (indicates which interrupts to unmask). As not all the traps require >>>>> to unmask the same interrupts, we would end up to have to a bunch of >>>>> if in the code to select the right unmasking. >>>> Ah, yes, this is the problem. We can provide callback to >>>> enter_hypervisor_from_guest(). >>> >>> I am not sure what you mean by this. Do you mean a callback that will >>> unmask the interrupts? >> Yes. You can pass function pointer to enter_hypervisor_from_guest(). To >> a function, that will unmask the interrupts. I'm sure that guest_vector >> macro can generate it for you. Something like this: >> >> .macro guest_vector compat, iflags, trap, save_x0_x1=1 >> entry hyp=0, compat=\compat, save_x0_x1=\save_x0_x1 >> /* >> * The vSError will be checked while >> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >> * is not set. If a vSError took place, the initial exception will >> be >> * skipped. Exit ASAP >> */ >> ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", >> "nop; nop", >> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >> ldr x0, =1f >> bl enter_hypervisor_from_guest >> mov x0, sp >> bl do_trap_\trap >> b 1f >> 2: >> msr daifclr, \iflags >> ret >> 1: >> exithyp=0, compat=\compat >> .endm > > TBH, I don't see what's the point you are trying to make here. Yes, > there are many way to write a code and possibility to make one > function. You could also create a skeleton macro for > enter_hypervisor_from_guest and generate N of them (one per set of > unmask interrupts) and call them. > > But are they really worth it? The point is that you are trying to split one entity into two. As I see it: semantically we have one function: enter_hypervisor_from_guest(). The purpose of this function is clear: finish transition from guest mode to hypervisor mode. But because of some architectural limitation (daifclr requires only immediate argument) you are forced to divide this function in two. I don't like this, because entry path now is more complex. To follow what is going you need to bounce between head.S and traps.c one extra time. Anyways, this is only my opinion. I'm not forcing you to implement enter_hypervisor_from_guest() in this way. I'm okay with enter_hypervisor_from_guest_preirq() as well. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Hi Julien, Julien Grall writes: > At the moment, SSBD workaround is re-enabled for Xen after interrupts > are unmasked. This means we may end up to execute some part of the > hypervisor if an interrupt is received before the workaround is > re-enabled. > > As the rest of enter_hypervisor_from_guest() does not require to have > interrupts masked, the function is now split in two parts: > 1) enter_hypervisor_from_guest_noirq() called with interrupts >masked. To summarize our discussion in this mail thread: providing that you'll rename enter_hypervisor_from_guest_noirq to enter_hypervisor_from_guest_preirq(): Reviewed-by: Volodymyr Babchuk > 2) enter_hypervisor_from_guest() called with interrupts unmasked. > > Note that while enter_hypervisor_from_guest_noirq() does not use the > on-stack context registers, it is still passed as parameter to match the > rest of the C functions called from the entry path. > > Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests") > Reported-by: Andrii Anisov > Signed-off-by: Julien Grall > > --- > > Note the Arm32 code has not been changed yet. I am also open on turn > both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from() > to functions not taking any parameters. > --- > xen/arch/arm/arm64/entry.S | 2 ++ > xen/arch/arm/traps.c | 16 +--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 9eafae516b..458d12f188 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -173,6 +173,8 @@ > ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > "nop; nop", > SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > +mov x0, sp > +bl enter_hypervisor_from_guest_noirq > msr daifclr, \iflags > mov x0, sp > bl enter_hypervisor_from_guest > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 20ba34ec91..5848dd8399 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v) > } > > /* > - * Actions that needs to be done after exiting the guest and before any > - * request from it is handled. > + * Actions that needs to be done after exiting the guest and before the > + * interrupts are unmasked. > */ > -void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs) > { > struct vcpu *v = current; > > /* If the guest has disabled the workaround, bring it back on. */ > if ( needs_ssbd_flip(v) ) > arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > +} > + > +/* > + * Actions that needs to be done after exiting the guest and before any > + * request from it is handled. Depending on the exception trap, this may > + * be called with interrupts unmasked. > + */ > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > +{ > +struct vcpu *v = current; > > /* > * If we pended a virtual abort, preserve it until it gets cleared. -- Volodymyr Babchuk at EPAM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 1/2] makefile: add support for *_defconfig targets
Ease up XEN configuration for non-standard builds, like armv8 tiny config. Signed-off-by: Volodymyr Babchuk --- Makefile | 4 xen/Makefile | 3 +++ 2 files changed, 7 insertions(+) diff --git a/Makefile b/Makefile index 829ac63741..ef1ea44ef1 100644 --- a/Makefile +++ b/Makefile @@ -54,6 +54,10 @@ build: $(TARGS_BUILD) build-xen: $(MAKE) -C xen build +.PHONY: %_defconfig +%_defconfig: + $(MAKE) -C xen $@ + .PHONY: build-tools build-tools: build-tools-public-headers $(MAKE) -C tools build diff --git a/xen/Makefile b/xen/Makefile index 1fd8ad5116..3c7e423132 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -269,6 +269,9 @@ kconfig := silentoldconfig oldconfig config menuconfig defconfig \ $(kconfig): $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@ +%_defconfig: + $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@ + include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG) $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" silentoldconfig -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 2/2] arm: rename tiny64.conf to tiny64_defconfig
As build system now supports *_defconfig rules it is good to be able to configure minimal XEN image with make tiny64_defconfig command. Signed-off-by: Volodymyr Babchuk Suggested-by: Andrii Anisov --- xen/arch/arm/configs/{tiny64.conf => tiny64_defconfig} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename xen/arch/arm/configs/{tiny64.conf => tiny64_defconfig} (100%) diff --git a/xen/arch/arm/configs/tiny64.conf b/xen/arch/arm/configs/tiny64_defconfig similarity index 100% rename from xen/arch/arm/configs/tiny64.conf rename to xen/arch/arm/configs/tiny64_defconfig -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1 1/2] makefile: add support for *_defconfig targets
Hello Jan, Jan Beulich writes: >>>> On 14.05.19 at 20:45, wrote: >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -269,6 +269,9 @@ kconfig := silentoldconfig oldconfig config menuconfig >> defconfig \ >> $(kconfig): >> $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) >> SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@ > > So the rule you add matches this one. Is there a reason ... > >> +%_defconfig: > > ... why you can't simply add this to the kconfig variable set a few > lines up? Oh - newer make doesn't like mixing pattern and > non-pattern rules. Yes, my first intention was to add this rule to the kconfig variable. Sadly make does not allows this > Perhaps worth a brief comment, to justify the > redundancy? Sure, will add in the next version. > Or alternatively, how about using $(wildcard ) > instead of a pattern rule, thus rejecting invalid targets right away, > rather than deferring to the recursive make to notice the error? I considered this, but I can't see how $(wildcard ) can be used. AFAIK, $(wildcard ) expects to find a file, matching the wildcard. But %_defconfig is the phony rule, so I can't imagine how to use $(wildcard ) in this case. On other hand, following rule checks the presence of required _defconfig file: %_defconfig: arch/$(SRCARCH)/configs/%_defconfig So, I can do in this way if you wish. BTW, I'll add .PHONY: %_defconfig in the next version. -- Best regards, Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig
As build system now supports *_defconfig rules it is good to be able to configure minimal XEN image with make tiny64_defconfig command. Signed-off-by: Volodymyr Babchuk --- xen/arch/arm/configs/{tiny64.conf => tiny64_defconfig} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename xen/arch/arm/configs/{tiny64.conf => tiny64_defconfig} (100%) diff --git a/xen/arch/arm/configs/tiny64.conf b/xen/arch/arm/configs/tiny64_defconfig similarity index 100% rename from xen/arch/arm/configs/tiny64.conf rename to xen/arch/arm/configs/tiny64_defconfig -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 1/2] makefile: add support for *_defconfig targets
Ease up XEN configuration for non-standard builds, like armv8 tiny config. Signed-off-by: Volodymyr Babchuk --- Changes from v2: - remove %_defconfig rule in favor of list of real *_defconfig files Makefile | 4 xen/Makefile | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 829ac63741..ef1ea44ef1 100644 --- a/Makefile +++ b/Makefile @@ -54,6 +54,10 @@ build: $(TARGS_BUILD) build-xen: $(MAKE) -C xen build +.PHONY: %_defconfig +%_defconfig: + $(MAKE) -C xen $@ + .PHONY: build-tools build-tools: build-tools-public-headers $(MAKE) -C tools build diff --git a/xen/Makefile b/xen/Makefile index 1fd8ad5116..c80914c31d 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -264,7 +264,7 @@ $(foreach base,arch/x86/mm/guest_walk_% \ kconfig := silentoldconfig oldconfig config menuconfig defconfig \ nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig \ - randconfig + randconfig $(notdir $(wildcard arch/$(SRCARCH)/configs/*_defconfig)) .PHONY: $(kconfig) $(kconfig): $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@ -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig
Hi Julien, Julien Grall writes: > Hi, > > First of all, please add a cover letter when you send a series. This > help for threading and also a place to commend on general feedback. Oh, okay. That was quite simple change and I didn't wanted to spam with extra emails. I will include cover letter next time. > Furthermore, please use scripts/{add, get}_maintainers.pl to find the > correct maintainers. While I agree that CCing REST is a good idea, you > haven't CCed all of them. Problem is that I used this script: $ ./scripts/get_maintainer.pl -f defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch Andrew Cooper George Dunlap Ian Jackson Jan Beulich Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Tim Deegan Wei Liu xen-devel@lists.xenproject.org I was quite surprised by result myself. Honestly, I wanted to CC only you and Stefano, but decided to play by the rules. Also, add_maintainers.pl just ignores this patch at all: % scripts/add_maintainers.pl -v 2 -d defconfig_v2 Processing: v2-0001-makefile-add-support-for-_defconfig-targets.patch Processing: v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch. Add -f to options? > > On 16/05/2019 14:37, Volodymyr Babchuk wrote: >> As build system now supports *_defconfig rules it is good to be able >> to configure minimal XEN image with > > I am afraid this is not correct. tiny64 will not be able to generate a > minimal config to boot on any platform supported by Xen. > > It is meant to be used as a base for tailoring your platform where all > the options are turned off by default. > > So I think offering a direct access is likely going to be misused in > most of the cases without proper documentation. In the original commit message Stefano suggested to use olddefconfig: " Add a tiny kconfig configuration. Enabled only the credit scheduler. It only carries non-default options (use make menuconfig or make olddefconfig to produce a complete .config file). " I don't see any significant difference between # cp tiny64.conf .config && make olddefconfig and # make tiny64_defconfig Anyways, it is up to you to accept or decline this particular patch. I mostly interested in the first patch in the series, because our build system depends on it. This very patch I sent out only because I wanted to tidy up things a bit. But if you are saying that it is intended to store minimal config in this way, I'm okay with it. -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig
Julien Grall writes: > Hi, > > On 20/05/2019 14:41, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >>> Hi, >>> >>> First of all, please add a cover letter when you send a series. This >>> help for threading and also a place to commend on general feedback. >> Oh, okay. That was quite simple change and I didn't wanted to spam with >> extra emails. I will include cover letter next time. >> >>> Furthermore, please use scripts/{add, get}_maintainers.pl to find the >>> correct maintainers. While I agree that CCing REST is a good idea, you >>> haven't CCed all of them. >> Problem is that I used this script: >> >> $ ./scripts/get_maintainer.pl -f >> defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch > > -f is to be used on actual file in the source tree. So the result > below makes sense. For actual patch, you have to drop the -f. Ah, I see. Without -f I'm getting the same message as with add-maintainers.pl: % ./scripts/get_maintainer.pl defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch. Add -f to options? [...] >> >> % scripts/add_maintainers.pl -v 2 -d defconfig_v2 >> Processing: v2-0001-makefile-add-support-for-_defconfig-targets.patch >> Processing: v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch >> ./scripts/get_maintainer.pl: file >> 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' >> doesn't appear to be a patch. Add -f to options? > > I have just tried it and can't find the same error. Could you provide > more details? Such as where to do call from the exact content of each > patches... My basic flow: % git format-patch -v2 -2 -o defconfig_v2 % scripts/add_maintainers.pl -v 2 -d defconfig_v2 Processing: v2-0001-makefile-add-support-for-_defconfig-targets.patch Processing: v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch. Add -f to options? Then perform: git send-email -to xen-devel@lists.xenproject.org defconfig_v2/v2-*.patch HEAD (prior to my patches) is at 278c64519c661c851d37e2a929f006fb8a1dcd01 git version 2.21.0 Contents of the patch is the exactly the same as in my original email. You can find both patches at [1]. >> >>> >>> On 16/05/2019 14:37, Volodymyr Babchuk wrote: >>>> As build system now supports *_defconfig rules it is good to be able >>>> to configure minimal XEN image with >>> >>> I am afraid this is not correct. tiny64 will not be able to generate a >>> minimal config to boot on any platform supported by Xen. >>> >>> It is meant to be used as a base for tailoring your platform where all >>> the options are turned off by default. >>> >>> So I think offering a direct access is likely going to be misused in >>> most of the cases without proper documentation. >> >> In the original commit message Stefano suggested to use olddefconfig: >> >> " Add a tiny kconfig configuration. Enabled only the credit scheduler. >> It only carries non-default options (use make menuconfig or make >> olddefconfig to produce a complete .config file). " >> >> I don't see any significant difference between > > Did you actually try the two approach and see how they differ? Yes. I did the following: % cp arch/arm/configs/tiny64_defconfig .config % make olddefconfig make -f /home/lorc/work/xen/xen/tools/kconfig/Makefile.kconfig ARCH=arm64 SRCARCH=arm HOSTCC="gcc" HOSTCXX="g++" olddefconfig make[1]: Entering directory '/home/lorc/work/xen/xen' gcc -Wp,-MD,tools/kconfig/.conf.o.d-D_GNU_SOURCE -DCURSES_LOC="" -DNCURSES_WIDECHAR=1 -DLOCALE -c -o tools/kconfig/conf.o tools/kconfig/conf.c gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d-D_GNU_SOURCE -DCURSES_LOC="" -DNCURSES_WIDECHAR=1 -DLOCALE -Itools/kconfig -c -o tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c gcc -o tools/kconfig/conf tools/kconfig/conf.o tools/kconfig/zconf.tab.o tools/kconfig/conf -s --olddefconfig Kconfig make[1]: Leaving directory '/home/lorc/work/xen/xen' And % make tiny64_defconfig make -f /home/lorc/work/xen/xen/tools/kconfig/Makefile.kconfig ARCH=arm64 SRCARCH=arm HOSTCC="gcc" HOSTCXX="g++" tiny64_defconfig make[1]: Entering directory '/home/lorc/work/xen/xen' gcc -Wp,-MD,tools/kconfig/.conf.o.d-D_GNU_SOURCE
[Xen-devel] [PATCH v5 04/10] xen/arm: optee: add fast calls handling
This patch adds handling for the fast SMCs. As name suggests, those calls can't be preempted and are used for auxiliary tasks such as information retrieval. Most handlers are quite trivial, with exception for capabilities information. Capabilities exchange should be filtered out, so only caps known to mediator are used. Also mediator disables static SHM memory capability, because it can't share OP-TEE memory with a domain. Only domain can share memory with OP-TEE, so it ensures that OP-TEE supports dynamic SHM. Basically, static SHM is a reserved memory region which is always mapped into OP-TEE address space. It belongs to OP-TEE. Normally, NW is allowed to access there, so it can communicate with OP-TEE. On other hand, dynamic SHM is NW's own memory, which it can share with OP-TEE. OP-TEE maps this memory dynamically, when it wants to access it. Because mediator can't share one static SHM region with all guests, it just disables it for all of them. It is possible to make exception for Dom0, but it requires separate handling for buffers allocated from that region. Thus, it is not implemented yet. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Handler does not use forward_call(). Instead it calls OP-TEE directly with arm_smccc_smc(). - Handler modifies only those guest registers that are should be touched according to OP-TEE protocol specification. - Added OPTEE_MEDIATOR_SMC_COUNT definition. Changes from v2: - Defined known capabilities explicitly - Fixed code style --- xen/arch/arm/tee/optee.c | 97 1 file changed, 97 insertions(+) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index e9b69bd2d2..6c51caa41a 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -32,9 +32,17 @@ #include #include +/* Number of SMCs known to the mediator */ +#define OPTEE_MEDIATOR_SMC_COUNT 11 + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) +#define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR +#define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ + OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ + OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) + /* Domain context */ struct optee_domain { }; @@ -120,22 +128,111 @@ static int optee_relinquish_resources(struct domain *d) return 0; } +static void handle_exchange_capabilities(struct cpu_user_regs *regs) +{ +struct arm_smccc_res resp; +uint32_t caps; + +/* Filter out unknown guest caps */ +caps = get_user_reg(regs, 1); +caps &= OPTEE_KNOWN_NSEC_CAPS; + +arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0, + OPTEE_CLIENT_ID(current->domain), &resp); +if ( resp.a0 != OPTEE_SMC_RETURN_OK ) { +set_user_reg(regs, 0, resp.a0); +return; +} + +caps = resp.a1; + +/* Filter out unknown OP-TEE caps */ +caps &= OPTEE_KNOWN_SEC_CAPS; + +/* Drop static SHM_RPC cap */ +caps &= ~OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM; + +/* Don't allow guests to work without dynamic SHM */ +if ( !(caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) ) +{ +set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL); +return; +} + +set_user_reg(regs, 0, OPTEE_SMC_RETURN_OK); +set_user_reg(regs, 1, caps); +} + static bool optee_handle_call(struct cpu_user_regs *regs) { +struct arm_smccc_res resp; + if ( !current->domain->arch.tee ) return false; switch ( get_user_reg(regs, 0) ) { case OPTEE_SMC_CALLS_COUNT: +set_user_reg(regs, 0, OPTEE_MEDIATOR_SMC_COUNT); +return true; + case OPTEE_SMC_CALLS_UID: +arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0, + 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); +return true; + case OPTEE_SMC_CALLS_REVISION: +arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0, + OPTEE_CLIENT_ID(current->domain), &resp); +set_user_reg(regs, 0, resp.a0); +set_user_reg(regs, 1, resp.a1); +return true; + case OPTEE_SMC_CALL_GET_OS_UUID: +arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0, + 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); +return true; + case OPTEE_SMC_CALL_GET_OS_REV
[Xen-devel] [PATCH v5 06/10] xen/arm: optee: add support for RPC SHM buffers
OP-TEE usually uses the same idea with command buffers (see previous commit) to issue RPC requests. Problem is that initially it has no buffer, where it can write request. So the first RPC request it makes is special: it requests NW to allocate shared buffer for other RPC requests. Usually this buffer is allocated only once for every OP-TEE thread and it remains allocated all the time until guest shuts down. Guest can ask OP-TEE to disable RPC buffers caching, in this case OP-TEE will ask guest to allocate/free buffer for the each RPC. Mediator needs to pin this buffer to make sure that page will be not free while it is shared with OP-TEE. Life cycle of this buffer is controlled by OP-TEE. It asks guest to create buffer and it asks it to free it. So it there is not much sense to limit number of those buffers, because we already limit the number of concurrent standard calls and prevention of RPC buffer allocation will impair OP-TEE functionality. Those buffers can be freed in two ways: either OP-TEE issues OPTEE_SMC_RPC_FUNC_FREE RPC request or guest tries to disable buffer caching by calling OPTEE_SMC_DISABLE_SHM_CACHE function. In the latter case OP-TEE will return cookie of the SHM buffer it just freed. OP-TEE expects that this RPC buffer have size of OPTEE_MSG_NONCONTIG_PAGE_SIZE, which equals to 4096 and is aligned with the same size. So, basically it expects one 4k page from the guest. This is the same as Xen's PAGE_SIZE. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - handle_rpc_func_alloc() now calls do_call_with_arg() directly Changes from v3: - Removed MAX_RPC_SHMS constant. Now this value depends on number of OP-TEE threads - Various formatting fixes - Added checks for guest memory type Changes from v2: - Added check to ensure that guests does not return two SHM buffers with the same cookie - Fixed coding style - Storing RPC parameters during RPC return to make sure, that guest will not change them during call continuation --- xen/arch/arm/tee/optee.c | 149 +-- 1 file changed, 145 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index f092492849..175789fb00 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -81,9 +81,17 @@ struct optee_std_call { register_t rpc_params[2]; }; +/* Pre-allocated SHM buffer for RPC commands */ +struct shm_rpc { +struct list_head list; +struct page_info *guest_page; +uint64_t cookie; +}; + /* Domain context */ struct optee_domain { struct list_head call_list; +struct list_head shm_rpc_list; atomic_t call_count; spinlock_t lock; }; @@ -158,6 +166,7 @@ static int optee_domain_init(struct domain *d) } INIT_LIST_HEAD(&ctx->call_list); +INIT_LIST_HEAD(&ctx->shm_rpc_list); atomic_set(&ctx->call_count, 0); spin_lock_init(&ctx->lock); @@ -199,7 +208,11 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx) struct optee_std_call *call; int count; -/* Make sure that guest does not execute more than max_optee_threads */ +/* + * Make sure that guest does not execute more than max_optee_threads. + * This also indirectly limits number of RPC SHM buffers, because OP-TEE + * allocates one such buffer per standard call. + */ count = atomic_add_unless(&ctx->call_count, 1, max_optee_threads); if ( count == max_optee_threads ) return ERR_PTR(-ENOSPC); @@ -294,10 +307,80 @@ static void put_std_call(struct optee_domain *ctx, struct optee_std_call *call) spin_unlock(&ctx->lock); } +static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, +gfn_t gfn, uint64_t cookie) +{ +struct shm_rpc *shm_rpc, *shm_rpc_tmp; + +shm_rpc = xzalloc(struct shm_rpc); +if ( !shm_rpc ) +return ERR_PTR(-ENOMEM); + +/* This page will be shared with OP-TEE, so we need to pin it. */ +shm_rpc->guest_page = get_domain_ram_page(gfn); +if ( !shm_rpc->guest_page ) +goto err; + +shm_rpc->cookie = cookie; + +spin_lock(&ctx->lock); +/* Check if there is existing SHM with the same cookie. */ +list_for_each_entry( shm_rpc_tmp, &ctx->shm_rpc_list, list ) +{ +if ( shm_rpc_tmp->cookie == cookie ) +{ +spin_unlock(&ctx->lock); +gdprintk(XENLOG_WARNING, "Guest tries to use the same RPC SHM cookie %lx\n", + cookie); +goto err; +} +} + +list_add_tail(&shm_rpc->list, &ctx->shm_rpc_list); +spin_unlock(&ctx->lock); + +return shm_rpc; + +err: +if ( shm_rpc->guest_pag
[Xen-devel] [PATCH v5 08/10] xen/arm: optee: add support for RPC commands
OP-TEE can issue multiple RPC requests. We are interested mostly in request that asks NW to allocate/free shared memory for OP-TEE needs, because mediator needs to do address translation in the same way as it was done for shared buffers registered by NW. OP-TEE can ask NW to allocate multiple buffers during the call. We know that if OP-TEE asks for another buffer, we can free pglist for the previous one. As mediator now accesses shared command buffer, we need to shadow it in the same way, as we shadow request buffers for STD calls. Earlier, we just passed address of this buffer to OP-TEE, but now we need to read and write to it, so it should be shadowed. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v3: - return value of access_guest_memory_by_ipa() now checked - changed how information about shared buffer is stored in call context - domheap now used instead of xenheap - various coding style fixes Changes from v2: - Use access_guest_memory_by_ipa() instead of direct mapping --- xen/arch/arm/tee/optee.c | 229 +-- 1 file changed, 222 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 4b41bcde9f..0a1684ba15 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -47,6 +47,9 @@ */ #define TEEC_ORIGIN_COMMS 0x0002 +/* "Non-specific cause" as in GP TEE Client API Specification */ +#define TEEC_ERROR_GENERIC 0x + /* * "Input parameters were invalid" as described * in GP TEE Client API Specification. @@ -89,6 +92,7 @@ struct optee_std_call { paddr_t guest_arg_ipa; int optee_thread_id; int rpc_op; +uint64_t rpc_data_cookie; bool in_flight; register_t rpc_params[2]; }; @@ -97,6 +101,9 @@ struct optee_std_call { struct shm_rpc { struct list_head list; struct page_info *guest_page; +struct page_info *xen_arg_pg; +struct optee_msg_arg *xen_arg; +gfn_t gfn; uint64_t cookie; }; @@ -350,10 +357,18 @@ static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, if ( !shm_rpc ) return ERR_PTR(-ENOMEM); +shm_rpc->xen_arg_pg = alloc_domheap_page(current->domain, 0); +if ( !shm_rpc->xen_arg_pg ) +{ +xfree(shm_rpc); +return ERR_PTR(-ENOMEM); +} + /* This page will be shared with OP-TEE, so we need to pin it. */ shm_rpc->guest_page = get_domain_ram_page(gfn); if ( !shm_rpc->guest_page ) goto err; +shm_rpc->gfn = gfn; shm_rpc->cookie = cookie; @@ -376,6 +391,8 @@ static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, return shm_rpc; err: +free_domheap_page(shm_rpc->xen_arg_pg); + if ( shm_rpc->guest_page ) put_page(shm_rpc->guest_page); xfree(shm_rpc); @@ -404,12 +421,32 @@ static void free_shm_rpc(struct optee_domain *ctx, uint64_t cookie) if ( !found ) return; +free_domheap_page(shm_rpc->xen_arg_pg); + ASSERT(shm_rpc->guest_page); put_page(shm_rpc->guest_page); xfree(shm_rpc); } +static struct shm_rpc *find_shm_rpc(struct optee_domain *ctx, uint64_t cookie) +{ +struct shm_rpc *shm_rpc; + +spin_lock(&ctx->lock); +list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list ) +{ +if ( shm_rpc->cookie == cookie ) +{ +spin_unlock(&ctx->lock); +return shm_rpc; +} +} +spin_unlock(&ctx->lock); + +return NULL; +} + static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx, uint64_t cookie, unsigned int pages_cnt, @@ -928,10 +965,13 @@ static void free_shm_buffers(struct optee_domain *ctx, } /* Handle RPC return from OP-TEE */ -static void handle_rpc_return(struct arm_smccc_res *res, - struct cpu_user_regs *regs, - struct optee_std_call *call) +static int handle_rpc_return(struct optee_domain *ctx, + struct arm_smccc_res *res, + struct cpu_user_regs *regs, + struct optee_std_call *call) { +int ret = 0; + call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(res->a0); call->rpc_params[0] = res->a1; call->rpc_params[1] = res->a2; @@ -941,6 +981,51 @@ static void handle_rpc_return(struct arm_smccc_res *res, set_user_reg(regs, 1, res->a1); set_user_reg(regs, 2, res->a2); set_user_reg(regs, 3, res->a3); + +if ( call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD ) +{ +/* Copy RPC request from shadowed buffer to guest */ +u
[Xen-devel] [PATCH v5 10/10] tools/arm: optee: create optee firmware node in DT if tee=optee
If TEE support is enabled with "tee=optee" option in xl.cfg, then we need to inform guest about available TEE, by creating corresponding node in the guest's device tree. Signed-off-by: Volodymyr Babchuk Reviewed-by: Julien Grall --- This patch depends on patches to optee.c. Changes from v4: - "native" option replaced with "optee" Changes from v3: - "smc" method replaced with "hvc" - Coding style fixes --- tools/libxl/libxl_arm.c | 29 + 1 file changed, 29 insertions(+) diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 6b72c00960..bf31b9b3ca 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -420,6 +420,32 @@ static int make_psci_node(libxl__gc *gc, void *fdt) return 0; } +static int make_optee_node(libxl__gc *gc, void *fdt) +{ +int res; +LOG(DEBUG, "Creating OP-TEE node in dtb"); + +res = fdt_begin_node(fdt, "firmware"); +if (res) return res; + +res = fdt_begin_node(fdt, "optee"); +if (res) return res; + +res = fdt_property_compat(gc, fdt, 1, "linaro,optee-tz"); +if (res) return res; + +res = fdt_property_string(fdt, "method", "hvc"); +if (res) return res; + +res = fdt_end_node(fdt); +if (res) return res; + +res = fdt_end_node(fdt); +if (res) return res; + +return 0; +} + static int make_memory_nodes(libxl__gc *gc, void *fdt, const struct xc_dom_image *dom) { @@ -933,6 +959,9 @@ next_resize: if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) ); +if (info->tee == LIBXL_TEE_TYPE_OPTEE) +FDT( make_optee_node(gc, fdt) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) ); -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v5 02/10] xen/arm: optee: add OP-TEE header files
This header files describes protocol between OP-TEE and OP-TEE client driver in Linux. They are needed for upcoming OP-TEE mediator, which is added in the next patch. Reason to add those headers in separate patch is to ease up review. Those files were taken from linux tree (drivers/tee/optee/) and mangled a bit to compile with XEN. Signed-off-by: Volodymyr Babchuk --- Changes from v4: - Updated to latest OP-TEE version because of adding OPTEE_SMC_GET_THREAD_COUNT call which will be released with OP-TEE 3.5.0 Changes from v3: - Updated to latest OP-TEE version because virtualization support to OP-TEE was merged into mainline. --- xen/include/asm-arm/tee/optee_msg.h | 444 ++ xen/include/asm-arm/tee/optee_smc.h | 556 2 files changed, 1000 insertions(+) create mode 100644 xen/include/asm-arm/tee/optee_msg.h create mode 100644 xen/include/asm-arm/tee/optee_smc.h diff --git a/xen/include/asm-arm/tee/optee_msg.h b/xen/include/asm-arm/tee/optee_msg.h new file mode 100644 index 00..10747b2aa8 --- /dev/null +++ b/xen/include/asm-arm/tee/optee_msg.h @@ -0,0 +1,444 @@ +/* + * Copyright (c) 2015-2016, Linaro Limited + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ +#ifndef _OPTEE_MSG_H +#define _OPTEE_MSG_H + +#include +#include + +/* + * This file defines the OP-TEE message protocol used to communicate + * with an instance of OP-TEE running in secure world. + * + * This file is divided into three sections. + * 1. Formatting of messages. + * 2. Requests from normal world + * 3. Requests from secure world, Remote Procedure Call (RPC), handled by + *tee-supplicant. + */ + +/* + * Part 1 - formatting of messages + */ + +#define OPTEE_MSG_ATTR_TYPE_NONE 0x0 +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT0x1 +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT 0x2 +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT0x3 +#define OPTEE_MSG_ATTR_TYPE_RMEM_INPUT 0x5 +#define OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT0x6 +#define OPTEE_MSG_ATTR_TYPE_RMEM_INOUT 0x7 +#define OPTEE_MSG_ATTR_TYPE_TMEM_INPUT 0x9 +#define OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT0xa +#define OPTEE_MSG_ATTR_TYPE_TMEM_INOUT 0xb + +#define OPTEE_MSG_ATTR_TYPE_MASK GENMASK(7, 0) + +/* + * Meta parameter to be absorbed by the Secure OS and not passed + * to the Trusted Application. + * + * Currently only used with OPTEE_MSG_CMD_OPEN_SESSION. + */ +#define OPTEE_MSG_ATTR_METABIT(8) + +/* + * Pointer to a list of pages used to register user-defined SHM buffer. + * Used with OPTEE_MSG_ATTR_TYPE_TMEM_*. + * buf_ptr should point to the beginning of the buffer. Buffer will contain + * list of page addresses. OP-TEE core can reconstruct contiguous buffer from + * that page addresses list. Page addresses are stored as 64 bit values. + * Last entry on a page should point to the next page of buffer. + * Every entry in buffer should point to a 4k page beginning (12 least + * significant bits must be equal to zero). + * + * 12 least significant bints of optee_msg_param.u.tmem.buf_ptr should hold page + * offset of the user buffer. + * + * So, entries should be placed like members of this structure: + * + * struct page_data { + * uint64_t pages_array[OPTEE_MSG_NONCONTIG_PAGE_SIZE/sizeof(uint64_t) - 1]; + * uint64_t next_page_data; + * }; + * + * Structure is designed to exactly fit into the page size + * OPTEE_MSG_NONCONTIG_PAGE_SIZE which is
[Xen-devel] [PATCH v5 07/10] xen/arm: optee: add support for arbitrary shared memory
Shared memory is widely used by NW (Normal World) to communicate with TAs (Trusted Applications) in OP-TEE. NW can share part of own memory with TA or with OP-TEE core, by registering it in OP-TEE, or by providing a temporal reference. Anyways, information about such memory buffers are sent to OP-TEE as a list of pages. This mechanism is described in optee_msg.h. Mediator should step in when NW tries to share memory with OP-TEE for two reasons: 1. Do address translation from IPA to PA. 2. Pin domain pages while they are mapped into OP-TEE or TA address space, so domain can't transfer this pages to other domain or balloon out them. Address translation is done by translate_noncontig(...) function. It allocates new buffer from domheap and then walks on guest provided list of pages, translates addresses and stores PAs into newly allocated buffer. This buffer will be provided to OP-TEE instead of original buffer from the guest. This buffer will be freed at the end of standard call. In the same time this function pins pages and stores them in struct optee_shm_buf object. This object will live all the time, when given SHM buffer is known to OP-TEE. It will be freed after guest unregisters shared buffer. At this time pages will be unpinned. Guest can share buffer with OP-TEE for duration for one call, or permanently, using OPTEE_MSG_CMD_REGISTER_SHM call. We need to handle both options. Also we want to limit total size of shared buffers. As it is not possible to get limit from OP-TEE, we need to choose some arbitrary value. Currently limit is 16384 of 4K pages. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v3: - Reworked pagelists storage - there is no more static storage for 5 buffers, instead structure with all data is allocated dynamically - Now this code uses domheap instead of xenheap - Various style fixes - gdprintk() fixes Changes from v2: - Made sure that guest does not tries to register shared buffer with the same cookie twice - Fixed coding style - Use access_guest_memory_by_ipa() instead of direct memory mapping --- xen/arch/arm/tee/optee.c | 413 +++ 1 file changed, 413 insertions(+) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 175789fb00..4b41bcde9f 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -53,9 +53,21 @@ */ #define TEEC_ERROR_BAD_PARAMETERS 0x0006 +/* "System ran out of resources" as in GP TEE Client API Specification */ +#define TEEC_ERROR_OUT_OF_MEMORY 0x000C + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) +/* + * Maximum total number of pages that guest can share with + * OP-TEE. Currently value is selected arbitrary. Actual number of + * pages depends on free heap in OP-TEE. As we can't do any + * assumptions about OP-TEE heap usage, we limit number of pages + * arbitrary. + */ +#define MAX_TOTAL_SMH_BUF_PG16384 + #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ @@ -88,11 +100,31 @@ struct shm_rpc { uint64_t cookie; }; +/* Shared memory buffer for arbitrary data */ +struct optee_shm_buf { +struct list_head list; +uint64_t cookie; +unsigned int page_cnt; +/* + * Shadowed container for list of pages that guest tries to share + * with OP-TEE. This is not the list of pages that guest shared + * with OP-TEE, but container for list of those pages. Check + * OPTEE_MSG_ATTR_NONCONTIG definition in optee_msg.h for more + * information. + */ +struct page_info *pg_list; +unsigned int pg_list_order; +/* Pinned guest pages that are shared with OP-TEE */ +struct page_info *pages[]; +}; + /* Domain context */ struct optee_domain { struct list_head call_list; struct list_head shm_rpc_list; +struct list_head optee_shm_buf_list; atomic_t call_count; +atomic_t optee_shm_buf_pages; spinlock_t lock; }; @@ -167,7 +199,9 @@ static int optee_domain_init(struct domain *d) INIT_LIST_HEAD(&ctx->call_list); INIT_LIST_HEAD(&ctx->shm_rpc_list); +INIT_LIST_HEAD(&ctx->optee_shm_buf_list); atomic_set(&ctx->call_count, 0); +atomic_set(&ctx->optee_shm_buf_pages, 0); spin_lock_init(&ctx->lock); d->arch.tee = ctx; @@ -376,11 +410,142 @@ static void free_shm_rpc(struct optee_domain *ctx, uint64_t cookie) xfree(shm_rpc); } +static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx, +uint64_t cookie, +
[Xen-devel] [PATCH v5 03/10] xen/arm: optee: add OP-TEE mediator skeleton
Add very basic OP-TEE mediator. It can probe for OP-TEE presence, tell it about domain creation/destruction and then return an error to all calls to the guest. This code issues two non-preemptible calls to OP-TEE: to create and to destroy client context. They can't block in OP-TEE, as they are considered "fast calls" in terms of ARM SMCCC. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Removed OPTEE_ENABLED macro. Empty (for now) struct optee_domain is used instead. - Removed forward_call() function, mediator now will return OPTEE_SMC_RETURN_ENOTAVAIL for all unimplemented calls - Fixed mistake when OPTEE_SMC_FUNCID_GET_OS_REVISION instead of OPTEE_SMC_CALL_GET_OS_REVISION was used - OP-TEE is informed about domain destruction in optee_relinquish_resources() - removed optee_domain_destroy() function because all job is done in the optee_relinquish_resources() function Changes from v3: - Introduced optee_relinquish_resources() function to free mediator resources in a more controllable way Changes from v2: - Fixed coding style - Introduced tee/Kconfig - Fixed error messages --- xen/arch/arm/Kconfig | 2 + xen/arch/arm/domain.c | 3 +- xen/arch/arm/tee/Kconfig | 4 + xen/arch/arm/tee/Makefile | 1 + xen/arch/arm/tee/optee.c | 166 ++ xen/include/asm-arm/domain.h | 3 + xen/include/public/arch-arm.h | 1 + 7 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 xen/arch/arm/tee/Kconfig create mode 100644 xen/arch/arm/tee/optee.c diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index e527b2f885..99e6f0ebb2 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -237,3 +237,5 @@ source "arch/arm/platforms/Kconfig" source "common/Kconfig" source "drivers/Kconfig" + +source "arch/arm/tee/Kconfig" diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 0c8e50f48f..94e6f47f75 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -649,7 +649,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } -if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE ) +if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE && + config->arch.tee_type != tee_get_type() ) { dprintk(XENLOG_INFO, "Unsupported TEE type\n"); return -EINVAL; diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig new file mode 100644 index 00..5b829db2e9 --- /dev/null +++ b/xen/arch/arm/tee/Kconfig @@ -0,0 +1,4 @@ +config OPTEE + bool "Enable OP-TEE mediator" + default n + depends on TEE diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile index c54d4796ff..982c879684 100644 --- a/xen/arch/arm/tee/Makefile +++ b/xen/arch/arm/tee/Makefile @@ -1 +1,2 @@ obj-y += tee.o +obj-$(CONFIG_OPTEE) += optee.o diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c new file mode 100644 index 00..e9b69bd2d2 --- /dev/null +++ b/xen/arch/arm/tee/optee.c @@ -0,0 +1,166 @@ +/* + * xen/arch/arm/tee/optee.c + * + * OP-TEE mediator. It sits in between OP-TEE and guests and performs + * actual calls to OP-TEE when some guest tries to interact with + * OP-TEE. As OP-TEE does not know about second stage MMU translation, + * mediator does this translation and performs other housekeeping tasks. + * + * OP-TEE ABI/protocol is described in two header files: + * - optee_smc.h provides information about SMCs: all possible calls, + *register allocation and return codes. + * - optee_msg.h provides format for messages that are passed with + *standard call OPTEE_SMC_CALL_WITH_ARG. + * + * Volodymyr Babchuk + * Copyright (c) 2018-2019 EPAM Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include + +#include +#include +#include +#include + +/* Client ID 0 is reserved for the hypervisor itself */ +#define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) + +/* Domain context */ +struct optee_domain { +}; + +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; + +/* Chec
[Xen-devel] [PATCH v5 00/10] TEE mediator (and OP-TEE) support in XEN
Hello community, This is the fifth version of OP-TEE mediator in XEN. OP-TEE 3.5.0 was released when I worked on this version of the pathes. This is the first release where virtualization support is available. This release includes both original virtualization patches and new SMC that retrevies number of threads from OP-TEE. Many thanks to both OP-TEE and Xen communities for reviewing and helping with my changes to the both projects. Global changes from v4: - Substantial rework of OP-TEE mediator. Now it tries to return meaningful error codes back to the guest. - OP-TEE mediator does not use struct cpu_user_regs as a storage for parameters and return values when calling OP-TEE. This makes it compatbile with requirement from SMCCC. - tee=native option replaced with tee=optee - Authorship and s-o-b tag reset to my EPAM mail address Overall changes from v3: - Patch "arm: add tee_enabled flag to xen_arch_domainconfig" was squashed into "xen/arm: add generic TEE mediator framework" - I implemented more elaborate error repoting to a guest. Now guest will get meaningful error codes instead of generic ARM_SMCCC_ERR_UNKNOWN_FUNCTION. Per-patch changes are described in corresponding emails. Changes from v2: - Use domain flags insted of domctl interface to enable optee for guests - Remove patch "libxc: add xc_dom_tee_enable(...) function" because of previous change - Mediator now stores own context in arch part of struct domain, so I removed patch "optee: add domain contexts" Per-patch changes are described in corresponding emails. v2: This is v2 of patch series for OP-TEE mediator support in XEN. Changes from v1: - Added domctl interface, so now xl decides what domain should work with TEE - Removed XSM support due to change described above - Patch with OP-TEE mediator was splited to 7 separate patches - Removed patch with call_smccc() function. Now this series depend on Julien Grall's series "xen/arm: SMCCC fixup and improvement" [3] = v1: This is follow for patch series [1]. There was lots of discussions for that series and I tried to address all of them in this new patchset. Currently, I had a working solution for OP-TEE virtualization and it is being upstreamed right now ([2]). So, I think it is a good time to introduce support in XEN as well. This series include generic TEE mediator framework and full-scale OP-TEE mediator which is working with mentioned chages in OP-TEE. So, multiple domains can work simultaneously with OP-TEE. I added XSM support, so now it is possible to control which domains can work with TEEs. Also I changed way how TEE discovery is done. Now it is very generic and should support any platform. [1] https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg01451.html [2] https://github.com/OP-TEE/optee_os/pull/2370 [3] https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg02138.html Volodymyr Babchuk (10): xen/arm: add generic TEE mediator framework xen/arm: optee: add OP-TEE header files xen/arm: optee: add OP-TEE mediator skeleton xen/arm: optee: add fast calls handling xen/arm: optee: add std call handling xen/arm: optee: add support for RPC SHM buffers xen/arm: optee: add support for arbitrary shared memory xen/arm: optee: add support for RPC commands tools/arm: tee: add "tee" option for xl.cfg tools/arm: optee: create optee firmware node in DT if tee=optee MAINTAINERS |6 + docs/man/xl.cfg.5.pod.in| 19 + tools/libxl/libxl.h |5 + tools/libxl/libxl_arm.c | 42 + tools/libxl/libxl_types.idl |6 + tools/xl/xl_parse.c |9 + xen/arch/arm/Kconfig|9 + xen/arch/arm/Makefile |1 + xen/arch/arm/domain.c | 19 + xen/arch/arm/setup.c|2 + xen/arch/arm/tee/Kconfig|4 + xen/arch/arm/tee/Makefile |2 + xen/arch/arm/tee/optee.c| 1536 +++ xen/arch/arm/tee/tee.c | 93 ++ xen/arch/arm/vsmc.c |5 + xen/arch/arm/xen.lds.S |7 + xen/include/asm-arm/domain.h|4 + xen/include/asm-arm/tee/optee_msg.h | 444 xen/include/asm-arm/tee/optee_smc.h | 556 ++ xen/include/asm-arm/tee/tee.h | 109 ++ xen/include/public/arch-arm.h |4 + 21 files changed, 2882 insertions(+) create mode 100644 xen/arch/arm/tee/Kconfig create mode 100644 xen/arch/arm/tee/Makefile create mode 100644 xen/arch/arm/tee/optee.c create mode 100644 xen/arch/arm/tee/tee.c create mode 100644 xen/include/asm-arm/tee/optee_msg.h create mode 100644 xen/include/asm-arm/tee/optee_smc.h create mode 100644 xen/include/asm-arm/tee/tee.h -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists
[Xen-devel] [PATCH v5 05/10] xen/arm: optee: add std call handling
The main way to communicate with OP-TEE is to issue standard SMCCC call. "Standard" is a SMCCC term and it means that call can be interrupted and OP-TEE can return control to NW before completing the call. In contrast with fast calls, where arguments and return values are passed in registers, standard calls use shared memory. Register pair a1,a2 holds 64-bit PA of command buffer, where all arguments are stored and which is used to return data. OP-TEE internally copies contents of this buffer into own secure memory before accessing and validating any data in command buffer. This is done to make sure that NW will not change contents of the validated parameters. Mediator needs to do the same for number of reasons: 1. To make sure that guest will not change data after validation. 2. To translate IPAs to PAs in the command buffer (this is not done in this patch). 3. To hide translated address from guest, so it will not be able to do IPA->PA translation by misusing mediator. During standard call OP-TEE can issue multiple "RPC returns", asking NW to do some work for OP-TEE. NW then issues special call OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call. Thus, mediator needs to maintain context for original standard call during multiple SMCCC calls. Standard call is considered complete, when returned value is not a RPC request. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Code uses arm_smccc_smc() directly, instead of forward_call() - do_call_with_arg() function now accepts register values as parameters, so it can be called by RPC handlers with correct values for the given RPC type - optee_probe() calls OPTEE_SMC_GET_THREAD_COUNT. This call is merged into OP-TEE mainline and will be released with OP-TEE v3.5.0 - Removed DEF_MAX_OPTEE_THREADS because it is expected that OP-TEE would support OPTEE_SMC_GET_THREAD_COUNT - Moved map/unmap_xen_arg() outside the spinlocks - Added get_domain_ram_page() helper function - Check the number of parameters, that are supplied by guest Changes from v3: - Added ability to read number of threads from OP-TEE, if it supports this feature - Pages are allocated from domheap, instead of xenheap - Added comments for complex code Changes from v2: - renamed struct domain_ctx to struct optee_domain - fixed coding style - Now I use access_guest_memory_by_ipa() instead of mappings to read command buffer - Added tracking for in flight calls, so guest can't resume the same call from two CPUs simultaniously --- xen/arch/arm/tee/optee.c | 510 ++- 1 file changed, 507 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 6c51caa41a..f092492849 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -25,8 +25,13 @@ */ #include +#include +#include +#include +#include #include +#include #include #include #include @@ -35,6 +40,19 @@ /* Number of SMCs known to the mediator */ #define OPTEE_MEDIATOR_SMC_COUNT 11 +/* + * "The return code is an error that originated within the underlying + * communications stack linking the rich OS with the TEE" as described + * in GP TEE Client API Specification. + */ +#define TEEC_ORIGIN_COMMS 0x0002 + +/* + * "Input parameters were invalid" as described + * in GP TEE Client API Specification. + */ +#define TEEC_ERROR_BAD_PARAMETERS 0x0006 + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) @@ -43,8 +61,31 @@ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) +static unsigned int __read_mostly max_optee_threads; + +/* + * Call context. OP-TEE can issue multiple RPC returns during one call. + * We need to preserve context during them. + */ +struct optee_std_call { +struct list_head list; +/* Page where shadowed copy of call arguments is stored */ +struct page_info *xen_arg_pg; +/* Above page mapped into XEN */ +struct optee_msg_arg *xen_arg; +/* Address of original call arguments */ +paddr_t guest_arg_ipa; +int optee_thread_id; +int rpc_op; +bool in_flight; +register_t rpc_params[2]; +}; + /* Domain context */ struct optee_domain { +struct list_head call_list; +atomic_t call_count; +spinlock_t lock; }; static bool optee_probe(void) @@ -66,6 +107,23 @@ static bool optee_probe(void) (uint32_t)resp.a3 != OPTEE_MSG_UID_3 ) return false; +/* Read number of threads */ +arm_smccc_smc(OPTEE_SMC_GET_THREAD_COUNT, &resp); +if ( resp.a0 == OPTEE_SMC_RETURN_OK ) +{ +max_optee_threads = resp.a1; +
[Xen-devel] [PATCH v5 01/10] xen/arm: add generic TEE mediator framework
This patch adds basic framework for TEE mediators. Guests can't talk to TEE directly, we need some entity that will intercept request and decide what to do with them. "TEE mediator" is a such entity. This is how it works: user can build XEN with multiple TEE mediators (see the next patches, where OP-TEE mediator is introduced). TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END() macros. At run-time, during initialization, framework calls probe() function for each available mediator driver to find which TEE is installed on the platform. Then generic vSMC handler will call selected mediator when it intercept SMC/HVC that belongs to TEE OS or TEE application. Signed-off-by: Volodymyr Babchuk --- Changes from v4: - Added tee_get_type() function, which returns id of currently available TEE - Removed "dom0_tee_enabled" command line option. Dom0 now always uses currently available TEE. - Added TEE type sanity check in arch_sanitise_domain_config() - tee_domain_init() now internally checks if requested TEE type corresponds to available TEE - removed tee_domain_destroy() function because it is not used by anyone Changes from v3: - tee_enable() renamed to tee_domain_init() - Added tee_relinquish_resources() function along with changes to domain_relinquish_resources() - Added command-line parameter dom0_tee_enabled, which controls if tee is enabled for Dom0. It is disabled by default - Instead of boolean tee state (enabled/disabled) I introduced enumeration with two values: none or native. It is possible to add other types of tee in the future Changes from v2: - Removed empty tee/Kconfig file Changes from v1: - Removed tee_remove() function - CONFIG_TEE depends on EXPERT - tee_domain_created() converted to tee_enable() - tee_init() is called using initcall() mechanism - tee_handle_smc() renamed to tee_handle_call() Changes from "RFC" version: - renamed CONFIG_ARM_TEE to CONFIG_TEE - changed discovery mechanism: instead of UUID mathing, TEE-specific probing is used --- MAINTAINERS | 6 ++ xen/arch/arm/Kconfig | 7 +++ xen/arch/arm/Makefile | 1 + xen/arch/arm/domain.c | 18 ++ xen/arch/arm/setup.c | 2 + xen/arch/arm/tee/Makefile | 1 + xen/arch/arm/tee/tee.c| 93 + xen/arch/arm/vsmc.c | 5 ++ xen/arch/arm/xen.lds.S| 7 +++ xen/include/asm-arm/domain.h | 1 + xen/include/asm-arm/tee/tee.h | 109 ++ xen/include/public/arch-arm.h | 3 + 12 files changed, 253 insertions(+) create mode 100644 xen/arch/arm/tee/Makefile create mode 100644 xen/arch/arm/tee/tee.c create mode 100644 xen/include/asm-arm/tee/tee.h diff --git a/MAINTAINERS b/MAINTAINERS index a208bbe304..17906b8321 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -385,6 +385,12 @@ F: config/Stubdom.mk.in F: m4/stubdom.m4 F: stubdom/ +TEE MEDIATORS +M: Volodymyr Babchuk +S: Supported +F: xen/arch/arm/tee/ +F: xen/include/asm-arm/tee + TOOLSTACK M: Ian Jackson M: Wei Liu diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 581de67b6b..e527b2f885 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -105,6 +105,13 @@ config HARDEN_BRANCH_PREDICTOR If unsure, say Y. +config TEE + bool "Enable TEE mediators support" if EXPERT = "y" + default n + help + This option enables generic TEE mediators support. It allows guests + to access real TEE via one of TEE mediators implemented in XEN. + endmenu menu "ARM errata workaround via the alternative framework" diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index cb902cb6fe..5c2aa34557 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -5,6 +5,7 @@ subdir-$(CONFIG_ACPI) += acpi ifneq ($(CONFIG_NO_PLAT),y) subdir-y += platforms endif +subdir-$(CONFIG_TEE) += tee obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o obj-y += bootfdt.init.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 6dc633ed50..0c8e50f48f 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -648,6 +649,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } +if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE ) +{ +dprintk(XENLOG_INFO, "Unsupported TEE type\n"); +return -EINVAL; +} + return 0; } @@ -705,6 +712,9 @@ int arch_domain_create(struct domain *d, if ( (rc = domain_vtimer_init(d, &config->arch)) != 0 ) goto fail; +if ( (rc = tee_domain_init(d, con
[Xen-devel] [PATCH v5 09/10] tools/arm: tee: add "tee" option for xl.cfg
This enumeration controls TEE type for a domain. Currently there is two possible options: either 'none' or 'optee'. 'none' is the default value and it basically disables TEE support at all. 'native' enables access to a "real" OP-TEE installed on a platform. It is possible to add another types in the future. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - "native" option was replaced with "optee" - "tee" property was moved from arch-specific section to the global one. Documentation moved inside "Devices" section. Changes from v3: - tee_enabled renamed to tee_type. Currently two types are supported as described in the commit message - Add LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE definition Changes from v2: - Use arch.tee_enabled instead of separate domctl --- docs/man/xl.cfg.5.pod.in| 19 +++ tools/libxl/libxl.h | 5 + tools/libxl/libxl_arm.c | 13 + tools/libxl/libxl_types.idl | 6 ++ tools/xl/xl_parse.c | 9 + 5 files changed, 52 insertions(+) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index c7d70e618b..73c64dc896 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1544,6 +1544,25 @@ Set maximum height for pointer device. =back +=item B + +Set TEE type for the guest. TEE is a Trusted Execution Environment -- separate +secuse OS found on some platforms. + +=over 4 + +=item B<"none"> + +Disable TEE support at all. This is the default value. + +=item B<"optee"> + +Allow guest to access to OP-TEE enabled on the platform. Guest will not be created +if platform does not have OP-TEE with virtualization feature or if OP-TEE will +deny access. + +=back + =back =head2 Paravirtualised (PV) Guest Specific Options diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 482499a6c0..294a92f645 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -273,6 +273,11 @@ */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 +/* + * libxl_domain_build_info has the arch_arm.tee field. + */ +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 + /* * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing * 'soft reset' for domains and there is 'soft_reset' shutdown reason diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 141e159043..6b72c00960 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -89,6 +89,19 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } +switch (d_config->b_info.tee) { +case LIBXL_TEE_TYPE_NONE: +config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE; +break; +case LIBXL_TEE_TYPE_OPTEE: +config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_OPTEE; +break; +default: +LOG(ERROR, "Unknown TEE type %d", +d_config->b_info.tee); +return ERROR_FAIL; +} + return 0; } diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index cb4702fd7a..4eaccd2cc7 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -460,6 +460,11 @@ libxl_gic_version = Enumeration("gic_version", [ (0x30, "v3") ], init_val = "LIBXL_GIC_VERSION_DEFAULT") +libxl_tee_type = Enumeration("tee_type", [ +(0, "none"), +(1, "optee") +], init_val = "LIBXL_TEE_TYPE_NONE") + libxl_rdm_reserve = Struct("rdm_reserve", [ ("strategy",libxl_rdm_reserve_strategy), ("policy", libxl_rdm_reserve_policy), @@ -537,6 +542,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("nested_hvm", libxl_defbool), ("apic", libxl_defbool), ("dm_restrict", libxl_defbool), +("tee", libxl_tee_type), ("u", KeyedUnion(None, libxl_domain_type, "type", [("hvm", Struct(None, [("firmware", string), ("bios", libxl_bios_type), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 352cd214dd..d98ad0cffb 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2690,6 +2690,15 @@ skip_usbdev: } } +if (!xlu_cfg_get_string (config, "tee", &buf, 1)) { +e = libxl_tee_type_from_string(buf, &b_info->tee); +if (e) { +fprintf(stderr, +"Unknown tee \"%s\" specified\n", buf); +exit(-ERROR_FAIL); +} +} + parse_vkb_list(config, d_config); xlu_cfg_destroy(config); -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] get_maintainer: Improve patch recognition
From: Joe Perches From: Joe Perches There are mode change and rename only patches that are unrecognized by the get_maintainer.pl script. Recognize them. Reported-by: Heinrich Schuchardt CC: Julien Grall Signed-off-by: Joe Perches Signed-off-by: Volodymyr Babchuk --- scripts/get_maintainer.pl | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl index d528da738c..174dfb7e40 100755 --- a/scripts/get_maintainer.pl +++ b/scripts/get_maintainer.pl @@ -445,7 +445,18 @@ foreach my $file (@ARGV) { while (<$patch>) { my $patch_line = $_; - if (m/^\+\+\+\s+(\S+)/ or m/^---\s+(\S+)/) { + if (m/^ mode change [0-7]+ => [0-7]+ (\S+)\s*$/) { + my $filename = $1; + push(@files, $filename); + } elsif (m/^rename (?:from|to) (\S+)\s*$/) { + my $filename = $1; + push(@files, $filename); + } elsif (m/^diff --git a\/(\S+) b\/(\S+)\s*$/) { + my $filename1 = $1; + my $filename2 = $2; + push(@files, $filename1); + push(@files, $filename2); + } elsif (m/^\+\+\+\s+(\S+)/ or m/^---\s+(\S+)/) { my $filename = $1; if ($1 ne "/dev/null") { #Ignore the no-file placeholder $filename =~ s@^[^/]*/@@; -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > Sorry for the late reply. It's okay, no worries. > On 5/20/19 3:57 PM, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> Hi, >>> >>> On 20/05/2019 14:41, Volodymyr Babchuk wrote: >>>> Julien Grall writes: >>>> >>>>> Hi, >>>>> >>>>> First of all, please add a cover letter when you send a series. This >>>>> help for threading and also a place to commend on general feedback. >>>> Oh, okay. That was quite simple change and I didn't wanted to spam with >>>> extra emails. I will include cover letter next time. >>>> >>>>> Furthermore, please use scripts/{add, get}_maintainers.pl to find the >>>>> correct maintainers. While I agree that CCing REST is a good idea, you >>>>> haven't CCed all of them. >>>> Problem is that I used this script: >>>> >>>> $ ./scripts/get_maintainer.pl -f >>>> defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch [...] >> >> Contents of the patch is the exactly the same as in my original >> email. You can find both patches at [1]. > > It looks like the problem is because the second patch only contains > renaming. Linux recently fixed it with the following commit: > > 0455c74788fd "get_maintainer: improve patch recognition" > > I guess we need to port the patch in Xen. Volodymyr, would you mind to > send a patch for it? Yes, I have sent it. It is the first time I'm sending ported patches. I hope, I did it in the right way :) [...] >>>> >>>> and >>>> >>>> # make tiny64_defconfig >>> >>> ... this one will hide the questions. >>> >>>> >>>> Anyways, it is up to you to accept or decline this particular patch. I >>>> mostly interested in the first patch in the series, because our build >>>> system depends on it. This very patch I sent out only because I wanted >>>> to tidy up things a bit. But if you are saying that it is intended to >>>> store minimal config in this way, I'm okay with it. >>> >>> The point of review is to discuss on the approach and find a common >>> agreement. >>> >>> If you read my previous e-mail, I didn't completely reject the >>> approach in my previous e-mail. I pointed out that the user may be >>> misled of the name and hence documentation would be useful. >> >> I'm okay with this. Any ideas how to document it? > > We don't seem to have a place today where we document the defconfig. I > am thinking to put that in docs/misc/arm. > > I would document the purpose of each config. The documentation could > be in a separate patch. Okay. Will it be okay, if I'll send it as a separate patch? You can commit all three patches in a row. Or should I sent another version with all three patches? -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] get_maintainer: Improve patch recognition
Hi Jan, Jan Beulich writes: >>>> On 29.05.19 at 13:35, wrote: >> From: Joe Perches >> >> From: Joe Perches >> >> There are mode change and rename only patches that are unrecognized >> by the get_maintainer.pl script. >> >> Recognize them. >> [ Upstream commit 0455c74788fd5aad4399f00e3fbbb7e87450ca58 ] >> Reported-by: Heinrich Schuchardt >> CC: Julien Grall >> Signed-off-by: Joe Perches >> Signed-off-by: Volodymyr Babchuk > > Mentioning the original Linux commit ID would have been nice. Oh, thank you. This is the first time I'm porting the patch from the other tree. -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 00/10] TEE mediator (and OP-TEE) support in XEN
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > I tried to apply the patches to staging but fail because the patches > contains =20. Do you have a tree with the series applied? Sure, you can find them at [1] [1] https://github.com/lorc/xen/tree/optee_v5 -- Best regards, Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 00/10] TEE mediator (and OP-TEE) support in XEN
Hello Julien, Julien Grall writes: > On 6/1/19 5:07 PM, Volodymyr Babchuk wrote: >> >> Hi Julien, > > Hi Volodymyr, > >> Julien Grall writes: >> >>> Hi Volodymyr, >>> >>> I tried to apply the patches to staging but fail because the patches >>> contains =20. Do you have a tree with the series applied? >> >> Sure, you can find them at [1] > > Thank you! The branch is based on master. This is fairly behind > staging. Could you rebase this on top of staging? > > I will go through the series next week. As this is a tech preview, I > am planning to merge this version unless I find something horribly > wrong in it. Thank you for review. I really appreciate this. I just want to clarify, what I should do next. If I got you right, I should send v6, rebased on staging and with your comments addressed. Is this right? -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 02/10] xen/arm: optee: add OP-TEE header files
Hi Julien, Julien Grall writes: > Hi, > > On 21/05/2019 22:25, Volodymyr Babchuk wrote: >> This header files describes protocol between OP-TEE and OP-TEE client >> driver in Linux. They are needed for upcoming OP-TEE mediator, which >> is added in the next patch. >> Reason to add those headers in separate patch is to ease up review. >> Those files were taken from linux tree (drivers/tee/optee/) and mangled >> a bit to compile with XEN. > > Can you mention the version of the Linux tree you use? This would help > to track change in the future. Actually this commit description is not valid anymore. After I added calls to inform OP-TEE about guest creation/destruction, those files does not correspond to any Linux version anymore. So, I'll take this files from optee_os repository instead and I'll update the commit message accordingly. Can I keep your acked-by tag in such case? -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 01/10] xen/arm: add generic TEE mediator framework
Hi Julien, Julien Grall writes: > On 06/06/2019 17:02, Julien Grall wrote: >> Hi Volodymyr, >> >> On 5/21/19 10:25 PM, Volodymyr Babchuk wrote: >>> +static inline bool tee_handle_call(struct cpu_user_regs *regs) >>> +{ >>> + return false; >>> +} >>> + >>> +static inline int tee_domain_init(struct domain *d, uint16_t tee_type) >>> +{ >>> + return -ENODEV; >>> +} >> >> I had a report that Xen fails to boot with this series and >> !CONFIG_TEE. This is because you return an error here in all the >> case some domain creation will always fail. Thanks for reporting. I forgot to test that case :( >> Instead this should check that tee_type is always NONE or else return an >> error. >> >> Also, please at least check that your series does not break boot >> when CONFIG_TEE is not selected. It would also be ideal (but not >> mandatory) if you can check that it does not break on non-OPTEE >> platform when !CONFIG_TEE is selected. > > I just realized this paragraph may not be clear. What I meant is we > need to at least test there are no regression when booting when with > CONFIG_TEE=n. > > For CONFIG_TEE=y, it would be good to test that it still boots on > platform not providing OP-TEE. This is not critical because the config > cannot be selected without CONFIG_XEN_EXPERT=y. I fixed CONFIG_TEE=n issue in the new version, which I'm going to send later today. Also I made optee_probe() (with CONFIG_OPTEE=y of course) to return false to emulated platform without OP-TEE. System boots and works as usual. Also I addressed your other comments for this patch. -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v6 01/10] xen/arm: add generic TEE mediator framework
This patch adds basic framework for TEE mediators. Guests can't talk to TEE directly, we need some entity that will intercept request and decide what to do with them. "TEE mediator" is a such entity. This is how it works: user can build XEN with multiple TEE mediators (see the next patches, where OP-TEE mediator is introduced). TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END() macros. At run-time, during initialization, framework calls probe() function for each available mediator driver to find which TEE is installed on the platform. Then generic vSMC handler will call selected mediator when it intercept SMC/HVC that belongs to TEE OS or TEE application. Signed-off-by: Volodymyr Babchuk Reviewed-by: Julien Grall --- Changes from v5: - Fixed bug, when XEN won't boot with CONFIG_TEE=n - Fixed coding style - Added __read_mostly attribute to *cur_mediator variable Changes from v4: - Added tee_get_type() function, which returns id of currently available TEE - Removed "dom0_tee_enabled" command line option. Dom0 now always uses currently available TEE. - Added TEE type sanity check in arch_sanitise_domain_config() - tee_domain_init() now internally checks if requested TEE type corresponds to available TEE - removed tee_domain_destroy() function because it is not used by anyone Changes from v3: - tee_enable() renamed to tee_domain_init() - Added tee_relinquish_resources() function along with changes to domain_relinquish_resources() - Added command-line parameter dom0_tee_enabled, which controls if tee is enabled for Dom0. It is disabled by default - Instead of boolean tee state (enabled/disabled) I introduced enumeration with two values: none or native. It is possible to add other types of tee in the future Changes from v2: - Removed empty tee/Kconfig file Changes from v1: - Removed tee_remove() function - CONFIG_TEE depends on EXPERT - tee_domain_created() converted to tee_enable() - tee_init() is called using initcall() mechanism - tee_handle_smc() renamed to tee_handle_call() Changes from "RFC" version: - renamed CONFIG_ARM_TEE to CONFIG_TEE - changed discovery mechanism: instead of UUID mathing, TEE-specific probing is used --- MAINTAINERS | 6 ++ xen/arch/arm/Kconfig | 7 +++ xen/arch/arm/Makefile | 1 + xen/arch/arm/domain.c | 18 ++ xen/arch/arm/setup.c | 2 + xen/arch/arm/tee/Makefile | 1 + xen/arch/arm/tee/tee.c| 98 + xen/arch/arm/vsmc.c | 5 ++ xen/arch/arm/xen.lds.S| 7 +++ xen/include/asm-arm/domain.h | 1 + xen/include/asm-arm/tee/tee.h | 112 ++ xen/include/public/arch-arm.h | 5 ++ 12 files changed, 263 insertions(+) create mode 100644 xen/arch/arm/tee/Makefile create mode 100644 xen/arch/arm/tee/tee.c create mode 100644 xen/include/asm-arm/tee/tee.h diff --git a/MAINTAINERS b/MAINTAINERS index 6fbdc2bdcb..ab32e7f409 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -381,6 +381,12 @@ F: config/Stubdom.mk.in F: m4/stubdom.m4 F: stubdom/ +TEE MEDIATORS +M: Volodymyr Babchuk +S: Supported +F: xen/arch/arm/tee/ +F: xen/include/asm-arm/tee + TOOLSTACK M: Ian Jackson M: Wei Liu diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 585b57f023..caaf377a33 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -106,6 +106,13 @@ config HARDEN_BRANCH_PREDICTOR If unsure, say Y. +config TEE + bool "Enable TEE mediators support" if EXPERT = "y" + default n + help + This option enables generic TEE mediators support. It allows guests + to access real TEE via one of TEE mediators implemented in XEN. + endmenu menu "ARM errata workaround via the alternative framework" diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index cb902cb6fe..5c2aa34557 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -5,6 +5,7 @@ subdir-$(CONFIG_ACPI) += acpi ifneq ($(CONFIG_NO_PLAT),y) subdir-y += platforms endif +subdir-$(CONFIG_TEE) += tee obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o obj-y += bootfdt.init.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ad1b106bd7..d27a137f7a 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -647,6 +648,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } +if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE ) +{ +dprintk(XENLOG_INFO, "Unsupported TEE type\n"); +return -EINVAL; +} + return 0; } @@ -704,6 +711
[Xen-devel] [PATCH v6 00/10] TEE mediator (and OP-TEE) support in XEN
Hello community, This is the next version of OP-TEE support series. In case of any issues with mail (Julien Grall had some troubles with =20 sequences in the patches, thanks to our corporate Exchange, I assume), this series can be pulled from [4]. Note: I deliberately removed Jan Beulich from CC list, because he explicitly indicated that he is not interested in this series ([5]). List of changes is below. More specific changes are described along with the corresponding patches. === v5: - Series rebased to staging branch instead of master one. - OP-TEE protocol headers was taken from OP-TEE tree instead of Linux one - Added acked-by tags - Fixed (and tested) issue when XEN would not boot if it is build with CONFIG_TEE=n v4: - Substantial rework of OP-TEE mediator. Now it tries to return meaningful error codes back to the guest. - OP-TEE mediator does not use struct cpu_user_regs as a storage for parameters and return values when calling OP-TEE. This makes it compatbile with requirement from SMCCC. - tee=native option replaced with tee=optee - Authorship and s-o-b tag reset to my EPAM mail address v3: - Patch "arm: add tee_enabled flag to xen_arch_domainconfig" was squashed into "xen/arm: add generic TEE mediator framework" - I implemented more elaborate error repoting to a guest. Now guest will get meaningful error codes instead of generic ARM_SMCCC_ERR_UNKNOWN_FUNCTION. v2: - Use domain flags insted of domctl interface to enable optee for guests - Remove patch "libxc: add xc_dom_tee_enable(...) function" because of previous change - Mediator now stores own context in arch part of struct domain, so I removed patch "optee: add domain contexts" Per-patch changes are described in corresponding emails. v2: This is v2 of patch series for OP-TEE mediator support in XEN. Changes from v1: - Added domctl interface, so now xl decides what domain should work with TEE - Removed XSM support due to change described above - Patch with OP-TEE mediator was splited to 7 separate patches - Removed patch with call_smccc() function. Now this series depend on Julien Grall's series "xen/arm: SMCCC fixup and improvement" [3] = v1: This is follow for patch series [1]. There was lots of discussions for that series and I tried to address all of them in this new patchset. Currently, I had a working solution for OP-TEE virtualization and it is being upstreamed right now ([2]). So, I think it is a good time to introduce support in XEN as well. This series include generic TEE mediator framework and full-scale OP-TEE mediator which is working with mentioned chages in OP-TEE. So, multiple domains can work simultaneously with OP-TEE. I added XSM support, so now it is possible to control which domains can work with TEEs. Also I changed way how TEE discovery is done. Now it is very generic and should support any platform. [1] https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg01451.html [2] https://github.com/OP-TEE/optee_os/pull/2370 [3] https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg02138.html [4] https://github.com/lorc/xen/tree/optee_v6 [5] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01805.html Volodymyr Babchuk (10): xen/arm: add generic TEE mediator framework xen/arm: optee: add OP-TEE header files xen/arm: optee: add OP-TEE mediator skeleton xen/arm: optee: add fast calls handling xen/arm: optee: add std call handling xen/arm: optee: add support for RPC SHM buffers xen/arm: optee: add support for arbitrary shared memory xen/arm: optee: add support for RPC commands tools/arm: tee: add "tee" option for xl.cfg tools/arm: optee: create optee firmware node in DT if tee=optee MAINTAINERS |6 + docs/man/xl.cfg.5.pod.in| 21 + tools/libxl/libxl.h |5 + tools/libxl/libxl_arm.c | 42 + tools/libxl/libxl_types.idl |6 + tools/xl/xl_parse.c |9 + xen/arch/arm/Kconfig|9 + xen/arch/arm/Makefile |1 + xen/arch/arm/domain.c | 19 + xen/arch/arm/setup.c|2 + xen/arch/arm/tee/Kconfig|4 + xen/arch/arm/tee/Makefile |2 + xen/arch/arm/tee/optee.c| 1540 +++ xen/arch/arm/tee/tee.c | 98 ++ xen/arch/arm/vsmc.c |5 + xen/arch/arm/xen.lds.S |7 + xen/include/asm-arm/domain.h|4 + xen/include/asm-arm/tee/optee_msg.h | 310 + xen/include/asm-arm/tee/optee_rpc_cmd.h | 318 + xen/include/asm-arm/tee/optee_smc.h | 564 + xen/include/asm-arm/tee/tee.h | 112 ++ xen/include/public/arch-arm.h |6 + 22 files changed
[Xen-devel] [PATCH v6 02/10] xen/arm: optee: add OP-TEE header files
This header files describes protocol between OP-TEE OS and OP-TEE clients, which are running in Normal World. This headers are needed for upcoming OP-TEE mediator, which is added in the next patch. Reason to add those headers in separate patch is to ease up review. Those files were taken from OP-TEE OS 3.5.0 tree and mangled a bit to compile with XEN. Location of the files in the original tree: core/include/optee_msg.h core/include/optee_rpc_cmd.h core/arch/arm/include/sm/optee_smc.h [OP-TEE commit id 5df2a985b2ffd0b6f1107f12ca2a88203bf31328] Signed-off-by: Volodymyr Babchuk --- Changes from v5: - Used optee_os headers instead of linux ones Changes from v4: - Updated to latest OP-TEE version because of adding OPTEE_SMC_GET_THREAD_COUNT call which will be released with OP-TEE 3.5.0 Changes from v3: - Updated to latest OP-TEE version because virtualization support to OP-TEE was merged into mainline. --- xen/include/asm-arm/tee/optee_msg.h | 310 + xen/include/asm-arm/tee/optee_rpc_cmd.h | 318 + xen/include/asm-arm/tee/optee_smc.h | 564 3 files changed, 1192 insertions(+) create mode 100644 xen/include/asm-arm/tee/optee_msg.h create mode 100644 xen/include/asm-arm/tee/optee_rpc_cmd.h create mode 100644 xen/include/asm-arm/tee/optee_smc.h diff --git a/xen/include/asm-arm/tee/optee_msg.h b/xen/include/asm-arm/tee/optee_msg.h new file mode 100644 index 00..fe743dbde3 --- /dev/null +++ b/xen/include/asm-arm/tee/optee_msg.h @@ -0,0 +1,310 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (c) 2015-2017, Linaro Limited + */ +#ifndef _OPTEE_MSG_H +#define _OPTEE_MSG_H + +#include +#include + +/* + * This file defines the OP-TEE message protocol used to communicate + * with an instance of OP-TEE running in secure world. + */ + +/* + * Part 1 - formatting of messages + */ + +#define OPTEE_MSG_ATTR_TYPE_NONE 0x0 +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT0x1 +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT 0x2 +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT0x3 +#define OPTEE_MSG_ATTR_TYPE_RMEM_INPUT 0x5 +#define OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT0x6 +#define OPTEE_MSG_ATTR_TYPE_RMEM_INOUT 0x7 +#define OPTEE_MSG_ATTR_TYPE_TMEM_INPUT 0x9 +#define OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT0xa +#define OPTEE_MSG_ATTR_TYPE_TMEM_INOUT 0xb + +#define OPTEE_MSG_ATTR_TYPE_MASK GENMASK(7, 0) + +/* + * Meta parameter to be absorbed by the Secure OS and not passed + * to the Trusted Application. + * + * Currently only used with OPTEE_MSG_CMD_OPEN_SESSION. + */ +#define OPTEE_MSG_ATTR_METABIT(8, UL) + +/* + * Pointer to a list of pages used to register user-defined SHM buffer. + * Used with OPTEE_MSG_ATTR_TYPE_TMEM_*. + * buf_ptr should point to the beginning of the buffer. Buffer will contain + * list of page addresses. OP-TEE core can reconstruct contiguous buffer from + * that page addresses list. Page addresses are stored as 64 bit values. + * Last entry on a page should point to the next page of buffer. + * Every entry in buffer should point to a 4k page beginning (12 least + * significant bits must be equal to zero). + * + * 12 least significant of optee_msg_param.u.tmem.buf_ptr should hold page + * offset of user buffer. + * + * So, entries should be placed like members of this structure: + * + * struct page_data { + * uint64_t pages_array[OPTEE_MSG_NONCONTIG_PAGE_SIZE/sizeof(uint64_t) - 1]; + * uint64_t next_page_data; + * }; + * + * Structure is designed to exactly fit into the page size + * OPTEE_MSG_NONCONTIG_PAGE_SIZE which is a standard 4KB page. + * + * The size of 4KB is chosen because this is the smallest page size for ARM + * architectures. If REE uses larger pages, it should divide them to 4KB ones. + */ +#define OPTEE_MSG_ATTR_NONCONTIG BIT(9, UL) + +/* + * Memory attributes for caching passed with temp memrefs. The actual value + * used is defined outside the message protocol with the exception of + * OPTEE_MSG_ATTR_CACHE_PREDEFINED which means the attributes already + * defined for the memory range should be used. If optee_smc.h is used as + * bearer of this protocol OPTEE_SMC_SHM_* is used for values. + */ +#define OPTEE_MSG_ATTR_CACHE_SHIFT 16 +#define OPTEE_MSG_ATTR_CACHE_MASK GENMASK(2, 0) +#define OPTEE_MSG_ATTR_CACHE_PREDEFINED0 + +/* + * Same values as TEE_LOGIN_* from TEE Internal API + */ +#define OPTEE_MSG_LOGIN_PUBLIC 0x +#define OPTEE_MSG_LOGIN_USER 0x0001 +#define OPTEE_MSG_LOGIN_GROUP 0x0002 +#define OPTEE_MSG_LOGIN_APPLICATION0x0004 +#define OPTEE_MSG_LOGIN_APPLICATION_USER
[Xen-devel] [PATCH v6 09/10] tools/arm: tee: add "tee" option for xl.cfg
This enumeration controls TEE type for a domain. Currently there is two possible options: either 'none' or 'optee'. 'none' is the default value and it basically disables TEE support at all. 'optee' enables access to the OP-TEE running on a host machine. This requires special OP-TEE build with virtualization support enabled. Signed-off-by: Volodymyr Babchuk --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v5: - Replaced "native" with "optee" in the commit description. - Updated and extended documentation based on Julien Grall's and Ian Jackson's suggestions. Changes from v4: - "native" option was replaced with "optee" - "tee" property was moved from arch-specific section to the global one. Documentation moved inside "Devices" section. Changes from v3: - tee_enabled renamed to tee_type. Currently two types are supported as described in the commit message - Add LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE definition Changes from v2: - Use arch.tee_enabled instead of separate domctl --- docs/man/xl.cfg.5.pod.in| 21 + tools/libxl/libxl.h | 5 + tools/libxl/libxl_arm.c | 13 + tools/libxl/libxl_types.idl | 6 ++ tools/xl/xl_parse.c | 9 + 5 files changed, 54 insertions(+) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index c99d40307e..e65ab6111f 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1544,6 +1544,27 @@ Set maximum height for pointer device. =back +=item B + +B Set TEE type for the guest. TEE is a Trusted Execution +Environment -- separate secure OS found on some platforms. B can be one of the: + +=over 4 + +=item B + +Disable TEE support at all. This is the default value. + +=item B + +Allow a guest to use OP-TEE. Note that a virtualization-aware OP-TEE +is required for this. If this option is selected, guest will be able +to access to the real OP-TEE OS running on the host. Guest creation +will fail if OP-TEE have no resources for a new guest. Number of supported +guests depends on OP-TEE configuration. + +=back + =back =head2 Paravirtualised (PV) Guest Specific Options diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 9bacfb97f0..1fe6ea2bd8 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -273,6 +273,11 @@ */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 +/* + * libxl_domain_build_info has the arch_arm.tee field. + */ +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 + /* * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing * 'soft reset' for domains and there is 'soft_reset' shutdown reason diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 141e159043..6b72c00960 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -89,6 +89,19 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } +switch (d_config->b_info.tee) { +case LIBXL_TEE_TYPE_NONE: +config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE; +break; +case LIBXL_TEE_TYPE_OPTEE: +config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_OPTEE; +break; +default: +LOG(ERROR, "Unknown TEE type %d", +d_config->b_info.tee); +return ERROR_FAIL; +} + return 0; } diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index b61399ce36..fa5ee65463 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -460,6 +460,11 @@ libxl_gic_version = Enumeration("gic_version", [ (0x30, "v3") ], init_val = "LIBXL_GIC_VERSION_DEFAULT") +libxl_tee_type = Enumeration("tee_type", [ +(0, "none"), +(1, "optee") +], init_val = "LIBXL_TEE_TYPE_NONE") + libxl_rdm_reserve = Struct("rdm_reserve", [ ("strategy",libxl_rdm_reserve_strategy), ("policy", libxl_rdm_reserve_policy), @@ -537,6 +542,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("nested_hvm", libxl_defbool), ("apic", libxl_defbool), ("dm_restrict", libxl_defbool), +("tee", libxl_tee_type), ("u", KeyedUnion(None, libxl_domain_type, "type", [("hvm", Struct(None, [("firmware", string), ("bios", libxl_bios_type), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index e105bda2bb..0604374ef3 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2691,6 +2691,15 @@ skip_usbdev: }
[Xen-devel] [PATCH v6 10/10] tools/arm: optee: create optee firmware node in DT if tee=optee
If TEE support is enabled with "tee=optee" option in xl.cfg, then we need to inform guest about available TEE, by creating corresponding node in the guest's device tree. Signed-off-by: Volodymyr Babchuk Reviewed-by: Julien Grall Acked-by: Ian Jackson --- This patch depends on patches to optee.c. Changes from v4: - "native" option replaced with "optee" Changes from v3: - "smc" method replaced with "hvc" - Coding style fixes --- tools/libxl/libxl_arm.c | 29 + 1 file changed, 29 insertions(+) diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 6b72c00960..bf31b9b3ca 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -420,6 +420,32 @@ static int make_psci_node(libxl__gc *gc, void *fdt) return 0; } +static int make_optee_node(libxl__gc *gc, void *fdt) +{ +int res; +LOG(DEBUG, "Creating OP-TEE node in dtb"); + +res = fdt_begin_node(fdt, "firmware"); +if (res) return res; + +res = fdt_begin_node(fdt, "optee"); +if (res) return res; + +res = fdt_property_compat(gc, fdt, 1, "linaro,optee-tz"); +if (res) return res; + +res = fdt_property_string(fdt, "method", "hvc"); +if (res) return res; + +res = fdt_end_node(fdt); +if (res) return res; + +res = fdt_end_node(fdt); +if (res) return res; + +return 0; +} + static int make_memory_nodes(libxl__gc *gc, void *fdt, const struct xc_dom_image *dom) { @@ -933,6 +959,9 @@ next_resize: if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) ); +if (info->tee == LIBXL_TEE_TYPE_OPTEE) +FDT( make_optee_node(gc, fdt) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) ); -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v6 07/10] xen/arm: optee: add support for arbitrary shared memory
Shared memory is widely used by NW (Normal World) to communicate with TAs (Trusted Applications) in OP-TEE. NW can share part of own memory with TA or with OP-TEE core, by registering it in OP-TEE, or by providing a temporal reference. Anyways, information about such memory buffers are sent to OP-TEE as a list of pages. This mechanism is described in optee_msg.h. Mediator should step in when NW tries to share memory with OP-TEE for two reasons: 1. Do address translation from IPA to PA. 2. Pin domain pages while they are mapped into OP-TEE or TA address space, so domain can't transfer this pages to other domain or balloon out them. Address translation is done by translate_noncontig(...) function. It allocates new buffer from domheap and then walks on guest provided list of pages, translates addresses and stores PAs into newly allocated buffer. This buffer will be provided to OP-TEE instead of original buffer from the guest. This buffer will be freed at the end of standard call. In the same time this function pins pages and stores them in struct optee_shm_buf object. This object will live all the time, when given SHM buffer is known to OP-TEE. It will be freed after guest unregisters shared buffer. At this time pages will be unpinned. Guest can share buffer with OP-TEE for duration for one call, or permanently, using OPTEE_MSG_CMD_REGISTER_SHM call. We need to handle both options. Also we want to limit total size of shared buffers. As it is not possible to get limit from OP-TEE, we need to choose some arbitrary value. Currently limit is 16384 of 4K pages. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v5: - Added BUILD_BUG_ON(PAGE_SIZE != 4096) - Updated "TODO" comment about 16384 calls to lookup_and_pin_guest_ram_addr() Changes from v3: - Reworked pagelists storage - there is no more static storage for 5 buffers, instead structure with all data is allocated dynamically - Now this code uses domheap instead of xenheap - Various style fixes - gdprintk() fixes Changes from v2: - Made sure that guest does not tries to register shared buffer with the same cookie twice - Fixed coding style - Use access_guest_memory_by_ipa() instead of direct memory mapping --- xen/arch/arm/tee/optee.c | 416 +++ 1 file changed, 416 insertions(+) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 175789fb00..d4888acd8d 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -53,9 +53,21 @@ */ #define TEEC_ERROR_BAD_PARAMETERS 0x0006 +/* "System ran out of resources" as in GP TEE Client API Specification */ +#define TEEC_ERROR_OUT_OF_MEMORY 0x000C + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) +/* + * Maximum total number of pages that guest can share with + * OP-TEE. Currently value is selected arbitrary. Actual number of + * pages depends on free heap in OP-TEE. As we can't do any + * assumptions about OP-TEE heap usage, we limit number of pages + * arbitrary. + */ +#define MAX_TOTAL_SMH_BUF_PG16384 + #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ @@ -88,11 +100,31 @@ struct shm_rpc { uint64_t cookie; }; +/* Shared memory buffer for arbitrary data */ +struct optee_shm_buf { +struct list_head list; +uint64_t cookie; +unsigned int page_cnt; +/* + * Shadowed container for list of pages that guest tries to share + * with OP-TEE. This is not the list of pages that guest shared + * with OP-TEE, but container for list of those pages. Check + * OPTEE_MSG_ATTR_NONCONTIG definition in optee_msg.h for more + * information. + */ +struct page_info *pg_list; +unsigned int pg_list_order; +/* Pinned guest pages that are shared with OP-TEE */ +struct page_info *pages[]; +}; + /* Domain context */ struct optee_domain { struct list_head call_list; struct list_head shm_rpc_list; +struct list_head optee_shm_buf_list; atomic_t call_count; +atomic_t optee_shm_buf_pages; spinlock_t lock; }; @@ -167,7 +199,9 @@ static int optee_domain_init(struct domain *d) INIT_LIST_HEAD(&ctx->call_list); INIT_LIST_HEAD(&ctx->shm_rpc_list); +INIT_LIST_HEAD(&ctx->optee_shm_buf_list); atomic_set(&ctx->call_count, 0); +atomic_set(&ctx->optee_shm_buf_pages, 0); spin_lock_init(&ctx->lock); d->arch.tee = ctx; @@ -376,11 +410,142 @@ static void free_shm_rpc(struct optee_domain *ctx, uint64_t cookie) xfree(shm_rpc); }
[Xen-devel] [PATCH v6 04/10] xen/arm: optee: add fast calls handling
This patch adds handling for the fast SMCs. As name suggests, those calls can't be preempted and are used for auxiliary tasks such as information retrieval. Most handlers are quite trivial, with exception for capabilities information. Capabilities exchange should be filtered out, so only caps known to mediator are used. Also mediator disables static SHM memory capability, because it can't share OP-TEE memory with a domain. Only domain can share memory with OP-TEE, so it ensures that OP-TEE supports dynamic SHM. Basically, static SHM is a reserved memory region which is always mapped into OP-TEE address space. It belongs to OP-TEE. Normally, NW is allowed to access there, so it can communicate with OP-TEE. On other hand, dynamic SHM is NW's own memory, which it can share with OP-TEE. OP-TEE maps this memory dynamically, when it wants to access it. Because mediator can't share one static SHM region with all guests, it just disables it for all of them. It is possible to make exception for Dom0, but it requires separate handling for buffers allocated from that region. Thus, it is not implemented yet. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Gral --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Handler does not use forward_call(). Instead it calls OP-TEE directly with arm_smccc_smc(). - Handler modifies only those guest registers that are should be touched according to OP-TEE protocol specification. - Added OPTEE_MEDIATOR_SMC_COUNT definition. Changes from v2: - Defined known capabilities explicitly - Fixed code style --- xen/arch/arm/tee/optee.c | 97 1 file changed, 97 insertions(+) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index e9b69bd2d2..6c51caa41a 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -32,9 +32,17 @@ #include #include +/* Number of SMCs known to the mediator */ +#define OPTEE_MEDIATOR_SMC_COUNT 11 + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) +#define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR +#define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ + OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ + OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) + /* Domain context */ struct optee_domain { }; @@ -120,22 +128,111 @@ static int optee_relinquish_resources(struct domain *d) return 0; } +static void handle_exchange_capabilities(struct cpu_user_regs *regs) +{ +struct arm_smccc_res resp; +uint32_t caps; + +/* Filter out unknown guest caps */ +caps = get_user_reg(regs, 1); +caps &= OPTEE_KNOWN_NSEC_CAPS; + +arm_smccc_smc(OPTEE_SMC_EXCHANGE_CAPABILITIES, caps, 0, 0, 0, 0, 0, + OPTEE_CLIENT_ID(current->domain), &resp); +if ( resp.a0 != OPTEE_SMC_RETURN_OK ) { +set_user_reg(regs, 0, resp.a0); +return; +} + +caps = resp.a1; + +/* Filter out unknown OP-TEE caps */ +caps &= OPTEE_KNOWN_SEC_CAPS; + +/* Drop static SHM_RPC cap */ +caps &= ~OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM; + +/* Don't allow guests to work without dynamic SHM */ +if ( !(caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) ) +{ +set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL); +return; +} + +set_user_reg(regs, 0, OPTEE_SMC_RETURN_OK); +set_user_reg(regs, 1, caps); +} + static bool optee_handle_call(struct cpu_user_regs *regs) { +struct arm_smccc_res resp; + if ( !current->domain->arch.tee ) return false; switch ( get_user_reg(regs, 0) ) { case OPTEE_SMC_CALLS_COUNT: +set_user_reg(regs, 0, OPTEE_MEDIATOR_SMC_COUNT); +return true; + case OPTEE_SMC_CALLS_UID: +arm_smccc_smc(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0, + 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); +return true; + case OPTEE_SMC_CALLS_REVISION: +arm_smccc_smc(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0, + OPTEE_CLIENT_ID(current->domain), &resp); +set_user_reg(regs, 0, resp.a0); +set_user_reg(regs, 1, resp.a1); +return true; + case OPTEE_SMC_CALL_GET_OS_UUID: +arm_smccc_smc(OPTEE_SMC_CALL_GET_OS_UUID, 0, 0, 0, 0, 0, 0, + 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); +return true; + case OPTEE_SMC_CALL_GET_OS_REV
[Xen-devel] [PATCH v6 08/10] xen/arm: optee: add support for RPC commands
OP-TEE can issue multiple RPC requests. We are interested mostly in request that asks NW to allocate/free shared memory for OP-TEE needs, because mediator needs to do address translation in the same way as it was done for shared buffers registered by NW. OP-TEE can ask NW to allocate multiple buffers during the call. We know that if OP-TEE asks for another buffer, we can free pglist for the previous one. As mediator now accesses shared command buffer, we need to shadow it in the same way, as we shadow request buffers for STD calls. Earlier, we just passed address of this buffer to OP-TEE, but now we need to read and write to it, so it should be shadowed. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v5: - There was change to RPC command names, because of different header file was used (see comments to patch 2 "xen/arm: optee: add OP-TEE header files"). This is non-functional change. Changes from v3: - return value of access_guest_memory_by_ipa() now checked - changed how information about shared buffer is stored in call context - domheap now used instead of xenheap - various coding style fixes Changes from v2: - Use access_guest_memory_by_ipa() instead of direct mapping --- xen/arch/arm/tee/optee.c | 230 +-- 1 file changed, 223 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index d4888acd8d..28d34360fc 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -36,6 +36,7 @@ #include #include #include +#include /* Number of SMCs known to the mediator */ #define OPTEE_MEDIATOR_SMC_COUNT 11 @@ -47,6 +48,9 @@ */ #define TEEC_ORIGIN_COMMS 0x0002 +/* "Non-specific cause" as in GP TEE Client API Specification */ +#define TEEC_ERROR_GENERIC 0x + /* * "Input parameters were invalid" as described * in GP TEE Client API Specification. @@ -89,6 +93,7 @@ struct optee_std_call { paddr_t guest_arg_ipa; int optee_thread_id; int rpc_op; +uint64_t rpc_data_cookie; bool in_flight; register_t rpc_params[2]; }; @@ -97,6 +102,9 @@ struct optee_std_call { struct shm_rpc { struct list_head list; struct page_info *guest_page; +struct page_info *xen_arg_pg; +struct optee_msg_arg *xen_arg; +gfn_t gfn; uint64_t cookie; }; @@ -350,10 +358,18 @@ static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, if ( !shm_rpc ) return ERR_PTR(-ENOMEM); +shm_rpc->xen_arg_pg = alloc_domheap_page(current->domain, 0); +if ( !shm_rpc->xen_arg_pg ) +{ +xfree(shm_rpc); +return ERR_PTR(-ENOMEM); +} + /* This page will be shared with OP-TEE, so we need to pin it. */ shm_rpc->guest_page = get_domain_ram_page(gfn); if ( !shm_rpc->guest_page ) goto err; +shm_rpc->gfn = gfn; shm_rpc->cookie = cookie; @@ -376,6 +392,8 @@ static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, return shm_rpc; err: +free_domheap_page(shm_rpc->xen_arg_pg); + if ( shm_rpc->guest_page ) put_page(shm_rpc->guest_page); xfree(shm_rpc); @@ -404,12 +422,32 @@ static void free_shm_rpc(struct optee_domain *ctx, uint64_t cookie) if ( !found ) return; +free_domheap_page(shm_rpc->xen_arg_pg); + ASSERT(shm_rpc->guest_page); put_page(shm_rpc->guest_page); xfree(shm_rpc); } +static struct shm_rpc *find_shm_rpc(struct optee_domain *ctx, uint64_t cookie) +{ +struct shm_rpc *shm_rpc; + +spin_lock(&ctx->lock); +list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list ) +{ +if ( shm_rpc->cookie == cookie ) +{ +spin_unlock(&ctx->lock); +return shm_rpc; +} +} +spin_unlock(&ctx->lock); + +return NULL; +} + static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx, uint64_t cookie, unsigned int pages_cnt, @@ -931,10 +969,13 @@ static void free_shm_buffers(struct optee_domain *ctx, } /* Handle RPC return from OP-TEE */ -static void handle_rpc_return(struct arm_smccc_res *res, - struct cpu_user_regs *regs, - struct optee_std_call *call) +static int handle_rpc_return(struct optee_domain *ctx, + struct arm_smccc_res *res, + struct cpu_user_regs *regs, + struct optee_std_call *call) { +int ret = 0; + call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(res->a0); call->rpc_param
[Xen-devel] [PATCH v6 06/10] xen/arm: optee: add support for RPC SHM buffers
OP-TEE usually uses the same idea with command buffers (see previous commit) to issue RPC requests. Problem is that initially it has no buffer, where it can write request. So the first RPC request it makes is special: it requests NW to allocate shared buffer for other RPC requests. Usually this buffer is allocated only once for every OP-TEE thread and it remains allocated all the time until guest shuts down. Guest can ask OP-TEE to disable RPC buffers caching, in this case OP-TEE will ask guest to allocate/free buffer for the each RPC. Mediator needs to pin this buffer to make sure that page will be not free while it is shared with OP-TEE. Life cycle of this buffer is controlled by OP-TEE. It asks guest to create buffer and it asks it to free it. So it there is not much sense to limit number of those buffers, because we already limit the number of concurrent standard calls and prevention of RPC buffer allocation will impair OP-TEE functionality. Those buffers can be freed in two ways: either OP-TEE issues OPTEE_SMC_RPC_FUNC_FREE RPC request or guest tries to disable buffer caching by calling OPTEE_SMC_DISABLE_SHM_CACHE function. In the latter case OP-TEE will return cookie of the SHM buffer it just freed. OP-TEE expects that this RPC buffer have size of OPTEE_MSG_NONCONTIG_PAGE_SIZE, which equals to 4096 and is aligned with the same size. So, basically it expects one 4k page from the guest. This is the same as Xen's PAGE_SIZE. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - handle_rpc_func_alloc() now calls do_call_with_arg() directly Changes from v3: - Removed MAX_RPC_SHMS constant. Now this value depends on number of OP-TEE threads - Various formatting fixes - Added checks for guest memory type Changes from v2: - Added check to ensure that guests does not return two SHM buffers with the same cookie - Fixed coding style - Storing RPC parameters during RPC return to make sure, that guest will not change them during call continuation --- xen/arch/arm/tee/optee.c | 149 +-- 1 file changed, 145 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index f092492849..175789fb00 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -81,9 +81,17 @@ struct optee_std_call { register_t rpc_params[2]; }; +/* Pre-allocated SHM buffer for RPC commands */ +struct shm_rpc { +struct list_head list; +struct page_info *guest_page; +uint64_t cookie; +}; + /* Domain context */ struct optee_domain { struct list_head call_list; +struct list_head shm_rpc_list; atomic_t call_count; spinlock_t lock; }; @@ -158,6 +166,7 @@ static int optee_domain_init(struct domain *d) } INIT_LIST_HEAD(&ctx->call_list); +INIT_LIST_HEAD(&ctx->shm_rpc_list); atomic_set(&ctx->call_count, 0); spin_lock_init(&ctx->lock); @@ -199,7 +208,11 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx) struct optee_std_call *call; int count; -/* Make sure that guest does not execute more than max_optee_threads */ +/* + * Make sure that guest does not execute more than max_optee_threads. + * This also indirectly limits number of RPC SHM buffers, because OP-TEE + * allocates one such buffer per standard call. + */ count = atomic_add_unless(&ctx->call_count, 1, max_optee_threads); if ( count == max_optee_threads ) return ERR_PTR(-ENOSPC); @@ -294,10 +307,80 @@ static void put_std_call(struct optee_domain *ctx, struct optee_std_call *call) spin_unlock(&ctx->lock); } +static struct shm_rpc *allocate_and_pin_shm_rpc(struct optee_domain *ctx, +gfn_t gfn, uint64_t cookie) +{ +struct shm_rpc *shm_rpc, *shm_rpc_tmp; + +shm_rpc = xzalloc(struct shm_rpc); +if ( !shm_rpc ) +return ERR_PTR(-ENOMEM); + +/* This page will be shared with OP-TEE, so we need to pin it. */ +shm_rpc->guest_page = get_domain_ram_page(gfn); +if ( !shm_rpc->guest_page ) +goto err; + +shm_rpc->cookie = cookie; + +spin_lock(&ctx->lock); +/* Check if there is existing SHM with the same cookie. */ +list_for_each_entry( shm_rpc_tmp, &ctx->shm_rpc_list, list ) +{ +if ( shm_rpc_tmp->cookie == cookie ) +{ +spin_unlock(&ctx->lock); +gdprintk(XENLOG_WARNING, "Guest tries to use the same RPC SHM cookie %lx\n", + cookie); +goto err; +} +} + +list_add_tail(&shm_rpc->list, &ctx->shm_rpc_list); +spin_unlock(&ctx->lock); + +return shm_rpc; + +err: +if ( s
[Xen-devel] [PATCH v6 05/10] xen/arm: optee: add std call handling
The main way to communicate with OP-TEE is to issue standard SMCCC call. "Standard" is a SMCCC term and it means that call can be interrupted and OP-TEE can return control to NW before completing the call. In contrast with fast calls, where arguments and return values are passed in registers, standard calls use shared memory. Register pair a1,a2 holds 64-bit PA of command buffer, where all arguments are stored and which is used to return data. OP-TEE internally copies contents of this buffer into own secure memory before accessing and validating any data in command buffer. This is done to make sure that NW will not change contents of the validated parameters. Mediator needs to do the same for number of reasons: 1. To make sure that guest will not change data after validation. 2. To translate IPAs to PAs in the command buffer (this is not done in this patch). 3. To hide translated address from guest, so it will not be able to do IPA->PA translation by misusing mediator. During standard call OP-TEE can issue multiple "RPC returns", asking NW to do some work for OP-TEE. NW then issues special call OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call. Thus, mediator needs to maintain context for original standard call during multiple SMCCC calls. Standard call is considered complete, when returned value is not a RPC request. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Code uses arm_smccc_smc() directly, instead of forward_call() - do_call_with_arg() function now accepts register values as parameters, so it can be called by RPC handlers with correct values for the given RPC type - optee_probe() calls OPTEE_SMC_GET_THREAD_COUNT. This call is merged into OP-TEE mainline and will be released with OP-TEE v3.5.0 - Removed DEF_MAX_OPTEE_THREADS because it is expected that OP-TEE would support OPTEE_SMC_GET_THREAD_COUNT - Moved map/unmap_xen_arg() outside the spinlocks - Added get_domain_ram_page() helper function - Check the number of parameters, that are supplied by guest Changes from v3: - Added ability to read number of threads from OP-TEE, if it supports this feature - Pages are allocated from domheap, instead of xenheap - Added comments for complex code Changes from v2: - renamed struct domain_ctx to struct optee_domain - fixed coding style - Now I use access_guest_memory_by_ipa() instead of mappings to read command buffer - Added tracking for in flight calls, so guest can't resume the same call from two CPUs simultaniously --- xen/arch/arm/tee/optee.c | 510 ++- 1 file changed, 507 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 6c51caa41a..f092492849 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -25,8 +25,13 @@ */ #include +#include +#include +#include +#include #include +#include #include #include #include @@ -35,6 +40,19 @@ /* Number of SMCs known to the mediator */ #define OPTEE_MEDIATOR_SMC_COUNT 11 +/* + * "The return code is an error that originated within the underlying + * communications stack linking the rich OS with the TEE" as described + * in GP TEE Client API Specification. + */ +#define TEEC_ORIGIN_COMMS 0x0002 + +/* + * "Input parameters were invalid" as described + * in GP TEE Client API Specification. + */ +#define TEEC_ERROR_BAD_PARAMETERS 0x0006 + /* Client ID 0 is reserved for the hypervisor itself */ #define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) @@ -43,8 +61,31 @@ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) +static unsigned int __read_mostly max_optee_threads; + +/* + * Call context. OP-TEE can issue multiple RPC returns during one call. + * We need to preserve context during them. + */ +struct optee_std_call { +struct list_head list; +/* Page where shadowed copy of call arguments is stored */ +struct page_info *xen_arg_pg; +/* Above page mapped into XEN */ +struct optee_msg_arg *xen_arg; +/* Address of original call arguments */ +paddr_t guest_arg_ipa; +int optee_thread_id; +int rpc_op; +bool in_flight; +register_t rpc_params[2]; +}; + /* Domain context */ struct optee_domain { +struct list_head call_list; +atomic_t call_count; +spinlock_t lock; }; static bool optee_probe(void) @@ -66,6 +107,23 @@ static bool optee_probe(void) (uint32_t)resp.a3 != OPTEE_MSG_UID_3 ) return false; +/* Read number of threads */ +arm_smccc_smc(OPTEE_SMC_GET_THREAD_COUNT, &resp); +if ( resp.a0 == OPTEE_SMC_RETURN_OK ) +{ +m
[Xen-devel] [PATCH v6 03/10] xen/arm: optee: add OP-TEE mediator skeleton
Add very basic OP-TEE mediator. It can probe for OP-TEE presence, tell it about domain creation/destruction and then return an error to all calls to the guest. This code issues two non-preemptible calls to OP-TEE: to create and to destroy client context. They can't block in OP-TEE, as they are considered "fast calls" in terms of ARM SMCCC. Signed-off-by: Volodymyr Babchuk Acked-by: Julien Grall --- All the patches to optee.c should be merged together. They were split to ease up review. But they depend heavily on each other. Changes from v4: - Removed OPTEE_ENABLED macro. Empty (for now) struct optee_domain is used instead. - Removed forward_call() function, mediator now will return OPTEE_SMC_RETURN_ENOTAVAIL for all unimplemented calls - Fixed mistake when OPTEE_SMC_FUNCID_GET_OS_REVISION instead of OPTEE_SMC_CALL_GET_OS_REVISION was used - OP-TEE is informed about domain destruction in optee_relinquish_resources() - removed optee_domain_destroy() function because all job is done in the optee_relinquish_resources() function Changes from v3: - Introduced optee_relinquish_resources() function to free mediator resources in a more controllable way Changes from v2: - Fixed coding style - Introduced tee/Kconfig - Fixed error messages --- xen/arch/arm/Kconfig | 2 + xen/arch/arm/domain.c | 3 +- xen/arch/arm/tee/Kconfig | 4 + xen/arch/arm/tee/Makefile | 1 + xen/arch/arm/tee/optee.c | 166 ++ xen/include/asm-arm/domain.h | 3 + xen/include/public/arch-arm.h | 1 + 7 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 xen/arch/arm/tee/Kconfig create mode 100644 xen/arch/arm/tee/optee.c diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index caaf377a33..04d399ffbf 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -238,3 +238,5 @@ source "arch/arm/platforms/Kconfig" source "common/Kconfig" source "drivers/Kconfig" + +source "arch/arm/tee/Kconfig" diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index d27a137f7a..e8657447d7 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -648,7 +648,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } -if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE ) +if ( config->arch.tee_type != XEN_DOMCTL_CONFIG_TEE_NONE && + config->arch.tee_type != tee_get_type() ) { dprintk(XENLOG_INFO, "Unsupported TEE type\n"); return -EINVAL; diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig new file mode 100644 index 00..5b829db2e9 --- /dev/null +++ b/xen/arch/arm/tee/Kconfig @@ -0,0 +1,4 @@ +config OPTEE + bool "Enable OP-TEE mediator" + default n + depends on TEE diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile index c54d4796ff..982c879684 100644 --- a/xen/arch/arm/tee/Makefile +++ b/xen/arch/arm/tee/Makefile @@ -1 +1,2 @@ obj-y += tee.o +obj-$(CONFIG_OPTEE) += optee.o diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c new file mode 100644 index 00..e9b69bd2d2 --- /dev/null +++ b/xen/arch/arm/tee/optee.c @@ -0,0 +1,166 @@ +/* + * xen/arch/arm/tee/optee.c + * + * OP-TEE mediator. It sits in between OP-TEE and guests and performs + * actual calls to OP-TEE when some guest tries to interact with + * OP-TEE. As OP-TEE does not know about second stage MMU translation, + * mediator does this translation and performs other housekeeping tasks. + * + * OP-TEE ABI/protocol is described in two header files: + * - optee_smc.h provides information about SMCs: all possible calls, + *register allocation and return codes. + * - optee_msg.h provides format for messages that are passed with + *standard call OPTEE_SMC_CALL_WITH_ARG. + * + * Volodymyr Babchuk + * Copyright (c) 2018-2019 EPAM Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include + +#include +#include +#include +#include + +/* Client ID 0 is reserved for the hypervisor itself */ +#define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1) + +/* Domain context */ +struct optee_domain { +}; + +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; + +
Re: [Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig
Hello Julien, Jan, Julien Grall writes: >>>>> On 16.05.19 at 15:37, wrote: >>> As build system now supports *_defconfig rules it is good to be able >>> to configure minimal XEN image with >>> >>> make tiny64_defconfig >>> >>> command. >>> >>> Signed-off-by: Volodymyr Babchuk >> >> you had objections here, and the first patch makes little sense >> without the 2nd. May I ask what the verdict is, i.e. whether I should >> drop these two from my list of pending patches? > > Volodymyr was going to resend the series with documentation (as a > separate patch). But I would be happy to take #1 and #2 assuming that > documentation patch is going to be sent. Yes, sorry for the delay. I'm going to send resend the series soon. But I can see, that first two patches are already in the staging branch. Should I resend the whole series in this case? Or single patch with the missing documentation will be sufficient? And another, slightly related question: I'm not sure what to do with my patch to get_maintainer.pl script. Should I resend the new version? Jan had comments only to commit message... -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 02/11] arm: add tee_enabled flag to xen_arch_domainconfig
Hello Julien, Julien Grall writes: > Hi, > > On 12/18/18 9:11 PM, Volodymyr Babchuk wrote: >> From: Volodymyr Babchuk >> >> This flag enables TEE support for a domain. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> xen/arch/arm/domain.c | 4 >> xen/arch/arm/domctl.c | 1 + >> xen/include/public/arch-arm.h | 3 +++ >> 3 files changed, 8 insertions(+) >> , >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 11b618515b..f04041931d 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -702,6 +702,10 @@ int arch_domain_create(struct domain *d, >> if ( (rc = domain_vtimer_init(d, &config->arch)) != 0 ) >> goto fail; >> +if ( config->arch.tee_enabled ) >> +if ( (rc = tee_enable(d)) != 0 ) > > This function does not yet exist. But I think it would make sense to > fold this patch in the next one. If you were talking about tee_enable(), then it was introduced in the previous patch. Sure, I'll squash this patch into the previous one. >> +goto fail; >> + >> update_domain_wallclock_time(d); >> /* >> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c >> index 20691528a6..f019e035e8 100644 >> --- a/xen/arch/arm/domctl.c >> +++ b/xen/arch/arm/domctl.c >> @@ -13,6 +13,7 @@ >> #include >> #include >> #include >> +#include >> #include >> void arch_get_domain_info(const struct domain *d, >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index eb424e8286..b7a010e99e 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -323,6 +323,9 @@ struct xen_arch_domainconfig { >> * >>*/ >> uint32_t clock_frequency; >> + >> +/* IN */ >> +uint8_t tee_enabled; > > Can you move this after gic_version? So we don't introduce more padding. Sure. >> }; >> #endif /* __XEN__ || __XEN_TOOLS__ */ >> >> > > Cheers, -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 04/11] optee: add OP-TEE mediator skeleton
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > On 12/18/18 9:11 PM, Volodymyr Babchuk wrote: >> From: Volodymyr Babchuk >> >> Add very basic OP-TEE mediator. It can probe for OP-TEE presence, >> tell it about domain creation/destruction and forward all known >> calls. >> >> This is all what is needed for Dom0 to work with OP-TEE as long >> as Dom0 shares 1:1 mapped pages with OP-TEE. Any attempt to call >> OP-TEE from DomU will fail and can lead to spectacular results. Also, >> problems can arise if Dom0 uses pages mapped from other domains. >> So, this patch should not be merged without next patches in the series. > > The last sentence is not necessary in the commit message. You can add it > after "---". So it is stripped down when the patch is committed, yet we > know it should not be merged :). Oh, okay. >> >> This code issues two non-preemptible calls to OP-TEE: to create and >> to destroy client context. They can't block in OP-TEE, but OP-TEE can >> wait on a splinlocks, so there is no maximal execution time >> guaranteed. > > This paragraph is worrying. From the discussion we had on the previous > version and some discussions at Linaro Connect with OP-TEE folks, I > gathered this call should not take a long time. Yes, this is right. It **should** not take a long time. And currently I can't see why it would take a long time anyways. But, you know, theoretically, there is no upper limit on waiting for spinlock. > In general spinlock should have no contention or wait should be limited. > In particular, in the context of OP-TEE I would expect something is > really wrong if you wait on a spinlock for a really long time as you > block the normal world. So maybe this is just a misunderstanding of the > commit message? Yes, I think it is a misunderstanding. OP-TEE uses spinlocks in the same way as XEN or kernel - for short atomic operations. But as spinlock() function have no timeout argument, theoretically it would block for a long time. I think, I'll remove that clause from the commit message. Just for clarity :) > >> >> Signed-off-by: Volodymyr Babchuk >> --- >> >> Changes from v2: >>- Fixed coding style >>- Introduced tee/Kconfig >>- Fixed error messages >> >> xen/arch/arm/Kconfig| 2 + >> xen/arch/arm/tee/Kconfig| 4 + >> xen/arch/arm/tee/Makefile | 1 + >> xen/arch/arm/tee/optee.c| 151 >> xen/include/asm-arm/tee/optee_smc.h | 50 + >> 5 files changed, 208 insertions(+) >> create mode 100644 xen/arch/arm/tee/Kconfig >> create mode 100644 xen/arch/arm/tee/optee.c >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index e527b2f885..99e6f0ebb2 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -237,3 +237,5 @@ source "arch/arm/platforms/Kconfig" >> source "common/Kconfig" >> >> source "drivers/Kconfig" >> + >> +source "arch/arm/tee/Kconfig" >> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig >> new file mode 100644 >> index 00..5b829db2e9 >> --- /dev/null >> +++ b/xen/arch/arm/tee/Kconfig >> @@ -0,0 +1,4 @@ >> +config OPTEE >> +bool "Enable OP-TEE mediator" >> +default n >> +depends on TEE >> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile >> index c54d4796ff..982c879684 100644 >> --- a/xen/arch/arm/tee/Makefile >> +++ b/xen/arch/arm/tee/Makefile >> @@ -1 +1,2 @@ >> obj-y += tee.o >> +obj-$(CONFIG_OPTEE) += optee.o >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> new file mode 100644 >> index 00..73ad25ee0b >> --- /dev/null >> +++ b/xen/arch/arm/tee/optee.c >> @@ -0,0 +1,151 @@ >> +/* >> + * xen/arch/arm/tee/optee.c >> + * >> + * OP-TEE mediator >> + * >> + * Volodymyr Babchuk >> + * Copyright (c) 2018 EPAM Systems. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +/* Client ID 0 is reserved for hypervisor itself */ >> +#define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1) > > The second 'domain' should be bet
Re: [Xen-devel] [PATCH v3 06/11] optee: add std call handling
Hello Julien, Julien Grall writes: > Hi Volodymyr, > > On 18/12/2018 21:11, Volodymyr Babchuk wrote: >> From: Volodymyr Babchuk >> >> The main way to communicate with OP-TEE is to issue standard SMCCC >> call. "Standard" is a SMCCC term and it means that call can be >> interrupted and OP-TEE can return control to NW before completing >> the call. >> >> In contrast with fast calls, where arguments and return values >> are passed in registers, standard calls use shared memory. Register >> pair a1,a2 holds 64-bit PA of command buffer, where all arguments >> are stored and which is used to return data. OP-TEE internally >> copies contents of this buffer into own secure memory before accessing >> and validating any data in command buffer. This is done to make sure >> that NW will not change contents of the validated parameters. >> >> Mediator needs to do the same for number of reasons: >> >> 1. To make sure that guest will not change data after validation. >> 2. To translate IPAs to PAs in the command buffer (this is not done >> in this patch). >> 3. To hide translated address from guest, so it will not be able >> to do IPA->PA translation by misusing mediator. >> >> During standard call OP-TEE can issue multiple "RPC returns", asking >> NW to do some work for OP-TEE. NW then issues special call >> OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call. >> Thus, mediator needs to maintain context for original standard call >> during multiple SMCCC calls. >> >> Standard call is considered complete, when returned value is >> not a RPC request. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> >> Changes from v2: >>- renamed struct domain_ctx to struct optee_domain >>- fixed coding style >>- Now I use access_guest_memory_by_ipa() instead of mappings >> to read command buffer >>- Added tracking for in flight calls, so guest can't resume >> the same call from two CPUs simultaniously >> >> xen/arch/arm/tee/optee.c | 319 ++- >> xen/include/asm-arm/domain.h | 3 + >> 2 files changed, 320 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> index 584241b03a..dc90e2ed8e 100644 >> --- a/xen/arch/arm/tee/optee.c >> +++ b/xen/arch/arm/tee/optee.c >> @@ -12,6 +12,8 @@ >>*/ >> #include >> +#include >> +#include >> #include >> #include >> #include >> @@ -22,11 +24,38 @@ >> /* Client ID 0 is reserved for hypervisor itself */ >> #define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1) >> +/* >> + * Maximal number of concurrent standard calls from one guest. This >> + * corresponds to OPTEE configuration option CFG_NUM_THREADS, because >> + * OP-TEE spawns a thread for every standard call. > > Looking at OP-TEE, CFG_NUM_THREADS will vary depending on the > platform. Is there any way to probe that number of threads from Xen? Yes, this is per-platform configuration. Easiest way is to add Fast SMC to OP-TEE that will report this parameter. Jens, what do you think about adding additional call? > In any case, I think we should update the comment to reflect that this > seems to be the maximum CFG_NUM_THREADS supported by any upstream > platform. Actually, OP-TEE protocol have possibility to handle limited number of threads correctly. OP-TEE can report that all threads are busy and client will wait for a free one. XEN can do the same, although this is not implemented in this patch series. But I can add this. Basically this means that all can work correctly even with MAX_STD_CALLS==1. It just will be not so efficient. >> + */ >> +#define MAX_STD_CALLS 16 >> + >> #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR >> #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ >> OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ >> OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) >> +/* >> + * Call context. OP-TEE can issue multiple RPC returns during one call. >> + * We need to preserve context during them. >> + */ >> +struct optee_std_call { >> +struct list_head list; >> +struct optee_msg_arg *xen_arg; >> +paddr_t guest_arg_ipa; >> +int optee_thread_id; >> +int rpc_op; >> +bool in_flight; >> +}; >> + >> +/* Domain context */ >> +struct optee_domain { >> +struct list_head call_list; >&g
Re: [Xen-devel] [PATCH v3 06/11] optee: add std call handling
Hi Julien, Julien Grall writes: > Hi Volodymyr, > > On 17/01/2019 15:21, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >>> Hi Volodymyr, >>> >>> On 18/12/2018 21:11, Volodymyr Babchuk wrote: >>>> From: Volodymyr Babchuk >>>> >>>> The main way to communicate with OP-TEE is to issue standard SMCCC >>>> call. "Standard" is a SMCCC term and it means that call can be >>>> interrupted and OP-TEE can return control to NW before completing >>>> the call. >>>> >>>> In contrast with fast calls, where arguments and return values >>>> are passed in registers, standard calls use shared memory. Register >>>> pair a1,a2 holds 64-bit PA of command buffer, where all arguments >>>> are stored and which is used to return data. OP-TEE internally >>>> copies contents of this buffer into own secure memory before accessing >>>> and validating any data in command buffer. This is done to make sure >>>> that NW will not change contents of the validated parameters. >>>> >>>> Mediator needs to do the same for number of reasons: >>>> >>>> 1. To make sure that guest will not change data after validation. >>>> 2. To translate IPAs to PAs in the command buffer (this is not done >>>> in this patch). >>>> 3. To hide translated address from guest, so it will not be able >>>> to do IPA->PA translation by misusing mediator. >>>> >>>> During standard call OP-TEE can issue multiple "RPC returns", asking >>>> NW to do some work for OP-TEE. NW then issues special call >>>> OPTEE_SMC_CALL_RETURN_FROM_RPC to resume handling of the original call. >>>> Thus, mediator needs to maintain context for original standard call >>>> during multiple SMCCC calls. >>>> >>>> Standard call is considered complete, when returned value is >>>> not a RPC request. >>>> >>>> Signed-off-by: Volodymyr Babchuk >>>> --- >>>> >>>>Changes from v2: >>>> - renamed struct domain_ctx to struct optee_domain >>>> - fixed coding style >>>> - Now I use access_guest_memory_by_ipa() instead of mappings >>>> to read command buffer >>>> - Added tracking for in flight calls, so guest can't resume >>>> the same call from two CPUs simultaniously >>>> >>>>xen/arch/arm/tee/optee.c | 319 ++- >>>>xen/include/asm-arm/domain.h | 3 + >>>>2 files changed, 320 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >>>> index 584241b03a..dc90e2ed8e 100644 >>>> --- a/xen/arch/arm/tee/optee.c >>>> +++ b/xen/arch/arm/tee/optee.c >>>> @@ -12,6 +12,8 @@ >>>> */ >>>> #include >>>> +#include >>>> +#include >>>>#include >>>>#include >>>>#include >>>> @@ -22,11 +24,38 @@ >>>>/* Client ID 0 is reserved for hypervisor itself */ >>>>#define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1) >>>>+/* >>>> + * Maximal number of concurrent standard calls from one guest. This >>>> + * corresponds to OPTEE configuration option CFG_NUM_THREADS, because >>>> + * OP-TEE spawns a thread for every standard call. >>> >>> Looking at OP-TEE, CFG_NUM_THREADS will vary depending on the >>> platform. Is there any way to probe that number of threads from Xen? >> Yes, this is per-platform configuration. >> Easiest way is to add Fast SMC to OP-TEE that will report this >> parameter. >> Jens, what do you think about adding additional call? >> >>> In any case, I think we should update the comment to reflect that this >>> seems to be the maximum CFG_NUM_THREADS supported by any upstream >>> platform. >> >> Actually, OP-TEE protocol have possibility to handle limited number of >> threads correctly. OP-TEE can report that all threads are busy and >> client will wait for a free one. XEN can do the same, although this is not >> implemented in this patch series. But I can add this. > > Could you expand by wait? Will it block in OP-TEE/Xen or does it > return to the guest? It returns to the guest with response code OPTEE_SMC_RETURN_ETHREAD_LIMIT. Linux driver blocks calling a
Re: [Xen-devel] [PATCH v3 05/11] optee: add fast calls handling
Hello Julien, Julien Grall writes: [...] >> Because mediator can't share one static SHM region with all guests, >> it just disables it for all. > > I haven't seen an answer to my question on the previous version. For reminder: I'm sorry, I missed this somehow. > Would it make sense to still allow the hardware domain to access static SHM? Not really. If linux drivers sees that OP-TEE support dynamic SHM, it uses it by default. And all new versions of OP-TEE support dynamic SHM. So, it will add unnecessary code to enable feature which will not be used. But, actually, there is a caveat. Right now linux driver requires static SHM, even if it will not use it. I have submitted patch that fixes it, but it still not merged. So... what do you think? Should I add piece of code which is needed right now, but will not be used later? -- Best regards, Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 07/11] optee: add support for RPC SHM buffers
Julien Grall writes: > Hi Volodymyr, > > On 18/12/2018 21:11, Volodymyr Babchuk wrote: >> From: Volodymyr Babchuk >> >> OP-TEE usually uses the same idea with command buffers (see >> previous commit) to issue RPC requests. Problem is that initially >> it has no buffer, where it can write request. So the first RPC >> request it makes is special: it requests NW to allocate shared >> buffer for other RPC requests. Usually this buffer is allocated >> only once for every OP-TEE thread and it remains allocated all >> the time until shutdown. > > By shutdown you mean for the OS or the thread? Shutdown of OP-TEE actually. But guest can ask OP-TEE to de-allocate this buffers. And this is what linux drivers does when it unloads. So, basically, linux drivers says "I want to disable RPC buffer caching" and then OP-TEE issues number of RPCs to free those buffers. >> >> Mediator needs to pin this buffer(s) to make sure that domain can't >> transfer it to someone else. >> >> Life cycle of this buffer is controlled by OP-TEE. It asks guest >> to create buffer and it asks it to free it. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> >> Changes from v2: >>- Added check to ensure that guests does not return two SHM buffers >> with the same cookie >>- Fixed coding style >>- Storing RPC parameters during RPC return to make sure, that guest >> will not change them during call continuation >> xen/arch/arm/tee/optee.c | 140 >> ++- >> 1 file changed, 138 insertions(+), 2 deletions(-) >> >> 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. [...] >> @@ -227,11 +247,90 @@ static void put_std_call(struct optee_domain *ctx, >> struct optee_std_call *call) >> spin_unlock(&ctx->lock); >> } >> +static struct shm_rpc *allocate_and_pin_shm_rpc(struct >> optee_domain *ctx, >> +paddr_t gaddr, > > As I said on v3, I would prefer if you use gfn_t here. This would > introduce more safety. Sure, will do. [...] >> +shm_rpc->guest_page = get_page_from_gfn(current->domain, >> +paddr_to_pfn(gaddr), >> +NULL, >> +P2M_ALLOC); > > I think it would be wrong to share any page other than p2m_ram_rw with OP-TEE. > So it should be like this: shm_rpc->guest_page = get_page_from_gfn(current->domain, paddr_to_pfn(gaddr), &p2m, P2M_ALLOC); if ( !shm_rpc->guest_page || p2m != p2m_ram_rw) goto err; ? [...] >> +static void free_shm_rpc(struct optee_domain *ctx, uint64_t cookie) >> +{ >> +struct shm_rpc *shm_rpc; >> +bool found = false; >> + >> +spin_lock(&ctx->lock); >> + >> +list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list ) >> +{ >> +if ( shm_rpc->cookie == cookie ) >> +{ >> +found = true; >> +list_del(&shm_rpc->list); >> +break; >> +} >> +} >> +spin_unlock(&ctx->lock); > > I think you are missing an atomic_dec(&ctx->shm_rpc_count) here. Good catch. Thank you. [...] >> +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) ) >>
Re: [Xen-devel] [PATCH v3 05/11] optee: add fast calls handling
Hi, Julien Grall writes: > Hi, > > On 1/17/19 7:22 PM, Volodymyr Babchuk wrote: >> Julien Grall writes: >> >> [...] >>>> Because mediator can't share one static SHM region with all guests, >>>> it just disables it for all. >>> >>> I haven't seen an answer to my question on the previous version. For >>> reminder: >> I'm sorry, I missed this somehow. >> >> >>> Would it make sense to still allow the hardware domain to access static SHM? >> Not really. If linux drivers sees that OP-TEE support dynamic SHM, it >> uses it by default. And all new versions of OP-TEE support dynamic SHM. >> >> So, it will add unnecessary code to enable feature which will not be used. >> >> But, actually, there is a caveat. Right now linux driver requires static SHM, >> even if it will not use it. I have submitted patch that fixes it, but >> it still not merged. >> >> So... what do you think? Should I add piece of code which is needed >> right now, but will not be used later? > > I think it would be nice to have OP-TEE working in Dom0 with current > Linux. For the guest, we can require a more recent Linux if there are > no other solution. Okay, I'll take a look what can be done. Can I do this in a separate patch? -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 06/11] optee: add std call handling
Julien Grall writes: > On 1/17/19 7:13 PM, Volodymyr Babchuk wrote: >>>> Actually, OP-TEE protocol have possibility to handle limited number of >>>> threads correctly. OP-TEE can report that all threads are busy and >>>> client will wait for a free one. XEN can do the same, although this is not >>>> implemented in this patch series. But I can add this. >>> >>> Could you expand by wait? Will it block in OP-TEE/Xen or does it >>> return to the guest? >> It returns to the guest with response code >> OPTEE_SMC_RETURN_ETHREAD_LIMIT. Linux driver blocks calling application >> thread until one of the calls to OP-TEE is finished. Then driver awakens >> one of the blocked threads, so it can perform the call. > > Shouldn't not you return this value when you reach out MAX_STD_CALLS? Yes, I should. As I said earlier, this isn't done right now. But apparently will be done in the next version. > Actually, looking at the code, you don't seem to return in error code > when there are a failure in the mediator. Instead you seem to always > return "false". Which means the virtual SMCCC framework thinks the > call was never handled. However, this is not true, you handled the > call but the there was a failure during it. > > In general, handle_call should return false only if the call is > non-existent. In all the other case, you should feed a proper return > by yourself. Agree. At first I seen OP-TEE mediator as relatively thin shim between guest and OP-TEE. And I expected that it should not return errors to a good behaving guest. But logic becomes more complex and indeed, there are cases when even behaving guest can get an error. -- Best regards,Volodymyr Babchuk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel