[PATCH v2] powerpc: Ignore DSI error caused by the copy/paste instruction

[PATCH v2] powerpc: Ignore DSI error caused by the copy/paste instruction

[Haren Myneni]

DSI error will be generated when the paste operation is issued on
the suspended NX window due to NX state changes. The hypervisor
expects the partition to ignore this error during page pault
handling. To differentiate DSI caused by an actual HW configuration
or by the NX window, a new “ibm,pi-features” type value is defined.
Byte 0, bit 3 of pi-attribute-specifier-type is now defined to
indicate this DSI error. If this error is not ignored, the user
space can get SIGBUS when the NX request is issued.

This patch adds changes to read ibm,pi-features property and ignore
DSI error in the page fault handling if CPU_FTR_NX_DSI if defined.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
v2: Code cleanup as suggested by Christophe Leroy 

 arch/powerpc/include/asm/cputable.h |  5 ++--
 arch/powerpc/kernel/prom.c          | 36 +++++++++++++++++++++--------
 arch/powerpc/mm/fault.c             | 17 +++++++++++++-
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index ae8c3e13cfce..8dc9949b6365 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -192,6 +192,7 @@ static inline void cpu_feature_keys_init(void) { }
 #define CPU_FTR_P9_RADIX_PREFETCH_BUG	LONG_ASM_CONST(0x0002000000000000)
 #define CPU_FTR_ARCH_31			LONG_ASM_CONST(0x0004000000000000)
 #define CPU_FTR_DAWR1			LONG_ASM_CONST(0x0008000000000000)
+#define CPU_FTR_NX_DSI			LONG_ASM_CONST(0x0010000000000000)
 
 #ifndef __ASSEMBLY__
 
@@ -429,7 +430,7 @@ static inline void cpu_feature_keys_init(void) { }
 	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
 	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
 	    CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_P9_TLBIE_STQ_BUG | \
-	    CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR)
+	    CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR | CPU_FTR_NX_DSI)
 #define CPU_FTRS_POWER9_DD2_0 (CPU_FTRS_POWER9 | CPU_FTR_P9_RADIX_PREFETCH_BUG)
 #define CPU_FTRS_POWER9_DD2_1 (CPU_FTRS_POWER9 | \
 			       CPU_FTR_P9_RADIX_PREFETCH_BUG | \
@@ -451,7 +452,7 @@ static inline void cpu_feature_keys_init(void) { }
 	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
 	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
 	    CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
-	    CPU_FTR_DAWR | CPU_FTR_DAWR1)
+	    CPU_FTR_DAWR | CPU_FTR_DAWR1 | CPU_FTR_NX_DSI)
 #define CPU_FTRS_CELL	(CPU_FTR_LWSYNC | \
 	    CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
 	    CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index a730b951b64b..19047c582e9f 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -137,7 +137,7 @@ static void __init move_device_tree(void)
 }
 
 /*
- * ibm,pa-features is a per-cpu property that contains a string of
+ * ibm,pa/pi-features is a per-cpu property that contains a string of
  * attribute descriptors, each of which has a 2 byte header plus up
  * to 254 bytes worth of processor attribute bits.  First header
  * byte specifies the number of bytes following the header.
@@ -149,15 +149,17 @@ static void __init move_device_tree(void)
  * is supported/not supported.  Note that the bit numbers are
  * big-endian to match the definition in PAPR.
  */
-static struct ibm_pa_feature {
+struct ibm_feature {
 	unsigned long	cpu_features;	/* CPU_FTR_xxx bit */
 	unsigned long	mmu_features;	/* MMU_FTR_xxx bit */
 	unsigned int	cpu_user_ftrs;	/* PPC_FEATURE_xxx bit */
 	unsigned int	cpu_user_ftrs2;	/* PPC_FEATURE2_xxx bit */
-	unsigned char	pabyte;		/* byte number in ibm,pa-features */
+	unsigned char	pabyte;		/* byte number in ibm,pa/pi-features */
 	unsigned char	pabit;		/* bit number (big-endian) */
 	unsigned char	invert;		/* if 1, pa bit set => clear feature */
-} ibm_pa_features[] __initdata = {
+};
+
+static struct ibm_feature ibm_pa_features[] __initdata = {
 	{ .pabyte = 0,  .pabit = 0, .cpu_user_ftrs = PPC_FEATURE_HAS_MMU },
 	{ .pabyte = 0,  .pabit = 1, .cpu_user_ftrs = PPC_FEATURE_HAS_FPU },
 	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
@@ -179,9 +181,19 @@ static struct ibm_pa_feature {
 	{ .pabyte = 64, .pabit = 0, .cpu_features = CPU_FTR_DAWR1 },
 };
 
+/*
+ * ibm,pi-features property provides the support of processor specific
+ * options not described in ibm,pa-features. Right now use byte 0, bit 3
+ * which indicates the occurrence of DSI interrupt when the paste operation
+ * on the suspended NX window.
+ */
+static struct ibm_feature ibm_pi_features[] __initdata = {
+	{ .pabyte = 0, .pabit = 3, .cpu_features  = CPU_FTR_NX_DSI },
+};
+
 static void __init scan_features(unsigned long node, const unsigned char *ftrs,
 				 unsigned long tablelen,
-				 struct ibm_pa_feature *fp,
+				 struct ibm_feature *fp,
 				 unsigned long ft_size)
 {
 	unsigned long i, len, bit;
@@ -218,17 +230,18 @@ static void __init scan_features(unsigned long node, const unsigned char *ftrs,
 	}
 }
 
-static void __init check_cpu_pa_features(unsigned long node)
+static void __init check_cpu_features(unsigned long node, char *name,
+				      struct ibm_feature *fp,
+				      unsigned long size)
 {
 	const unsigned char *pa_ftrs;
 	int tablelen;
 
-	pa_ftrs = of_get_flat_dt_prop(node, "ibm,pa-features", &tablelen);
+	pa_ftrs = of_get_flat_dt_prop(node, name, &tablelen);
 	if (pa_ftrs == NULL)
 		return;
 
-	scan_features(node, pa_ftrs, tablelen,
-		      ibm_pa_features, ARRAY_SIZE(ibm_pa_features));
+	scan_features(node, pa_ftrs, tablelen, fp, size);
 }
 
 #ifdef CONFIG_PPC_64S_HASH_MMU
@@ -380,7 +393,10 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
 			identify_cpu(0, be32_to_cpup(prop));
 
 		check_cpu_feature_properties(node);
-		check_cpu_pa_features(node);
+		check_cpu_features(node, "ibm,pa-features", ibm_pa_features,
+				   ARRAY_SIZE(ibm_pa_features));
+		check_cpu_features(node, "ibm,pi-features", ibm_pi_features,
+				   ARRAY_SIZE(ibm_pi_features));
 	}
 
 	identical_pvr_fixup(node);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 014005428687..cb949f12baa9 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -367,7 +367,22 @@ static void sanity_check_fault(bool is_write, bool is_user,
 #elif defined(CONFIG_PPC_8xx)
 #define page_fault_is_bad(__err)	((__err) & DSISR_NOEXEC_OR_G)
 #elif defined(CONFIG_PPC64)
-#define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_64S)
+static int page_fault_is_bad(unsigned long err)
+{
+	unsigned long flag = DSISR_BAD_FAULT_64S;
+
+	/*
+	 * PAPR 14.15.3.4.1
+	 * If byte 0, bit 3 of pi-attribute-specifier-type in
+	 * ibm,pi-features property is defined, ignore the DSI error
+	 * which is caused by the paste instruction on the
+	 * suspended NX window.
+	 */
+	if (cpu_has_feature(CPU_FTR_NX_DSI))
+		flag &= ~DSISR_BAD_COPYPASTE;
+
+	return (err & flag);
+}
 #else
 #define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_32S)
 #endif
-- 
2.26.3

Re: [PATCH v2] powerpc: Ignore DSI error caused by the copy/paste instruction

[Christophe Leroy]

Le 25/09/2022 à 22:26, Haren Myneni a écrit :
> 
> DSI error will be generated when the paste operation is issued on
> the suspended NX window due to NX state changes. The hypervisor
> expects the partition to ignore this error during page pault
> handling. To differentiate DSI caused by an actual HW configuration
> or by the NX window, a new “ibm,pi-features” type value is defined.
> Byte 0, bit 3 of pi-attribute-specifier-type is now defined to
> indicate this DSI error. If this error is not ignored, the user
> space can get SIGBUS when the NX request is issued.

Would be nice to mention at least one time in the message that NX stands 
to nest accelerator.

Otherwise, that's confusing with for exemple:
Commit 2e602847d9c2 ("KVM: PPC: Don't flush PTEs on NX/RO hit")
Commit c49643319715 ("powerpc/32s: Only leave NX unset on segments used 
for modules")


> 
> This patch adds changes to read ibm,pi-features property and ignore
> DSI error in the page fault handling if CPU_FTR_NX_DSI if defined.
> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> v2: Code cleanup as suggested by Christophe Leroy
> 
>   arch/powerpc/include/asm/cputable.h |  5 ++--
>   arch/powerpc/kernel/prom.c          | 36 +++++++++++++++++++++--------
>   arch/powerpc/mm/fault.c             | 17 +++++++++++++-
>   3 files changed, 45 insertions(+), 13 deletions(-)
> 

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 014005428687..cb949f12baa9 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -367,7 +367,22 @@ static void sanity_check_fault(bool is_write, bool is_user,
>   #elif defined(CONFIG_PPC_8xx)
>   #define page_fault_is_bad(__err)	((__err) & DSISR_NOEXEC_OR_G)
>   #elif defined(CONFIG_PPC64)
> -#define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_64S)
> +static int page_fault_is_bad(unsigned long err)
> +{
> +	unsigned long flag = DSISR_BAD_FAULT_64S;
> +
> +	/*
> +	 * PAPR 14.15.3.4.1
> +	 * If byte 0, bit 3 of pi-attribute-specifier-type in
> +	 * ibm,pi-features property is defined, ignore the DSI error
> +	 * which is caused by the paste instruction on the
> +	 * suspended NX window.
> +	 */
> +	if (cpu_has_feature(CPU_FTR_NX_DSI))
> +		flag &= ~DSISR_BAD_COPYPASTE;
> +
> +	return (err & flag);

You don't need parenthesis ( )

> +}
>   #else
>   #define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_32S)
>   #endif

Re: [PATCH v2] powerpc: Ignore DSI error caused by the copy/paste instruction

[Michael Ellerman]

Haren Myneni <haren@linux.ibm.com> writes:
> DSI error will be generated when the paste operation is issued on
> the suspended NX window due to NX state changes. The hypervisor

Please spell out DSI and NX on the first usage.

> expects the partition to ignore this error during page pault
> handling. To differentiate DSI caused by an actual HW configuration
> or by the NX window, a new “ibm,pi-features” type value is defined.
> Byte 0, bit 3 of pi-attribute-specifier-type is now defined to
> indicate this DSI error. If this error is not ignored, the user
> space can get SIGBUS when the NX request is issued.
>
> This patch adds changes to read ibm,pi-features property and ignore
> DSI error in the page fault handling if CPU_FTR_NX_DSI if defined.
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> v2: Code cleanup as suggested by Christophe Leroy 
>
>  arch/powerpc/include/asm/cputable.h |  5 ++--
>  arch/powerpc/kernel/prom.c          | 36 +++++++++++++++++++++--------
>  arch/powerpc/mm/fault.c             | 17 +++++++++++++-
>  3 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index ae8c3e13cfce..8dc9949b6365 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -192,6 +192,7 @@ static inline void cpu_feature_keys_init(void) { }
>  #define CPU_FTR_P9_RADIX_PREFETCH_BUG	LONG_ASM_CONST(0x0002000000000000)
>  #define CPU_FTR_ARCH_31			LONG_ASM_CONST(0x0004000000000000)
>  #define CPU_FTR_DAWR1			LONG_ASM_CONST(0x0008000000000000)
> +#define CPU_FTR_NX_DSI			LONG_ASM_CONST(0x0010000000000000)

Can we make this an MMU feature?

We have a lot more free MMU feature bits, it should just be a case of
s/cpu/mmu/ pretty much everywhere you use it.

>  #ifndef __ASSEMBLY__
>  
> @@ -429,7 +430,7 @@ static inline void cpu_feature_keys_init(void) { }
>  	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
>  	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
>  	    CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_P9_TLBIE_STQ_BUG | \
> -	    CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR)
> +	    CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR | CPU_FTR_NX_DSI)
>  #define CPU_FTRS_POWER9_DD2_0 (CPU_FTRS_POWER9 | CPU_FTR_P9_RADIX_PREFETCH_BUG)
>  #define CPU_FTRS_POWER9_DD2_1 (CPU_FTRS_POWER9 | \
>  			       CPU_FTR_P9_RADIX_PREFETCH_BUG | \
> @@ -451,7 +452,7 @@ static inline void cpu_feature_keys_init(void) { }
>  	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
>  	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
>  	    CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
> -	    CPU_FTR_DAWR | CPU_FTR_DAWR1)
> +	    CPU_FTR_DAWR | CPU_FTR_DAWR1 | CPU_FTR_NX_DSI)

You're turning that bit on by default for Power9 and Power10 - is that
correct?

If so do you have a documentation source for that?

cheers

Re: [PATCH v2] powerpc: Ignore DSI error caused by the copy/paste instruction

[Haren Myneni]

On Mon, 2022-09-26 at 05:55 +0000, Christophe Leroy wrote:
> 
> Le 25/09/2022 à 22:26, Haren Myneni a écrit :
> > DSI error will be generated when the paste operation is issued on
> > the suspended NX window due to NX state changes. The hypervisor
> > expects the partition to ignore this error during page pault
> > handling. To differentiate DSI caused by an actual HW configuration
> > or by the NX window, a new “ibm,pi-features” type value is defined.
> > Byte 0, bit 3 of pi-attribute-specifier-type is now defined to
> > indicate this DSI error. If this error is not ignored, the user
> > space can get SIGBUS when the NX request is issued.
> 
> Would be nice to mention at least one time in the message that NX
> stands 
> to nest accelerator.
> 
> Otherwise, that's confusing with for exemple:
> Commit 2e602847d9c2 ("KVM: PPC: Don't flush PTEs on NX/RO hit")
> Commit c49643319715 ("powerpc/32s: Only leave NX unset on segments
> used 
> for modules")

Thanks. I did not realize since VAS/NX code is added before. I will add
the description as you suggested. 

> 
> 
> > This patch adds changes to read ibm,pi-features property and ignore
> > DSI error in the page fault handling if CPU_FTR_NX_DSI if defined.
> > 
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > v2: Code cleanup as suggested by Christophe Leroy
> > 
> >   arch/powerpc/include/asm/cputable.h |  5 ++--
> >   arch/powerpc/kernel/prom.c          | 36 +++++++++++++++++++++---
> > -----
> >   arch/powerpc/mm/fault.c             | 17 +++++++++++++-
> >   3 files changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 014005428687..cb949f12baa9 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -367,7 +367,22 @@ static void sanity_check_fault(bool is_write,
> > bool is_user,
> >   #elif defined(CONFIG_PPC_8xx)
> >   #define page_fault_is_bad(__err)	((__err) & DSISR_NOEXEC_OR_G)
> >   #elif defined(CONFIG_PPC64)
> > -#define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_64S)
> > +static int page_fault_is_bad(unsigned long err)
> > +{
> > +	unsigned long flag = DSISR_BAD_FAULT_64S;
> > +
> > +	/*
> > +	 * PAPR 14.15.3.4.1
> > +	 * If byte 0, bit 3 of pi-attribute-specifier-type in
> > +	 * ibm,pi-features property is defined, ignore the DSI error
> > +	 * which is caused by the paste instruction on the
> > +	 * suspended NX window.
> > +	 */
> > +	if (cpu_has_feature(CPU_FTR_NX_DSI))
> > +		flag &= ~DSISR_BAD_COPYPASTE;
> > +
> > +	return (err & flag);
> 
> You don't need parenthesis ( )
> 
> > +}
> >   #else
> >   #define page_fault_is_bad(__err)	((__err) & DSISR_BAD_FAULT_32S)
> >   #endif