[PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>

[Nathan Chancellor]

On 6/21/2021 1:08 AM, Mark Rutland wrote:
> On Fri, Jun 18, 2021 at 09:53:31AM -0700, Nathan Chancellor wrote:
>> On Fri, Jun 18, 2021 at 04:18:35PM +0100, Mark Rutland wrote:
>>> On Thu, Jun 17, 2021 at 06:25:27PM -0700, Nathan Chancellor wrote:
>>>> Hi Mark,
>>>
>>> Hi Nathan,
>>>
>>>> On Wed, Jun 09, 2021 at 11:23:01AM +0100, Mark Rutland wrote:
>>>>> For histroical reasons, we define AARCH64_INSN_SIZE in
>>>>> <asm/alternative-macros.h>, but it would make more sense to do so in
>>>>> <asm/insn.h>. Let's move it into <asm/insn.h>, and add the necessary
>>>>> include directives for this.
>>>
>>>> I bisected a CONFIG_LTO_CLANG_THIN=y build failure that our CI reported
>>>> to this patch:
>>>>
>>>> https://builds.tuxbuild.com/1u4Fpx2FQkkgkyPxWtq0Ke4YFCQ/build.log
>>>
>>> Thanks for reporting this; the lopg is really helpful!
>>>
>>>> I have not had a whole ton of time to look into this (dealing with a
>>>> million fires it seems :^) but it is not immediately obvious to me why
>>>> this fails because include/linux/build_bug.h is included within
>>>> arch/arm64/include/asm/insn.h.
>>>
>>> The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
>>> includes <asm/insn.h>, creating a circular include chain:
>>>
>>> 	<linux/build_bug.h>
>>> 	<linux/compiler.h>
>>> 	<asm/rwonce.h>
>>> 	<asm/alternative-macros.h>
>>> 	<asm/insn.h>
>>> 	<linux/build-bug.h>
>>>
>>> ... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
>>> BUILD_BUG* definitions have happened yet.
>>
>> Aha, that would certainly explain it. I figured something like this
>> would be the root cause but figuring out header dependencies is not my
>> cup of tea.
>>
>>> Will, are you happy to take the fixup patch below, or would you prefer
>>> to drop this patch for now?
> 
>>>   arch/arm64/include/asm/alternative-macros.h | 2 +-
>>>   arch/arm64/include/asm/insn.h               | 5 +----
>>
>> Looks like arch/arm64/include/asm/insn-def.h is missing from this patch?
>>
>> If I add one with just the two deleted lines plus a header guard, the
>> build passes.
>>
>> Tested-by: Nathan Chancellor <nathan at kernel.org>
> 
> Whoops; that should have been as below.
> 
> Was that the same as you tested?

Yes, that is exactly what I did so the tag should still be good.

Cheers,
Nathan

> Thanks,
> Mark.
> 
> ---->8----
>  From 622fd784c57423b1a276fbbfb270b84839e3afa8 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland at arm.com>
> Date: Fri, 18 Jun 2021 16:11:22 +0100
> Subject: [PATCH] arm64: insn: avoid circular include dependency
> 
> Nathan reports that when building with CONFIG_LTO_CLANG_THIN=y, the
> build fails due to BUILD_BUG_ON() not being defined before its uss in
> <asm/insn.h>.
> 
> The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
> includes <asm/insn.h>, creating a circular include chain:
> 
>          <linux/build_bug.h>
>          <linux/compiler.h>
>          <asm/rwonce.h>
>          <asm/alternative-macros.h>
>          <asm/insn.h>
>          <linux/build-bug.h>
> 
> ... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
> BUILD_BUG* definitions have happened yet.
> 
> To avoid this, let's move AARCH64_INSN_SIZE into a header without any
> dependencies, such that it can always be safely included. At the same
> time, avoid including <asm/alternative.h> in <asm/insn.h>, which should
> no longer be necessary (and doesn't make sense when insn.h is consumed
> by userspace).
> 
> Reported-by: Nathan Chancellor <nathan at kernel.org>
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will at kernel.org>
> ---
>   arch/arm64/include/asm/alternative-macros.h | 2 +-
>   arch/arm64/include/asm/insn-def.h           | 9 +++++++++
>   arch/arm64/include/asm/insn.h               | 5 +----
>   3 files changed, 11 insertions(+), 5 deletions(-)
>   create mode 100644 arch/arm64/include/asm/insn-def.h
> 
> diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
> index 703fbf310b79..eba3173a2a2c 100644
> --- a/arch/arm64/include/asm/alternative-macros.h
> +++ b/arch/arm64/include/asm/alternative-macros.h
> @@ -3,7 +3,7 @@
>   #define __ASM_ALTERNATIVE_MACROS_H
>   
>   #include <asm/cpucaps.h>
> -#include <asm/insn.h>
> +#include <asm/insn-def.h>
>   
>   #define ARM64_CB_PATCH ARM64_NCAPS
>   
> diff --git a/arch/arm64/include/asm/insn-def.h b/arch/arm64/include/asm/insn-def.h
> new file mode 100644
> index 000000000000..2c075f615c6a
> --- /dev/null
> +++ b/arch/arm64/include/asm/insn-def.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_INSN_DEF_H
> +#define __ASM_INSN_DEF_H
> +
> +/* A64 instructions are always 32 bits. */
> +#define	AARCH64_INSN_SIZE		4
> +
> +#endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 1430b4973039..6b776c8667b2 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -10,10 +10,7 @@
>   #include <linux/build_bug.h>
>   #include <linux/types.h>
>   
> -#include <asm/alternative.h>
> -
> -/* A64 instructions are always 32 bits. */
> -#define	AARCH64_INSN_SIZE		4
> +#include <asm/insn-def.h>
>   
>   #ifndef __ASSEMBLY__
>   /*
>