[PATCH v2] CXL: Fix PSL error due to duplicate segment table entries

[PATCH v2] CXL: Fix PSL error due to duplicate segment table entries

[Ian Munsie]

In certain circumstances the PSL (Power Service Layer, which provides
translation services for CXL hardware) can send an interrupt for a segment miss
that the kernel has already handled. This can happen if multiple translations
for the same segment are queued in the PSL before the kernel has restarted the
first translation.

The CXL driver does not expect this situation and does not check if a segment
had already been handled. This could cause a duplicate segment table entry
which in turn caused a PSL error taking down the card.

This patch series fixes the issue by checking for existing entries in the
segment table that match the segment it is trying to insert to avoid inserting
duplicate entries.

Some of the code has been refactored to simplify it - the segment table
hash has been moved from cxl_load_segment to find_free_sste where it is
used and we have disabled the secondary hash in the segment table to
reduce the number of entries that need to be tested from 16 to 8. Due to
the large segment sizes we use it is extremely unlikely that the
secondary hash would ever have been used in practice, so this should not
have any negative impacts and may even improve performance.

copro_calculate_slb didn't use the correct ESID mask for 1T vs 256M segments,
which was not a problem as the extra bits were ignored. This series fixes it to
use the correct mask to make debugging easier and so that we can directly
compare the ESID values for duplicates without needing to worry about masking
in the comparison.

- Patch 1 disables the secondary hash in the segment table to simplify the code.

- Patch 2 cleans up and refactors cxl_load_segment and find_free_sste to move
  the hash calculation to where it is actually used.

- Patch 3 fixes the ESID returned by copro_calculate_slb to be properly masked
  based on the segment size.

- Patch 4 prevents duplicate segment table entries from being inserted to fix
  PSL errors resulting from this situation.

Changes since v1:
- Split patch out into separate patches for cleanups and bug fix

[PATCH v2 1/4] CXL: Disable secondary hash in segment table

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

This patch simplifies the process of finding a free segment table entry
by disabling the secondary hash. This reduces the number of possible
entries in the segment table for a given address from 16 to 8.

Due to the large segment sizes we use it is extremely unlikely that the
secondary hash would ever have been used in practice, so this should not
have any negative impacts and may even improve performance due to the
reduced number of comparisons that software & hardware need to perform.

This patch clears the SC bit in the hardware's state register
(CXL_PSL_SR_An) to disable the secondary hash in the hardware since we
can no longer fill out entries using it.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/fault.c  | 30 ++++++++----------------------
 drivers/misc/cxl/native.c |  4 ++--
 2 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index 69506eb..d0e97fd 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c
@@ -22,29 +22,19 @@
 #include "cxl.h"
 
 static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
-				       bool sec_hash,
-				       struct cxl_sste *secondary_group,
 				       unsigned int *lru)
 {
-	unsigned int i, entry;
+	unsigned int entry;
 	struct cxl_sste *sste, *group = primary_group;
 
-	for (i = 0; i < 2; i++) {
-		for (entry = 0; entry < 8; entry++) {
-			sste = group + entry;
-			if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
-				return sste;
-		}
-		if (!sec_hash)
-			break;
-		group = secondary_group;
+	for (entry = 0; entry < 8; entry++) {
+		sste = group + entry;
+		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
+			return sste;
 	}
 	/* Nothing free, select an entry to cast out */
-	if (sec_hash && (*lru & 0x8))
-		sste = secondary_group + (*lru & 0x7);
-	else
-		sste = primary_group + (*lru & 0x7);
-	*lru = (*lru + 1) & 0xf;
+	sste = primary_group + *lru;
+	*lru = (*lru + 1) & 0x7;
 
 	return sste;
 }
@@ -53,22 +43,18 @@ static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
 {
 	/* mask is the group index, we search primary and secondary here. */
 	unsigned int mask = (ctx->sst_size >> 7)-1; /* SSTP0[SegTableSize] */
-	bool sec_hash = 1;
 	struct cxl_sste *sste;
 	unsigned int hash;
 	unsigned long flags;
 
 
-	sec_hash = !!(cxl_p1n_read(ctx->afu, CXL_PSL_SR_An) & CXL_PSL_SR_An_SC);
-
 	if (slb->vsid & SLB_VSID_B_1T)
 		hash = (slb->esid >> SID_SHIFT_1T) & mask;
 	else /* 256M */
 		hash = (slb->esid >> SID_SHIFT) & mask;
 
 	spin_lock_irqsave(&ctx->sste_lock, flags);
-	sste = find_free_sste(ctx->sstp + (hash << 3), sec_hash,
-			      ctx->sstp + ((~hash & mask) << 3), &ctx->sst_lru);
+	sste = find_free_sste(ctx->sstp + (hash << 3), &ctx->sst_lru);
 
 	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
 			sste - ctx->sstp, slb->vsid, slb->esid);
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 623286a..d47532e 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -417,7 +417,7 @@ static int attach_afu_directed(struct cxl_context *ctx, u64 wed, u64 amr)
 	ctx->elem->haurp = 0; /* disable */
 	ctx->elem->sdr = cpu_to_be64(mfspr(SPRN_SDR1));
 
-	sr = CXL_PSL_SR_An_SC;
+	sr = 0;
 	if (ctx->master)
 		sr |= CXL_PSL_SR_An_MP;
 	if (mfspr(SPRN_LPCR) & LPCR_TC)
@@ -508,7 +508,7 @@ static int attach_dedicated(struct cxl_context *ctx, u64 wed, u64 amr)
 	u64 sr;
 	int rc;
 
-	sr = CXL_PSL_SR_An_SC;
+	sr = 0;
 	set_endian(sr);
 	if (ctx->master)
 		sr |= CXL_PSL_SR_An_MP;
-- 
2.1.1

[PATCH v2 2/4] CXL: Refactor cxl_load_segment and find_free_sste

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

This moves the segment table hash calculation from cxl_load_segment into
find_free_sste since that is the only place it is actually used.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/fault.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index d0e97fd..cb4f323 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c
@@ -21,20 +21,30 @@
 
 #include "cxl.h"
 
-static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
-				       unsigned int *lru)
+/* This finds a free SSTE for the given SLB */
+static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
+				       struct copro_slb *slb)
 {
+	struct cxl_sste *primary, *sste;
+	unsigned int mask = (ctx->sst_size >> 7) - 1; /* SSTP0[SegTableSize] */
 	unsigned int entry;
-	struct cxl_sste *sste, *group = primary_group;
+	unsigned int hash;
+
+	if (slb->vsid & SLB_VSID_B_1T)
+		hash = (slb->esid >> SID_SHIFT_1T) & mask;
+	else /* 256M */
+		hash = (slb->esid >> SID_SHIFT) & mask;
 
-	for (entry = 0; entry < 8; entry++) {
-		sste = group + entry;
+	primary = ctx->sstp + (hash << 3);
+
+	for (entry = 0, sste = primary; entry < 8; entry++, sste++) {
 		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
 			return sste;
 	}
+
 	/* Nothing free, select an entry to cast out */
-	sste = primary_group + *lru;
-	*lru = (*lru + 1) & 0x7;
+	sste = primary + ctx->sst_lru;
+	ctx->sst_lru = (ctx->sst_lru + 1) & 0x7;
 
 	return sste;
 }
@@ -42,19 +52,11 @@ static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
 static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
 {
 	/* mask is the group index, we search primary and secondary here. */
-	unsigned int mask = (ctx->sst_size >> 7)-1; /* SSTP0[SegTableSize] */
 	struct cxl_sste *sste;
-	unsigned int hash;
 	unsigned long flags;
 
-
-	if (slb->vsid & SLB_VSID_B_1T)
-		hash = (slb->esid >> SID_SHIFT_1T) & mask;
-	else /* 256M */
-		hash = (slb->esid >> SID_SHIFT) & mask;
-
 	spin_lock_irqsave(&ctx->sste_lock, flags);
-	sste = find_free_sste(ctx->sstp + (hash << 3), &ctx->sst_lru);
+	sste = find_free_sste(ctx, slb);
 
 	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
 			sste - ctx->sstp, slb->vsid, slb->esid);
-- 
2.1.1

[PATCH v2 3/4] powerpc/copro: Use appropriate ESID mask in copro_calculate_slb

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

This patch makes copro_calculate_slb mask the ESID by the correct mask
for 1T vs 256M segments.

This has no effect by itself as the extra bits were ignored, but it
makes debugging the segment table entries easier and means that we can
directly compare the ESID values for duplicates without needing to worry
about masking in the comparison.

This will be used to simplify a comparison in the following patch.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 arch/powerpc/mm/copro_fault.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/copro_fault.c b/arch/powerpc/mm/copro_fault.c
index 0f9939e..5a236f0 100644
--- a/arch/powerpc/mm/copro_fault.c
+++ b/arch/powerpc/mm/copro_fault.c
@@ -99,8 +99,6 @@ int copro_calculate_slb(struct mm_struct *mm, u64 ea, struct copro_slb *slb)
 	u64 vsid;
 	int psize, ssize;
 
-	slb->esid = (ea & ESID_MASK) | SLB_ESID_V;
-
 	switch (REGION_ID(ea)) {
 	case USER_REGION_ID:
 		pr_devel("%s: 0x%llx -- USER_REGION_ID\n", __func__, ea);
@@ -133,6 +131,7 @@ int copro_calculate_slb(struct mm_struct *mm, u64 ea, struct copro_slb *slb)
 	vsid |= mmu_psize_defs[psize].sllp |
 		((ssize == MMU_SEGSIZE_1T) ? SLB_VSID_B_1T : 0);
 
+	slb->esid = (ea & (ssize == MMU_SEGSIZE_1T ? ESID_MASK_1T : ESID_MASK)) | SLB_ESID_V;
 	slb->vsid = vsid;
 
 	return 0;
-- 
2.1.1

[PATCH v2 4/4] CXL: Fix PSL error due to duplicate segment table entries

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

In certain circumstances the PSL (Power Service Layer, which provides
translation services for CXL hardware) can send an interrupt for a
segment miss that the kernel has already handled. This can happen if
multiple translations for the same segment are queued in the PSL before
the kernel has restarted the first translation.

The CXL driver does not expect this situation and does not check if a
segment had already been handled. This could cause a duplicate segment
table entry which in turn caused a PSL error taking down the card.

This patch fixes the issue by checking for existing entries in the
segment table that match the segment it is trying to insert to avoid
inserting duplicate entries.

This patch requires "powerpc/copro: Use appropriate ESID mask in
copro_calculate_slb" for the sste_matches function to operate correctly.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/fault.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index cb4f323..c99e896 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c
@@ -21,11 +21,20 @@
 
 #include "cxl.h"
 
-/* This finds a free SSTE for the given SLB */
+static bool sste_matches(struct cxl_sste *sste, struct copro_slb *slb)
+{
+	return ((sste->vsid_data == cpu_to_be64(slb->vsid)) &&
+		(sste->esid_data == cpu_to_be64(slb->esid)));
+}
+
+/*
+ * This finds a free SSTE for the given SLB, or returns NULL if it's already in
+ * the segment table.
+ */
 static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
 				       struct copro_slb *slb)
 {
-	struct cxl_sste *primary, *sste;
+	struct cxl_sste *primary, *sste, *ret = NULL;
 	unsigned int mask = (ctx->sst_size >> 7) - 1; /* SSTP0[SegTableSize] */
 	unsigned int entry;
 	unsigned int hash;
@@ -38,15 +47,19 @@ static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
 	primary = ctx->sstp + (hash << 3);
 
 	for (entry = 0, sste = primary; entry < 8; entry++, sste++) {
-		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
-			return sste;
+		if (!ret && !(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
+			ret = sste;
+		if (sste_matches(sste, slb))
+			return NULL;
 	}
+	if (ret)
+		return ret;
 
 	/* Nothing free, select an entry to cast out */
-	sste = primary + ctx->sst_lru;
+	ret = primary + ctx->sst_lru;
 	ctx->sst_lru = (ctx->sst_lru + 1) & 0x7;
 
-	return sste;
+	return ret;
 }
 
 static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
@@ -57,12 +70,15 @@ static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
 
 	spin_lock_irqsave(&ctx->sste_lock, flags);
 	sste = find_free_sste(ctx, slb);
+	if (!sste)
+		goto out_unlock;
 
 	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
 			sste - ctx->sstp, slb->vsid, slb->esid);
 
 	sste->vsid_data = cpu_to_be64(slb->vsid);
 	sste->esid_data = cpu_to_be64(slb->esid);
+out_unlock:
 	spin_unlock_irqrestore(&ctx->sste_lock, flags);
 }
 
-- 
2.1.1

Re: [PATCH v2 1/4] CXL: Disable secondary hash in segment table

[Aneesh Kumar K.V]

Ian Munsie <imunsie@au1.ibm.com> writes:

> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This patch simplifies the process of finding a free segment table entry
> by disabling the secondary hash. This reduces the number of possible
> entries in the segment table for a given address from 16 to 8.
>
> Due to the large segment sizes we use it is extremely unlikely that the
> secondary hash would ever have been used in practice, so this should not
> have any negative impacts and may even improve performance due to the
> reduced number of comparisons that software & hardware need to perform.
>
> This patch clears the SC bit in the hardware's state register
> (CXL_PSL_SR_An) to disable the secondary hash in the hardware since we
> can no longer fill out entries using it.

This could have also gone as a code comment.


>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> ---
>  drivers/misc/cxl/fault.c  | 30 ++++++++----------------------
>  drivers/misc/cxl/native.c |  4 ++--
>  2 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
> index 69506eb..d0e97fd 100644
> --- a/drivers/misc/cxl/fault.c
> +++ b/drivers/misc/cxl/fault.c
> @@ -22,29 +22,19 @@
>  #include "cxl.h"
>  
>  static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
> -				       bool sec_hash,
> -				       struct cxl_sste *secondary_group,
>  				       unsigned int *lru)
>  {
> -	unsigned int i, entry;
> +	unsigned int entry;
>  	struct cxl_sste *sste, *group = primary_group;
>  
> -	for (i = 0; i < 2; i++) {
> -		for (entry = 0; entry < 8; entry++) {
> -			sste = group + entry;
> -			if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
> -				return sste;
> -		}
> -		if (!sec_hash)
> -			break;
> -		group = secondary_group;
> +	for (entry = 0; entry < 8; entry++) {
> +		sste = group + entry;
> +		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
> +			return sste;
>  	}
>  	/* Nothing free, select an entry to cast out */
> -	if (sec_hash && (*lru & 0x8))
> -		sste = secondary_group + (*lru & 0x7);
> -	else
> -		sste = primary_group + (*lru & 0x7);
> -	*lru = (*lru + 1) & 0xf;
> +	sste = primary_group + *lru;
> +	*lru = (*lru + 1) & 0x7;
>  
>  	return sste;
>  }
> @@ -53,22 +43,18 @@ static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
>  {
>  	/* mask is the group index, we search primary and secondary here. */
>  	unsigned int mask = (ctx->sst_size >> 7)-1; /* SSTP0[SegTableSize] */
> -	bool sec_hash = 1;
>  	struct cxl_sste *sste;
>  	unsigned int hash;
>  	unsigned long flags;
>  
>  
> -	sec_hash = !!(cxl_p1n_read(ctx->afu, CXL_PSL_SR_An) & CXL_PSL_SR_An_SC);
> -
>  	if (slb->vsid & SLB_VSID_B_1T)
>  		hash = (slb->esid >> SID_SHIFT_1T) & mask;
>  	else /* 256M */
>  		hash = (slb->esid >> SID_SHIFT) & mask;
>  
>  	spin_lock_irqsave(&ctx->sste_lock, flags);
> -	sste = find_free_sste(ctx->sstp + (hash << 3), sec_hash,
> -			      ctx->sstp + ((~hash & mask) << 3), &ctx->sst_lru);
> +	sste = find_free_sste(ctx->sstp + (hash << 3), &ctx->sst_lru);
>  
>  	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
>  			sste - ctx->sstp, slb->vsid, slb->esid);
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 623286a..d47532e 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -417,7 +417,7 @@ static int attach_afu_directed(struct cxl_context *ctx, u64 wed, u64 amr)
>  	ctx->elem->haurp = 0; /* disable */
>  	ctx->elem->sdr = cpu_to_be64(mfspr(SPRN_SDR1));
>  
> -	sr = CXL_PSL_SR_An_SC;
> +	sr = 0;
>  	if (ctx->master)
>  		sr |= CXL_PSL_SR_An_MP;
>  	if (mfspr(SPRN_LPCR) & LPCR_TC)
> @@ -508,7 +508,7 @@ static int attach_dedicated(struct cxl_context *ctx, u64 wed, u64 amr)
>  	u64 sr;
>  	int rc;
>  
> -	sr = CXL_PSL_SR_An_SC;
> +	sr = 0;
>  	set_endian(sr);
>  	if (ctx->master)
>  		sr |= CXL_PSL_SR_An_MP;
> -- 
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] CXL: Refactor cxl_load_segment and find_free_sste

[Aneesh Kumar K.V]

Ian Munsie <imunsie@au1.ibm.com> writes:

> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This moves the segment table hash calculation from cxl_load_segment into
> find_free_sste since that is the only place it is actually used.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>


Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> ---
>  drivers/misc/cxl/fault.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
> index d0e97fd..cb4f323 100644
> --- a/drivers/misc/cxl/fault.c
> +++ b/drivers/misc/cxl/fault.c
> @@ -21,20 +21,30 @@
>  
>  #include "cxl.h"
>  
> -static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
> -				       unsigned int *lru)
> +/* This finds a free SSTE for the given SLB */
> +static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
> +				       struct copro_slb *slb)
>  {
> +	struct cxl_sste *primary, *sste;
> +	unsigned int mask = (ctx->sst_size >> 7) - 1; /* SSTP0[SegTableSize] */
>  	unsigned int entry;
> -	struct cxl_sste *sste, *group = primary_group;
> +	unsigned int hash;
> +
> +	if (slb->vsid & SLB_VSID_B_1T)
> +		hash = (slb->esid >> SID_SHIFT_1T) & mask;
> +	else /* 256M */
> +		hash = (slb->esid >> SID_SHIFT) & mask;
>  
> -	for (entry = 0; entry < 8; entry++) {
> -		sste = group + entry;
> +	primary = ctx->sstp + (hash << 3);
> +
> +	for (entry = 0, sste = primary; entry < 8; entry++, sste++) {
>  		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
>  			return sste;
>  	}
> +
>  	/* Nothing free, select an entry to cast out */
> -	sste = primary_group + *lru;
> -	*lru = (*lru + 1) & 0x7;
> +	sste = primary + ctx->sst_lru;
> +	ctx->sst_lru = (ctx->sst_lru + 1) & 0x7;
>  
>  	return sste;
>  }
> @@ -42,19 +52,11 @@ static struct cxl_sste* find_free_sste(struct cxl_sste *primary_group,
>  static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
>  {
>  	/* mask is the group index, we search primary and secondary here. */
> -	unsigned int mask = (ctx->sst_size >> 7)-1; /* SSTP0[SegTableSize] */
>  	struct cxl_sste *sste;
> -	unsigned int hash;
>  	unsigned long flags;
>  
> -
> -	if (slb->vsid & SLB_VSID_B_1T)
> -		hash = (slb->esid >> SID_SHIFT_1T) & mask;
> -	else /* 256M */
> -		hash = (slb->esid >> SID_SHIFT) & mask;
> -
>  	spin_lock_irqsave(&ctx->sste_lock, flags);
> -	sste = find_free_sste(ctx->sstp + (hash << 3), &ctx->sst_lru);
> +	sste = find_free_sste(ctx, slb);
>  
>  	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
>  			sste - ctx->sstp, slb->vsid, slb->esid);
> -- 
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] powerpc/copro: Use appropriate ESID mask in copro_calculate_slb

[Aneesh Kumar K.V]

Ian Munsie <imunsie@au1.ibm.com> writes:

> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This patch makes copro_calculate_slb mask the ESID by the correct mask
> for 1T vs 256M segments.
>
> This has no effect by itself as the extra bits were ignored, but it
> makes debugging the segment table entries easier and means that we can
> directly compare the ESID values for duplicates without needing to worry
> about masking in the comparison.
>
> This will be used to simplify a comparison in the following patch.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> ---
>  arch/powerpc/mm/copro_fault.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/mm/copro_fault.c b/arch/powerpc/mm/copro_fault.c
> index 0f9939e..5a236f0 100644
> --- a/arch/powerpc/mm/copro_fault.c
> +++ b/arch/powerpc/mm/copro_fault.c
> @@ -99,8 +99,6 @@ int copro_calculate_slb(struct mm_struct *mm, u64 ea, struct copro_slb *slb)
>  	u64 vsid;
>  	int psize, ssize;
>  
> -	slb->esid = (ea & ESID_MASK) | SLB_ESID_V;
> -
>  	switch (REGION_ID(ea)) {
>  	case USER_REGION_ID:
>  		pr_devel("%s: 0x%llx -- USER_REGION_ID\n", __func__, ea);
> @@ -133,6 +131,7 @@ int copro_calculate_slb(struct mm_struct *mm, u64 ea, struct copro_slb *slb)
>  	vsid |= mmu_psize_defs[psize].sllp |
>  		((ssize == MMU_SEGSIZE_1T) ? SLB_VSID_B_1T : 0);
>  
> +	slb->esid = (ea & (ssize == MMU_SEGSIZE_1T ? ESID_MASK_1T : ESID_MASK)) | SLB_ESID_V;
>  	slb->vsid = vsid;
>  
>  	return 0;
> -- 
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/4] CXL: Fix PSL error due to duplicate segment table entries

[Aneesh Kumar K.V]

Ian Munsie <imunsie@au1.ibm.com> writes:

> From: Ian Munsie <imunsie@au1.ibm.com>
>
> In certain circumstances the PSL (Power Service Layer, which provides
> translation services for CXL hardware) can send an interrupt for a
> segment miss that the kernel has already handled. This can happen if
> multiple translations for the same segment are queued in the PSL before
> the kernel has restarted the first translation.
>
> The CXL driver does not expect this situation and does not check if a
> segment had already been handled. This could cause a duplicate segment
> table entry which in turn caused a PSL error taking down the card.
>
> This patch fixes the issue by checking for existing entries in the
> segment table that match the segment it is trying to insert to avoid
> inserting duplicate entries.
>
> This patch requires "powerpc/copro: Use appropriate ESID mask in
> copro_calculate_slb" for the sste_matches function to operate correctly.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> ---
>  drivers/misc/cxl/fault.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
> index cb4f323..c99e896 100644
> --- a/drivers/misc/cxl/fault.c
> +++ b/drivers/misc/cxl/fault.c
> @@ -21,11 +21,20 @@
>  
>  #include "cxl.h"
>  
> -/* This finds a free SSTE for the given SLB */
> +static bool sste_matches(struct cxl_sste *sste, struct copro_slb
> *slb)

inline ?

> +{
> +	return ((sste->vsid_data == cpu_to_be64(slb->vsid)) &&
> +		(sste->esid_data == cpu_to_be64(slb->esid)));

why comparing converting to big endian ? The natural way is to do it
other way right ?. 

          (be64_to_cpu(sste->vsid_data) == slb->vsid)

> +}
> +
> +/*
> + * This finds a free SSTE for the given SLB, or returns NULL if it's already in
> + * the segment table.
> + */
>  static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
>  				       struct copro_slb *slb)
>  {
> -	struct cxl_sste *primary, *sste;
> +	struct cxl_sste *primary, *sste, *ret = NULL;
>  	unsigned int mask = (ctx->sst_size >> 7) - 1; /* SSTP0[SegTableSize] */
>  	unsigned int entry;
>  	unsigned int hash;
> @@ -38,15 +47,19 @@ static struct cxl_sste* find_free_sste(struct cxl_context *ctx,
>  	primary = ctx->sstp + (hash << 3);
>  
>  	for (entry = 0, sste = primary; entry < 8; entry++, sste++) {
> -		if (!(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
> -			return sste;
> +		if (!ret && !(be64_to_cpu(sste->esid_data) & SLB_ESID_V))
> +			ret = sste;
> +		if (sste_matches(sste, slb))
> +			return NULL;
>  	}
> +	if (ret)
> +		return ret;
>  
>  	/* Nothing free, select an entry to cast out */
> -	sste = primary + ctx->sst_lru;
> +	ret = primary + ctx->sst_lru;
>  	ctx->sst_lru = (ctx->sst_lru + 1) & 0x7;
>  
> -	return sste;
> +	return ret;
>  }
>  
>  static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
> @@ -57,12 +70,15 @@ static void cxl_load_segment(struct cxl_context *ctx, struct copro_slb *slb)
>  
>  	spin_lock_irqsave(&ctx->sste_lock, flags);
>  	sste = find_free_sste(ctx, slb);
> +	if (!sste)
> +		goto out_unlock;
>  
>  	pr_devel("CXL Populating SST[%li]: %#llx %#llx\n",
>  			sste - ctx->sstp, slb->vsid, slb->esid);
>  
>  	sste->vsid_data = cpu_to_be64(slb->vsid);
>  	sste->esid_data = cpu_to_be64(slb->esid);
> +out_unlock:
>  	spin_unlock_irqrestore(&ctx->sste_lock, flags);
>  }
>  
> -- 
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/