Re: [PATCH 1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices

[Paul Burton <paulburton@kernel.org>]

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
>