From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 67DA0CDB47E for ; Fri, 13 Oct 2023 09:49:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:In-Reply-To:References: Message-ID:Date:Subject:CC:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=mT7569qPGOdlzJorGbUhRPy94BcdviUvlDfV4H35ZOc=; b=iOR2MVggZPbpjQ /T6gnDMyK2xENZ5z60ID/G5Hq1FVmHfd5hT5/HNqfkzSa1AFg0z9l4GQUzc4VZDiDrSLqVStuTney wJiiAHk6QUnkHGE5ZkK5TVsuQiqw3b4rgqV9WIeM+pwbCI49jBC+LvE8VMy1CXjrNqU3sv61L5cYY GLJcJWGk4ginwh4J/ZKlkxtr1giedEXuy+A7TSE/Sbjp8DM9OCrA30kGLy7Jc6M9Wwh8BAyX3DiAD rGP+A/P2lwF26z+Dy477dyv+0E1+Ui/2A6z/IBf0N9UUOSY3Kw0oIKqDGmRdZQPMMCRt7fid0g8wv 9roTV6NY2d1wIZuHpmbg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qrEn8-003068-0j; Fri, 13 Oct 2023 09:49:10 +0000 Received: from mx08-00178001.pphosted.com ([91.207.212.93]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qrEn4-00305c-2l for linux-arm-kernel@lists.infradead.org; Fri, 13 Oct 2023 09:49:08 +0000 Received: from pps.filterd (m0369457.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39D9SVN8004572; Fri, 13 Oct 2023 11:49:03 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h= from:to:cc:subject:date:message-id:references:in-reply-to :content-type:content-transfer-encoding:mime-version; s= selector1; bh=XIJllFa3lyrGANH10umtce4GqB7bYAU7W5gGSo/oanM=; b=kS EbrAdoW/ajKQZOaFa4aEjskTCZc6GkQfC2kOo7Xob43CKKQIrimm+u6yM2y7Rd73 irTAv5M8hH47Uv0klQ2wj7dBndp8C0UlVIfb0FFqoSHQ1u13c5NVQVoEvFRCfczY w8Le1kmRkmLVLLEhWma4n9f7HPNIXs8+XDF0G6jRE3ujlvalWYRv6BEXqFBuPDcE /CSpeuzJ7D8xULXZd4zAC55iCPcZi2Ua/U4h7WsuMK7lWOSwWH+BEBeZMQz5XXHR tHv+ReAxX0+EEZAIuU6MGhpsk7FVuhNZh3q+myy6O1Lm2ZfEkvDctNvvjllbs/0A 9elVHDYzbcmiSEboBuLw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3tkj9h89np-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 13 Oct 2023 11:49:02 +0200 (MEST) Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 0F269100056; Fri, 13 Oct 2023 11:49:01 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node2.st.com [10.75.129.70]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id E732121A208; Fri, 13 Oct 2023 11:49:01 +0200 (CEST) Received: from SHFDAG1NODE1.st.com (10.75.129.69) by SHFDAG1NODE2.st.com (10.75.129.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Fri, 13 Oct 2023 11:48:59 +0200 Received: from SHFDAG1NODE1.st.com ([fe80::117e:c4ab:ed81:6cb1]) by SHFDAG1NODE1.st.com ([fe80::117e:c4ab:ed81:6cb1%14]) with mapi id 15.01.2507.027; Fri, 13 Oct 2023 11:48:59 +0200 From: Etienne CARRIERE - foss To: Sumit Garg CC: "linux-kernel@vger.kernel.org" , "Jens Wiklander" , "linux-arm-kernel@lists.infradead.org" , "op-tee@lists.trustedfirmware.org" , Jerome Forissier Subject: Re: [PATCH v10 3/4] tee: optee: support tracking system threads Thread-Topic: [PATCH v10 3/4] tee: optee: support tracking system threads Thread-Index: AQHZ9gLa0eDXIY4qt063d7Hb9IRsn7A8YsSAgAfLecSAAxABgIAANORE///qbICAACKwav//45oAgAAkeqw= Date: Fri, 13 Oct 2023 09:48:59 +0000 Message-ID: <62e525fefd934329822a16edd9b6b5ab@foss.st.com> References: <20231003140637.31346-1-etienne.carriere@foss.st.com> <20231003140637.31346-4-etienne.carriere@foss.st.com> <5ba5ab8d2cc7477588dd0f6c7531cf16@foss.st.com> , In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.201.20.20] MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-13_03,2023-10-12_01,2023-05-22_02 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231013_024907_174984_12B1B11E X-CRM114-Status: GOOD ( 53.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org > From: Sumit Garg > Sent: Friday, October 13, 2023 11:36 AM > > On Fri, 13 Oct 2023 at 14:53, Etienne CARRIERE - foss > wrote: > > > > > From: Sumit Garg > > > Sent: Friday, October 13, 2023 11:13 AM > > > > > > On Fri, 13 Oct 2023 at 14:09, Etienne CARRIERE - foss > > > wrote: > > > > > > > > > From: Sumit Garg > > > > > Sent: Friday, October 13, 2023 9:21 AM > > > > > > > > > > On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss > > > > > wrote: > > > > > > > > > > > > > From: Sumit Garg > > > > > > > Sent: Friday, October 6, 2023 11:33 AM > > > > > > > > > > > > > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere > > > > > > > wrote: > > > > > > > > > > > > > > > > Adds support in the OP-TEE driver to keep track of reserved system > > > > > > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system > > > > > > > > sessions. > > > > > > > > > > > > > > > > The optee_cq_*() functions are updated to handle this if enabled, > > > > > > > > that is when TEE describes how many thread context it supports > > > > > > > > and when at least 1 session has registered as a system session > > > > > > > > (using tee_client_system_session()). > > > > > > > > > > > > > > > > For sake of simplicity, initialization of call queue management > > > > > > > > is factorized into new helper function optee_cq_init(). > > > > > > > > > > > > > > > > The SMC ABI part of the driver enables this tracking, but the > > > > > > > > FF-A ABI part does not. > > > > > > > > > > > > > > > > > > > > > > > > Co-developed-by: Jens Wiklander > > > > > > > > Signed-off-by: Jens Wiklander > > > > > > > > Co-developed-by: Sumit Garg > > > > > > > > Signed-off-by: Sumit Garg > > > > > > > > Signed-off-by: Etienne Carriere > > > > > > > > --- > > > > > > > > Changes since v9: > > > > > > > > - Add a reference counter for TEE system thread provisioning. We reserve > > > > > > > > a TEE thread context for system session only when there is at least > > > > > > > > 1 opened system session. > > > > > > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in > > > > > > > > patch v8. Using a single list can prevent a waiting system thread from > > > > > > > > being resumed if the executing system thread wakes a normal waiter in > > > > > > > > the list. > > > > > > > > > > > > > > How would that be possible? The system thread wakeup > > > > > > > (free_thread_threshold = 0) is given priority over normal thread > > > > > > > wakeup (free_thread_threshold = 1). I think a single queue list would > > > > > > > be sufficient as demonstrated in v9. > > > > > > > > > > > > > > > > > > > Hello Sumit, > > > > > > > > > > > > I think a system session can be trapped waiting when using a single queue list. > > > > > > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter. > > > > > > > > > > > > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init), > > > > > > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2). > > > > > > > > > > > > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0), > > > > > > Now comes the other system session: it goes to the waitqueue list. > > > > > > Now comes a normal session invocation: it goes to the waitqueue list, 1st position. > > > > > > > > > > > > Now, TEE system thread returns to Linux: > > > > > > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue. > > > > > > The 1st element in the waitqueue list is the last entered normal session invocation. > > > > > > However, that waiter won't switch local boolean 'need_wait' to false because ::free_thread_count=1 and ::sys_thread_req_count!=0. > > > > > > So no attempt to reach TEE and wake another waiter on return. > > > > > > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE. > > > > > > > > > > I suppose the following loop tries to wake-up every waiter to give > > > > > them a chance to enter OP-TEE. So with that system session would > > > > > always be prefered over normal session, right? > > > > > > > > No, the below loop will wake only the 1st waiter it finds in the list that is > > > > current waiting (completion_done() returns false). So if it finds a normal > > > > session, it will only wake this one which, in turn, will not try to reach the > > > > TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is > > > > not enough free threads. Because it does not reach the TEE, it will not > > > > it wake another waiter. > > > > > > > > > > Okay I see your point, so how about the following change on top of v9. > > > I still think having 2 queues is an overkill here. > > > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > > index df5fb5410b72..47f57054d9b7 100644 > > > --- a/drivers/tee/optee/call.c > > > +++ b/drivers/tee/optee/call.c > > > @@ -60,6 +60,7 @@ void optee_cq_wait_init(struct optee_call_queue *cq, > > > */ > > > init_completion(&w->c); > > > list_add_tail(&w->list_node, &cq->waiters); > > > + w->sys_thread = sys_thread; > > > > > > ... > > > > > > @@ -83,6 +84,14 @@ static void optee_cq_complete_one(struct > > > optee_call_queue *cq) > > > { > > > struct optee_call_waiter *w; > > > > > > + /* Try to wakeup system session capable threads first */ > > > + list_for_each_entry(w, &cq->waiters, list_node) { > > > + if (!completion_done(&w->c) && w->sys_thread) { > > > + complete(&w->c); > > > + return; > > > + } > > > + } > > > + > > > > Indeed, looking for system sessions first in the list would address the issue. > > I would test sys_thread firs: if (w->sys_thread && !completion_done(&w->c)) > > Ack. > > > > > That said, is it better to have 2 lists or to have 1 list possibly scanned twice? > > I would prefer to reuse the existing queue. Ok, I'll do. BR, Etienne > > -Sumit > > > I'm fine with both ways. > > > > etienne > > > > > > > list_for_each_entry(w, &cq->waiters, list_node) { > > > if (!completion_done(&w->c)) { > > > complete(&w->c); > > > diff --git a/drivers/tee/optee/optee_private.h > > > b/drivers/tee/optee/optee_private.h > > > index 6bb5cae09688..a7817ce9f90f 100644 > > > --- a/drivers/tee/optee/optee_private.h > > > +++ b/drivers/tee/optee/optee_private.h > > > @@ -43,6 +43,7 @@ typedef void (optee_invoke_fn)(unsigned long, > > > unsigned long, unsigned long, > > > struct optee_call_waiter { > > > struct list_head list_node; > > > struct completion c; > > > + bool sys_thread; > > > }; > > > > > > struct optee_call_queue { > > > > > > -Sumit > > > > > > > > > > > > > static void optee_cq_complete_one(struct optee_call_queue *cq) > > > > > { > > > > > struct optee_call_waiter *w; > > > > > > > > > > list_for_each_entry(w, &cq->waiters, list_node) { > > > > > if (!completion_done(&w->c)) { > > > > > complete(&w->c); > > > > > break; > > > > > } > > > > > } > > > > > } > > > > > > > > > > -Sumit > > > > > > > > > > > > > Note I've found a error in this patch v10, see below. > > > > > > > > BR, > > > > Etienne > > > > > > > > > > > > > > > > > > > > With 2 lists, we first treat system sessions to overcome that. > > > > > > Am I missing something? > > > > > > > > > > > > Best regards, > > > > > > Etienne > > > > > > > > > > > > > -Sumit > > > > > > > > > (snip) > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel