* [PATCH 28/29] crypto: talitos - Clean up includes in core driver file
From: Paul Louvel @ 2026-05-28 9:08 UTC (permalink / raw)
To: Herbert Xu, David S. Miller
Cc: Thomas Petazzoni, Herve Codina, Christophe Leroy, linux-crypto,
linux-kernel, Paul Louvel
In-Reply-To: <20260528-7-1-rc1_talitos_cleanup-v1-0-cb1ad6cdea49@bootlin.com>
Now that the crypto implementations have been moved into their own
translation units, the talitos core driver file no longer needs the
includes that were only required by the hash, skcipher, aead, and hwrng
code. Remove them, and drop duplicates already provided by talitos.h.
Also sort the remaining includes alphabetically.
---
drivers/crypto/talitos/talitos.c | 23 +++--------------------
1 file changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/crypto/talitos/talitos.c b/drivers/crypto/talitos/talitos.c
index a032907e900f..43939a9cd9ce 100644
--- a/drivers/crypto/talitos/talitos.c
+++ b/drivers/crypto/talitos/talitos.c
@@ -12,32 +12,15 @@
* All rights reserved.
*/
+#include <linux/crypto.h>
+#include <linux/io.h>
#include <linux/kernel.h>
-#include <linux/module.h>
#include <linux/mod_devicetable.h>
-#include <linux/crypto.h>
+#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_irq.h>
-#include <linux/platform_device.h>
-#include <linux/dma-mapping.h>
-#include <linux/io.h>
-#include <linux/spinlock.h>
-#include <linux/rtnetlink.h>
#include <linux/slab.h>
-#include <crypto/algapi.h>
-#include <crypto/aes.h>
-#include <crypto/internal/des.h>
-#include <crypto/sha1.h>
-#include <crypto/sha2.h>
-#include <crypto/md5.h>
-#include <crypto/internal/aead.h>
-#include <crypto/authenc.h>
-#include <crypto/internal/skcipher.h>
-#include <crypto/hash.h>
-#include <crypto/internal/hash.h>
-#include <crypto/scatterwalk.h>
-
#include "talitos.h"
/*
--
2.54.0
^ permalink raw reply related
* [PATCH 29/29] crypto: talitos - Remove TALITOS_DESC_SIZE macro
From: Paul Louvel @ 2026-05-28 9:08 UTC (permalink / raw)
To: Herbert Xu, David S. Miller
Cc: Thomas Petazzoni, Herve Codina, Christophe Leroy, linux-crypto,
linux-kernel, Paul Louvel
In-Reply-To: <20260528-7-1-rc1_talitos_cleanup-v1-0-cb1ad6cdea49@bootlin.com>
Now that struct talitos_desc no longer has the SEC1-only next_desc field
(it was moved into sec1_talitos_desc), TALITOS_DESC_SIZE is identical to
sizeof(struct talitos_desc) and no longer serves any purpose. Remove it
and use sizeof directly at each macro invocation.
Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
drivers/crypto/talitos/talitos-sec1.c | 10 +++++-----
drivers/crypto/talitos/talitos-sec2.c | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/talitos/talitos-sec1.c b/drivers/crypto/talitos/talitos-sec1.c
index e4f482520372..504ce9e23e59 100644
--- a/drivers/crypto/talitos/talitos-sec1.c
+++ b/drivers/crypto/talitos/talitos-sec1.c
@@ -190,7 +190,7 @@ static void sec1_dma_map_request(struct device *dev,
while (edesc) {
dma_desc = dma_map_single(dev, &edesc->desc.sec1.hdr,
- TALITOS_DESC_SIZE, DMA_BIDIRECTIONAL);
+ sizeof(struct talitos_desc), DMA_BIDIRECTIONAL);
if (!prev_edesc) {
request->dma_desc = dma_desc;
@@ -202,7 +202,7 @@ static void sec1_dma_map_request(struct device *dev,
prev_edesc->desc.sec1.next_desc = cpu_to_be32(dma_desc);
dma_sync_single_for_device(dev, prev_dma_desc,
- TALITOS_DESC_SIZE, DMA_TO_DEVICE);
+ sizeof(struct talitos_desc), DMA_TO_DEVICE);
next:
prev_edesc = edesc;
@@ -216,12 +216,12 @@ static void sec1_dma_unmap_request(struct device *dev,
{
struct talitos_edesc *edesc;
- dma_unmap_single(dev, request->dma_desc, TALITOS_DESC_SIZE,
+ dma_unmap_single(dev, request->dma_desc, sizeof(struct talitos_desc),
DMA_BIDIRECTIONAL);
edesc = container_of(request->desc, struct talitos_edesc, desc);
while (edesc->next_desc) {
dma_unmap_single(dev, be32_to_cpu(edesc->desc.sec1.next_desc),
- TALITOS_DESC_SIZE, DMA_BIDIRECTIONAL);
+ sizeof(struct talitos_desc), DMA_BIDIRECTIONAL);
edesc = edesc->next_desc;
}
}
@@ -239,7 +239,7 @@ static __be32 sec1_get_request_hdr(struct device *dev,
edesc = edesc->next_desc;
}
- dma_sync_single_for_cpu(dev, dma_desc, TALITOS_DESC_SIZE,
+ dma_sync_single_for_cpu(dev, dma_desc, sizeof(struct talitos_desc),
DMA_BIDIRECTIONAL);
return edesc->desc.sec1.hdr;
diff --git a/drivers/crypto/talitos/talitos-sec2.c b/drivers/crypto/talitos/talitos-sec2.c
index 52f783ddc8b6..0df3b22510c7 100644
--- a/drivers/crypto/talitos/talitos-sec2.c
+++ b/drivers/crypto/talitos/talitos-sec2.c
@@ -205,7 +205,7 @@ static void sec2_dma_map_request(struct device *dev,
struct talitos_desc *desc)
{
request->dma_desc =
- dma_map_single(dev, desc, TALITOS_DESC_SIZE, DMA_BIDIRECTIONAL);
+ dma_map_single(dev, desc, sizeof(struct talitos_desc), DMA_BIDIRECTIONAL);
}
static int sec2_talitos_handle_error(struct device *dev, u32 isr, u32 isr_lo)
@@ -346,14 +346,14 @@ static void sec2_init_task(struct device *dev)
static void sec2_dma_unmap_request(struct device *dev,
struct talitos_request *request)
{
- dma_unmap_single(dev, request->dma_desc, TALITOS_DESC_SIZE,
+ dma_unmap_single(dev, request->dma_desc, sizeof(struct talitos_desc),
DMA_BIDIRECTIONAL);
}
static __be32 sec2_get_request_hdr(struct device *dev,
struct talitos_request *request)
{
- dma_sync_single_for_cpu(dev, request->dma_desc, TALITOS_DESC_SIZE,
+ dma_sync_single_for_cpu(dev, request->dma_desc, sizeof(struct talitos_desc),
DMA_BIDIRECTIONAL);
return request->desc->sec2.hdr;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 0/3] Add support for qcrypto on shikra
From: Kuldeep Singh @ 2026-05-28 11:54 UTC (permalink / raw)
To: Eric Biggers
Cc: Thara Gopinath, Herbert Xu, David S. Miller, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Vinod Koul, Frank Li, Andy Gross, linux-arm-msm, linux-crypto,
devicetree, linux-kernel, dmaengine, Bartosz Golaszewski,
Bartosz Golaszewski, Gaurav Kashyap, Neeraj Soni
In-Reply-To: <20260525142843.GA2018@quark>
>> +Bartosz, Gaurav, Neeraj
>>
>> Hi Eric,
>>
>> GPCE is relevant in terms of providing hardware security.
>> There are multiple usecases coming up for example to handle DRM/secure
>> buffer usecases to improve overall throughput for secure content.
>>
>> Regarding performance, it's currently slower compared to arm CE but
>> provides an edge by giving hardware security which is considered more
>> secure.
>>
>> Btw, there's been performance improvement with new targets and we are
>> expecting to achieve far more better performance with new SoCs family.
>> Pakala: GPCE - 550MBps, ARMv8 - 8GBps
>> Kaanapali: GPCE - 3GBps, ARMv8 - 10GBps
>>
>> Please note, there's almost 5x improvement in kaanapali compared to
>> pakala. Though overall is still slower compared to arm but as mentioned,
>> expecting better performance with hardware improvements as we progress.
>>
>> Also, currently qce driver exhibit stability issues and that's what we
>> are putting effort in stabilizing the software on immediate basis.
>>
>> There's parallel effort ongoing by Bartosz to introduce baseline for
>> secure buffer usecases.
>> https://lore.kernel.org/lkml/20260522-qcom-qce-cmd-descr-v18-0-99103926bafc@oss.qualcomm.com/
>> There's active development ongoing and i believe lowering cra_priority
>> for qce is fine as of now and can scale values once qce becomes
>> performance efficient.
>>
>> Please share your thoughts. Thanks!
>
> ARMv8 Crypto Extensions are "hardware" as well, just in the CPU. They
> provide constant-time execution, for example.
>
> Granted, they don't protect from power analysis and electromagnetic
> emanation attacks. Does QCE actually provide those protections, though?
QCE doesn't provide these protections currently.
What i wanted to highlight was there are certain security usecases which
are possible via dedicated crypto engine only and not via arm cpu.
> Either way, it doesn't really matter in this case. There are multiple
> aspects to security, and before even considering these advanced
> protections, the basics of security need to be absolutely solid. That
> is, the driver needs to always compute the crypto algorithms correctly,
> and it needs to be completely robust when fuzzed by unprivileged
> userspace (because it can accessed in that way).
> Yet, this driver "exhibits stability issues", fails the self-tests, and
> doesn't even have exclusive access to the hardware! These are all
> security bugs. That very much defeats the claimed point. (Plus, due to
> the performance issues no one wants to use it in Linux anyway.)
Sure, we are analyzing self-tests failures and are committed to fix any
hung/stability issue in any aspect but i do feel it should not be a
blocker to add new soc id support.
Also, could you please elaborate more on "exclusive access to hardware"?
Do you mean the hardware can be accessed by multiple execution
environment like TEE and Linux?
--
Regards
Kuldeep
^ permalink raw reply
* [PATCH v2 3/5] crypto: hisilicon/hpre - implement full backlog support for hpre driver
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
In-Reply-To: <20260528115531.174593-1-wuzongyu1@huawei.com>
From: lizhi <lizhi206@huawei.com>
When the hardware queue returns -EBUSY, requests are queued instead of
being failed immediately. The driver retries queued requests from the
completion path after earlier requests have finished.
This reduces request failures caused by temporary hardware congestion and
improves throughput and stability under high load.
Signed-off-by: lizhi <lizhi206@huawei.com>
Signed-off-by: Zongyu Wu <wuzongyu1@huawei.com>
---
drivers/crypto/hisilicon/hpre/hpre_crypto.c | 223 +++++++++++++++-----
1 file changed, 166 insertions(+), 57 deletions(-)
diff --git a/drivers/crypto/hisilicon/hpre/hpre_crypto.c b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
index 09077abbf6ad..5d5c4d5a9fbc 100644
--- a/drivers/crypto/hisilicon/hpre/hpre_crypto.c
+++ b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
@@ -30,6 +30,7 @@ struct hpre_ctx;
#define HPRE_DH_G_FLAG 0x02
#define HPRE_TRY_SEND_TIMES 100
#define HPRE_INVLD_REQ_ID (-1)
+#define HPRE_ALG_TYPE_MASK 0x1F
#define HPRE_SQE_ALG_BITS 5
#define HPRE_SQE_DONE_SHIFT 30
@@ -39,6 +40,7 @@ struct hpre_ctx;
#define HPRE_DFX_US_TO_NS 1000
#define HPRE_ENABLE_HPCORE_SHIFT 7
+#define HPRE_ECDH_CLR_DATA_SHIFT 2
/* due to nist p521 */
#define HPRE_ECC_MAX_KSZ 66
@@ -138,6 +140,8 @@ struct hpre_asym_request {
int err;
hpre_cb cb;
struct timespec64 req_time;
+ struct crypto_async_request *base;
+ struct list_head list;
};
static inline unsigned int hpre_align_sz(void)
@@ -241,8 +245,8 @@ static void hpre_hw_data_clr_all(struct hpre_ctx *ctx,
struct scatterlist *dst,
struct scatterlist *src)
{
- struct device *dev = ctx->dev;
struct hpre_sqe *sqe = &req->req;
+ struct device *dev = ctx->dev;
dma_addr_t tmp;
tmp = le64_to_cpu(sqe->in);
@@ -270,6 +274,34 @@ static void hpre_hw_data_clr_all(struct hpre_ctx *ctx,
}
}
+static void hpre_ecdh_hw_data_clr_all(struct hpre_ctx *ctx,
+ struct hpre_asym_request *req,
+ struct scatterlist *dst,
+ struct scatterlist *src)
+{
+ struct hpre_sqe *sqe = &req->req;
+ struct device *dev = ctx->dev;
+ dma_addr_t dma;
+
+ dma = le64_to_cpu(sqe->in);
+ if (unlikely(dma_mapping_error(dev, dma)))
+ return;
+
+ /* req->src may contain garbage value, check both src and req->src before freeing */
+ if (src && req->src)
+ dma_free_coherent(dev, ctx->key_sz << HPRE_ECDH_CLR_DATA_SHIFT,
+ req->src, dma);
+
+ dma = le64_to_cpu(sqe->out);
+ if (unlikely(dma_mapping_error(dev, dma)))
+ return;
+
+ if (req->dst)
+ dma_free_coherent(dev, ctx->key_sz << 1, req->dst, dma);
+ if (dst)
+ dma_unmap_single(dev, dma, ctx->key_sz << 1, DMA_FROM_DEVICE);
+}
+
static int hpre_alg_res_post_hf(struct hpre_ctx *ctx, struct hpre_sqe *sqe,
void **kreq)
{
@@ -323,6 +355,94 @@ static bool hpre_is_bd_timeout(struct hpre_asym_request *req,
return true;
}
+static int hpre_send(struct hpre_ctx *ctx, struct hpre_sqe *msg)
+{
+ struct hpre_dfx *dfx = ctx->hpre->debug.dfx;
+ int cnt = 0;
+ int ret;
+
+ do {
+ ret = hisi_qp_send(ctx->qp, msg);
+ if (ret != -EBUSY)
+ break;
+ atomic64_inc(&dfx[HPRE_SEND_BUSY_CNT].value);
+ } while (cnt++ < HPRE_TRY_SEND_TIMES);
+
+ if (likely(!ret)) {
+ atomic64_inc(&dfx[HPRE_SEND_CNT].value);
+ return ret;
+ }
+
+ if (ret != -EBUSY)
+ atomic64_inc(&dfx[HPRE_SEND_FAIL_CNT].value);
+
+ return ret;
+}
+
+static int hpre_send_backlog(struct hpre_ctx *ctx, struct hpre_sqe *msg)
+{
+ struct hpre_dfx *dfx = ctx->hpre->debug.dfx;
+ int ret;
+
+ ret = hisi_qp_send(ctx->qp, msg);
+ if (likely(!ret))
+ atomic64_inc(&dfx[HPRE_SEND_CNT].value);
+ else if (unlikely(ret != -EBUSY))
+ atomic64_inc(&dfx[HPRE_SEND_FAIL_CNT].value);
+ else
+ atomic64_inc(&dfx[HPRE_SEND_BUSY_CNT].value);
+
+ return ret;
+}
+
+static void hpre_alg_hw_data_clr_all(struct hpre_ctx *ctx, struct hpre_asym_request *h_req)
+{
+ switch (le32_to_cpu(h_req->req.dw0) & HPRE_ALG_TYPE_MASK) {
+ case HPRE_ALG_DH_G2:
+ case HPRE_ALG_DH:
+ hpre_hw_data_clr_all(ctx, h_req, h_req->areq.dh->dst, h_req->areq.dh->src);
+ break;
+ case HPRE_ALG_NC_NCRT:
+ case HPRE_ALG_NC_CRT:
+ hpre_hw_data_clr_all(ctx, h_req, h_req->areq.rsa->dst, h_req->areq.rsa->src);
+ break;
+ case HPRE_ALG_ECC_MUL:
+ hpre_ecdh_hw_data_clr_all(ctx, h_req, h_req->areq.ecdh->dst, h_req->areq.ecdh->src);
+ break;
+ default:
+ break;
+ }
+}
+
+static void hpre_alg_send_backlog(struct hisi_qp *qp)
+{
+ struct hpre_asym_request *req, *tmp;
+ int ret;
+
+ spin_lock_bh(&qp->backlog.lock);
+ list_for_each_entry_safe(req, tmp, &qp->backlog.list, list) {
+ ret = hpre_send_backlog(req->ctx, &req->req);
+ switch (ret) {
+ case 0:
+ list_del(&req->list);
+ crypto_request_complete(req->base, -EINPROGRESS);
+ break;
+ case -EBUSY:
+ /* Device is busy and stop send any request. */
+ goto unlock;
+ default:
+ /* Current no fallback for any send error. */
+ list_del(&req->list);
+ hpre_alg_hw_data_clr_all(req->ctx, req);
+ crypto_request_complete(req->base, -EIO);
+ break;
+ }
+ }
+
+unlock:
+ spin_unlock_bh(&qp->backlog.lock);
+}
+
static void hpre_dh_cb(struct hpre_ctx *ctx, void *resp)
{
struct hpre_dfx *dfx = ctx->hpre->debug.dfx;
@@ -377,6 +497,7 @@ static void hpre_alg_cb(struct hisi_qp *qp, void *resp)
}
h_req->cb(h_req->ctx, resp);
+ hpre_alg_send_backlog(qp);
}
static int hpre_ctx_init(struct hpre_ctx *ctx, u8 type)
@@ -450,25 +571,39 @@ static int hpre_msg_request_set(struct hpre_ctx *ctx, void *req, bool is_rsa)
return 0;
}
-static int hpre_send(struct hpre_ctx *ctx, struct hpre_sqe *msg)
+static int hpre_alg_try_enqueue(struct hpre_asym_request *hpre_req)
{
- struct hpre_dfx *dfx = ctx->hpre->debug.dfx;
- int ctr = 0;
+ struct hisi_qp *qp = hpre_req->ctx->qp;
+
+ /* Check if any request is already backlogged */
+ if (!list_empty(&qp->backlog.list))
+ return -EBUSY;
+
+ /* Try to enqueue to HW ring */
+ return hpre_send_backlog(hpre_req->ctx, &hpre_req->req);
+}
+
+static int hpre_alg_send_message(struct hpre_asym_request *hpre_req)
+{
+ struct hisi_qp *qp = hpre_req->ctx->qp;
int ret;
- do {
- atomic64_inc(&dfx[HPRE_SEND_CNT].value);
- ret = hisi_qp_send(ctx->qp, msg);
- if (ret != -EBUSY)
- break;
- atomic64_inc(&dfx[HPRE_SEND_BUSY_CNT].value);
- } while (ctr++ < HPRE_TRY_SEND_TIMES);
+ if (!(hpre_req->base->flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
+ ret = hpre_send(hpre_req->ctx, &hpre_req->req);
+ if (ret == -EBUSY)
+ return -ENOSPC;
+ } else {
+ ret = hpre_alg_try_enqueue(hpre_req);
+ if (ret == -EBUSY) {
+ spin_lock_bh(&qp->backlog.lock);
+ list_add_tail(&hpre_req->list, &qp->backlog.list);
+ spin_unlock_bh(&qp->backlog.lock);
+ return -EBUSY;
+ }
+ }
if (likely(!ret))
- return ret;
-
- if (ret != -EBUSY)
- atomic64_inc(&dfx[HPRE_SEND_FAIL_CNT].value);
+ return -EINPROGRESS;
return ret;
}
@@ -482,6 +617,7 @@ static int hpre_dh_compute_value(struct kpp_request *req)
struct hpre_sqe *msg = &hpre_req->req;
int ret;
+ hpre_req->base = &req->base;
ret = hpre_msg_request_set(ctx, req, false);
if (unlikely(ret))
return ret;
@@ -503,14 +639,12 @@ static int hpre_dh_compute_value(struct kpp_request *req)
else
msg->dw0 = cpu_to_le32(le32_to_cpu(msg->dw0) | HPRE_ALG_DH);
- /* success */
- ret = hpre_send(ctx, msg);
- if (likely(!ret))
- return -EINPROGRESS;
+ ret = hpre_alg_send_message(hpre_req);
+ if (likely(ret == -EINPROGRESS || ret == -EBUSY))
+ return ret;
clear_all:
hpre_hw_data_clr_all(ctx, hpre_req, req->dst, req->src);
-
return ret;
}
@@ -755,6 +889,7 @@ static int hpre_rsa_enc(struct akcipher_request *req)
struct hpre_sqe *msg = &hpre_req->req;
int ret;
+ hpre_req->base = &req->base;
/* For unsupported key size and unavailable devices, use soft tfm instead */
if (ctx->fallback) {
akcipher_request_set_tfm(req, ctx->rsa.soft_tfm);
@@ -781,10 +916,9 @@ static int hpre_rsa_enc(struct akcipher_request *req)
if (unlikely(ret))
goto clear_all;
- /* success */
- ret = hpre_send(ctx, msg);
- if (likely(!ret))
- return -EINPROGRESS;
+ ret = hpre_alg_send_message(hpre_req);
+ if (likely(ret == -EINPROGRESS || ret == -EBUSY))
+ return ret;
clear_all:
hpre_hw_data_clr_all(ctx, hpre_req, req->dst, req->src);
@@ -801,6 +935,7 @@ static int hpre_rsa_dec(struct akcipher_request *req)
struct hpre_sqe *msg = &hpre_req->req;
int ret;
+ hpre_req->base = &req->base;
/* For unsupported key size and unavailable devices, use soft tfm instead */
if (ctx->fallback) {
akcipher_request_set_tfm(req, ctx->rsa.soft_tfm);
@@ -834,10 +969,9 @@ static int hpre_rsa_dec(struct akcipher_request *req)
if (unlikely(ret))
goto clear_all;
- /* success */
- ret = hpre_send(ctx, msg);
- if (likely(!ret))
- return -EINPROGRESS;
+ ret = hpre_alg_send_message(hpre_req);
+ if (likely(ret == -EINPROGRESS || ret == -EBUSY))
+ return ret;
clear_all:
hpre_hw_data_clr_all(ctx, hpre_req, req->dst, req->src);
@@ -1387,32 +1521,6 @@ static int hpre_ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
return 0;
}
-static void hpre_ecdh_hw_data_clr_all(struct hpre_ctx *ctx,
- struct hpre_asym_request *req,
- struct scatterlist *dst,
- struct scatterlist *src)
-{
- struct device *dev = ctx->dev;
- struct hpre_sqe *sqe = &req->req;
- dma_addr_t dma;
-
- dma = le64_to_cpu(sqe->in);
- if (unlikely(dma_mapping_error(dev, dma)))
- return;
-
- if (src && req->src)
- dma_free_coherent(dev, ctx->key_sz << 2, req->src, dma);
-
- dma = le64_to_cpu(sqe->out);
- if (unlikely(dma_mapping_error(dev, dma)))
- return;
-
- if (req->dst)
- dma_free_coherent(dev, ctx->key_sz << 1, req->dst, dma);
- if (dst)
- dma_unmap_single(dev, dma, ctx->key_sz << 1, DMA_FROM_DEVICE);
-}
-
static void hpre_ecdh_cb(struct hpre_ctx *ctx, void *resp)
{
unsigned int curve_sz = hpre_ecdh_get_curvesz(ctx->curve_id);
@@ -1538,6 +1646,7 @@ static int hpre_ecdh_compute_value(struct kpp_request *req)
struct hpre_sqe *msg = &hpre_req->req;
int ret;
+ hpre_req->base = &req->base;
ret = hpre_ecdh_msg_request_set(ctx, req);
if (unlikely(ret)) {
dev_err(dev, "failed to set ecdh request, ret = %d!\n", ret);
@@ -1563,9 +1672,9 @@ static int hpre_ecdh_compute_value(struct kpp_request *req)
msg->dw0 = cpu_to_le32(le32_to_cpu(msg->dw0) | HPRE_ALG_ECC_MUL);
msg->resv1 = ctx->enable_hpcore << HPRE_ENABLE_HPCORE_SHIFT;
- ret = hpre_send(ctx, msg);
- if (likely(!ret))
- return -EINPROGRESS;
+ ret = hpre_alg_send_message(hpre_req);
+ if (likely(ret == -EINPROGRESS || ret == -EBUSY))
+ return ret;
clear_all:
hpre_ecdh_hw_data_clr_all(ctx, hpre_req, req->dst, req->src);
--
2.33.0
^ permalink raw reply related
* [PATCH v2 1/5] crypto: hisilicon/zip - add backlog support for zip
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
In-Reply-To: <20260528115531.174593-1-wuzongyu1@huawei.com>
From: Chenghai Huang <huangchenghai2@huawei.com>
When the hardware queue is busy, requests are now queued instead of
being failed immediately. Queued requests are retried when earlier
requests complete, which prevents transient failures under heavy load.
The backlog path also provides a fallback mechanism while the hardware
is temporarily unavailable, such as during device reset.
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
Signed-off-by: Zongyu Wu <wuzongyu1@huawei.com>
---
drivers/crypto/hisilicon/zip/zip_crypto.c | 286 ++++++++++++++--------
1 file changed, 183 insertions(+), 103 deletions(-)
diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
index 70adde049b53..1c7e64c067d0 100644
--- a/drivers/crypto/hisilicon/zip/zip_crypto.c
+++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
@@ -28,6 +28,7 @@
#define HZIP_ALG_DEFLATE GENMASK(5, 4)
#define HZIP_ALG_LZ4 BIT(8)
+#define HZIP_INVAL_REQ_ID ((u16)0xFFFF)
static DEFINE_MUTEX(zip_algs_lock);
static unsigned int zip_available_devs;
@@ -55,11 +56,11 @@ struct hisi_zip_req {
dma_addr_t dma_src;
dma_addr_t dma_dst;
struct hisi_zip_qp_ctx *qp_ctx;
+ struct list_head list;
u16 req_id;
};
struct hisi_zip_req_q {
- struct hisi_zip_req *q;
unsigned long *req_bitmap;
spinlock_t req_lock;
u16 size;
@@ -135,12 +136,10 @@ static int hisi_zip_fallback_do_work(struct acomp_req *acomp_req, bool is_decomp
return ret;
}
-static struct hisi_zip_req *hisi_zip_create_req(struct hisi_zip_qp_ctx *qp_ctx,
- struct acomp_req *req)
+static int hisi_zip_create_req(struct hisi_zip_req *req)
{
+ struct hisi_zip_qp_ctx *qp_ctx = req->qp_ctx;
struct hisi_zip_req_q *req_q = &qp_ctx->req_q;
- struct hisi_zip_req *q = req_q->q;
- struct hisi_zip_req *req_cache;
int req_id;
spin_lock(&req_q->req_lock);
@@ -149,28 +148,26 @@ static struct hisi_zip_req *hisi_zip_create_req(struct hisi_zip_qp_ctx *qp_ctx,
if (req_id >= req_q->size) {
spin_unlock(&req_q->req_lock);
dev_dbg(&qp_ctx->qp->qm->pdev->dev, "req cache is full!\n");
- return ERR_PTR(-EAGAIN);
+ return -EBUSY;
}
set_bit(req_id, req_q->req_bitmap);
spin_unlock(&req_q->req_lock);
- req_cache = q + req_id;
- req_cache->req_id = req_id;
- req_cache->req = req;
- req_cache->qp_ctx = qp_ctx;
+ req->req_id = req_id;
- return req_cache;
+ return 0;
}
-static void hisi_zip_remove_req(struct hisi_zip_qp_ctx *qp_ctx,
- struct hisi_zip_req *req)
+static void hisi_zip_remove_req(struct hisi_zip_req *req)
{
- struct hisi_zip_req_q *req_q = &qp_ctx->req_q;
+ struct hisi_zip_req_q *req_q = &req->qp_ctx->req_q;
spin_lock(&req_q->req_lock);
clear_bit(req->req_id, req_q->req_bitmap);
spin_unlock(&req_q->req_lock);
+
+ req->req_id = HZIP_INVAL_REQ_ID;
}
static void hisi_zip_fill_addr(struct hisi_zip_sqe *sqe, struct hisi_zip_req *req)
@@ -247,19 +244,21 @@ static void hisi_zip_fill_sqe(struct hisi_zip_ctx *ctx, struct hisi_zip_sqe *sqe
ops->fill_sqe_type(sqe, ops->sqe_type);
}
-static int hisi_zip_do_work(struct hisi_zip_qp_ctx *qp_ctx,
- struct hisi_zip_req *req)
+static void hisi_zip_enqueue_backlog(struct hisi_zip_req *req)
+{
+ struct hisi_qp *qp = req->qp_ctx->qp;
+
+ spin_lock_bh(&qp->backlog.lock);
+ list_add_tail(&req->list, &qp->backlog.list);
+ spin_unlock_bh(&qp->backlog.lock);
+}
+
+static int hisi_zip_map_req_buffers(struct hisi_zip_req *req)
{
+ struct hisi_zip_qp_ctx *qp_ctx = req->qp_ctx;
struct hisi_acc_sgl_pool *pool = qp_ctx->sgl_pool;
- struct hisi_zip_dfx *dfx = &qp_ctx->zip_dev->dfx;
+ struct device *dev = &qp_ctx->qp->qm->pdev->dev;
struct acomp_req *a_req = req->req;
- struct hisi_qp *qp = qp_ctx->qp;
- struct device *dev = &qp->qm->pdev->dev;
- struct hisi_zip_sqe zip_sqe;
- int ret;
-
- if (unlikely(!a_req->src || !a_req->slen || !a_req->dst || !a_req->dlen))
- return -EINVAL;
req->hw_src = hisi_acc_sg_buf_map_to_hw_sgl(dev, a_req->src, pool,
req->req_id << 1, &req->dma_src,
@@ -274,33 +273,110 @@ static int hisi_zip_do_work(struct hisi_zip_qp_ctx *qp_ctx,
(req->req_id << 1) + 1,
&req->dma_dst, DMA_FROM_DEVICE);
if (IS_ERR(req->hw_dst)) {
- ret = PTR_ERR(req->hw_dst);
- dev_err(dev, "failed to map the dst buffer to hw sgl (%d)!\n",
- ret);
- goto err_unmap_input;
+ dev_err(dev, "failed to map the dst buffer to hw sgl (%ld)!\n",
+ PTR_ERR(req->hw_dst));
+ hisi_acc_sg_buf_unmap(dev, a_req->src, req->hw_src, DMA_TO_DEVICE);
+ return PTR_ERR(req->hw_dst);
}
+ return 0;
+}
+
+static void hisi_zip_unmap_req_buffers(struct hisi_zip_req *req)
+{
+ struct device *dev = &req->qp_ctx->qp->qm->pdev->dev;
+ struct acomp_req *a_req = req->req;
+
+ hisi_acc_sg_buf_unmap(dev, a_req->dst, req->hw_dst, DMA_FROM_DEVICE);
+ hisi_acc_sg_buf_unmap(dev, a_req->src, req->hw_src, DMA_TO_DEVICE);
+}
+
+static int hisi_zip_do_work(struct hisi_zip_req *req)
+{
+ struct hisi_zip_qp_ctx *qp_ctx = req->qp_ctx;
+ struct hisi_zip_dfx *dfx = &qp_ctx->zip_dev->dfx;
+ struct hisi_qp *qp = qp_ctx->qp;
+ struct hisi_zip_sqe zip_sqe;
+ int ret;
+
hisi_zip_fill_sqe(qp_ctx->ctx, &zip_sqe, qp_ctx->req_type, req);
/* send command to start a task */
- atomic64_inc(&dfx->send_cnt);
ret = hisi_qp_send(qp, &zip_sqe);
- if (unlikely(ret < 0)) {
- atomic64_inc(&dfx->send_busy_cnt);
- ret = -EAGAIN;
- dev_dbg_ratelimited(dev, "failed to send request!\n");
- goto err_unmap_output;
+ if (likely(!ret)) {
+ atomic64_inc(&dfx->send_cnt);
+ return -EINPROGRESS;
}
- return -EINPROGRESS;
+ if (ret == -EBUSY)
+ atomic64_inc(&dfx->send_busy_cnt);
-err_unmap_output:
- hisi_acc_sg_buf_unmap(dev, a_req->dst, req->hw_dst, DMA_FROM_DEVICE);
-err_unmap_input:
- hisi_acc_sg_buf_unmap(dev, a_req->src, req->hw_src, DMA_TO_DEVICE);
return ret;
}
+static void hisi_zip_send_backlog_soft(struct hisi_zip_qp_ctx *qp_ctx)
+{
+ bool is_decomp = qp_ctx->qp->alg_type;
+ struct hisi_zip_req *req, *tmp;
+ int ret;
+
+ list_for_each_entry_safe(req, tmp, &qp_ctx->qp->backlog.list, list) {
+ list_del(&req->list);
+
+ if (req->req_id != HZIP_INVAL_REQ_ID) {
+ hisi_zip_unmap_req_buffers(req);
+ hisi_zip_remove_req(req);
+ }
+
+ ret = hisi_zip_fallback_do_work(req->req, is_decomp);
+
+ /* Wake up the busy thread first, then return the errno. */
+ if (req->req->base.complete) {
+ acomp_request_complete(req->req, -EINPROGRESS);
+ acomp_request_complete(req->req, ret);
+ }
+ }
+}
+
+static void hisi_zip_send_backlog(struct hisi_qp *qp)
+{
+ struct hisi_zip_req *req, *tmp;
+ int ret;
+
+ spin_lock_bh(&qp->backlog.lock);
+ list_for_each_entry_safe(req, tmp, &qp->backlog.list, list) {
+ if (req->req_id == HZIP_INVAL_REQ_ID) {
+ ret = hisi_zip_create_req(req);
+ if (ret)
+ continue;
+
+ ret = hisi_zip_map_req_buffers(req);
+ if (unlikely(ret)) {
+ hisi_zip_remove_req(req);
+ hisi_zip_send_backlog_soft(req->qp_ctx);
+ goto unlock;
+ }
+ }
+
+ ret = hisi_zip_do_work(req);
+ switch (ret) {
+ case -EINPROGRESS:
+ list_del(&req->list);
+ if (req->req->base.complete)
+ acomp_request_complete(req->req, -EINPROGRESS);
+ break;
+ case -EBUSY:
+ goto unlock;
+ default:
+ hisi_zip_send_backlog_soft(req->qp_ctx);
+ goto unlock;
+ }
+ }
+
+unlock:
+ spin_unlock_bh(&qp->backlog.lock);
+}
+
static u32 hisi_zip_get_status(struct hisi_zip_sqe *sqe)
{
return sqe->dw3 & HZIP_BD_STATUS_M;
@@ -333,73 +409,97 @@ static void hisi_zip_acomp_cb(struct hisi_qp *qp, void *data)
err = -EIO;
}
- hisi_acc_sg_buf_unmap(dev, acomp_req->dst, req->hw_dst, DMA_FROM_DEVICE);
- hisi_acc_sg_buf_unmap(dev, acomp_req->src, req->hw_src, DMA_TO_DEVICE);
+ hisi_zip_unmap_req_buffers(req);
acomp_req->dlen = ops->get_dstlen(sqe);
+ hisi_zip_remove_req(req);
if (acomp_req->base.complete)
acomp_request_complete(acomp_req, err);
- hisi_zip_remove_req(qp_ctx, req);
+ hisi_zip_send_backlog(qp);
}
-static int hisi_zip_acompress(struct acomp_req *acomp_req)
+static int hisi_zip_do_comp(struct hisi_zip_req *req, bool is_decompress)
{
+ struct acomp_req *acomp_req = req->req;
struct hisi_zip_ctx *ctx = crypto_tfm_ctx(acomp_req->base.tfm);
- struct hisi_zip_qp_ctx *qp_ctx = &ctx->qp_ctx[HZIP_QPC_COMP];
- struct hisi_zip_req *req;
- struct device *dev;
+ struct hisi_qp *qp = req->qp_ctx->qp;
int ret;
- if (ctx->fallback)
- return hisi_zip_fallback_do_work(acomp_req, 0);
+ if (unlikely(!acomp_req->src || !acomp_req->slen ||
+ !acomp_req->dst || !acomp_req->dlen))
+ return -EINVAL;
- dev = &qp_ctx->qp->qm->pdev->dev;
+ if (ctx->fallback)
+ return hisi_zip_fallback_do_work(acomp_req, is_decompress);
+
+ /* Check whether any request is being queued */
+ if ((acomp_req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG) &&
+ !list_empty(&qp->backlog.list)) {
+ req->req_id = HZIP_INVAL_REQ_ID;
+ hisi_zip_enqueue_backlog(req);
+ return -EBUSY;
+ }
- req = hisi_zip_create_req(qp_ctx, acomp_req);
- if (IS_ERR(req))
- return PTR_ERR(req);
+ ret = hisi_zip_create_req(req);
+ if (ret && (acomp_req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
+ /* all req bitmaps are used add to backlog list */
+ req->req_id = HZIP_INVAL_REQ_ID;
+ hisi_zip_enqueue_backlog(req);
+ return -EBUSY;
+ } else if (unlikely(ret)) {
+ return -ENOSPC;
+ }
- ret = hisi_zip_do_work(qp_ctx, req);
- if (unlikely(ret != -EINPROGRESS)) {
- dev_info_ratelimited(dev, "failed to do compress (%d)!\n", ret);
- hisi_zip_remove_req(qp_ctx, req);
+ ret = hisi_zip_map_req_buffers(req);
+ if (unlikely(ret))
+ goto remove_req;
+
+ ret = hisi_zip_do_work(req);
+ if (ret == -EBUSY && (acomp_req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
+ /* hardwre busy add to backlog list */
+ hisi_zip_enqueue_backlog(req);
+ } else if (unlikely(ret != -EINPROGRESS)) {
+ dev_info_ratelimited(&qp->qm->pdev->dev,
+ "failed to do %scompress (%d)!\n",
+ qp->alg_type ? "de" : "", ret);
+ ret = -ENOSPC;
+ goto unmap_req;
}
return ret;
+
+unmap_req:
+ hisi_zip_unmap_req_buffers(req);
+remove_req:
+ hisi_zip_remove_req(req);
+ return ret;
}
-static int hisi_zip_adecompress(struct acomp_req *acomp_req)
+static int hisi_zip_acompress(struct acomp_req *acomp_req)
{
struct hisi_zip_ctx *ctx = crypto_tfm_ctx(acomp_req->base.tfm);
- struct hisi_zip_qp_ctx *qp_ctx = &ctx->qp_ctx[HZIP_QPC_DECOMP];
- struct hisi_zip_req *req;
- struct device *dev;
- int ret;
-
- if (ctx->fallback)
- return hisi_zip_fallback_do_work(acomp_req, 1);
+ struct hisi_zip_req *req = acomp_request_ctx(acomp_req);
- dev = &qp_ctx->qp->qm->pdev->dev;
-
- req = hisi_zip_create_req(qp_ctx, acomp_req);
- if (IS_ERR(req))
- return PTR_ERR(req);
+ req->req = acomp_req;
+ req->qp_ctx = &ctx->qp_ctx[HZIP_QPC_COMP];
+ return hisi_zip_do_comp(req, HZIP_ALG_TYPE_COMP);
+}
- ret = hisi_zip_do_work(qp_ctx, req);
- if (unlikely(ret != -EINPROGRESS)) {
- dev_info_ratelimited(dev, "failed to do decompress (%d)!\n",
- ret);
- hisi_zip_remove_req(qp_ctx, req);
- }
+static int hisi_zip_adecompress(struct acomp_req *acomp_req)
+{
+ struct hisi_zip_ctx *ctx = crypto_tfm_ctx(acomp_req->base.tfm);
+ struct hisi_zip_req *req = acomp_request_ctx(acomp_req);
- return ret;
+ req->req = acomp_req;
+ req->qp_ctx = &ctx->qp_ctx[HZIP_QPC_DECOMP];
+ return hisi_zip_do_comp(req, HZIP_ALG_TYPE_DECOMP);
}
static int hisi_zip_decompress(struct acomp_req *acomp_req)
{
- return hisi_zip_fallback_do_work(acomp_req, 1);
+ return hisi_zip_fallback_do_work(acomp_req, HZIP_ALG_TYPE_DECOMP);
}
static const struct hisi_zip_sqe_ops hisi_zip_ops = {
@@ -463,7 +563,7 @@ static int hisi_zip_create_req_q(struct hisi_zip_ctx *ctx)
{
u16 q_depth = ctx->qp_ctx[0].qp->sq_depth;
struct hisi_zip_req_q *req_q;
- int i, ret;
+ int i;
for (i = 0; i < HZIP_CTX_Q_NUM; i++) {
req_q = &ctx->qp_ctx[i].req_q;
@@ -471,43 +571,21 @@ static int hisi_zip_create_req_q(struct hisi_zip_ctx *ctx)
req_q->req_bitmap = bitmap_zalloc(req_q->size, GFP_KERNEL);
if (!req_q->req_bitmap) {
- ret = -ENOMEM;
- if (i == 0)
- return ret;
-
- goto err_free_comp_q;
+ bitmap_free(ctx->qp_ctx[HZIP_QPC_COMP].req_q.req_bitmap);
+ return -ENOMEM;
}
spin_lock_init(&req_q->req_lock);
-
- req_q->q = kzalloc_objs(struct hisi_zip_req, req_q->size);
- if (!req_q->q) {
- ret = -ENOMEM;
- if (i == 0)
- goto err_free_comp_bitmap;
- else
- goto err_free_decomp_bitmap;
- }
}
return 0;
-
-err_free_decomp_bitmap:
- bitmap_free(ctx->qp_ctx[HZIP_QPC_DECOMP].req_q.req_bitmap);
-err_free_comp_q:
- kfree(ctx->qp_ctx[HZIP_QPC_COMP].req_q.q);
-err_free_comp_bitmap:
- bitmap_free(ctx->qp_ctx[HZIP_QPC_COMP].req_q.req_bitmap);
- return ret;
}
static void hisi_zip_release_req_q(struct hisi_zip_ctx *ctx)
{
int i;
- for (i = 0; i < HZIP_CTX_Q_NUM; i++) {
- kfree(ctx->qp_ctx[i].req_q.q);
+ for (i = 0; i < HZIP_CTX_Q_NUM; i++)
bitmap_free(ctx->qp_ctx[i].req_q.req_bitmap);
- }
}
static int hisi_zip_create_sgl_pool(struct hisi_zip_ctx *ctx)
@@ -620,6 +698,7 @@ static struct acomp_alg hisi_zip_acomp_deflate = {
.cra_module = THIS_MODULE,
.cra_priority = HZIP_ALG_PRIORITY,
.cra_ctxsize = sizeof(struct hisi_zip_ctx),
+ .cra_reqsize = sizeof(struct hisi_zip_req),
}
};
@@ -658,6 +737,7 @@ static struct acomp_alg hisi_zip_acomp_lz4 = {
.cra_module = THIS_MODULE,
.cra_priority = HZIP_ALG_PRIORITY,
.cra_ctxsize = sizeof(struct hisi_zip_ctx),
+ .cra_reqsize = sizeof(struct hisi_zip_req),
}
};
--
2.33.0
^ permalink raw reply related
* [PATCH v2 4/5] crypto: hisilicon/sec2 - fix resource leakage issues in non-backlog mode
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
In-Reply-To: <20260528115531.174593-1-wuzongyu1@huawei.com>
From: Wenkai Lin <linwenkai6@hisilicon.com>
The problem that resources are not released in non-backlog mode is fixed.
There are three cases:
1. If the device is abnormal, the send function returns EAGAIN and
prints an error log.
2. If the device queue is full, the send function returns ENOSPC and
does not print anything.
3. The task is sent normally, and EINPROGRESS is returned.
In addition, the step of switching to software computing is deleted so that
the caller can correctly handle the actual situation.
Fixes: f0ae287c5045 ("crypto: hisilicon/sec2 - implement full backlog mode for sec")
Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
Signed-off-by: Zongyu Wu <wuzongyu1@huawei.com>
---
drivers/crypto/hisilicon/sec2/sec_crypto.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/hisilicon/sec2/sec_crypto.c b/drivers/crypto/hisilicon/sec2/sec_crypto.c
index 62125cf1f849..901abe8761e5 100644
--- a/drivers/crypto/hisilicon/sec2/sec_crypto.c
+++ b/drivers/crypto/hisilicon/sec2/sec_crypto.c
@@ -345,6 +345,9 @@ static int sec_alg_send_message_retry(struct sec_req *req)
ret = qp_send_message(req);
} while (ret == -EBUSY && ctr++ < SEC_RETRY_MAX_CNT);
+ if (ret == -EBUSY)
+ return -ENOSPC;
+
return ret;
}
@@ -1983,14 +1986,11 @@ static int sec_process(struct sec_ctx *ctx, struct sec_req *req)
sec_update_iv(req, ctx->alg_type);
ret = ctx->req_op->bd_send(ctx, req);
- if (unlikely((ret != -EBUSY && ret != -EINPROGRESS))) {
+ if (likely(ret == -EINPROGRESS || ret == -EBUSY))
+ return ret;
+ else if (ret != -ENOSPC)
dev_err_ratelimited(ctx->dev, "send sec request failed!\n");
- goto err_send_req;
- }
- return ret;
-
-err_send_req:
/* As failing, restore the IV from user */
if (ctx->c_ctx.c_mode == SEC_CMODE_CBC && !req->c_req.encrypt) {
if (ctx->alg_type == SEC_SKCIPHER)
@@ -2005,12 +2005,6 @@ static int sec_process(struct sec_ctx *ctx, struct sec_req *req)
err_uninit_req:
sec_request_uninit(req);
- if (ctx->alg_type == SEC_AEAD)
- ret = sec_aead_soft_crypto(ctx, req->aead_req.aead_req,
- req->c_req.encrypt);
- else
- ret = sec_skcipher_soft_crypto(ctx, req->c_req.sk_req,
- req->c_req.encrypt);
return ret;
}
--
2.33.0
^ permalink raw reply related
* [PATCH v2 0/5] crypto: hisilicon - improve backlog handling
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
This series improves backlog handling for HiSilicon crypto drivers.
The ZIP and HPRE drivers are extended to support backlog queuing when
the hardware queue is temporarily busy. Instead of failing requests
immediately under hardware congestion, requests can now be queued and
resubmitted when previous requests complete.
In addition, three issues in the hisilicon/sec2 driver have been fixed:
- A UAF problem in the backlog path.
- A resource leak issue in non-backlog mode (The software fallback
mechanism has also been removed).
- When the CTR task is terminated, the IV is restored to ensure that
subsequent tasks use the correct IV.
Changes in v2:
- Obtain the service type through enum, not qp->alg_type.
- Move the backlog list empty check to after the req creation,
as frequently checking and skipping in the send_backlog can
affect performance.
- Fix a resource leak issue in non-backlog mode.
- A fix for the CTR mode issue is added.
- Link to v1: https://lore.kernel.org/all/20260518142956.3593934-1-wuzongyu1@huawei.com/
Chenghai Huang (1):
crypto: hisilicon/zip - add backlog support for zip
Wenkai Lin (3):
crypto: hisilicon/sec2 - fix UAF in sec_alg_send_backlog
crypto: hisilicon/sec2 - fix resource leakage issues in non-backlog
mode
crypto: hisilicon/sec2 - restore iv for ctr mode
lizhi (1):
crypto: hisilicon/hpre - implement full backlog support for hpre
driver
drivers/crypto/hisilicon/hpre/hpre_crypto.c | 223 +++++++++++----
drivers/crypto/hisilicon/sec2/sec_crypto.c | 48 ++--
drivers/crypto/hisilicon/zip/zip_crypto.c | 286 +++++++++++++-------
3 files changed, 371 insertions(+), 186 deletions(-)
--
2.33.0
^ permalink raw reply
* [PATCH v2 2/5] crypto: hisilicon/sec2 - fix UAF in sec_alg_send_backlog
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
In-Reply-To: <20260528115531.174593-1-wuzongyu1@huawei.com>
From: Wenkai Lin <linwenkai6@hisilicon.com>
After crypto_request_complete() is invoked, the crypto core may
immediately free the request structure and its associated tfm context.
Consequently, the sec_ctx and qp_ctx are also released.
However, sec_alg_send_backlog() can still attempt to access these
structures when processing queued requests, resulting in a
use-after-free (UAF) bug.
Fix this by accessing the backlog list through the long-term qp memory
and using the ctx memory only when the backlog list is not empty.
Fixes: f0ae287c5045 ("crypto: hisilicon/sec2 - implement full backlog mode for sec")
Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
Signed-off-by: Zongyu Wu <wuzongyu1@huawei.com>
---
drivers/crypto/hisilicon/sec2/sec_crypto.c | 23 +++++++++++-----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/hisilicon/sec2/sec_crypto.c b/drivers/crypto/hisilicon/sec2/sec_crypto.c
index 77e0e03cbcab..62125cf1f849 100644
--- a/drivers/crypto/hisilicon/sec2/sec_crypto.c
+++ b/drivers/crypto/hisilicon/sec2/sec_crypto.c
@@ -234,13 +234,15 @@ static int qp_send_message(struct sec_req *req)
return -EINPROGRESS;
}
-static void sec_alg_send_backlog_soft(struct sec_ctx *ctx, struct sec_qp_ctx *qp_ctx)
+static void sec_alg_send_backlog_soft(struct hisi_qp *qp)
{
struct sec_req *req, *tmp;
+ struct sec_ctx *ctx;
int ret;
- list_for_each_entry_safe(req, tmp, &qp_ctx->qp->backlog.list, list) {
+ list_for_each_entry_safe(req, tmp, &qp->backlog.list, list) {
list_del(&req->list);
+ ctx = req->qp_ctx->ctx;
ctx->req_op->buf_unmap(ctx, req);
if (req->req_id >= 0)
sec_free_req_id(req);
@@ -258,9 +260,8 @@ static void sec_alg_send_backlog_soft(struct sec_ctx *ctx, struct sec_qp_ctx *qp
}
}
-static void sec_alg_send_backlog(struct sec_ctx *ctx, struct sec_qp_ctx *qp_ctx)
+static void sec_alg_send_backlog(struct hisi_qp *qp)
{
- struct hisi_qp *qp = qp_ctx->qp;
struct sec_req *req, *tmp;
int ret;
@@ -277,7 +278,7 @@ static void sec_alg_send_backlog(struct sec_ctx *ctx, struct sec_qp_ctx *qp_ctx)
goto unlock;
default:
/* Release memory resources and send all requests through software. */
- sec_alg_send_backlog_soft(ctx, qp_ctx);
+ sec_alg_send_backlog_soft(qp);
goto unlock;
}
}
@@ -306,6 +307,7 @@ static void sec_req_cb(struct hisi_qp *qp, void *resp)
ctx->req_op->buf_unmap(ctx, req);
ctx->req_op->callback(ctx, req, err);
+ sec_alg_send_backlog(qp);
}
static void sec_req_cb3(struct hisi_qp *qp, void *resp)
@@ -331,6 +333,7 @@ static void sec_req_cb3(struct hisi_qp *qp, void *resp)
ctx->req_op->buf_unmap(ctx, req);
ctx->req_op->callback(ctx, req, err);
+ sec_alg_send_backlog(qp);
}
static int sec_alg_send_message_retry(struct sec_req *req)
@@ -1673,8 +1676,6 @@ static void sec_update_iv(struct sec_req *req, enum sec_alg_type alg_type)
static void sec_skcipher_callback(struct sec_ctx *ctx, struct sec_req *req,
int err)
{
- struct sec_qp_ctx *qp_ctx = req->qp_ctx;
-
if (req->req_id >= 0)
sec_free_req_id(req);
@@ -1684,7 +1685,6 @@ static void sec_skcipher_callback(struct sec_ctx *ctx, struct sec_req *req,
sec_update_iv(req, SEC_SKCIPHER);
crypto_request_complete(req->base, err);
- sec_alg_send_backlog(ctx, qp_ctx);
}
static void set_aead_auth_iv(struct sec_ctx *ctx, struct sec_req *req)
@@ -1923,7 +1923,7 @@ static void sec_aead_callback(struct sec_ctx *c, struct sec_req *req, int err)
struct aead_request *a_req = req->aead_req.aead_req;
struct crypto_aead *tfm = crypto_aead_reqtfm(a_req);
size_t authsize = crypto_aead_authsize(tfm);
- struct sec_qp_ctx *qp_ctx = req->qp_ctx;
+ int error = err;
size_t sz;
if (!err && req->c_req.encrypt) {
@@ -1934,15 +1934,14 @@ static void sec_aead_callback(struct sec_ctx *c, struct sec_req *req, int err)
authsize, a_req->cryptlen + a_req->assoclen);
if (unlikely(sz != authsize)) {
dev_err(c->dev, "copy out mac err!\n");
- err = -EINVAL;
+ error = -EINVAL;
}
}
if (req->req_id >= 0)
sec_free_req_id(req);
- crypto_request_complete(req->base, err);
- sec_alg_send_backlog(c, qp_ctx);
+ crypto_request_complete(req->base, error);
}
static void sec_request_uninit(struct sec_req *req)
--
2.33.0
^ permalink raw reply related
* [PATCH v2 5/5] crypto: hisilicon/sec2 - restore iv for ctr mode
From: ZongYu Wu @ 2026-05-28 11:55 UTC (permalink / raw)
To: herbert, davem
Cc: linux-kernel, linux-crypto, fanghao11, liulongfang, qianweili,
wangzhou1, huangchenghai2, linwenkai6
In-Reply-To: <20260528115531.174593-1-wuzongyu1@huawei.com>
From: Wenkai Lin <linwenkai6@hisilicon.com>
Upon termination of the CTR task, the initial vector (IV) is
restored to guarantee valid IV availability for subsequent tasks.
Fixes: 7b44c0eecd6a ("crypto: hisilicon/sec - add new skcipher mode for SEC")
Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
Signed-off-by: Zongyu Wu <wuzongyu1@huawei.com>
---
drivers/crypto/hisilicon/sec2/sec_crypto.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/hisilicon/sec2/sec_crypto.c b/drivers/crypto/hisilicon/sec2/sec_crypto.c
index 901abe8761e5..03f873f68828 100644
--- a/drivers/crypto/hisilicon/sec2/sec_crypto.c
+++ b/drivers/crypto/hisilicon/sec2/sec_crypto.c
@@ -1970,6 +1970,7 @@ static int sec_request_init(struct sec_ctx *ctx, struct sec_req *req)
static int sec_process(struct sec_ctx *ctx, struct sec_req *req)
{
+ bool need_copy_iv = false;
int ret;
ret = sec_request_init(ctx, req);
@@ -1982,8 +1983,10 @@ static int sec_process(struct sec_ctx *ctx, struct sec_req *req)
/* Output IV as decrypto */
if (!req->c_req.encrypt && (ctx->c_ctx.c_mode == SEC_CMODE_CBC ||
- ctx->c_ctx.c_mode == SEC_CMODE_CTR))
+ ctx->c_ctx.c_mode == SEC_CMODE_CTR)) {
+ need_copy_iv = true;
sec_update_iv(req, ctx->alg_type);
+ }
ret = ctx->req_op->bd_send(ctx, req);
if (likely(ret == -EINPROGRESS || ret == -EBUSY))
@@ -1992,7 +1995,7 @@ static int sec_process(struct sec_ctx *ctx, struct sec_req *req)
dev_err_ratelimited(ctx->dev, "send sec request failed!\n");
/* As failing, restore the IV from user */
- if (ctx->c_ctx.c_mode == SEC_CMODE_CBC && !req->c_req.encrypt) {
+ if (need_copy_iv) {
if (ctx->alg_type == SEC_SKCIPHER)
memcpy(req->c_req.sk_req->iv, req->c_req.c_ivin,
ctx->c_ctx.ivsize);
--
2.33.0
^ permalink raw reply related
* Re: [PATCH 1/2] gpu: host1x: Allow entries in BO caches to be freed
From: Thierry Reding @ 2026-05-28 12:16 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Thierry Reding, David Airlie, Simona Vetter, Jonathan Hunter,
Akhil R, Herbert Xu, David S. Miller, Aaron Kling, dri-devel,
linux-tegra, linux-kernel, linux-crypto
In-Reply-To: <20260515-host1x-bocache-leak-v1-1-a0375f68aeab@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]
On Fri, May 15, 2026 at 11:34:51AM +0900, Mikko Perttunen wrote:
> When a buffer object is pinned via host1x_bo_pin() with a cache, the
> resulting mapping is kept in the cache so it can be reused on subsequent
> pins. Each mapping held a reference to the underlying host1x_bo (taken
> in tegra_bo_pin / gather_bo_pin), so as long as a mapping was cached,
> the bo itself could not be freed.
>
> However, the only way to remove the cached mapping was through the free
> path of the buffer object. This meant that if a bo got cached, it could
> never get freed again.
>
> Resolve the circularity by holding a weak reference to the bo from the
> cache side. This is done by having the .pin callbacks not bump the bo's
> refcount -- instead the common Host1x bo code does so, except for the
> cache reference.
>
> Also move the remove-cache-mapping-on-free code into a common function
> inside Host1x code. This is only called from the TegraDRM GEM buffers
> since those are the only ones that can be cached at the moment.
>
> Reported-by: Aaron Kling <webgeek1234@gmail.com>
> Fixes: 1f39b1dfa53c ("drm/tegra: Implement buffer object cache")
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> drivers/gpu/drm/tegra/gem.c | 13 ++-------
> drivers/gpu/drm/tegra/submit.c | 3 +--
> drivers/gpu/host1x/bus.c | 60 +++++++++++++++++++++++++++++++++++++++++-
> include/linux/host1x.h | 7 +++++
> 4 files changed, 69 insertions(+), 14 deletions(-)
Applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/7] platform/surface: SAM: Add support for Surface Pro 12in
From: Ilpo Järvinen @ 2026-05-28 12:42 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, Hans de Goede,
Douglas Anderson, Jessica Zhang, linux-arm-msm, devicetree, LKML,
linux-crypto, dri-devel, linux-input, platform-driver-x86
In-Reply-To: <ab458aadea651396d9ea7629419a32dc7510c593.1778822464.git.harrison.vanderbyl@gmail.com>
On Fri, 15 May 2026, Harrison Vanderbyl wrote:
> Add a SAM client device node group and registry entry for the
> Microsoft Surface Pro, 12-inch with Snapdragon.
>
> This set enables the use of the following devices.
> 1: cover keyboard
> 2: cover touchpad
> 3: pen stash events.
>
> The battery info and charger info devices have been
> purposefully omitted as they are also reported by
> other drivers and cause conflicts.
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> ---
> .../surface/surface_aggregator_registry.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 0599d5adf02e..884049961415 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -422,6 +422,19 @@ static const struct software_node *ssam_node_group_sp11[] = {
> NULL,
> };
>
> +/* Devices for Surface Pro 12" first edition (ARM/QCOM) */
> +static const struct software_node *ssam_node_group_sp12in[] = {
> + &ssam_node_root,
> + &ssam_node_hub_kip,
> + &ssam_node_tmp_sensors,
> + &ssam_node_hid_kip_keyboard,
> + &ssam_node_hid_sam_penstash,
Hi,
Could you please confirm this penstash is correct (sam vs kip)?
Sashiko suggested it might be wrong but take it's report with a grain of
salt, it's AI after all and sometimes seems to extrapolate about HW to a
sibling HW without any real knowledge:
https://sashiko.dev/#/patchset/cover.1778822464.git.harrison.vanderbyl%40gmail.com
--
i.
> + &ssam_node_hid_kip_touchpad,
> + &ssam_node_hid_kip_fwupd,
> + &ssam_node_pos_tablet_switch,
> + NULL,
> +};
> +
> /* -- SSAM platform/meta-hub driver. ---------------------------------------- */
>
> static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
> @@ -500,6 +513,8 @@ static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = {
> { .compatible = "microsoft,arcata", (void *)ssam_node_group_sp9_5g },
> /* Surface Pro 11 (ARM/QCOM) */
> { .compatible = "microsoft,denali", (void *)ssam_node_group_sp11 },
> + /* Surface Pro 12in First Edition (ARM/QCOM) */
> + { .compatible = "microsoft,surface-pro-12in", (void *)ssam_node_group_sp12in },
> /* Surface Laptop 7 */
> { .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 },
> { .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 },
^ permalink raw reply
* Re: [PATCH 0/3] Add support for qcrypto on shikra
From: Bartosz Golaszewski @ 2026-05-28 13:13 UTC (permalink / raw)
To: Kuldeep Singh
Cc: Thara Gopinath, Herbert Xu, David S. Miller, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Vinod Koul, Frank Li, Andy Gross, linux-arm-msm, linux-crypto,
devicetree, linux-kernel, dmaengine, Bartosz Golaszewski,
Bartosz Golaszewski, Gaurav Kashyap, Neeraj Soni, Eric Biggers
In-Reply-To: <e49c4a45-6455-47f3-a91f-c32c1a0b99be@oss.qualcomm.com>
On Thu, 28 May 2026 13:54:51 +0200, Kuldeep Singh
<kuldeep.singh@oss.qualcomm.com> said:
>>> +Bartosz, Gaurav, Neeraj
>>>
>>> Hi Eric,
>>>
>>> GPCE is relevant in terms of providing hardware security.
>>> There are multiple usecases coming up for example to handle DRM/secure
>>> buffer usecases to improve overall throughput for secure content.
>>>
>>> Regarding performance, it's currently slower compared to arm CE but
>>> provides an edge by giving hardware security which is considered more
>>> secure.
>>>
>>> Btw, there's been performance improvement with new targets and we are
>>> expecting to achieve far more better performance with new SoCs family.
>>> Pakala: GPCE - 550MBps, ARMv8 - 8GBps
>>> Kaanapali: GPCE - 3GBps, ARMv8 - 10GBps
>>>
>>> Please note, there's almost 5x improvement in kaanapali compared to
>>> pakala. Though overall is still slower compared to arm but as mentioned,
>>> expecting better performance with hardware improvements as we progress.
>>>
>>> Also, currently qce driver exhibit stability issues and that's what we
>>> are putting effort in stabilizing the software on immediate basis.
>>>
>>> There's parallel effort ongoing by Bartosz to introduce baseline for
>>> secure buffer usecases.
>>> https://lore.kernel.org/lkml/20260522-qcom-qce-cmd-descr-v18-0-99103926bafc@oss.qualcomm.com/
>>> There's active development ongoing and i believe lowering cra_priority
>>> for qce is fine as of now and can scale values once qce becomes
>>> performance efficient.
>>>
>>> Please share your thoughts. Thanks!
>>
>> ARMv8 Crypto Extensions are "hardware" as well, just in the CPU. They
>> provide constant-time execution, for example.
>>
>> Granted, they don't protect from power analysis and electromagnetic
>> emanation attacks. Does QCE actually provide those protections, though?
>
> QCE doesn't provide these protections currently.
> What i wanted to highlight was there are certain security usecases which
> are possible via dedicated crypto engine only and not via arm cpu.
>> Either way, it doesn't really matter in this case. There are multiple
>> aspects to security, and before even considering these advanced
>> protections, the basics of security need to be absolutely solid. That
>> is, the driver needs to always compute the crypto algorithms correctly,
>> and it needs to be completely robust when fuzzed by unprivileged
>> userspace (because it can accessed in that way).
> > Yet, this driver "exhibits stability issues", fails the self-tests, and
>> doesn't even have exclusive access to the hardware! These are all
>> security bugs. That very much defeats the claimed point. (Plus, due to
>> the performance issues no one wants to use it in Linux anyway.)
>
> Sure, we are analyzing self-tests failures and are committed to fix any
> hung/stability issue in any aspect but i do feel it should not be a
> blocker to add new soc id support.
>
> Also, could you please elaborate more on "exclusive access to hardware"?
> Do you mean the hardware can be accessed by multiple execution
> environment like TEE and Linux?
> --
> Regards
> Kuldeep
>
>
Eric: FYI I do plan - and have been allowed to by Qualcomm - to work on this
driver further to refactor and improve it. However, the BAM locking series[1]
needs to be queued first as it significantly changes the way the driver works.
Any help with reviewing and getting these patches merged is appreciated. I
don't want to start sending more patches before the 14 commit series gets queued
first.
Vinod: the series has been reviewed and tested. The NAND team at qualcomm is
telling me they're using it internally already to fix a race between the modem
firmware and linux. Can we get it queued for v7.2 please? This will make further
refactoring easier.
I know about the self-tests etc., I will address them next.
Bart
[1] https://lore.kernel.org/all/20260526-qcom-qce-cmd-descr-v19-0-08472fdcbf4a@oss.qualcomm.com/
^ permalink raw reply
* Re: [PATCH v2 7/7] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: Dmitry Baryshkov @ 2026-05-28 13:19 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, Herbert Xu,
davem, neil.armstrong, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, jikos, bentiss, luzmaximilian, hansg,
ilpo.jarvinen, Douglas Anderson, Jessica Zhang, linux-arm-msm,
devicetree, linux-kernel, linux-crypto, dri-devel, linux-input,
platform-driver-x86
In-Reply-To: <8ac29ee38ba80a3fbde8bfe43b74b9b936b31cb1.1778822464.git.harrison.vanderbyl@gmail.com>
On Fri, May 15, 2026 at 03:41:52PM +1000, Harrison Vanderbyl wrote:
> Initial device tree for Microsoft Surface Pro 12in
>
> Currently supported:
> - UFS
> - Touchscreen
> - Pen
> - USB 3.2 x2 (DP Alt Mode)
> - Audio
> - Wifi
> - Bluetooth
> - CDSP
> - ADSP
> - GPU
>
> Not currently supported:
> - Accelerometer
> - Front, Back and IR cameras
> - IRIS video decoder
>
> Tested on Surface_Pro_12in_1st_Ed_with_Snapdragon_2110
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 2 +
> .../dts/qcom/x1p42100-microsoft-sp12in.dts | 1201 +++++++++++++++++
> 2 files changed, 1203 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 3/7] platform/surface: SAM: Add support for Surface Pro 12in
From: Harrison Vanderbyl @ 2026-05-28 13:33 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: airlied, andersson, bentiss, conor+dt, davem, devicetree,
dianders, dri-devel, hansg, herbert, jesszhan0024, jikos,
konradybcio, krzk+dt, linux-arm-msm, linux-crypto, linux-input,
linux-kernel, luzmaximilian, maarten.lankhorst, mripard,
neil.armstrong, platform-driver-x86, robh, simona, tzimmermann
In-Reply-To: <6808166a-423c-c801-497a-ed95cccc8d0c@linux.intel.com>
On Thu, 28 May 2026, Ilpo Järvinen wrote:
> Could you please confirm this penstash is correct (sam vs kip)?
>
> Sashiko suggested it might be wrong but take it's report with a grain of
> salt, it's AI after all and sometimes seems to extrapolate about HW to a
> sibling HW without any real knowledge:
Sam is correct here, unlike the surface pro 11,
The pen stash is on the reverse of the screen,
not on the detachable keyboard.
An image of the pen stash in use is available here:
https://cdn-dynmedia-1.microsoft.com/is/image/microsoftcorp/B04-Surface-Pro-12-inch-1Ed-Family-Rear?wid=1200&hei=900&qlt=90&bgc=F2F2F2F2&fmt=jpg
When using the above config,
pen stash events can be seen with evtest.
Thanks,
Harrison
^ permalink raw reply
* Re: [PATCH v2 3/7] platform/surface: SAM: Add support for Surface Pro 12in
From: Ilpo Järvinen @ 2026-05-28 13:43 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: airlied, andersson, bentiss, conor+dt, davem, devicetree,
dianders, dri-devel, Hans de Goede, herbert, jesszhan0024, jikos,
konradybcio, krzk+dt, linux-arm-msm, linux-crypto, linux-input,
LKML, luzmaximilian, maarten.lankhorst, mripard, neil.armstrong,
platform-driver-x86, robh, simona, tzimmermann
In-Reply-To: <20260528133353.33312-1-harrison.vanderbyl@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]
On Thu, 28 May 2026, Harrison Vanderbyl wrote:
> On Thu, 28 May 2026, Ilpo Järvinen wrote:
> > Could you please confirm this penstash is correct (sam vs kip)?
> >
> > Sashiko suggested it might be wrong but take it's report with a grain of
> > salt, it's AI after all and sometimes seems to extrapolate about HW to a
> > sibling HW without any real knowledge:
>
> Sam is correct here, unlike the surface pro 11,
> The pen stash is on the reverse of the screen,
> not on the detachable keyboard.
>
> An image of the pen stash in use is available here:
> https://cdn-dynmedia-1.microsoft.com/is/image/microsoftcorp/B04-Surface-Pro-12-inch-1Ed-Family-Rear?wid=1200&hei=900&qlt=90&bgc=F2F2F2F2&fmt=jpg
>
> When using the above config,
> pen stash events can be seen with evtest.
Thanks for confirming (not a big surprise AI couldn't know the spec but
it was worth checking if it would have been because of copy-paste).
I've applied this patch 3 now to the review-ilpo-next branch.
--
i.
^ permalink raw reply
* Re: [PATCH 0/3] Add support for qcrypto on shikra
From: Dmitry Baryshkov @ 2026-05-28 13:50 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kuldeep Singh, Thara Gopinath, Herbert Xu, David S. Miller,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Vinod Koul, Frank Li, Andy Gross, linux-arm-msm,
linux-crypto, devicetree, linux-kernel, dmaengine,
Bartosz Golaszewski, Gaurav Kashyap, Neeraj Soni, Eric Biggers
In-Reply-To: <CAMRc=MfC6CEwOXYttsav3mwqyJ2F4sburBj+zNJ25qMoweyL-Q@mail.gmail.com>
On Thu, May 28, 2026 at 09:13:23AM -0400, Bartosz Golaszewski wrote:
> On Thu, 28 May 2026 13:54:51 +0200, Kuldeep Singh
> <kuldeep.singh@oss.qualcomm.com> said:
> >>> +Bartosz, Gaurav, Neeraj
>
> I know about the self-tests etc., I will address them next.
My 2c, the self-tests would be more important, as they are fixes. Doing
the crypto in a wrong way is a bad idea...
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v5 4/7] x86/sev: Add support to perform RMP optimizations asynchronously
From: Ackerley Tng @ 2026-05-28 14:45 UTC (permalink / raw)
To: Ashish Kalra, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <6f1ec3d8ebcf3aaceccc099c07d0deb545dd4ab9.1779133590.git.ashish.kalra@amd.com>
Ashish Kalra <Ashish.Kalra@amd.com> writes:
Thank you Ashish!
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> When SEV-SNP is enabled, all writes to memory are checked to ensure
> integrity of SNP guest memory. This imposes performance overhead on the
> whole system.
>
> RMPOPT is a new instruction that minimizes the performance overhead of
> RMP checks on the hypervisor and on non-SNP guests by allowing RMP
> checks to be skipped for 1GB regions of memory that are known not to
> contain any SEV-SNP guest memory.
>
> Add support for performing RMP optimizations asynchronously using a
> dedicated workqueue.
>
> Enable RMPOPT optimizations globally for all system RAM up to 2TB at
This should also be updated to say "Enable RMPOPT optimizations for up
to 2TB worth of system RAM at..."
The current phrasing sounds like only addresses [0, 2TB) are allowed to
be optimized, but actually any address [start, start + 2TB) can be
optimized?
> RMP initialization time. RMP checks can initially be skipped for 1GB
> memory ranges that do not contain SEV-SNP guest memory (excluding
> preassigned pages such as the RMP table and firmware pages). As SNP
> guests are launched, RMPUPDATE will disable the corresponding RMPOPT
> optimizations.
>
> Suggested-by: Thomas Lendacky <thomas.lendacky@amd.com>
> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> Reviewed-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> arch/x86/virt/svm/sev.c | 167 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 164 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 82f9dc7a57c3..8876cac052d5 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -19,6 +19,7 @@
> #include <linux/iommu.h>
> #include <linux/amd-iommu.h>
> #include <linux/nospec.h>
> +#include <linux/workqueue.h>
>
> #include <asm/sev.h>
> #include <asm/processor.h>
> @@ -125,7 +126,18 @@ static void *rmp_bookkeeping __ro_after_init;
> static u64 probed_rmp_base, probed_rmp_size;
>
> static cpumask_t rmpopt_cpumask;
> -static phys_addr_t rmpopt_pa_start;
> +static phys_addr_t rmpopt_pa_start, rmpopt_pa_end;
> +
> +enum rmpopt_function {
> + RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS,
> + RMPOPT_FUNC_REPORT_STATUS
> +};
> +
> +#define RMPOPT_WORK_TIMEOUT 10000
> +
> +static struct workqueue_struct *rmpopt_wq;
> +static struct delayed_work rmpopt_delayed_work;
> +static DEFINE_MUTEX(rmpopt_wq_mutex);
>
> static LIST_HEAD(snp_leaked_pages_list);
> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> @@ -564,12 +576,21 @@ EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>
> static void rmpopt_cleanup(void)
> {
> + guard(mutex)(&rmpopt_wq_mutex);
> +
> + if (!rmpopt_wq)
> + return;
> +
> + cancel_delayed_work_sync(&rmpopt_delayed_work);
> + destroy_workqueue(rmpopt_wq);
> +
> cpus_read_lock();
> wrmsrq_on_cpus(&rmpopt_cpumask, MSR_AMD64_RMPOPT_BASE, 0);
> cpus_read_unlock();
>
> cpumask_clear(&rmpopt_cpumask);
> - rmpopt_pa_start = 0;
> + rmpopt_pa_start = rmpopt_pa_end = 0;
> + rmpopt_wq = NULL;
> }
>
> void snp_shutdown(void)
> @@ -587,6 +608,105 @@ void snp_shutdown(void)
> }
> EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>
> +static inline bool __rmpopt(u64 rax, u64 rcx)
Perhaps use pa_start instead of rax and op_type for rcx?
> +{
> + bool optimized;
> +
> + asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
> + : "=@ccc" (optimized)
> + : "a" (rax), "c" (rcx)
> + : "memory", "cc");
> +
> + return optimized;
> +}
> +
> +static void rmpopt(u64 pa)
> +{
> + u64 rax = ALIGN_DOWN(pa, SZ_1G);
> + u64 rcx = RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS;
> +
And pa_start and op_type here too.
> + __rmpopt(rax, rcx);
> +}
> +
> +/*
> + * 'val' is a system physical address.
> + */
> +static void rmpopt_smp(void *val)
> +{
> + rmpopt((u64)val);
> +}
> +
> +/*
> + * RMPOPT optimizations skip RMP checks at 1GB granularity if this
> + * range of memory does not contain any SNP guest memory.
> + */
> +static void rmpopt_work_handler(struct work_struct *work)
> +{
> + bool current_cpu_cleared = false;
> + phys_addr_t pa;
> + int this_cpu;
> +
> + pr_info("Attempt RMP optimizations on physical address range @1GB alignment [0x%016llx - 0x%016llx]\n",
> + rmpopt_pa_start, rmpopt_pa_end);
> +
> + /*
> + * RMPOPT scans the RMP table, stores the result of the scan in the
> + * reserved processor memory. The RMP scan is the most expensive
> + * part. If a second RMPOPT occurs, it can skip the expensive scan
> + * if they can see a cached result in the reserved processor memory.
> + *
> + * Do RMPOPT on one CPU alone. Then, follow that up with RMPOPT
> + * on every other primary thread. This potentially allows the
I like the leader and follower comments below, thanks! With this
leader/follower setup, will the followers definitely see the cached scan
results, or might the followers still potentially not benefit from the
caching? If it's still only "potentially", why?
> + * followers to use the "cached" scan results to avoid repeating
> + * full scans.
> + */
> +
> + /*
> + * Pin the worker to the current CPU for the leader loop so that
> + * this_cpu remains valid and the RMPOPT instruction executes on
> + * the CPU that was cleared from the cpumask. The workqueue is
> + * WQ_UNBOUND, so without pinning, the scheduler could migrate
> + * the worker between the cpumask manipulation and the leader
> + * loop, causing the leader to run on a different CPU while
> + * this_cpu's core is skipped entirely.
> + *
> + * Use migrate_disable() rather than get_cpu() to prevent
> + * migration while still allowing preemption.
> + *
> + * Note: rmpopt_cpumask is modified here without holding
> + * rmpopt_wq_mutex. This is safe because the delayed_work
> + * mechanism guarantees single-threaded execution of this
> + * handler, and rmpopt_cleanup() calls cancel_delayed_work_sync()
> + * to ensure handler completion before tearing down the cpumask.
> + */
> + migrate_disable();
> + this_cpu = smp_processor_id();
> + if (cpumask_test_cpu(this_cpu, &rmpopt_cpumask)) {
> + cpumask_clear_cpu(this_cpu, &rmpopt_cpumask);
> + current_cpu_cleared = true;
> + }
> +
Instead of reusing the global rmpopt_cpumask, why not make a copy of
rmpopt_cpumask for this function? Then this function won't have to
figure out current_cpu_cleared or restore rmpopt_cpumask at the end.
I'm thinking to also drop the test and clear, this function can just
always clear, like
cpumask_clear_cpu(smp_processor_id(), followers_cpumask);
and later
on_each_cpu_mask(&followers_cpumask, ...);
Actually, if for whatever reason cpumask_test_cpu(this_cpu,
&rmpopt_cpumask) above returns false, would that mean somehow some cpu
exists that wasn't enabled right when rmpopt was initialized? If yes,
what happens if we call rmpopt() on a cpu where it wasn't initialized?
> + /* Leader: prime the RMPOPT cache on this CPU */
> + for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
> + rmpopt(pa);
> +
> + migrate_enable();
> +
> + /* Followers: run RMPOPT on all other cores */
> + cpus_read_lock();
> + for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G) {
> + on_each_cpu_mask(&rmpopt_cpumask, rmpopt_smp,
> + (void *)pa, true);
> +
> + /* Give a chance for other threads to run */
> + cond_resched();
> + }
> + cpus_read_unlock();
> +
> + if (current_cpu_cleared)
> + cpumask_set_cpu(this_cpu, &rmpopt_cpumask);
> +}
> +
>
> [...snip...]
>
^ permalink raw reply
* [PATCH] crypto: sun4i-ss: restrict PRNG seed length to prevent heap overflow
From: Tianchu Chen @ 2026-05-28 14:53 UTC (permalink / raw)
To: clabbe.montjoie, herbert, davem
Cc: wens, jernej.skrabec, samuel, linux-crypto, linux-arm-kernel,
linux-sunxi, linux-kernel
From: Tianchu Chen <flynnnchen@tencent.com>
sun4i_ss_prng_seed() copies the user-supplied seed into ss->seed
using the user-provided length with no bounds check. The crypto core
does not enforce slen <= seedsize before calling into the driver, so a
userspace caller via AF_ALG setsockopt(ALG_SET_KEY) can pass up to
sysctl_optmem_max bytes, overflowing the fixed-size buffer and
corrupting adjacent heap memory.
Add a length check rejecting seeds larger than the buffer.
Discovered by Atuin - Automated Vulnerability Discovery Engine.
Fixes: 6298e948215f ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
Cc: stable@vger.kernel.org
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c
index 491fcb7b8..010fa891c 100644
--- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c
+++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-prng.c
@@ -8,6 +8,8 @@ int sun4i_ss_prng_seed(struct crypto_rng *tfm, const u8 *seed,
struct rng_alg *alg = crypto_rng_alg(tfm);
algt = container_of(alg, struct sun4i_ss_alg_template, alg.rng);
+ if (slen > sizeof(algt->ss->seed))
+ return -EINVAL;
memcpy(algt->ss->seed, seed, slen);
return 0;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH 0/3] Add support for qcrypto on shikra
From: Bartosz Golaszewski @ 2026-05-28 15:13 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Kuldeep Singh, Thara Gopinath, Herbert Xu, David S. Miller,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Vinod Koul, Frank Li, Andy Gross, linux-arm-msm,
linux-crypto, devicetree, linux-kernel, dmaengine,
Bartosz Golaszewski, Gaurav Kashyap, Neeraj Soni, Eric Biggers,
Bartosz Golaszewski
In-Reply-To: <lj7geczhthury476ilkjym2k5fblo5pqroefsbdfgh5jcf7zy2@qrss5xc7umn3>
On Thu, 28 May 2026 15:50:10 +0200, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> said:
> On Thu, May 28, 2026 at 09:13:23AM -0400, Bartosz Golaszewski wrote:
>> On Thu, 28 May 2026 13:54:51 +0200, Kuldeep Singh
>> <kuldeep.singh@oss.qualcomm.com> said:
>> >>> +Bartosz, Gaurav, Neeraj
>>
>> I know about the self-tests etc., I will address them next.
>
> My 2c, the self-tests would be more important, as they are fixes. Doing
> the crypto in a wrong way is a bad idea...
>
Then let that be "in parallel". :)
Bart
^ permalink raw reply
* [PATCH] crypto: qat - validate RSA CRT component lengths
From: Giovanni Cabiddu @ 2026-05-28 15:57 UTC (permalink / raw)
To: herbert
Cc: linux-crypto, qat-linux, Giovanni Cabiddu, stable, Ahsan Atta,
Laurent M Coquerel
The generic RSA key parser (rsa_helper.c) bounds each CRT component (p,
q, dp, dq, qinv) by the modulus size n_sz, but qat_rsa_setkey_crt()
allocates half-size DMA buffers (key_sz / 2) and right-aligns each
component with:
memcpy(dst + half_key_sz - len, src, len)
When a CRT component is larger than half_key_sz the subtraction
underflows and memcpy writes past the DMA buffer, causing memory
corruption.
Add a len > half_key_sz check next to the existing !len check for each
of the five CRT components so the driver falls back to the non-CRT path
instead of writing out of bounds.
Fixes: 879f77e9071f ("crypto: qat - Add RSA CRT mode")
Cc: stable@vger.kernel.org
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Reviewed-by: Ahsan Atta <ahsan.atta@intel.com>
Reviewed-by: Laurent M Coquerel <laurent.m.coquerel@intel.com>
Tested-by: Laurent M Coquerel <laurent.m.coquerel@intel.com>
---
drivers/crypto/intel/qat/qat_common/qat_asym_algs.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c b/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
index e09b9edfce42..75c15c8e41db 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
@@ -1085,7 +1085,7 @@ static void qat_rsa_setkey_crt(struct qat_rsa_ctx *ctx, struct rsa_key *rsa_key)
ptr = rsa_key->p;
len = rsa_key->p_sz;
qat_rsa_drop_leading_zeros(&ptr, &len);
- if (!len)
+ if (!len || len > half_key_sz)
goto err;
ctx->p = dma_alloc_coherent(dev, half_key_sz, &ctx->dma_p, GFP_KERNEL);
if (!ctx->p)
@@ -1096,7 +1096,7 @@ static void qat_rsa_setkey_crt(struct qat_rsa_ctx *ctx, struct rsa_key *rsa_key)
ptr = rsa_key->q;
len = rsa_key->q_sz;
qat_rsa_drop_leading_zeros(&ptr, &len);
- if (!len)
+ if (!len || len > half_key_sz)
goto free_p;
ctx->q = dma_alloc_coherent(dev, half_key_sz, &ctx->dma_q, GFP_KERNEL);
if (!ctx->q)
@@ -1107,7 +1107,7 @@ static void qat_rsa_setkey_crt(struct qat_rsa_ctx *ctx, struct rsa_key *rsa_key)
ptr = rsa_key->dp;
len = rsa_key->dp_sz;
qat_rsa_drop_leading_zeros(&ptr, &len);
- if (!len)
+ if (!len || len > half_key_sz)
goto free_q;
ctx->dp = dma_alloc_coherent(dev, half_key_sz, &ctx->dma_dp,
GFP_KERNEL);
@@ -1119,7 +1119,7 @@ static void qat_rsa_setkey_crt(struct qat_rsa_ctx *ctx, struct rsa_key *rsa_key)
ptr = rsa_key->dq;
len = rsa_key->dq_sz;
qat_rsa_drop_leading_zeros(&ptr, &len);
- if (!len)
+ if (!len || len > half_key_sz)
goto free_dp;
ctx->dq = dma_alloc_coherent(dev, half_key_sz, &ctx->dma_dq,
GFP_KERNEL);
@@ -1131,7 +1131,7 @@ static void qat_rsa_setkey_crt(struct qat_rsa_ctx *ctx, struct rsa_key *rsa_key)
ptr = rsa_key->qinv;
len = rsa_key->qinv_sz;
qat_rsa_drop_leading_zeros(&ptr, &len);
- if (!len)
+ if (!len || len > half_key_sz)
goto free_dq;
ctx->qinv = dma_alloc_coherent(dev, half_key_sz, &ctx->dma_qinv,
GFP_KERNEL);
base-commit: a36f3ace3504ad60981e21e3159655bcded4764f
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/3] net: Remove support for AIO on sockets
From: Jens Axboe @ 2026-05-28 16:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: demiobenour, Herbert Xu, David S. Miller, Eric Dumazet,
Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn, Jakub Kicinski,
Simon Horman, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
James Clark, Jonathan Corbet, Shuah Khan, Eric Biggers,
Ard Biesheuvel, linux-crypto, linux-kernel, io-uring, netdev,
linux-perf-users, linux-doc, Toke Høiland-Jørgensen,
linux-api, David Howells
In-Reply-To: <ahanjVfIDlCmeCUE@infradead.org>
On 5/27/26 2:13 AM, Christoph Hellwig wrote:
> On Tue, May 26, 2026 at 09:58:27AM -0600, Jens Axboe wrote:
>>> The current TCP zerocopy implementation provides completion notification
>>> through the socket error code, which is freaking weird and doesn't
>>> integrate well with either io_uring or in-kernel callers.
>>
>> We already have that via io_uring
>
> Where? And how do make that available to in-kernel users like
> storage protocols and network file system, which really suffer from
> the current MSG_SPLICE_PAGES semantics.
For zero copy, on both the receive and send side. Since we have a proper
notification channel, that's what we use rather than the hack that is
the error queue.
>> , and without needing msg_kiocb or the
>
> What do you think is the downside of using a kiocb here like for
> everything else with async notifications?
Where would the notifications go? You'd end up inventing something new
to propagate them to userspace then. The io_uring side does not rely on
using msg_kiocb, and iirc that part was only ever used for the crypto
stuff and largely broken. Which is why I do agree with just yanking it
out.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/3] Add support for qcrypto on shikra
From: Eric Biggers @ 2026-05-28 17:52 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Dmitry Baryshkov, Kuldeep Singh, Thara Gopinath, Herbert Xu,
David S. Miller, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Vinod Koul, Frank Li, Andy Gross,
linux-arm-msm, linux-crypto, devicetree, linux-kernel, dmaengine,
Bartosz Golaszewski, Gaurav Kashyap, Neeraj Soni
In-Reply-To: <CAMRc=Me6cqasdBknbAjUZ5BqcpERYwV+NvseRJp4P0aTSYAMUw@mail.gmail.com>
On Thu, May 28, 2026 at 11:13:47AM -0400, Bartosz Golaszewski wrote:
> On Thu, 28 May 2026 15:50:10 +0200, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> said:
> > On Thu, May 28, 2026 at 09:13:23AM -0400, Bartosz Golaszewski wrote:
> >> On Thu, 28 May 2026 13:54:51 +0200, Kuldeep Singh
> >> <kuldeep.singh@oss.qualcomm.com> said:
> >> >>> +Bartosz, Gaurav, Neeraj
> >>
> >> I know about the self-tests etc., I will address them next.
> >
> > My 2c, the self-tests would be more important, as they are fixes. Doing
> > the crypto in a wrong way is a bad idea...
> >
>
> Then let that be "in parallel". :)
The race conditions between Linux and other environments (modem, TEE,
etc) are of course about correctness as well, even though the self-tests
don't expose race condition bugs. The self-tests have always just done
a few serialized tests. That's sufficient for CPU-based code, but not
for offload drivers, which need to be stress-tested to find the
concurrency bugs that occur during actual use.
Is there a plan to improve the tests to do stress testing as well?
It's kind of odd that they don't do that yet. But it makes sense: the
CPU-based code doesn't need it, while the offload driver authors have
never cared enough about correctness and test coverage to add it.
I still don't really see a path forward here, given the track record and
poor performance numbers. This approach just doesn't work.
- Eric
^ permalink raw reply
* Re: [PATCH v5 2/7] x86/msr: add wrmsrq_on_cpus helper
From: Kalra, Ashish @ 2026-05-28 19:37 UTC (permalink / raw)
To: Borislav Petkov, Dave Hansen
Cc: tglx, mingo, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb, pbonzini, aik,
Michael.Roth, KPrateek.Nayak, Tycho.Andersen, Nathan.Fontenot,
ackerleytng, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <20260528004332.GDahePtGqVp2boiEJL@fat_crate.local>
Hello Boris and Dave,
On 5/27/2026 7:43 PM, Borislav Petkov wrote:
> On Wed, May 27, 2026 at 02:38:05PM -0700, Dave Hansen wrote:
>> This one is my doing.
>
> I know.
>
> But hey, maybe we should not disagree on the public ML because the submitter
> might disappear like the last one. :-P
>
>> wrmsr_on_cpus() is kinda a mess. I think it only has a single user. It's
>> also not very flexible because it needs a 'struct msr __percpu *msrs'
>> argument where each MSR has a value in memory.
>
> Right, we did that a looong time ago.
>
> The only reason I'd have for per-CPU MSR structs is reading different MSR
> values on different cores, modifying only the bits you need and then *keeping*
> the remaining values as they were. And that interface allows you to do that
> while this new thing won't.
>
> And I'm going to venture a guess here that adding a simpler interface which
> simply forces a new value ontop of a whole MSR could cause a lot of subtle
> bugs when people don't pay attention to keep the old values.
>
>> The use case for RMPOPT is that all CPUs get the same value. It'd be a
>> little awkward to go create a percpu data structure to duplcate the same
>> value to call wrmsr_on_cpus(). The RMPOPT case is also arguably
>> performance sensitive since it's done during boot. It should do the IPIs
>> in parallel.
>
> Oh sure, my meaning was to create something that serves both purposes.
>
>> toggle_ecc_err_reporting(), on the other hand, is done at module init
>> time. It's not really performance sensitive. It's probably pretty easy
>> to zap wrmsr_on_cpus() and just have toggle_ecc_err_reporting() do
>> something slightly less efficient.
>
> Sure. That's fine.
>
>> Yeah, the
>>
>> wrmsr_on_cpus()
>> wrmsrq_on_cpus()
>>
>> naming pain is real. There's little chance of bugs coming from it
>> because the function signatures are *SO* different. But, it certainly
>> could confuse humans for a minute.
>
> Yap.
>
>> But the real solution to this is axing wrmsr_on_cpus().
>
> Yap, for example. Basically reingeneering the whole
> write-MSRs-on-multiple-CPUs functionality is what I meant.
>
>> Which I think we could do after killing its one user which the attached
>> (completely untested) patch does. The only downside of the patch is that it
>> does RDMSR via IPIs one CPU at a time. But, looking at the code, I'm not
>> sure anyone would care. If anyone did, I _think_ all those MSRs have the
>> same value and the code could be simplified further. But that would take
>> more than 3 minutes.
>>
>> It's also possible that my grepping was bad or I'm completely
>> misunderstanding amd64_edac.c. Cluebat welcome if I'm being dense.
>
> Looks ok to me, we can surely do that. I even hw to test it. I think...
>
>> BTW, I also don't feel the need to make Ashish go do any of this edac
>> cleanup. I think it can just be done in parallel. But I wouldn't stop
>> him if he volunteered.
>
> Why not?
>
> It has always been the case: cleanups and bug fixes first, new features ontop.
>
> So yeah, modulo figuring out how to redefine the *msr_on_cpus() interface,
> I think this all makes sense.
snp_setup_rmpopt() runs once during init and rmpopt_cleanup() runs once during shutdown. The batch IPI optimization
is irrelevant here. This RMPOPT_BASE MSR setup/programming is not in a performance critical path.
A simple loop would be perfectly fine and avoids the need for the wrmsrq_on_cpus() helper entirely:
for_each_cpu(cpu, &rmpopt_cpumask)
wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
Calling wrmsrq_on_cpus() here for programming RMPOPT_BASE MSR:
- wrmsrq_on_cpus(&rmpopt_cpumask, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+ for_each_cpu(cpu, &rmpopt_cpumask)
+ wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
So i will drop this helper patch.
Thanks,
Ashish
>
> Thx.
>
^ permalink raw reply
* Re: [PATCH v5 2/7] x86/msr: add wrmsrq_on_cpus helper
From: Dave Hansen @ 2026-05-28 19:50 UTC (permalink / raw)
To: Kalra, Ashish, Borislav Petkov
Cc: tglx, mingo, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb, pbonzini, aik,
Michael.Roth, KPrateek.Nayak, Tycho.Andersen, Nathan.Fontenot,
ackerleytng, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <2d164e19-5cc6-47ca-9150-f4d432dd10c4@amd.com>
On 5/28/26 12:37, Kalra, Ashish wrote:
> A simple loop would be perfectly fine and avoids the need for the wrmsrq_on_cpus() helper entirely:
>
> for_each_cpu(cpu, &rmpopt_cpumask)
> wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
I'm glad we're on the same page finally. I just hope we can get to this
point more quickly next time. I started off with exactly this
suggestion, but someone chimed in to the thread and said it was "slower":
> https://lore.kernel.org/lkml/6a50d050-f602-43fd-a44a-cecedd9823eb@amd.com/
^ permalink raw reply
* Re: [PATCH v5 2/7] x86/msr: add wrmsrq_on_cpus helper
From: Kalra, Ashish @ 2026-05-28 19:55 UTC (permalink / raw)
To: Dave Hansen, Borislav Petkov
Cc: tglx, mingo, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb, pbonzini, aik,
Michael.Roth, KPrateek.Nayak, Tycho.Andersen, Nathan.Fontenot,
ackerleytng, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <c40dcb8c-5706-4c0f-ac85-c22957b9e192@intel.com>
Hello Dave,
On 5/28/2026 2:50 PM, Dave Hansen wrote:
> On 5/28/26 12:37, Kalra, Ashish wrote:
>> A simple loop would be perfectly fine and avoids the need for the wrmsrq_on_cpus() helper entirely:
>>
>> for_each_cpu(cpu, &rmpopt_cpumask)
>> wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
>
> I'm glad we're on the same page finally. I just hope we can get to this
> point more quickly next time. I started off with exactly this
> suggestion, but someone chimed in to the thread and said it was "slower":
>
>> https://lore.kernel.org/lkml/6a50d050-f602-43fd-a44a-cecedd9823eb@amd.com/
>
Yes, actually i should have made it explicitly clear that we need to do it in
parallel especially for issuing the RMPOPT instruction itself, as that is
in a performance critical path (and for that we are using on_each_cpu_mask()).
Thanks,
Ashish
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox