Message ID | 20231009153427.20951-14-brgl@bgdev.pl |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a888:0:b0:403:3b70:6f57 with SMTP id x8csp1952711vqo; Mon, 9 Oct 2023 08:36:31 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG4ngVxB1pGZyFMVTi5MOgHqhrno5d3xtXGfPnUY2J59QHHyl/8T3nVlL/nOORQHIAAxktF X-Received: by 2002:a05:6a21:2d89:b0:138:68b9:138e with SMTP id ty9-20020a056a212d8900b0013868b9138emr8069pzb.8.1696865791266; Mon, 09 Oct 2023 08:36:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696865791; cv=none; d=google.com; s=arc-20160816; b=FzIWtlDvRc6Eio+nUuT9mRL8lW5ndRKs70biH6GFQJWW8qQlDhjnmvCz17rJu2QXr/ z3shUk+UXeXuvxm+f7XbdiSBdCkmiBjLxfXP0ls1yAUp/2eVwcHtNWGodAtofVKn3+96 Gxm62FzDx8lVm+iQV/WXb+piGuQYwd5RRY14JIREBRwfYXFlA7b0W8cmSbA61KKMuzoy iIg3mazgMcKwktIJ85ouXvFtLSohptNsXYf0m2rZ4zzQwRlfCJR0cCbItreQKrUcS0VP YiGyEOYpqgmiyhEm8gqVDYBkC1Xs4qAYvESeJpW32uE/i75uobTAWrNY2XvY8dKRZqVL zKjg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=bf2AqtKJ/+ji6DGMnGdvnoiBlKH8gD5Uamvdkv0NVy0=; fh=lirm1ccAeXZZB1cIo+N1DqklpgcFmD/vJ7PCWNIL0HU=; b=M6/E3gqWDR/fSfC4HM6nXztzBSqkexkWPqMdMcDpnVsWgh4hUqFJA/j5vv3MGYzwK6 kpC/NexfcwbfjzWyK8k5r7Pjw5wbmJtTcbn00ICRNGO4g63P+d4+8yGxpxrhBxism4Ca XP8LEDeBRKx47hD48dMalpHDhTRZauAftad3ECEYEs56JaPAImYLpe4bqEAZg+jrZvoK c/8e4Q5iCnVSMreEbdgzpVQ26jlKS/dufjTrRCDsNkMceRqffTDDpKRLlu+y1rlCGrF5 QJ/+BBQbfXfZ97T4M7+0BIE6AygugsC9BxDyItvtP1rO1reKVavucFLJ/GCtmn7Qu2Pj vW4Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=I+qUoC4N; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id 38-20020a631266000000b00578d2d19575si9574299pgs.237.2023.10.09.08.36.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Oct 2023 08:36:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=I+qUoC4N; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 3A2A480B28BA; Mon, 9 Oct 2023 08:36:26 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1376859AbjJIPfp (ORCPT <rfc822;ezelljr.billy@gmail.com> + 18 others); Mon, 9 Oct 2023 11:35:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1377735AbjJIPfZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 9 Oct 2023 11:35:25 -0400 Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1F677125 for <linux-kernel@vger.kernel.org>; Mon, 9 Oct 2023 08:34:58 -0700 (PDT) Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-3248e90f032so4639894f8f.1 for <linux-kernel@vger.kernel.org>; Mon, 09 Oct 2023 08:34:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1696865696; x=1697470496; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=bf2AqtKJ/+ji6DGMnGdvnoiBlKH8gD5Uamvdkv0NVy0=; b=I+qUoC4N3GfMwbwX6itAhk5/f67zdJW99P9ZnOl279uU+K/NMtetG+oD3pT3Ucj3ic bquJ3nDIirDvYYRHMmMIgqvr+mHb34ms9miNYDjTh0Lz5HfvpA/cYTceqM5zNSmRqcnO WvVloMxnKxQWMoZ3x+lHbOo55x3JuN37rA60l5b+mv/7scqrA2A+Roya+6a4SYi/dUgO QfVgi9Qiie50iH/JSTJeAF41g+c4PqGRcQy29nKQha/y18Qt3EQ9+wgEE01jOLDNXyqM OG0BTQDT5d4oJoPPsaWCxusqH/F3MNh9pKHY/TWcobpQbITB7rvxyh7unVUQS3eu2KLe Q3FQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696865696; x=1697470496; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=bf2AqtKJ/+ji6DGMnGdvnoiBlKH8gD5Uamvdkv0NVy0=; b=AWkvvdYCQGVHV/dCI05TDir0Zh853hsV6ByrPIVS65iAohmvcFIkqQ38dOj3XsbvQb iOOWxL4EMGeLC5vkO6CPFeHViugjmPYjsMznsX2COlpmY/Lz7WeZtnrgRaILTu/d9Oc9 H6+odFyyY5EbTs7aDsFr4GsBYYDC63wLsdIe6QD6wmhYkWsclK7dzBDqlk7aVJvmnWHx IVVcnT1tQJwxq5vgTlG9M8ffjcSG9IG08F5JOwFObMrXKeqz3V7ZLTnNLu8LXB9OJ3LW T+aG2QFV+DBWClTSTXTGmnywTW3UG6h0cReFY/g8pQDuj4d9tb3PQnJdSBaIKKrgCSB6 jt4g== X-Gm-Message-State: AOJu0Yxip2Qc1Navsn9SSsT0KALnFmOmRhRm0nHIfvPneBmwvgfKHR8I pA7y2It9F7abfeX4DqJwXNKp8g== X-Received: by 2002:a5d:6909:0:b0:317:70da:abdd with SMTP id t9-20020a5d6909000000b0031770daabddmr14203281wru.59.1696865696468; Mon, 09 Oct 2023 08:34:56 -0700 (PDT) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:f20d:2959:7545:e99f]) by smtp.gmail.com with ESMTPSA id b3-20020adff243000000b0031431fb40fasm10016521wrp.89.2023.10.09.08.34.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Oct 2023 08:34:56 -0700 (PDT) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Elliot Berman <quic_eberman@quicinc.com>, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Guru Das Srinagesh <quic_gurus@quicinc.com>, Andrew Halaney <ahalaney@redhat.com>, Maximilian Luz <luzmaximilian@gmail.com>, Alex Elder <elder@linaro.org>, Srini Kandagatla <srinivas.kandagatla@linaro.org> Cc: linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@quicinc.com, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [PATCH v3 13/15] firmware: qcom: tzmem: enable SHM Bridge support Date: Mon, 9 Oct 2023 17:34:25 +0200 Message-Id: <20231009153427.20951-14-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231009153427.20951-1-brgl@bgdev.pl> References: <20231009153427.20951-1-brgl@bgdev.pl> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=2.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_SBL_CSS, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 09 Oct 2023 08:36:26 -0700 (PDT) X-Spam-Level: ** X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779292743982803617 X-GMAIL-MSGID: 1779292743982803617 |
Series |
arm64: qcom: add and enable SHM Bridge support
|
|
Commit Message
Bartosz Golaszewski
Oct. 9, 2023, 3:34 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Add a new Kconfig option for selecting the SHM Bridge mode of operation for the TrustZone memory allocator. If enabled at build-time, it will still be checked for availability at run-time. If the architecture doesn't support SHM Bridge, the allocator will work just like in the default mode. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- drivers/firmware/qcom/Kconfig | 10 +++++ drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-)
Comments
On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > for the TrustZone memory allocator. > > If enabled at build-time, it will still be checked for availability at > run-time. If the architecture doesn't support SHM Bridge, the allocator > will work just like in the default mode. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom/Kconfig | 10 +++++ > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- > 2 files changed, 76 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > index 237da40de832..e01407e31ae4 100644 > --- a/drivers/firmware/qcom/Kconfig > +++ b/drivers/firmware/qcom/Kconfig > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT > Use the default allocator mode. The memory is page-aligned, non-cachable > and contiguous. > > +config QCOM_TZMEM_MODE_SHMBRIDGE > + bool "SHM Bridge" > + help > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > + buffer. > + > + With this selected, all buffers passed to the TrustZone must be allocated > + using the TZMem allocator or else the TrustZone will refuse to use them. > + > endchoice > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > index eee51fed756e..b3137844fe43 100644 > --- a/drivers/firmware/qcom/qcom_tzmem.c > +++ b/drivers/firmware/qcom/qcom_tzmem.c > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > } > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > + > +#include <linux/firmware/qcom/qcom_scm.h> > + > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > + > +static bool qcom_tzmem_using_shm_bridge; > + > +static int qcom_tzmem_init(void) > +{ > + int ret; > + > + ret = qcom_scm_shm_bridge_enable(); > + if (ret == -EOPNOTSUPP) { > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > + ret = 0; > + } > + > + if (!ret) > + qcom_tzmem_using_shm_bridge = true; Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make sense? Setting ret to 0 and then claiming we're using shm_bridge seems wrong to me. > + > + return ret; > +} > + > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > +{ > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; > + int ret; > + > + if (!qcom_tzmem_using_shm_bridge) > + return 0; > + > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); Is there any sanity checking that can be done here? I assume bits 0-11 are all flag fields (or at least unrelated to size which I assume at a minimum must be 4k aka bit 12). > + > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); Consider __free(kfree) + return_ptr() usage? > + if (!handle) > + return -ENOMEM; > + > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > + ipfn_and_s_perm, size_and_flags, > + QCOM_SCM_VMID_HLOS, handle); > + if (ret) { > + kfree(handle); > + return ret; > + } > + > + pool->priv = handle; > + > + return 0; > +} > + > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > +{ > + u64 *handle = pool->priv; > + > + if (!qcom_tzmem_using_shm_bridge) > + return; > + > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > + kfree(handle); > +} > + > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > /** > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > -- > 2.39.2 >
On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote: > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > > for the TrustZone memory allocator. > > > > If enabled at build-time, it will still be checked for availability at > > run-time. If the architecture doesn't support SHM Bridge, the allocator > > will work just like in the default mode. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/firmware/qcom/Kconfig | 10 +++++ > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- > > 2 files changed, 76 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > index 237da40de832..e01407e31ae4 100644 > > --- a/drivers/firmware/qcom/Kconfig > > +++ b/drivers/firmware/qcom/Kconfig > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT > > Use the default allocator mode. The memory is page-aligned, non-cachable > > and contiguous. > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > + bool "SHM Bridge" > > + help > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > + buffer. > > + > > + With this selected, all buffers passed to the TrustZone must be allocated > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > + > > endchoice > > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > index eee51fed756e..b3137844fe43 100644 > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > } > > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > > + > > +#include <linux/firmware/qcom/qcom_scm.h> > > + > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > > + > > +static bool qcom_tzmem_using_shm_bridge; > > + > > +static int qcom_tzmem_init(void) > > +{ > > + int ret; > > + > > + ret = qcom_scm_shm_bridge_enable(); > > + if (ret == -EOPNOTSUPP) { > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > > + ret = 0; > > + } > > + > > + if (!ret) > > + qcom_tzmem_using_shm_bridge = true; > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make > sense? Setting ret to 0 and then claiming we're using shm_bridge seems > wrong to me. > > > + > > + return ret; > > +} > > + > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > > +{ > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; > > + int ret; > > + > > + if (!qcom_tzmem_using_shm_bridge) > > + return 0; > > + > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); > > Is there any sanity checking that can be done here? I assume bits 0-11 are all > flag fields (or at least unrelated to size which I assume at a minimum > must be 4k aka bit 12). I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is probably ok for all future users, but I do think some sanity would be nice to indicate the size's allowed for SHM bridge. > > > + > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > > Consider __free(kfree) + return_ptr() usage? > > > + if (!handle) > > + return -ENOMEM; > > + > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > > + ipfn_and_s_perm, size_and_flags, > > + QCOM_SCM_VMID_HLOS, handle); > > + if (ret) { > > + kfree(handle); > > + return ret; > > + } > > + > > + pool->priv = handle; > > + > > + return 0; > > +} > > + > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > +{ > > + u64 *handle = pool->priv; > > + > > + if (!qcom_tzmem_using_shm_bridge) > > + return; > > + > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > > + kfree(handle); > > +} > > + > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > > > /** > > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > > -- > > 2.39.2 > >
On Thu, Oct 12, 2023 at 12:17 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote: > > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > > > for the TrustZone memory allocator. > > > > > > If enabled at build-time, it will still be checked for availability at > > > run-time. If the architecture doesn't support SHM Bridge, the allocator > > > will work just like in the default mode. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > drivers/firmware/qcom/Kconfig | 10 +++++ > > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- > > > 2 files changed, 76 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > > index 237da40de832..e01407e31ae4 100644 > > > --- a/drivers/firmware/qcom/Kconfig > > > +++ b/drivers/firmware/qcom/Kconfig > > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT > > > Use the default allocator mode. The memory is page-aligned, non-cachable > > > and contiguous. > > > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > > + bool "SHM Bridge" > > > + help > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > > + buffer. > > > + > > > + With this selected, all buffers passed to the TrustZone must be allocated > > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > > + > > > endchoice > > > > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > > index eee51fed756e..b3137844fe43 100644 > > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > > > } > > > > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > > > + > > > +#include <linux/firmware/qcom/qcom_scm.h> > > > + > > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > > > + > > > +static bool qcom_tzmem_using_shm_bridge; > > > + > > > +static int qcom_tzmem_init(void) > > > +{ > > > + int ret; > > > + > > > + ret = qcom_scm_shm_bridge_enable(); > > > + if (ret == -EOPNOTSUPP) { > > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > > > + ret = 0; > > > + } > > > + > > > + if (!ret) > > > + qcom_tzmem_using_shm_bridge = true; > > > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make > > sense? Setting ret to 0 and then claiming we're using shm_bridge seems > > wrong to me. > > You answered yourself in the previous email. The size cannot be less than 4096 bytes. There's no need to check it anymore than that IMO. Bart > > > + > > > + return ret; > > > +} > > > + > > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > > > +{ > > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; > > > + int ret; > > > + > > > + if (!qcom_tzmem_using_shm_bridge) > > > + return 0; > > > + > > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); > > > > Is there any sanity checking that can be done here? I assume bits 0-11 are all > > flag fields (or at least unrelated to size which I assume at a minimum > > must be 4k aka bit 12). > > I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is > probably ok for all future users, but I do think some sanity would be > nice to indicate the size's allowed for SHM bridge. > > > > > > + > > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > > > > Consider __free(kfree) + return_ptr() usage? > > > > > + if (!handle) > > > + return -ENOMEM; > > > + > > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > > > + ipfn_and_s_perm, size_and_flags, > > > + QCOM_SCM_VMID_HLOS, handle); > > > + if (ret) { > > > + kfree(handle); > > > + return ret; > > > + } > > > + > > > + pool->priv = handle; > > > + > > > + return 0; > > > +} > > > + > > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > +{ > > > + u64 *handle = pool->priv; > > > + > > > + if (!qcom_tzmem_using_shm_bridge) > > > + return; > > > + > > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > > > + kfree(handle); > > > +} > > > + > > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > > > > > /** > > > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > > > -- > > > 2.39.2 > > > >
On Fri, Oct 13, 2023 at 10:32:00AM +0200, Bartosz Golaszewski wrote: > On Thu, Oct 12, 2023 at 12:17 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote: > > > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > > > > for the TrustZone memory allocator. > > > > > > > > If enabled at build-time, it will still be checked for availability at > > > > run-time. If the architecture doesn't support SHM Bridge, the allocator > > > > will work just like in the default mode. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > --- > > > > drivers/firmware/qcom/Kconfig | 10 +++++ > > > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- > > > > 2 files changed, 76 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > > > index 237da40de832..e01407e31ae4 100644 > > > > --- a/drivers/firmware/qcom/Kconfig > > > > +++ b/drivers/firmware/qcom/Kconfig > > > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT > > > > Use the default allocator mode. The memory is page-aligned, non-cachable > > > > and contiguous. > > > > > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > > > + bool "SHM Bridge" > > > > + help > > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > > > + buffer. > > > > + > > > > + With this selected, all buffers passed to the TrustZone must be allocated > > > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > > > + > > > > endchoice > > > > > > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > > > index eee51fed756e..b3137844fe43 100644 > > > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > > > > > } > > > > > > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > > > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > > > > + > > > > +#include <linux/firmware/qcom/qcom_scm.h> > > > > + > > > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > > > > + > > > > +static bool qcom_tzmem_using_shm_bridge; > > > > + > > > > +static int qcom_tzmem_init(void) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = qcom_scm_shm_bridge_enable(); > > > > + if (ret == -EOPNOTSUPP) { > > > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > > > > + ret = 0; > > > > + } > > > > + > > > > + if (!ret) > > > > + qcom_tzmem_using_shm_bridge = true; > > > > > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make > > > sense? Setting ret to 0 and then claiming we're using shm_bridge seems > > > wrong to me. > > > > > You answered yourself in the previous email. The size cannot be less > than 4096 bytes. There's no need to check it anymore than that IMO. > Ok, I still think that would be nice but your call. But this comment was about the above ret = 0 assignment. I am thinking that's incorrect, because you fail to enable SHM bridge with -EOPNOTSUPP, then you go ahead and tell the code to claim it is supported with the if (!ret) conditional. This results in SHM bridge trying to be used when its really not supported right? Looks to me that you're really trying to return 0, not ret = 0. > Bart > > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > > > > +{ > > > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; > > > > + int ret; > > > > + > > > > + if (!qcom_tzmem_using_shm_bridge) > > > > + return 0; > > > > + > > > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > > > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > > > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > > > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); > > > > > > Is there any sanity checking that can be done here? I assume bits 0-11 are all > > > flag fields (or at least unrelated to size which I assume at a minimum > > > must be 4k aka bit 12). > > > > I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is > > probably ok for all future users, but I do think some sanity would be > > nice to indicate the size's allowed for SHM bridge. > > > > > > > > > + > > > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > > > > > > Consider __free(kfree) + return_ptr() usage? > > > > > > > + if (!handle) > > > > + return -ENOMEM; > > > > + > > > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > > > > + ipfn_and_s_perm, size_and_flags, > > > > + QCOM_SCM_VMID_HLOS, handle); > > > > + if (ret) { > > > > + kfree(handle); > > > > + return ret; > > > > + } > > > > + > > > > + pool->priv = handle; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > +{ > > > > + u64 *handle = pool->priv; > > > > + > > > > + if (!qcom_tzmem_using_shm_bridge) > > > > + return; > > > > + > > > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > > > > + kfree(handle); > > > > +} > > > > + > > > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > > > > > > > /** > > > > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > > > > -- > > > > 2.39.2 > > > > > > >
On Fri, Oct 13, 2023 at 3:31 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Fri, Oct 13, 2023 at 10:32:00AM +0200, Bartosz Golaszewski wrote: > > On Thu, Oct 12, 2023 at 12:17 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Wed, Oct 11, 2023 at 04:14:32PM -0500, Andrew Halaney wrote: > > > > On Mon, Oct 09, 2023 at 05:34:25PM +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > > > > > for the TrustZone memory allocator. > > > > > > > > > > If enabled at build-time, it will still be checked for availability at > > > > > run-time. If the architecture doesn't support SHM Bridge, the allocator > > > > > will work just like in the default mode. > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > --- > > > > > drivers/firmware/qcom/Kconfig | 10 +++++ > > > > > drivers/firmware/qcom/qcom_tzmem.c | 67 +++++++++++++++++++++++++++++- > > > > > 2 files changed, 76 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > > > > > index 237da40de832..e01407e31ae4 100644 > > > > > --- a/drivers/firmware/qcom/Kconfig > > > > > +++ b/drivers/firmware/qcom/Kconfig > > > > > @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT > > > > > Use the default allocator mode. The memory is page-aligned, non-cachable > > > > > and contiguous. > > > > > > > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > > > > + bool "SHM Bridge" > > > > > + help > > > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > > > > + buffer. > > > > > + > > > > > + With this selected, all buffers passed to the TrustZone must be allocated > > > > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > > > > + > > > > > endchoice > > > > > > > > > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > > > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > > > > index eee51fed756e..b3137844fe43 100644 > > > > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > > > > @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > > > > > > > } > > > > > > > > > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > > > > > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > > > > > + > > > > > +#include <linux/firmware/qcom/qcom_scm.h> > > > > > + > > > > > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > > > > > + > > > > > +static bool qcom_tzmem_using_shm_bridge; > > > > > + > > > > > +static int qcom_tzmem_init(void) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = qcom_scm_shm_bridge_enable(); > > > > > + if (ret == -EOPNOTSUPP) { > > > > > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > > > > > + ret = 0; > > > > > + } > > > > > + > > > > > + if (!ret) > > > > > + qcom_tzmem_using_shm_bridge = true; > > > > > > > > Does the qcom_scm_shm_bridge_enable() returning -EOPNOTSUPP case make > > > > sense? Setting ret to 0 and then claiming we're using shm_bridge seems > > > > wrong to me. > > > > > > > > You answered yourself in the previous email. The size cannot be less > > than 4096 bytes. There's no need to check it anymore than that IMO. > > > > Ok, I still think that would be nice but your call. > > But this comment was about the above ret = 0 assignment. I am thinking > that's incorrect, because you fail to enable SHM bridge with > -EOPNOTSUPP, then you go ahead and tell the code to claim it is > supported with the if (!ret) conditional. This results in SHM bridge > trying to be used when its really not supported right? > > Looks to me that you're really trying to return 0, not ret = 0. > Gah! You're right, I should have waited with v4. Oh well, I'll respin it next week. Bart > > Bart > > > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > > > > > +{ > > > > > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; > > > > > + int ret; > > > > > + > > > > > + if (!qcom_tzmem_using_shm_bridge) > > > > > + return 0; > > > > > + > > > > > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > > > > > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > > > > > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > > > > > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); > > > > > > > > Is there any sanity checking that can be done here? I assume bits 0-11 are all > > > > flag fields (or at least unrelated to size which I assume at a minimum > > > > must be 4k aka bit 12). > > > > > > I guess qcom_tzmem_pool_new's PAGE_ALIGN would make sure this is > > > probably ok for all future users, but I do think some sanity would be > > > nice to indicate the size's allowed for SHM bridge. > > > > > > > > > > > > + > > > > > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > > > > > > > > Consider __free(kfree) + return_ptr() usage? > > > > > > > > > + if (!handle) > > > > > + return -ENOMEM; > > > > > + > > > > > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > > > > > + ipfn_and_s_perm, size_and_flags, > > > > > + QCOM_SCM_VMID_HLOS, handle); > > > > > + if (ret) { > > > > > + kfree(handle); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + pool->priv = handle; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > > > > +{ > > > > > + u64 *handle = pool->priv; > > > > > + > > > > > + if (!qcom_tzmem_using_shm_bridge) > > > > > + return; > > > > > + > > > > > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > > > > > + kfree(handle); > > > > > +} > > > > > + > > > > > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > > > > > > > > > /** > > > > > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > > > > > -- > > > > > 2.39.2 > > > > > > > > > > >
diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig index 237da40de832..e01407e31ae4 100644 --- a/drivers/firmware/qcom/Kconfig +++ b/drivers/firmware/qcom/Kconfig @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT Use the default allocator mode. The memory is page-aligned, non-cachable and contiguous. +config QCOM_TZMEM_MODE_SHMBRIDGE + bool "SHM Bridge" + help + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as + in the 'Default' allocator but is also explicitly marked as an SHM Bridge + buffer. + + With this selected, all buffers passed to the TrustZone must be allocated + using the TZMem allocator or else the TrustZone will refuse to use them. + endchoice config QCOM_SCM_DOWNLOAD_MODE_DEFAULT diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c index eee51fed756e..b3137844fe43 100644 --- a/drivers/firmware/qcom/qcom_tzmem.c +++ b/drivers/firmware/qcom/qcom_tzmem.c @@ -55,7 +55,72 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) } -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) + +#include <linux/firmware/qcom/qcom_scm.h> + +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 + +static bool qcom_tzmem_using_shm_bridge; + +static int qcom_tzmem_init(void) +{ + int ret; + + ret = qcom_scm_shm_bridge_enable(); + if (ret == -EOPNOTSUPP) { + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); + ret = 0; + } + + if (!ret) + qcom_tzmem_using_shm_bridge = true; + + return ret; +} + +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) +{ + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms, *handle; + int ret; + + if (!qcom_tzmem_using_shm_bridge) + return 0; + + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); + + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, + ipfn_and_s_perm, size_and_flags, + QCOM_SCM_VMID_HLOS, handle); + if (ret) { + kfree(handle); + return ret; + } + + pool->priv = handle; + + return 0; +} + +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) +{ + u64 *handle = pool->priv; + + if (!qcom_tzmem_using_shm_bridge) + return; + + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); + kfree(handle); +} + +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ /** * qcom_tzmem_pool_new() - Create a new TZ memory pool.