Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices
@ 2020-02-02 17:00 Wang Xuerui
  2020-02-02 17:00 ` [PATCH 2/2] Revert "MIPS: asm: local: add barriers for Loongson" Wang Xuerui
  2020-02-10 19:24 ` [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Paul Burton
  0 siblings, 2 replies; 3+ messages in thread
From: Wang Xuerui @ 2020-02-02 17:00 UTC (permalink / raw)
  To: linux-mips; +Cc: Huacai Chen, Wang Xuerui

From: Huacai Chen <chenhc@lemote.com>

Local ops (and other similar cases) don't need SYNCs before LL/SC
because there is only one writer, so don't always fail on missing SYNCs.
Print a notice instead.

[git@xen0n.name: Massaged commit message and symbol naming.]

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Wang Xuerui <git@xen0n.name>
---
 arch/mips/tools/loongson3-llsc-check.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/mips/tools/loongson3-llsc-check.c b/arch/mips/tools/loongson3-llsc-check.c
index 0ebddd0ae46f..c485950b7a36 100644
--- a/arch/mips/tools/loongson3-llsc-check.c
+++ b/arch/mips/tools/loongson3-llsc-check.c
@@ -138,6 +138,12 @@ static bool is_branch(uint32_t insn, int *off)
 	}
 }
 
+#define REPORT_OK		0x0
+#define REPORT_LL		0x1
+#define REPORT_BRANCH_TARGET	0x2
+
+static int err_report = REPORT_OK;
+
 static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
 {
 	ssize_t i, max, sc_pos;
@@ -149,8 +155,8 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
 	 * execute after the LL & cause erroneous results.
 	 */
 	if (!is_sync(le32toh(code[-1]))) {
+		err_report |= REPORT_LL;
 		fprintf(stderr, "%" PRIx64 ": LL not preceded by sync\n", pc);
-		return -EINVAL;
 	}
 
 	/* Find the matching SC instruction */
@@ -185,9 +191,9 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
 			continue;
 
 		/* ...but if not, we have a problem */
+		err_report |= REPORT_BRANCH_TARGET;
 		fprintf(stderr, "%" PRIx64 ": Branch target not a sync\n",
 			pc + (i * 4));
-		return -EINVAL;
 	}
 
 	return 0;
@@ -297,6 +303,13 @@ int main(int argc, char *argv[])
 			goto out_munmap;
 	}
 
+	if (err_report & REPORT_LL)
+		fprintf(stderr, "Notice: there are LLs not preceded by"
+				" syncs, please confirm manually.\n");
+	if (err_report & REPORT_BRANCH_TARGET)
+		fprintf(stderr, "Notice: there are branches within LL/SC blocks"
+				" not targeting syncs, please confirm manually.\n");
+
 	status = EXIT_SUCCESS;
 out_munmap:
 	munmap(vmlinux, st.st_size);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH 2/2] Revert "MIPS: asm: local: add barriers for Loongson"
  2020-02-02 17:00 [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Wang Xuerui
@ 2020-02-02 17:00 ` Wang Xuerui
  2020-02-10 19:24 ` [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Paul Burton
  1 sibling, 0 replies; 3+ messages in thread
From: Wang Xuerui @ 2020-02-02 17:00 UTC (permalink / raw)
  To: linux-mips; +Cc: Huacai Chen, Wang Xuerui

From: Huacai Chen <chenhc@lemote.com>

This reverts commit 3e86460ebe2340df6a33b35a55312cc369bdcbd0.

Local ops don't need SYNCs before LL because there is only one writer,
the erratum is not triggered.

The LLSCCHK violations are made not to fail the build in the previous
patch so just revert the additions.

[git@xen0n.name: Massaged commit message.]

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Wang Xuerui <git@xen0n.name>
---
 arch/mips/include/asm/local.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/mips/include/asm/local.h b/arch/mips/include/asm/local.h
index fef0fda8f82f..02783e141c32 100644
--- a/arch/mips/include/asm/local.h
+++ b/arch/mips/include/asm/local.h
@@ -37,7 +37,6 @@ static __inline__ long local_add_return(long i, local_t * l)
 		__asm__ __volatile__(
 		"	.set	push					\n"
 		"	.set	arch=r4000				\n"
-			__SYNC(full, loongson3_war) "			\n"
 		"1:"	__LL	"%1, %2		# local_add_return	\n"
 		"	addu	%0, %1, %3				\n"
 			__SC	"%0, %2					\n"
@@ -53,7 +52,6 @@ static __inline__ long local_add_return(long i, local_t * l)
 		__asm__ __volatile__(
 		"	.set	push					\n"
 		"	.set	"MIPS_ISA_ARCH_LEVEL"			\n"
-			__SYNC(full, loongson3_war) "			\n"
 		"1:"	__LL	"%1, %2		# local_add_return	\n"
 		"	addu	%0, %1, %3				\n"
 			__SC	"%0, %2					\n"
@@ -86,7 +84,6 @@ static __inline__ long local_sub_return(long i, local_t * l)
 		__asm__ __volatile__(
 		"	.set	push					\n"
 		"	.set	arch=r4000				\n"
-			__SYNC(full, loongson3_war) "			\n"
 		"1:"	__LL	"%1, %2		# local_sub_return	\n"
 		"	subu	%0, %1, %3				\n"
 			__SC	"%0, %2					\n"
@@ -102,7 +99,6 @@ static __inline__ long local_sub_return(long i, local_t * l)
 		__asm__ __volatile__(
 		"	.set	push					\n"
 		"	.set	"MIPS_ISA_ARCH_LEVEL"			\n"
-			__SYNC(full, loongson3_war) "			\n"
 		"1:"	__LL	"%1, %2		# local_sub_return	\n"
 		"	subu	%0, %1, %3				\n"
 			__SC	"%0, %2					\n"
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices
  2020-02-02 17:00 [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Wang Xuerui
  2020-02-02 17:00 ` [PATCH 2/2] Revert "MIPS: asm: local: add barriers for Loongson" Wang Xuerui
@ 2020-02-10 19:24 ` Paul Burton
  1 sibling, 0 replies; 3+ messages in thread
From: Paul Burton @ 2020-02-10 19:24 UTC (permalink / raw)
  To: Wang Xuerui; +Cc: linux-mips, Huacai Chen

Hi Wang & Huacai,

On Mon, Feb 03, 2020 at 01:00:51AM +0800, Wang Xuerui wrote:
> From: Huacai Chen <chenhc@lemote.com>
> 
> Local ops (and other similar cases) don't need SYNCs before LL/SC
> because there is only one writer, so don't always fail on missing SYNCs.
> Print a notice instead.
> 
> [git@xen0n.name: Massaged commit message and symbol naming.]

I really dislike this patch; the whole point of the compile-time check
is to make problems fatal & easy to find. By turning them into mere
warnings you make it possible to ignore real problems, and by making
some of the warnings expected with patch 2 you make it much more likely
that real issues will be ignored.

I'd suggest that if you want to remove the SYNCs in asm/local.h then you
could come up with some way to annotate those places in a way that the
loongson3-llsc-check tool can detect & avoid erroring on. That way real
bugs will still be found, presuming the annotations are used correctly.
For example, you could come up with a macro to use instead of __SYNC()
that generates an ELF section listing the PC values of locations that
don't require the sync. The checker can take those into account, and we
can remove the section from the final VDSO image.

As it is, I'd much rather have asm/local.h incur the slight overhead
from extra sync instructions than make the checker useless.

Thanks,
    Paul

> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Signed-off-by: Wang Xuerui <git@xen0n.name>
> ---
>  arch/mips/tools/loongson3-llsc-check.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/tools/loongson3-llsc-check.c b/arch/mips/tools/loongson3-llsc-check.c
> index 0ebddd0ae46f..c485950b7a36 100644
> --- a/arch/mips/tools/loongson3-llsc-check.c
> +++ b/arch/mips/tools/loongson3-llsc-check.c
> @@ -138,6 +138,12 @@ static bool is_branch(uint32_t insn, int *off)
>  	}
>  }
>  
> +#define REPORT_OK		0x0
> +#define REPORT_LL		0x1
> +#define REPORT_BRANCH_TARGET	0x2
> +
> +static int err_report = REPORT_OK;
> +
>  static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
>  {
>  	ssize_t i, max, sc_pos;
> @@ -149,8 +155,8 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
>  	 * execute after the LL & cause erroneous results.
>  	 */
>  	if (!is_sync(le32toh(code[-1]))) {
> +		err_report |= REPORT_LL;
>  		fprintf(stderr, "%" PRIx64 ": LL not preceded by sync\n", pc);
> -		return -EINVAL;
>  	}
>  
>  	/* Find the matching SC instruction */
> @@ -185,9 +191,9 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz)
>  			continue;
>  
>  		/* ...but if not, we have a problem */
> +		err_report |= REPORT_BRANCH_TARGET;
>  		fprintf(stderr, "%" PRIx64 ": Branch target not a sync\n",
>  			pc + (i * 4));
> -		return -EINVAL;
>  	}
>  
>  	return 0;
> @@ -297,6 +303,13 @@ int main(int argc, char *argv[])
>  			goto out_munmap;
>  	}
>  
> +	if (err_report & REPORT_LL)
> +		fprintf(stderr, "Notice: there are LLs not preceded by"
> +				" syncs, please confirm manually.\n");
> +	if (err_report & REPORT_BRANCH_TARGET)
> +		fprintf(stderr, "Notice: there are branches within LL/SC blocks"
> +				" not targeting syncs, please confirm manually.\n");
> +
>  	status = EXIT_SUCCESS;
>  out_munmap:
>  	munmap(vmlinux, st.st_size);
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-02 17:00 [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Wang Xuerui
2020-02-02 17:00 ` [PATCH 2/2] Revert "MIPS: asm: local: add barriers for Loongson" Wang Xuerui
2020-02-10 19:24 ` [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices Paul Burton

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git