* [PATCH] crypto: talitos - eliminate unneeded 'done' functions at build time
From: Christophe Leroy @ 2019-06-17 21:14 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
When building for SEC1 only, talitos2_done functions are unneeded
and should go away.
For this, use has_ftr_sec1() which will always return true when only
SEC1 support is being built, allowing GCC to drop TALITOS2 functions.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
---
taken out of the "Additional fixes on Talitos driver" series as it can be applied independently
drivers/crypto/talitos.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 3b3e99f1cddb..b0ddf2e19e7f 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3414,7 +3414,7 @@ static int talitos_probe(struct platform_device *ofdev)
if (err)
goto err_out;
- if (of_device_is_compatible(np, "fsl,sec1.0")) {
+ if (has_ftr_sec1(priv)) {
if (priv->num_channels == 1)
tasklet_init(&priv->done_task[0], talitos1_done_ch0,
(unsigned long)dev);
--
2.13.3
^ permalink raw reply related
* [PATCH v4 0/4] Additional fixes on Talitos driver
From: Christophe Leroy @ 2019-06-17 21:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
This series is the last set of fixes for the Talitos driver.
We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
[ 3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[ 3.450982] random: fast init done
[ 12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 43.310737] Bug in SEC1, padding ourself
[ 45.603318] random: crng init done
[ 54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[ 54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'
[ 1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[ 1.229197] random: fast init done
[ 2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[ 2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[ 4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[ 4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[ 4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 6.631781] random: crng init done
[ 11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[ 11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'
v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.
v3:
- removed stable reference in patch 1
- reworded patch 1 to include name of patch 2 for the dependency.
- mentionned this dependency in patch 2 as well.
- corrected the Fixes: sha1 in patch 4
v4:
- using scatterwalk_ffwd() instead of opencodying SG list forwarding.
- Added a patch to fix sg_copy_to_buffer() when sg->offset() is greater than PAGE_SIZE,
otherwise sg_copy_to_buffer() fails when the list has been forwarded with scatterwalk_ffwd().
- taken the patch "crypto: talitos - eliminate unneeded 'done' functions at build time"
out of the series because it is independent.
- added a helper to find the header field associated to a request in flush_channe()
Christophe Leroy (4):
lib/scatterlist: Fix mapping iterator when sg->offset is greater than
PAGE_SIZE
crypto: talitos - move struct talitos_edesc into talitos.h
crypto: talitos - fix hash on SEC1.
crypto: talitos - drop icv_ool
drivers/crypto/talitos.c | 102 +++++++++++++++++++----------------------------
drivers/crypto/talitos.h | 28 +++++++++++++
lib/scatterlist.c | 9 ++++-
3 files changed, 76 insertions(+), 63 deletions(-)
--
2.13.3
^ permalink raw reply
* [PATCH v4 1/4] lib/scatterlist: Fix mapping iterator when sg->offset is greater than PAGE_SIZE
From: Christophe Leroy @ 2019-06-17 21:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1560805614.git.christophe.leroy@c-s.fr>
All mapping iterator logic is based on the assumption that sg->offset
is always lower than PAGE_SIZE.
But there are situations where sg->offset is such that the SG item
is on the second page. In that case sg_copy_to_buffer() fails
properly copying the data into the buffer. One of the reason is
that the data will be outside the kmapped area used to access that
data.
This patch fixes the issue by adjusting the mapping iterator
offset and pgoffset fields such that offset is always lower than
PAGE_SIZE.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Fixes: 4225fc8555a9 ("lib/scatterlist: use page iterator in the mapping iterator")
Cc: stable@vger.kernel.org
---
lib/scatterlist.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 739dc9fe2c55..39f00659898f 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -678,7 +678,7 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
{
if (!miter->__remaining) {
struct scatterlist *sg;
- unsigned long pgoffset;
+ unsigned long pgoffset, offset;
if (!__sg_page_iter_next(&miter->piter))
return false;
@@ -686,7 +686,12 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
sg = miter->piter.sg;
pgoffset = miter->piter.sg_pgoffset;
- miter->__offset = pgoffset ? 0 : sg->offset;
+ offset = pgoffset ? 0 : sg->offset;
+ while (offset >= PAGE_SIZE) {
+ miter->piter.sg_pgoffset = ++pgoffset;
+ offset -= PAGE_SIZE;
+ }
+ miter->__offset = offset;
miter->__remaining = sg->offset + sg->length -
(pgoffset << PAGE_SHIFT) - miter->__offset;
miter->__remaining = min_t(unsigned long, miter->__remaining,
--
2.13.3
^ permalink raw reply related
* [PATCH v4 2/4] crypto: talitos - move struct talitos_edesc into talitos.h
From: Christophe Leroy @ 2019-06-17 21:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1560805614.git.christophe.leroy@c-s.fr>
Moves struct talitos_edesc into talitos.h so that it can be used
from any place in talitos.c
It will be required for next patch ("crypto: talitos - fix hash
on SEC1")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: stable@vger.kernel.org
---
drivers/crypto/talitos.c | 30 ------------------------------
drivers/crypto/talitos.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a7704b53529d..95dc3957f358 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -951,36 +951,6 @@ static int aead_des3_setkey(struct crypto_aead *authenc,
goto out;
}
-/*
- * talitos_edesc - s/w-extended descriptor
- * @src_nents: number of segments in input scatterlist
- * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
- * @iv_dma: dma address of iv for checking continuity and link table
- * @dma_len: length of dma mapped link_tbl space
- * @dma_link_tbl: bus physical address of link_tbl/buf
- * @desc: h/w descriptor
- * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
- * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
- *
- * if decrypting (with authcheck), or either one of src_nents or dst_nents
- * is greater than 1, an integrity check value is concatenated to the end
- * of link_tbl data
- */
-struct talitos_edesc {
- int src_nents;
- int dst_nents;
- bool icv_ool;
- dma_addr_t iv_dma;
- int dma_len;
- dma_addr_t dma_link_tbl;
- struct talitos_desc desc;
- union {
- struct talitos_ptr link_tbl[0];
- u8 buf[0];
- };
-};
-
static void talitos_sg_unmap(struct device *dev,
struct talitos_edesc *edesc,
struct scatterlist *src,
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 32ad4fc679ed..95f78c6d9206 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -42,6 +42,36 @@ struct talitos_desc {
#define TALITOS_DESC_SIZE (sizeof(struct talitos_desc) - sizeof(__be32))
+/*
+ * talitos_edesc - s/w-extended descriptor
+ * @src_nents: number of segments in input scatterlist
+ * @dst_nents: number of segments in output scatterlist
+ * @icv_ool: whether ICV is out-of-line
+ * @iv_dma: dma address of iv for checking continuity and link table
+ * @dma_len: length of dma mapped link_tbl space
+ * @dma_link_tbl: bus physical address of link_tbl/buf
+ * @desc: h/w descriptor
+ * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
+ * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
+ *
+ * if decrypting (with authcheck), or either one of src_nents or dst_nents
+ * is greater than 1, an integrity check value is concatenated to the end
+ * of link_tbl data
+ */
+struct talitos_edesc {
+ int src_nents;
+ int dst_nents;
+ bool icv_ool;
+ dma_addr_t iv_dma;
+ int dma_len;
+ dma_addr_t dma_link_tbl;
+ struct talitos_desc desc;
+ union {
+ struct talitos_ptr link_tbl[0];
+ u8 buf[0];
+ };
+};
+
/**
* talitos_request - descriptor submission request
* @desc: descriptor pointer (kernel virtual)
--
2.13.3
^ permalink raw reply related
* [PATCH v4 4/4] crypto: talitos - drop icv_ool
From: Christophe Leroy @ 2019-06-17 21:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1560805614.git.christophe.leroy@c-s.fr>
icv_ool is not used anymore, drop it.
Fixes: e345177ded17 ("crypto: talitos - fix AEAD processing.")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/crypto/talitos.c | 3 ---
drivers/crypto/talitos.h | 2 --
2 files changed, 5 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index ab6bd45addf7..c9d686a0e805 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1285,9 +1285,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
is_ipsec_esp && !encrypt);
tbl_off += ret;
- /* ICV data */
- edesc->icv_ool = !encrypt;
-
if (!encrypt && is_ipsec_esp) {
struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 95f78c6d9206..1469b956948a 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -46,7 +46,6 @@ struct talitos_desc {
* talitos_edesc - s/w-extended descriptor
* @src_nents: number of segments in input scatterlist
* @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
* @iv_dma: dma address of iv for checking continuity and link table
* @dma_len: length of dma mapped link_tbl space
* @dma_link_tbl: bus physical address of link_tbl/buf
@@ -61,7 +60,6 @@ struct talitos_desc {
struct talitos_edesc {
int src_nents;
int dst_nents;
- bool icv_ool;
dma_addr_t iv_dma;
int dma_len;
dma_addr_t dma_link_tbl;
--
2.13.3
^ permalink raw reply related
* [PATCH v4 3/4] crypto: talitos - fix hash on SEC1.
From: Christophe Leroy @ 2019-06-17 21:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1560805614.git.christophe.leroy@c-s.fr>
On SEC1, hash provides wrong result when performing hashing in several
steps with input data SG list has more than one element. This was
detected with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
[ 44.185947] alg: hash: md5-talitos test failed (wrong result) on test vector 6, cfg="random: may_sleep use_finup src_divs=[<reimport>25.88%@+8063, <flush>24.19%@+9588, 28.63%@+16333, <reimport>4.60%@+6756, 16.70%@+16281] dst_divs=[71.61%@alignmask+16361, 14.36%@+7756, 14.3%@+"
[ 44.325122] alg: hash: sha1-talitos test failed (wrong result) on test vector 3, cfg="random: inplace use_final src_divs=[<flush,nosimd>16.56%@+16378, <reimport>52.0%@+16329, 21.42%@alignmask+16380, 10.2%@alignmask+16380] iv_offset=39"
[ 44.493500] alg: hash: sha224-talitos test failed (wrong result) on test vector 4, cfg="random: use_final nosimd src_divs=[<reimport>52.27%@+7401, <reimport>17.34%@+16285, <flush>17.71%@+26, 12.68%@+10644] iv_offset=43"
[ 44.673262] alg: hash: sha256-talitos test failed (wrong result) on test vector 4, cfg="random: may_sleep use_finup src_divs=[<reimport>60.6%@+12790, 17.86%@+1329, <reimport>12.64%@alignmask+16300, 8.29%@+15, 0.40%@+13506, <reimport>0.51%@+16322, <reimport>0.24%@+16339] dst_divs"
This is due to two issues:
- We have an overlap between the buffer used for copying the input
data (SEC1 doesn't do scatter/gather) and the chained descriptor.
- Data copy is wrong when the previous hash left less than one
blocksize of data to hash, implying a complement of the previous
block with a few bytes from the new request.
Fix it by:
- Moving the second descriptor after the buffer, as moving the buffer
after the descriptor would make it more complex for other cipher
operations (AEAD, ABLKCIPHER)
- Skip the bytes taken from the new request to complete the previous
one by moving the SG list forward.
Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/crypto/talitos.c | 69 ++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 95dc3957f358..ab6bd45addf7 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -320,6 +320,21 @@ static int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
return -EINPROGRESS;
}
+static __be32 get_request_hdr(struct talitos_request *request, bool is_sec1)
+{
+ struct talitos_edesc *edesc;
+
+ if (!is_sec1)
+ return request->desc->hdr;
+
+ if (!request->desc->next_desc)
+ return request->desc->hdr1;
+
+ edesc = container_of(request->desc, struct talitos_edesc, desc);
+
+ return ((struct talitos_desc *)(edesc->buf + edesc->dma_len))->hdr1;
+}
+
/*
* process what was done, notify callback of error if not
*/
@@ -341,12 +356,7 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
/* descriptors with their done bits set don't get the error */
rmb();
- if (!is_sec1)
- hdr = request->desc->hdr;
- else if (request->desc->next_desc)
- hdr = (request->desc + 1)->hdr1;
- else
- hdr = request->desc->hdr1;
+ hdr = get_request_hdr(request, is_sec1);
if ((hdr & DESC_HDR_DONE) == DESC_HDR_DONE)
status = 0;
@@ -476,8 +486,14 @@ static u32 current_desc_hdr(struct device *dev, int ch)
}
}
- if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
- return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
+ if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc) {
+ struct talitos_edesc *edesc;
+
+ edesc = container_of(priv->chan[ch].fifo[iter].desc,
+ struct talitos_edesc, desc);
+ return ((struct talitos_desc *)
+ (edesc->buf + edesc->dma_len))->hdr;
+ }
return priv->chan[ch].fifo[iter].desc->hdr;
}
@@ -1402,15 +1418,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
edesc->dst_nents = dst_nents;
edesc->iv_dma = iv_dma;
edesc->dma_len = dma_len;
- if (dma_len) {
- void *addr = &edesc->link_tbl[0];
-
- if (is_sec1 && !dst)
- addr += sizeof(struct talitos_desc);
- edesc->dma_link_tbl = dma_map_single(dev, addr,
+ if (dma_len)
+ edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
edesc->dma_len,
DMA_BIDIRECTIONAL);
- }
+
return edesc;
}
@@ -1722,14 +1734,16 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
struct talitos_private *priv = dev_get_drvdata(dev);
bool is_sec1 = has_ftr_sec1(priv);
struct talitos_desc *desc = &edesc->desc;
- struct talitos_desc *desc2 = desc + 1;
+ struct talitos_desc *desc2 = (struct talitos_desc *)
+ (edesc->buf + edesc->dma_len);
unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
if (desc->next_desc &&
desc->ptr[5].ptr != desc2->ptr[5].ptr)
unmap_single_talitos_ptr(dev, &desc2->ptr[5], DMA_FROM_DEVICE);
- talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
+ if (req_ctx->psrc)
+ talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
/* When using hashctx-in, must unmap it. */
if (from_talitos_ptr_len(&edesc->desc.ptr[1], is_sec1))
@@ -1796,7 +1810,6 @@ static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
static int common_nonsnoop_hash(struct talitos_edesc *edesc,
struct ahash_request *areq, unsigned int length,
- unsigned int offset,
void (*callback) (struct device *dev,
struct talitos_desc *desc,
void *context, int error))
@@ -1835,9 +1848,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
sg_count = edesc->src_nents ?: 1;
if (is_sec1 && sg_count > 1)
- sg_pcopy_to_buffer(req_ctx->psrc, sg_count,
- edesc->buf + sizeof(struct talitos_desc),
- length, req_ctx->nbuf);
+ sg_copy_to_buffer(req_ctx->psrc, sg_count, edesc->buf, length);
else if (length)
sg_count = dma_map_sg(dev, req_ctx->psrc, sg_count,
DMA_TO_DEVICE);
@@ -1850,7 +1861,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
DMA_TO_DEVICE);
} else {
sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
- &desc->ptr[3], sg_count, offset, 0);
+ &desc->ptr[3], sg_count, 0, 0);
if (sg_count > 1)
sync_needed = true;
}
@@ -1874,7 +1885,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
if (is_sec1 && req_ctx->nbuf && length) {
- struct talitos_desc *desc2 = desc + 1;
+ struct talitos_desc *desc2 = (struct talitos_desc *)
+ (edesc->buf + edesc->dma_len);
dma_addr_t next_desc;
memset(desc2, 0, sizeof(*desc2));
@@ -1895,7 +1907,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
DMA_TO_DEVICE);
copy_talitos_ptr(&desc2->ptr[2], &desc->ptr[2], is_sec1);
sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
- &desc2->ptr[3], sg_count, offset, 0);
+ &desc2->ptr[3], sg_count, 0, 0);
if (sg_count > 1)
sync_needed = true;
copy_talitos_ptr(&desc2->ptr[5], &desc->ptr[5], is_sec1);
@@ -2006,7 +2018,6 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
struct device *dev = ctx->dev;
struct talitos_private *priv = dev_get_drvdata(dev);
bool is_sec1 = has_ftr_sec1(priv);
- int offset = 0;
u8 *ctx_buf = req_ctx->buf[req_ctx->buf_idx];
if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
@@ -2046,6 +2057,8 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
sg_chain(req_ctx->bufsl, 2, areq->src);
req_ctx->psrc = req_ctx->bufsl;
} else if (is_sec1 && req_ctx->nbuf && req_ctx->nbuf < blocksize) {
+ int offset;
+
if (nbytes_to_hash > blocksize)
offset = blocksize - req_ctx->nbuf;
else
@@ -2058,7 +2071,8 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
sg_copy_to_buffer(areq->src, nents,
ctx_buf + req_ctx->nbuf, offset);
req_ctx->nbuf += offset;
- req_ctx->psrc = areq->src;
+ req_ctx->psrc = scatterwalk_ffwd(req_ctx->bufsl, areq->src,
+ offset);
} else
req_ctx->psrc = areq->src;
@@ -2098,8 +2112,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
if (ctx->keylen && (req_ctx->first || req_ctx->last))
edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_HMAC;
- return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, offset,
- ahash_done);
+ return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, ahash_done);
}
static int ahash_update(struct ahash_request *areq)
--
2.13.3
^ permalink raw reply related
* [PATCH] powerpc/mm/32s: fix condition that is always true
From: Andreas Schwab @ 2019-06-17 21:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: j.neuschaefer, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <1e412310cc18ea654fb2ce4c935654d8d1069f27.1550775950.git.christophe.leroy@c-s.fr>
Move a misplaced paren that makes the condition always true.
Fixes: 63b2bc619565 ("powerpc/mm/32s: Use BATs for STRICT_KERNEL_RWX")
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
arch/powerpc/mm/pgtable_32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index d53188dee18f..35cb96cfc258 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -360,7 +360,7 @@ void mark_initmem_nx(void)
unsigned long numpages = PFN_UP((unsigned long)_einittext) -
PFN_DOWN((unsigned long)_sinittext);
- if (v_block_mapped((unsigned long)_stext) + 1)
+ if (v_block_mapped((unsigned long)_stext + 1))
mmu_mark_initmem_nx();
else
change_page_attr(page, numpages, PAGE_KERNEL);
--
2.22.0
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply related
* [PATCH] selftests/powerpc: Fix earlyclobber in tm-vmxcopy
From: Gustavo Romero @ 2019-06-17 21:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey
In some cases, compiler can allocate the same register for operand 'res'
and 'vecoutptr', resulting in segfault at 'stxvd2x 40,0,%[vecoutptr]'
because base register will contain 1, yielding a false-positive.
This is because output 'res' must be marked as an earlyclobber operand so
it may not overlap an input operand ('vecoutptr').
Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com>
---
tools/testing/selftests/powerpc/tm/tm-vmxcopy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
index 147c6dc..c1e788a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
+++ b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
@@ -79,7 +79,7 @@ int test_vmxcopy()
"5:;"
"stxvd2x 40,0,%[vecoutptr];"
- : [res]"=r"(aborted)
+ : [res]"=&r"(aborted)
: [vecinptr]"r"(&vecin),
[vecoutptr]"r"(&vecout),
[map]"r"(a)
--
2.7.4
^ permalink raw reply related
* [PATCH v2] powerpc/32s: fix suspend/resume when IBATs 4-7 are used
From: Christophe Leroy @ 2019-06-17 21:42 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andreas Schwab
Cc: linuxppc-dev, linux-kernel
Previously, only IBAT1 and IBAT2 were used to map kernel linear mem.
Since commit 63b2bc619565 ("powerpc/mm/32s: Use BATs for
STRICT_KERNEL_RWX"), we may have all 8 BATs used for mapping
kernel text. But the suspend/restore functions only save/restore
BATs 0 to 3, and clears BATs 4 to 7.
Make suspend and restore functions respectively save and reload
the 8 BATs on CPUs having MMU_FTR_USE_HIGH_BATS feature.
Reported-by: Andreas Schwab <schwab@linux-m68k.org>
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: using SPRN_DBATxU/xL to avoid build failure.
arch/powerpc/kernel/swsusp_32.S | 73 +++++++++++++++++++++++++++++----
arch/powerpc/platforms/powermac/sleep.S | 68 +++++++++++++++++++++++++++---
2 files changed, 128 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/swsusp_32.S b/arch/powerpc/kernel/swsusp_32.S
index 7a919e9a3400..cbdf86228eaa 100644
--- a/arch/powerpc/kernel/swsusp_32.S
+++ b/arch/powerpc/kernel/swsusp_32.S
@@ -25,11 +25,19 @@
#define SL_IBAT2 0x48
#define SL_DBAT3 0x50
#define SL_IBAT3 0x58
-#define SL_TB 0x60
-#define SL_R2 0x68
-#define SL_CR 0x6c
-#define SL_LR 0x70
-#define SL_R12 0x74 /* r12 to r31 */
+#define SL_DBAT4 0x60
+#define SL_IBAT4 0x68
+#define SL_DBAT5 0x70
+#define SL_IBAT5 0x78
+#define SL_DBAT6 0x80
+#define SL_IBAT6 0x88
+#define SL_DBAT7 0x90
+#define SL_IBAT7 0x98
+#define SL_TB 0xa0
+#define SL_R2 0xa8
+#define SL_CR 0xac
+#define SL_LR 0xb0
+#define SL_R12 0xb4 /* r12 to r31 */
#define SL_SIZE (SL_R12 + 80)
.section .data
@@ -114,6 +122,41 @@ _GLOBAL(swsusp_arch_suspend)
mfibatl r4,3
stw r4,SL_IBAT3+4(r11)
+BEGIN_MMU_FTR_SECTION
+ mfspr r4,SPRN_DBAT4U
+ stw r4,SL_DBAT4(r11)
+ mfspr r4,SPRN_DBAT4L
+ stw r4,SL_DBAT4+4(r11)
+ mfspr r4,SPRN_DBAT5U
+ stw r4,SL_DBAT5(r11)
+ mfspr r4,SPRN_DBAT5L
+ stw r4,SL_DBAT5+4(r11)
+ mfspr r4,SPRN_DBAT6U
+ stw r4,SL_DBAT6(r11)
+ mfspr r4,SPRN_DBAT6L
+ stw r4,SL_DBAT6+4(r11)
+ mfspr r4,SPRN_DBAT7U
+ stw r4,SL_DBAT7(r11)
+ mfspr r4,SPRN_DBAT7L
+ stw r4,SL_DBAT7+4(r11)
+ mfspr r4,SPRN_IBAT4U
+ stw r4,SL_IBAT4(r11)
+ mfspr r4,SPRN_IBAT4L
+ stw r4,SL_IBAT4+4(r11)
+ mfspr r4,SPRN_IBAT5U
+ stw r4,SL_IBAT5(r11)
+ mfspr r4,SPRN_IBAT5L
+ stw r4,SL_IBAT5+4(r11)
+ mfspr r4,SPRN_IBAT6U
+ stw r4,SL_IBAT6(r11)
+ mfspr r4,SPRN_IBAT6L
+ stw r4,SL_IBAT6+4(r11)
+ mfspr r4,SPRN_IBAT7U
+ stw r4,SL_IBAT7(r11)
+ mfspr r4,SPRN_IBAT7L
+ stw r4,SL_IBAT7+4(r11)
+END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS)
+
#if 0
/* Backup various CPU config stuffs */
bl __save_cpu_setup
@@ -279,27 +322,41 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
mtibatu 3,r4
lwz r4,SL_IBAT3+4(r11)
mtibatl 3,r4
-#endif
-
BEGIN_MMU_FTR_SECTION
- li r4,0
+ lwz r4,SL_DBAT4(r11)
mtspr SPRN_DBAT4U,r4
+ lwz r4,SL_DBAT4+4(r11)
mtspr SPRN_DBAT4L,r4
+ lwz r4,SL_DBAT5(r11)
mtspr SPRN_DBAT5U,r4
+ lwz r4,SL_DBAT5+4(r11)
mtspr SPRN_DBAT5L,r4
+ lwz r4,SL_DBAT6(r11)
mtspr SPRN_DBAT6U,r4
+ lwz r4,SL_DBAT6+4(r11)
mtspr SPRN_DBAT6L,r4
+ lwz r4,SL_DBAT7(r11)
mtspr SPRN_DBAT7U,r4
+ lwz r4,SL_DBAT7+4(r11)
mtspr SPRN_DBAT7L,r4
+ lwz r4,SL_IBAT4(r11)
mtspr SPRN_IBAT4U,r4
+ lwz r4,SL_IBAT4+4(r11)
mtspr SPRN_IBAT4L,r4
+ lwz r4,SL_IBAT5(r11)
mtspr SPRN_IBAT5U,r4
+ lwz r4,SL_IBAT5+4(r11)
mtspr SPRN_IBAT5L,r4
+ lwz r4,SL_IBAT6(r11)
mtspr SPRN_IBAT6U,r4
+ lwz r4,SL_IBAT6+4(r11)
mtspr SPRN_IBAT6L,r4
+ lwz r4,SL_IBAT7(r11)
mtspr SPRN_IBAT7U,r4
+ lwz r4,SL_IBAT7+4(r11)
mtspr SPRN_IBAT7L,r4
END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS)
+#endif
/* Flush all TLBs */
lis r4,0x1000
diff --git a/arch/powerpc/platforms/powermac/sleep.S b/arch/powerpc/platforms/powermac/sleep.S
index 6bbcbec97712..bd6085b470b7 100644
--- a/arch/powerpc/platforms/powermac/sleep.S
+++ b/arch/powerpc/platforms/powermac/sleep.S
@@ -33,10 +33,18 @@
#define SL_IBAT2 0x48
#define SL_DBAT3 0x50
#define SL_IBAT3 0x58
-#define SL_TB 0x60
-#define SL_R2 0x68
-#define SL_CR 0x6c
-#define SL_R12 0x70 /* r12 to r31 */
+#define SL_DBAT4 0x60
+#define SL_IBAT4 0x68
+#define SL_DBAT5 0x70
+#define SL_IBAT5 0x78
+#define SL_DBAT6 0x80
+#define SL_IBAT6 0x88
+#define SL_DBAT7 0x90
+#define SL_IBAT7 0x98
+#define SL_TB 0xa0
+#define SL_R2 0xa8
+#define SL_CR 0xac
+#define SL_R12 0xb0 /* r12 to r31 */
#define SL_SIZE (SL_R12 + 80)
.section .text
@@ -121,6 +129,41 @@ _GLOBAL(low_sleep_handler)
mfibatl r4,3
stw r4,SL_IBAT3+4(r1)
+BEGIN_MMU_FTR_SECTION
+ mfspr r4,SPRN_DBAT4U
+ stw r4,SL_DBAT4(r1)
+ mfspr r4,SPRN_DBAT4L
+ stw r4,SL_DBAT4+4(r1)
+ mfspr r4,SPRN_DBAT5U
+ stw r4,SL_DBAT5(r1)
+ mfspr r4,SPRN_DBAT5L
+ stw r4,SL_DBAT5+4(r1)
+ mfspr r4,SPRN_DBAT6U
+ stw r4,SL_DBAT6(r1)
+ mfspr r4,SPRN_DBAT6L
+ stw r4,SL_DBAT6+4(r1)
+ mfspr r4,SPRN_DBAT7U
+ stw r4,SL_DBAT7(r1)
+ mfspr r4,SPRN_DBAT7L
+ stw r4,SL_DBAT7+4(r1)
+ mfspr r4,SPRN_IBAT4U
+ stw r4,SL_IBAT4(r1)
+ mfspr r4,SPRN_IBAT4L
+ stw r4,SL_IBAT4+4(r1)
+ mfspr r4,SPRN_IBAT5U
+ stw r4,SL_IBAT5(r1)
+ mfspr r4,SPRN_IBAT5L
+ stw r4,SL_IBAT5+4(r1)
+ mfspr r4,SPRN_IBAT6U
+ stw r4,SL_IBAT6(r1)
+ mfspr r4,SPRN_IBAT6L
+ stw r4,SL_IBAT6+4(r1)
+ mfspr r4,SPRN_IBAT7U
+ stw r4,SL_IBAT7(r1)
+ mfspr r4,SPRN_IBAT7L
+ stw r4,SL_IBAT7+4(r1)
+END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS)
+
/* Backup various CPU config stuffs */
bl __save_cpu_setup
@@ -321,22 +364,37 @@ grackle_wake_up:
mtibatl 3,r4
BEGIN_MMU_FTR_SECTION
- li r4,0
+ lwz r4,SL_DBAT4(r1)
mtspr SPRN_DBAT4U,r4
+ lwz r4,SL_DBAT4+4(r1)
mtspr SPRN_DBAT4L,r4
+ lwz r4,SL_DBAT5(r1)
mtspr SPRN_DBAT5U,r4
+ lwz r4,SL_DBAT5+4(r1)
mtspr SPRN_DBAT5L,r4
+ lwz r4,SL_DBAT6(r1)
mtspr SPRN_DBAT6U,r4
+ lwz r4,SL_DBAT6+4(r1)
mtspr SPRN_DBAT6L,r4
+ lwz r4,SL_DBAT7(r1)
mtspr SPRN_DBAT7U,r4
+ lwz r4,SL_DBAT7+4(r1)
mtspr SPRN_DBAT7L,r4
+ lwz r4,SL_IBAT4(r1)
mtspr SPRN_IBAT4U,r4
+ lwz r4,SL_IBAT4+4(r1)
mtspr SPRN_IBAT4L,r4
+ lwz r4,SL_IBAT5(r1)
mtspr SPRN_IBAT5U,r4
+ lwz r4,SL_IBAT5+4(r1)
mtspr SPRN_IBAT5L,r4
+ lwz r4,SL_IBAT6(r1)
mtspr SPRN_IBAT6U,r4
+ lwz r4,SL_IBAT6+4(r1)
mtspr SPRN_IBAT6L,r4
+ lwz r4,SL_IBAT7(r1)
mtspr SPRN_IBAT7U,r4
+ lwz r4,SL_IBAT7+4(r1)
mtspr SPRN_IBAT7L,r4
END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS)
--
2.13.3
^ permalink raw reply related
* Re: [PATCH] powerpc/mm/32s: fix condition that is always true
From: Christophe Leroy @ 2019-06-17 21:47 UTC (permalink / raw)
To: Andreas Schwab; +Cc: j.neuschaefer, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <87muif52lv.fsf_-_@igel.home>
Le 17/06/2019 à 23:22, Andreas Schwab a écrit :
> Move a misplaced paren that makes the condition always true.
>
> Fixes: 63b2bc619565 ("powerpc/mm/32s: Use BATs for STRICT_KERNEL_RWX")
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/mm/pgtable_32.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index d53188dee18f..35cb96cfc258 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -360,7 +360,7 @@ void mark_initmem_nx(void)
> unsigned long numpages = PFN_UP((unsigned long)_einittext) -
> PFN_DOWN((unsigned long)_sinittext);
>
> - if (v_block_mapped((unsigned long)_stext) + 1)
> + if (v_block_mapped((unsigned long)_stext + 1))
> mmu_mark_initmem_nx();
> else
> change_page_attr(page, numpages, PAGE_KERNEL);
>
^ permalink raw reply
* Re: Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
From: Ram Pai @ 2019-06-17 23:51 UTC (permalink / raw)
To: Paul Mackerras
Cc: Madhavan Srinivasan, Michael Anderson, Claudio Carvalho, kvm-ppc,
Bharata B Rao, linuxppc-dev, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <20190617020632.yywfoqwfinjxs3pb@oak.ozlabs.ibm.com>
On Mon, Jun 17, 2019 at 12:06:32PM +1000, Paul Mackerras wrote:
> On Thu, Jun 06, 2019 at 02:36:09PM -0300, Claudio Carvalho wrote:
> > From: Ram Pai <linuxram@us.ibm.com>
> >
> > Add the ucall() function, which can be used to make ultravisor calls
> > with varied number of in and out arguments. Ultravisor calls can be made
> > from the host or guests.
> >
> > This copies the implementation of plpar_hcall().
>
> One point which I missed when I looked at this patch previously is
> that the ABI that we're defining here is different from the hcall ABI
> in that we are putting the ucall number in r0, whereas hcalls have the
> hcall number in r3. That makes ucalls more like syscalls, which have
> the syscall number in r0. So that last sentence quoted above is
> somewhat misleading.
>
> The thing we need to consider is that when SMFCTRL[E] = 0, a ucall
> instruction becomes a hcall (that is, sc 2 is executed as if it was
> sc 1). In that case, the first argument to the ucall will be
> interpreted as the hcall number. Mostly that will happen not to be a
> valid hcall number, but sometimes it might unavoidably be a valid but
> unintended hcall number.
>
> I think that will make it difficult to get ucalls to fail gracefully
> in the case where SMF/PEF is disabled. It seems like the assignment
> of ucall numbers was made so that they wouldn't overlap with valid
> hcall numbers; presumably that was so that we could tell when an hcall
> was actually intended to be a ucall. However, using a different GPR
> to pass the ucall number defeats that.
Right this is a valid point. Glad that you caught it, otherwise it would
have become a difficult to fix it in the future.
>
> I realize that there is ultravisor code in development that takes the
> ucall number in r0, and also that having the ucall number in r3 would
> possibly make life more difficult for the place where we call
> UV_RETURN in assembler code.
Its called from one place in the hypervisor, and the changes look
simple.
- LOAD_REG_IMMEDIATE(r0, UV_RETURN)
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
ld r7, VCPU_GPR(R7)(r4)
ld r6, VCPU_GPR(R6)(r4)
ld r4, VCPU_GPR(R4)(r4)
What am i missing?
> Nevertheless, perhaps we should consider
> changing the ABI to be like the hcall ABI before everything gets set
> in concrete.
yes.
Thanks Paul!
RP
^ permalink raw reply
* Re: [PATCH] ocxl: Allow contexts to be attached with a NULL mm
From: Andrew Donnellan @ 2019-06-18 1:50 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Frederic Barrat, Greg Kroah-Hartman, linuxppc-dev, linux-kernel,
Arnd Bergmann
In-Reply-To: <20190617044152.13707-1-alastair@au1.ibm.com>
On 17/6/19 2:41 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> If an OpenCAPI context is to be used directly by a kernel driver, there
> may not be a suitable mm to use.
>
> The patch makes the mm parameter to ocxl_context_attach optional.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
The one issue I can see here is that using mm == NULL bypasses our
method of enabling/disabling global TLBIs in mm_context_add_copro().
Discussing this privately with Alastair and Fred - this should be fine,
but perhaps we should document that.
> ---
> drivers/misc/ocxl/context.c | 9 ++++++---
> drivers/misc/ocxl/link.c | 12 ++++++++----
> 2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index bab9c9364184..994563a078eb 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -69,6 +69,7 @@ static void xsl_fault_error(void *data, u64 addr, u64 dsisr)
> int ocxl_context_attach(struct ocxl_context *ctx, u64 amr, struct mm_struct *mm)
> {
> int rc;
> + unsigned long pidr = 0;
>
> // Locks both status & tidr
> mutex_lock(&ctx->status_mutex);
> @@ -77,9 +78,11 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr, struct mm_struct *mm)
> goto out;
> }
>
> - rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
> - mm->context.id, ctx->tidr, amr, mm,
> - xsl_fault_error, ctx);
> + if (mm)
> + pidr = mm->context.id;
> +
> + rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid, pidr, ctx->tidr,
> + amr, mm, xsl_fault_error, ctx);
> if (rc)
> goto out;
>
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index cce5b0d64505..43542f124807 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -523,7 +523,8 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> pe->amr = cpu_to_be64(amr);
> pe->software_state = cpu_to_be32(SPA_PE_VALID);
>
> - mm_context_add_copro(mm);
> + if (mm)
> + mm_context_add_copro(mm);
> /*
> * Barrier is to make sure PE is visible in the SPA before it
> * is used by the device. It also helps with the global TLBI
> @@ -546,7 +547,8 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> * have a reference on mm_users. Incrementing mm_count solves
> * the problem.
> */
> - mmgrab(mm);
> + if (mm)
> + mmgrab(mm);
> trace_ocxl_context_add(current->pid, spa->spa_mem, pasid, pidr, tidr);
> unlock:
> mutex_unlock(&spa->spa_lock);
> @@ -652,8 +654,10 @@ int ocxl_link_remove_pe(void *link_handle, int pasid)
> if (!pe_data) {
> WARN(1, "Couldn't find pe data when removing PE\n");
> } else {
> - mm_context_remove_copro(pe_data->mm);
> - mmdrop(pe_data->mm);
> + if (pe_data->mm) {
> + mm_context_remove_copro(pe_data->mm);
> + mmdrop(pe_data->mm);
> + }
> kfree_rcu(pe_data, rcu);
> }
> unlock:
>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
From: Andrew Morton @ 2019-06-18 1:57 UTC (permalink / raw)
To: Christophe Leroy
Cc: Stephen Rothwell, Michal Hocko, Pavel Tatashin, linux-acpi,
Baoquan He, David Hildenbrand, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Wei Yang, linux-mm,
Mike Rapoport, Arun KS, Johannes Weiner, Dan Williams,
linuxppc-dev, Mel Gorman, Vlastimil Babka, Oscar Salvador
In-Reply-To: <701e8feb-cbf8-04c1-758c-046da9394ac1@c-s.fr>
On Sat, 15 Jun 2019 10:06:54 +0200 Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 14/06/2019 à 21:00, Andrew Morton a écrit :
> > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
> >
> >> We are using a mixture of "int" and "unsigned long". Let's make this
> >> consistent by using "unsigned long" everywhere. We'll do the same with
> >> memory block ids next.
> >>
> >> ...
> >>
> >> - int i, ret, section_count = 0;
> >> + unsigned long i;
> >>
> >> ...
> >>
> >> - unsigned int i;
> >> + unsigned long i;
> >
> > Maybe I did too much fortran back in the day, but I think the
> > expectation is that a variable called "i" has type "int".
> >
> > This?
> >
> >
> >
> > s/unsigned long i/unsigned long section_nr/
>
> From my point of view you degrade readability by doing that.
>
> section_nr_to_pfn(mem->start_section_nr + section_nr);
>
> Three times the word 'section_nr' in one line, is that worth it ? Gives
> me headache.
>
> Codying style says the following, which makes full sense in my opinion:
>
> LOCAL variable names should be short, and to the point. If you have
> some random integer loop counter, it should probably be called ``i``.
> Calling it ``loop_counter`` is non-productive, if there is no chance of it
> being mis-understood.
Well. It did say "integer". Calling an unsigned long `i' is flat out
misleading.
> What about just naming it 'nr' if we want to use something else than 'i' ?
Sure, that works.
^ permalink raw reply
* Re: [PATCH kernel] powerpc/pci/of: Parse unassigned resources
From: Sam Bobroff @ 2019-06-18 4:02 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Alistair Popple, kvm-ppc, Oliver O'Halloran, linuxppc-dev,
David Gibson
In-Reply-To: <a9f5d1d7-a8dc-91eb-8197-a7aa9b45957b@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 3749 bytes --]
On Fri, Jun 14, 2019 at 01:18:28PM +1000, Alexey Kardashevskiy wrote:
>
>
> On 14/06/2019 12:59, Alexey Kardashevskiy wrote:
> > The pseries platform uses the PCI_PROBE_DEVTREE method of PCI probing
> > which is basically reading "assigned-addresses" of every PCI device.
> > However if the property is missing or zero sized, then there is
> > no fallback of any kind and the PCI resources remain undiscovered, i.e.
> > pdev->resource[] array is empty.
> >
> > This adds a fallback which parses the "reg" property in pretty much same
> > way except it marks resources as "unset" which later makes Linux assign
> > those resources with proper addresses.
> >
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >
> > This is an attempts to boot linux directly under QEMU without slof/rtas;
> > the aim is to use petitboot instead and let the guest kernel configure
> > devices.
> >
> > QEMU does not allocate resources, it creates correct "reg" and zero length
> > "assigned-addresses" (which is probably a bug on its own) which is
> > normally populated by SLOF later but not during this exercise.
Hi Alexey,
This patch (fixed, as you point out below) also seems to improve hotplug
on pSeries.
Currently, the PCI hotplug driver for pSeries (rpaphp) uses generic PCI
scanning to add hot plugged devices, rather than slot power control,
because the slot power control method doesn't work.
AFAIK one of the reasons that slot power control doesn't work is that
the assigned-addresses node isn't populated by QEMU during hotplug, so I
tested this patch on a guest that has been modified to use that method.
In combination with a QEMU change to prevent PCI_PROBE_ONLY being set
(necessary to allow pcibios_finish_adding_to_bus() to do resource
allocation -- I assume you are using a similar change), I was able to
successfully hot plug a few devices!
So this change seems to be a step in the right direction.
(I also tested it with an unmodified guest, and it doesn't seem to harm
hotpluging via generic PCI scanning.)
One nit: I think that calling the variable "unset" is a bit confusing.
What about calling it "aa_missing" or something like that?
Cheers,
Sam.
> > ---
> > arch/powerpc/kernel/pci_of_scan.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> > index 64ad92016b63..cfe6ec3c6aaf 100644
> > --- a/arch/powerpc/kernel/pci_of_scan.c
> > +++ b/arch/powerpc/kernel/pci_of_scan.c
> > @@ -82,10 +82,18 @@ static void of_pci_parse_addrs(struct device_node *node, struct pci_dev *dev)
> > const __be32 *addrs;
> > u32 i;
> > int proplen;
> > + bool unset = false;
> >
> > addrs = of_get_property(node, "assigned-addresses", &proplen);
> > if (!addrs)
> > return;
>
>
> Ah. Of course, these 2 lines above should go, my bad. I'll repost if
> there are no other (and bigger) problems with this.
>
>
>
> > + if (!addrs || !proplen) {
> > + addrs = of_get_property(node, "reg", &proplen);
> > + if (!addrs || !proplen)
> > + return;
> > + unset = true;
> > + }
> > +
> > pr_debug(" parse addresses (%d bytes) @ %p\n", proplen, addrs);
> > for (; proplen >= 20; proplen -= 20, addrs += 5) {
> > flags = pci_parse_of_flags(of_read_number(addrs, 1), 0);
> > @@ -110,6 +118,8 @@ static void of_pci_parse_addrs(struct device_node *node, struct pci_dev *dev)
> > continue;
> > }
> > res->flags = flags;
> > + if (unset)
> > + res->flags |= IORESOURCE_UNSET;
> > res->name = pci_name(dev);
> > region.start = base;
> > region.end = base + size - 1;
> >
>
> --
> Alexey
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH kernel v3 0/3] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59
From: Shawn Anastasio @ 2019-06-18 4:26 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran, David Gibson
In-Reply-To: <382353e8-591c-1ec6-21d5-c81811efb097@anastas.io>
On 6/12/19 2:15 PM, Shawn Anastasio wrote:
> On 6/12/19 2:07 AM, Alexey Kardashevskiy wrote:
>>
>>
>> On 12/06/2019 15:05, Shawn Anastasio wrote:
>>> On 6/5/19 11:11 PM, Shawn Anastasio wrote:
>>>> On 5/30/19 2:03 AM, Alexey Kardashevskiy wrote:
>>>>> This is an attempt to allow DMA masks between 32..59 which are not
>>>>> large
>>>>> enough to use either a PHB3 bypass mode or a sketchy bypass. Depending
>>>>> on the max order, up to 40 is usually available.
>>>>>
>>>>>
>>>>> This is based on v5.2-rc2.
>>>>>
>>>>> Please comment. Thanks.
>>>>
>>>> I have tested this patch set with an AMD GPU that's limited to <64bit
>>>> DMA (I believe it's 40 or 42 bit). It successfully allows the card to
>>>> operate without falling back to 32-bit DMA mode as it does without
>>>> the patches.
>>>>
>>>> Relevant kernel log message:
>>>> ```
>>>> [ 0.311211] pci 0033:01 : [PE# 00] Enabling 64-bit DMA bypass
>>>> ```
>>>>
>>>> Tested-by: Shawn Anastasio <shawn@anastas.io>
>>>
>>> After a few days of further testing, I've started to run into stability
>>> issues with the patch applied and used with an AMD GPU. Specifically,
>>> the system sometimes spontaneously crashes. Not just EEH errors either,
>>> the whole system shuts down in what looks like a checkstop.
>>>
>>> Perhaps some subtle corruption is occurring?
>>
>> Have you tried this?
>>
>> https://patchwork.ozlabs.org/patch/1113506/
>
> I have not. I'll give it a shot and try it out for a few days to see
> if I'm able to reproduce the crashes.
A few days later and I was able to reproduce the checkstop while
watching a video in mpv. At this point the system had ~4 day
uptime and this wasn't the first video I watched during that time.
This is with https://patchwork.ozlabs.org/patch/1113506/ applied, too.
^ permalink raw reply
* [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
patch 1-3: Code refactor
patch 4: Speedup disabling breakpoint
patch 5: Fix length calculation for unaligned targets
Ravi Bangoria (5):
Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
Powerpc/hw-breakpoint: Refactor set_dawr()
Powerpc/hw-breakpoint: Optimize disable path
Powerpc/Watchpoint: Fix length calculation for unaligned target
arch/powerpc/include/asm/hw_breakpoint.h | 10 ++--
arch/powerpc/kernel/hw_breakpoint.c | 56 ++++++++++++----------
arch/powerpc/kernel/process.c | 61 +++++++++++++++++++-----
arch/powerpc/kernel/ptrace.c | 2 +-
4 files changed, 86 insertions(+), 43 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
do_dabr() was renamed with do_break() long ago. But I still see
some comments mentioning do_dabr(). Replace it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/kernel/hw_breakpoint.c | 2 +-
arch/powerpc/kernel/ptrace.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index a293a53b4365..1908e4fcc132 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -232,7 +232,7 @@ int hw_breakpoint_handler(struct die_args *args)
* Return early after invoking user-callback function without restoring
* DABR if the breakpoint is from ptrace which always operates in
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
- * generated in do_dabr().
+ * generated in do_break().
*/
if (bp->overflow_handler == ptrace_triggered) {
perf_bp_event(bp, regs);
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 684b0b315c32..44b823e5e8c8 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2373,7 +2373,7 @@ void ptrace_triggered(struct perf_event *bp,
/*
* Disable the breakpoint request here since ptrace has defined a
* one-shot behaviour for breakpoint exceptions in PPC64.
- * The SIGTRAP signal is generated automatically for us in do_dabr().
+ * The SIGTRAP signal is generated automatically for us in do_break().
* We don't have to do anything about that here
*/
attr = bp->attr;
--
2.20.1
^ permalink raw reply related
* [PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
Move feature availability check at the start of the function.
Rearrange comment to it's associated code. Use hw->address and
hw->len in the 512 bytes boundary check(to write if statement
in a single line). Add spacing between code blocks.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/kernel/hw_breakpoint.c | 34 +++++++++++++++--------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 1908e4fcc132..36bcf705df65 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -133,10 +133,13 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
const struct perf_event_attr *attr,
struct arch_hw_breakpoint *hw)
{
- int ret = -EINVAL, length_max;
+ int length_max;
+
+ if (!ppc_breakpoint_available())
+ return -ENODEV;
if (!bp)
- return ret;
+ return -EINVAL;
hw->type = HW_BRK_TYPE_TRANSLATE;
if (attr->bp_type & HW_BREAKPOINT_R)
@@ -145,34 +148,33 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
hw->type |= HW_BRK_TYPE_WRITE;
if (hw->type == HW_BRK_TYPE_TRANSLATE)
/* must set alteast read or write */
- return ret;
+ return -EINVAL;
+
if (!attr->exclude_user)
hw->type |= HW_BRK_TYPE_USER;
if (!attr->exclude_kernel)
hw->type |= HW_BRK_TYPE_KERNEL;
if (!attr->exclude_hv)
hw->type |= HW_BRK_TYPE_HYP;
+
hw->address = attr->bp_addr;
hw->len = attr->bp_len;
- /*
- * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
- * and breakpoint addresses are aligned to nearest double-word
- * HW_BREAKPOINT_ALIGN by rounding off to the lower address, the
- * 'symbolsize' should satisfy the check below.
- */
- if (!ppc_breakpoint_available())
- return -ENODEV;
length_max = 8; /* DABR */
if (dawr_enabled()) {
length_max = 512 ; /* 64 doublewords */
- /* DAWR region can't cross 512 boundary */
- if ((attr->bp_addr >> 9) !=
- ((attr->bp_addr + attr->bp_len - 1) >> 9))
+ /* DAWR region can't cross 512 bytes boundary */
+ if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
return -EINVAL;
}
- if (hw->len >
- (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
+
+ /*
+ * Since breakpoint length can be a maximum of length_max and
+ * breakpoint addresses are aligned to nearest double-word
+ * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
+ * the 'symbolsize' should satisfy the check below.
+ */
+ if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
return -EINVAL;
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
Remove unnecessary comments. Code itself is self explanatory.
And, ISA already talks about MRD field. I Don't think we need
to re-describe it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/kernel/process.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f0fbbf6a6a1f..f002d2ffff86 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -799,18 +799,11 @@ int set_dawr(struct arch_hw_breakpoint *brk)
dawr = brk->address;
- dawrx = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
- << (63 - 58); //* read/write bits */
- dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
- << (63 - 59); //* translate */
- dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
- >> 3; //* PRIM bits */
- /* dawr length is stored in field MDR bits 48:53. Matches range in
- doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
- 0b111111=64DW.
- brk->len is in bytes.
- This aligns up to double word size, shifts and does the bias.
- */
+ dawrx = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
+ dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
+ dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
+
+ /* brk->len is in bytes. */
mrd = ((brk->len + 7) >> 3) - 1;
dawrx |= (mrd & 0x3f) << (63 - 53);
--
2.20.1
^ permalink raw reply related
* [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
Directly setting dawr and dawrx with 0 should be enough to
disable watchpoint. No need to reset individual bits in
variable and then set in hw.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/include/asm/hw_breakpoint.h | 3 ++-
arch/powerpc/kernel/process.c | 12 ++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 78202d5fb13a..8acbbdd4a2d5 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -19,6 +19,7 @@ struct arch_hw_breakpoint {
/* Note: Don't change the the first 6 bits below as they are in the same order
* as the dabr and dabrx.
*/
+#define HW_BRK_TYPE_DISABLE 0x00
#define HW_BRK_TYPE_READ 0x01
#define HW_BRK_TYPE_WRITE 0x02
#define HW_BRK_TYPE_TRANSLATE 0x04
@@ -68,7 +69,7 @@ static inline void hw_breakpoint_disable(void)
struct arch_hw_breakpoint brk;
brk.address = 0;
- brk.type = 0;
+ brk.type = HW_BRK_TYPE_DISABLE;
brk.len = 0;
if (ppc_breakpoint_available())
__set_breakpoint(&brk);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f002d2ffff86..265fac9fb3a4 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -793,10 +793,22 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
return __set_dabr(dabr, dabrx);
}
+static int disable_dawr(void)
+{
+ if (ppc_md.set_dawr)
+ return ppc_md.set_dawr(0, 0);
+
+ mtspr(SPRN_DAWRX, 0);
+ return 0;
+}
+
int set_dawr(struct arch_hw_breakpoint *brk)
{
unsigned long dawr, dawrx, mrd;
+ if (brk->type == HW_BRK_TYPE_DISABLE)
+ return disable_dawr();
+
dawr = brk->address;
dawrx = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
--
2.20.1
^ permalink raw reply related
* [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target
From: Ravi Bangoria @ 2019-06-18 4:27 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, mikey, linux-kernel, npiggin, paulus, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
Watchpoint match range is always doubleword(8 bytes) aligned on
powerpc. If the given range is crossing doubleword boundary, we
need to increase the length such that next doubleword also get
covered. Ex,
address len = 6 bytes
|=========.
|------------v--|------v--------|
| | | | | | | | | | | | | | | | |
|---------------|---------------|
<---8 bytes--->
In such case, current code configures hw as:
start_addr = address & ~HW_BREAKPOINT_ALIGN
len = 8 bytes
And thus read/write in last 4 bytes of the given range is ignored.
Fix this by including next doubleword in the length. Watchpoint
exception handler already ignores extraneous exceptions, so no
changes required for that.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/include/asm/hw_breakpoint.h | 7 ++--
arch/powerpc/kernel/hw_breakpoint.c | 44 +++++++++++++-----------
arch/powerpc/kernel/process.c | 34 ++++++++++++++++--
3 files changed, 60 insertions(+), 25 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 8acbbdd4a2d5..749a357164d5 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -34,6 +34,8 @@ struct arch_hw_breakpoint {
#define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
HW_BRK_TYPE_HYP)
+#define HW_BREAKPOINT_ALIGN 0x7
+
#ifdef CONFIG_HAVE_HW_BREAKPOINT
#include <linux/kdebug.h>
#include <asm/reg.h>
@@ -45,8 +47,6 @@ struct pmu;
struct perf_sample_data;
struct task_struct;
-#define HW_BREAKPOINT_ALIGN 0x7
-
extern int hw_breakpoint_slots(int type);
extern int arch_bp_generic_fields(int type, int *gen_bp_type);
extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
@@ -76,7 +76,8 @@ static inline void hw_breakpoint_disable(void)
}
extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
int hw_breakpoint_handler(struct die_args *args);
-
+extern u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
+ unsigned long *start_addr, unsigned long *end_addr);
extern int set_dawr(struct arch_hw_breakpoint *brk);
extern bool dawr_force_enable;
static inline bool dawr_enabled(void)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 36bcf705df65..c122fd55aa44 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -126,6 +126,28 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
return 0;
}
+/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
+static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
+{
+ u16 length_max = 8;
+ u16 final_len;
+ unsigned long start_addr, end_addr;
+
+ final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
+
+ if (dawr_enabled()) {
+ length_max = 512;
+ /* DAWR region can't cross 512 bytes boundary */
+ if ((start_addr >> 9) != (end_addr >> 9))
+ return -EINVAL;
+ }
+
+ if (final_len > length_max)
+ return -EINVAL;
+
+ return 0;
+}
+
/*
* Validate the arch-specific HW Breakpoint register settings
*/
@@ -133,12 +155,10 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
const struct perf_event_attr *attr,
struct arch_hw_breakpoint *hw)
{
- int length_max;
-
if (!ppc_breakpoint_available())
return -ENODEV;
- if (!bp)
+ if (!bp || !attr->bp_len)
return -EINVAL;
hw->type = HW_BRK_TYPE_TRANSLATE;
@@ -160,23 +180,7 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
hw->address = attr->bp_addr;
hw->len = attr->bp_len;
- length_max = 8; /* DABR */
- if (dawr_enabled()) {
- length_max = 512 ; /* 64 doublewords */
- /* DAWR region can't cross 512 bytes boundary */
- if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
- return -EINVAL;
- }
-
- /*
- * Since breakpoint length can be a maximum of length_max and
- * breakpoint addresses are aligned to nearest double-word
- * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
- * the 'symbolsize' should satisfy the check below.
- */
- if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
- return -EINVAL;
- return 0;
+ return hw_breakpoint_validate_len(hw);
}
/*
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 265fac9fb3a4..159aaa70de46 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -802,9 +802,39 @@ static int disable_dawr(void)
return 0;
}
+/*
+ * Watchpoint match range is always doubleword(8 bytes) aligned on
+ * powerpc. If the given range is crossing doubleword boundary, we
+ * need to increase the length such that next doubleword also get
+ * covered. Ex,
+ *
+ * address len = 6 bytes
+ * |=========.
+ * |------------v--|------v--------|
+ * | | | | | | | | | | | | | | | | |
+ * |---------------|---------------|
+ * <---8 bytes--->
+ *
+ * In this case, we should configure hw as:
+ * start_addr = address & ~HW_BREAKPOINT_ALIGN
+ * len = 16 bytes
+ *
+ * @start_addr and @end_addr are inclusive.
+ */
+u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
+ unsigned long *start_addr,
+ unsigned long *end_addr)
+{
+ *start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
+ *end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
+ return *end_addr - *start_addr + 1;
+}
+
int set_dawr(struct arch_hw_breakpoint *brk)
{
unsigned long dawr, dawrx, mrd;
+ unsigned long start_addr, end_addr;
+ u16 final_len;
if (brk->type == HW_BRK_TYPE_DISABLE)
return disable_dawr();
@@ -815,8 +845,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
- /* brk->len is in bytes. */
- mrd = ((brk->len + 7) >> 3) - 1;
+ final_len = hw_breakpoint_get_final_len(brk, &start_addr, &end_addr);
+ mrd = ((final_len + 7) >> 3) - 1;
dawrx |= (mrd & 0x3f) << (63 - 53);
if (ppc_md.set_dawr)
--
2.20.1
^ permalink raw reply related
* [PATCH] powerpc/mm: Ensure Huge-page memory is free before allocation
From: Vaibhav Jain @ 2019-06-18 4:46 UTC (permalink / raw)
To: linuxppc-dev
Cc: Hari Bathini, Vaibhav Jain, Mahesh J Salgaonkar, Aneesh Kumar K.V
We recently discovered an bug where physical memory meant for
allocation of Huge-pages was inadvertently allocated by another component
during early boot. The behavior of memblock_reserve() where it wont
indicate whether an existing reserved block overlaps with the
requested reservation only makes such bugs hard to investigate.
Hence this patch proposes adding a memblock reservation check in
htab_dt_scan_hugepage_blocks() just before call to memblock_reserve()
to ensure that the physical memory thats being reserved for is not
already reserved by someone else. In case this happens we panic the
the kernel to ensure that user of this huge-page doesn't accidentally
stomp on memory allocated to someone else.
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/mm/book3s64/hash_utils.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 28ced26f2a00..a05be3adb8c9 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -516,6 +516,11 @@ static int __init htab_dt_scan_hugepage_blocks(unsigned long node,
printk(KERN_INFO "Huge page(16GB) memory: "
"addr = 0x%lX size = 0x%lX pages = %d\n",
phys_addr, block_size, expected_pages);
+
+ /* Ensure no one else has reserved memory for huge pages before */
+ BUG_ON(memblock_is_region_reserved(phys_addr,
+ block_size * expected_pages));
+
if (phys_addr + block_size * expected_pages <= memblock_end_of_DRAM()) {
memblock_reserve(phys_addr, block_size * expected_pages);
pseries_add_gpage(phys_addr, block_size, expected_pages);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor
From: Christophe Leroy @ 2019-06-18 6:01 UTC (permalink / raw)
To: Ravi Bangoria, mpe
Cc: mikey, linux-kernel, npiggin, paulus, naveen.n.rao, linuxppc-dev
In-Reply-To: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> patch 1-3: Code refactor
> patch 4: Speedup disabling breakpoint
> patch 5: Fix length calculation for unaligned targets
While you are playing with hw breakpoints, did you have a look at
https://github.com/linuxppc/issues/issues/38 ?
Christophe
>
> Ravi Bangoria (5):
> Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
> Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
> Powerpc/hw-breakpoint: Refactor set_dawr()
> Powerpc/hw-breakpoint: Optimize disable path
> Powerpc/Watchpoint: Fix length calculation for unaligned target
>
> arch/powerpc/include/asm/hw_breakpoint.h | 10 ++--
> arch/powerpc/kernel/hw_breakpoint.c | 56 ++++++++++++----------
> arch/powerpc/kernel/process.c | 61 +++++++++++++++++++-----
> arch/powerpc/kernel/ptrace.c | 2 +-
> 4 files changed, 86 insertions(+), 43 deletions(-)
>
^ permalink raw reply
* Re: [PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
From: Michael Neuling @ 2019-06-18 6:02 UTC (permalink / raw)
To: Ravi Bangoria, mpe
Cc: linux-kernel, npiggin, paulus, naveen.n.rao, linuxppc-dev
In-Reply-To: <20190618042732.5582-2-ravi.bangoria@linux.ibm.com>
> Subject: Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
Can you add the word "comment" to this subject. Currently it implies there are
code changes here.
Mikey
On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> do_dabr() was renamed with do_break() long ago. But I still see
> some comments mentioning do_dabr(). Replace it.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> arch/powerpc/kernel/hw_breakpoint.c | 2 +-
> arch/powerpc/kernel/ptrace.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c
> b/arch/powerpc/kernel/hw_breakpoint.c
> index a293a53b4365..1908e4fcc132 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -232,7 +232,7 @@ int hw_breakpoint_handler(struct die_args *args)
> * Return early after invoking user-callback function without restoring
> * DABR if the breakpoint is from ptrace which always operates in
> * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
> - * generated in do_dabr().
> + * generated in do_break().
> */
> if (bp->overflow_handler == ptrace_triggered) {
> perf_bp_event(bp, regs);
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 684b0b315c32..44b823e5e8c8 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -2373,7 +2373,7 @@ void ptrace_triggered(struct perf_event *bp,
> /*
> * Disable the breakpoint request here since ptrace has defined a
> * one-shot behaviour for breakpoint exceptions in PPC64.
> - * The SIGTRAP signal is generated automatically for us in do_dabr().
> + * The SIGTRAP signal is generated automatically for us in do_break().
> * We don't have to do anything about that here
> */
> attr = bp->attr;
^ permalink raw reply
* Re: [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()
From: Michael Neuling @ 2019-06-18 6:11 UTC (permalink / raw)
To: Ravi Bangoria, mpe
Cc: linux-kernel, npiggin, paulus, naveen.n.rao, linuxppc-dev
In-Reply-To: <20190618042732.5582-4-ravi.bangoria@linux.ibm.com>
This is going to collide with this patch
https://patchwork.ozlabs.org/patch/1109594/
Mikey
On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> Remove unnecessary comments. Code itself is self explanatory.
> And, ISA already talks about MRD field. I Don't think we need
> to re-describe it.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> arch/powerpc/kernel/process.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f0fbbf6a6a1f..f002d2ffff86 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -799,18 +799,11 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>
> dawr = brk->address;
>
> - dawrx = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
> - << (63 - 58); //* read/write bits */
> - dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
> - << (63 - 59); //* translate */
> - dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
> - >> 3; //* PRIM bits */
> - /* dawr length is stored in field MDR bits 48:53. Matches range in
> - doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
> - 0b111111=64DW.
> - brk->len is in bytes.
> - This aligns up to double word size, shifts and does the bias.
> - */
> + dawrx = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
> + dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
> + dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
> +
> + /* brk->len is in bytes. */
> mrd = ((brk->len + 7) >> 3) - 1;
> dawrx |= (mrd & 0x3f) << (63 - 53);
>
^ 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