[PATCH 1/2] AutoFDO: Remove the architecture specific config

[PATCH 1/2] AutoFDO: Remove the architecture specific config

[Rong Xu]

The CONFIG_AUTOFDO_CLANG option currently depends on
ARCH_SUPPORTS_AUTOFDO_CLANG, but this dependency seems unnecessary.

Remove ARCH_SUPPORTS_AUTOFDO_CLANG and allow users to control AutoFDO
builds solely through CONFIG_AUTOFDO_CLANG. This simplifies the kconfig
and avoids potential confusion.

Signed-off-by: Rong Xu <xur@google.com>
Suggested-by: Will Deacon <will@kernel.org>
---
 arch/Kconfig     | 4 ----
 arch/x86/Kconfig | 1 -
 2 files changed, 5 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 6682b2a53e342..0b36d74d47031 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -812,12 +812,8 @@ config LTO_CLANG_THIN
 	  If unsure, say Y.
 endchoice
 
-config ARCH_SUPPORTS_AUTOFDO_CLANG
-	bool
-
 config AUTOFDO_CLANG
 	bool "Enable Clang's AutoFDO build (EXPERIMENTAL)"
-	depends on ARCH_SUPPORTS_AUTOFDO_CLANG
 	depends on CC_IS_CLANG && CLANG_VERSION >= 170000
 	help
 	  This option enables Clang’s AutoFDO build. When
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9d7bd0ae48c42..9363d9cc9a00a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -128,7 +128,6 @@ config X86
 	select ARCH_SUPPORTS_LTO_CLANG
 	select ARCH_SUPPORTS_LTO_CLANG_THIN
 	select ARCH_SUPPORTS_RT
-	select ARCH_SUPPORTS_AUTOFDO_CLANG
 	select ARCH_SUPPORTS_PROPELLER_CLANG    if X86_64
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if X86_CMPXCHG64

base-commit: f92f4749861b06fed908d336b4dee1326003291b
-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/2] Propeller: Remove the architecture specific config

[Rong Xu]

The CONFIG_PROPELLER_CLANG option currently depends on
ARCH_SUPPORTS_PROPELLER_CLANG, but this dependency seems unnecessary.

Remove ARCH_SUPPORTS_PROPELLER_CLANG and allow users to control
Propeller builds solely through CONFIG_PROPELLER_CLANG. This simplifies
the kconfig and avoids potential confusion.

Signed-off-by: Rong Xu <xur@google.com>
Suggested-by: Will Deacon <will@kernel.org>
---
 arch/Kconfig     | 4 ----
 arch/x86/Kconfig | 1 -
 2 files changed, 5 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 0b36d74d47031..83731b858ba65 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -828,12 +828,8 @@ config AUTOFDO_CLANG
 
 	  If unsure, say N.
 
-config ARCH_SUPPORTS_PROPELLER_CLANG
-	bool
-
 config PROPELLER_CLANG
 	bool "Enable Clang's Propeller build"
-	depends on ARCH_SUPPORTS_PROPELLER_CLANG
 	depends on CC_IS_CLANG && CLANG_VERSION >= 190000
 	help
 	  This option enables Clang’s Propeller build. When the Propeller
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9363d9cc9a00a..6c633d93c6390 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -128,7 +128,6 @@ config X86
 	select ARCH_SUPPORTS_LTO_CLANG
 	select ARCH_SUPPORTS_LTO_CLANG_THIN
 	select ARCH_SUPPORTS_RT
-	select ARCH_SUPPORTS_PROPELLER_CLANG    if X86_64
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if X86_CMPXCHG64
 	select ARCH_USE_MEMTEST
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 2/2] Propeller: Remove the architecture specific config

[Nathan Chancellor]

Hi Rong,

On Wed, Dec 11, 2024 at 02:29:43PM -0800, Rong Xu wrote:
> The CONFIG_PROPELLER_CLANG option currently depends on
> ARCH_SUPPORTS_PROPELLER_CLANG, but this dependency seems unnecessary.
> 
> Remove ARCH_SUPPORTS_PROPELLER_CLANG and allow users to control
> Propeller builds solely through CONFIG_PROPELLER_CLANG. This simplifies
> the kconfig and avoids potential confusion.
> 
> Signed-off-by: Rong Xu <xur@google.com>
> Suggested-by: Will Deacon <will@kernel.org>

In commit d5dc95836147 ("kbuild: Add Propeller configuration for kernel
build"), you added .llvm_bb_addr_map to arch/x86/kernel/vmlinux.lds.S.
Was this to address a orphan section warning from the linker? Is that
same change needed in the linker scripts of the other architectures that
clang supports building in the kernel?

Cheers,
Nathan

> ---
>  arch/Kconfig     | 4 ----
>  arch/x86/Kconfig | 1 -
>  2 files changed, 5 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 0b36d74d47031..83731b858ba65 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -828,12 +828,8 @@ config AUTOFDO_CLANG
>  
>  	  If unsure, say N.
>  
> -config ARCH_SUPPORTS_PROPELLER_CLANG
> -	bool
> -
>  config PROPELLER_CLANG
>  	bool "Enable Clang's Propeller build"
> -	depends on ARCH_SUPPORTS_PROPELLER_CLANG
>  	depends on CC_IS_CLANG && CLANG_VERSION >= 190000
>  	help
>  	  This option enables Clang’s Propeller build. When the Propeller
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 9363d9cc9a00a..6c633d93c6390 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -128,7 +128,6 @@ config X86
>  	select ARCH_SUPPORTS_LTO_CLANG
>  	select ARCH_SUPPORTS_LTO_CLANG_THIN
>  	select ARCH_SUPPORTS_RT
> -	select ARCH_SUPPORTS_PROPELLER_CLANG    if X86_64
>  	select ARCH_USE_BUILTIN_BSWAP
>  	select ARCH_USE_CMPXCHG_LOCKREF		if X86_CMPXCHG64
>  	select ARCH_USE_MEMTEST
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH 2/2] Propeller: Remove the architecture specific config

[Rong Xu]

 (o

On Wed, Dec 11, 2024 at 3:06 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hi Rong,
>
> On Wed, Dec 11, 2024 at 02:29:43PM -0800, Rong Xu wrote:
> > The CONFIG_PROPELLER_CLANG option currently depends on
> > ARCH_SUPPORTS_PROPELLER_CLANG, but this dependency seems unnecessary.
> >
> > Remove ARCH_SUPPORTS_PROPELLER_CLANG and allow users to control
> > Propeller builds solely through CONFIG_PROPELLER_CLANG. This simplifies
> > the kconfig and avoids potential confusion.
> >
> > Signed-off-by: Rong Xu <xur@google.com>
> > Suggested-by: Will Deacon <will@kernel.org>
>
> In commit d5dc95836147 ("kbuild: Add Propeller configuration for kernel
> build"), you added .llvm_bb_addr_map to arch/x86/kernel/vmlinux.lds.S.
> Was this to address a orphan section warning from the linker? Is that
> same change needed in the linker scripts of the other architectures that
> clang supports building in the kernel?

The .llvm_bb_addr_map section is created when compiling with the
-fbasic-block-address-map flag (or the older -fbasic-block-sections=labels
option), with LLVM. We need the change in the linker script to group the
sections together and emit in the final vmlinux.

This applies to all architectures, as LLVM can generate these sections for
any architecture when the corresponding option is used.

The statement that AutoFDO and Propeller are only supported on certain
platforms really refers to the availability of *native* profile generation:
the native profile generation is only available on these platforms.

Hope this helps,

-Rong

> Cheers,
> Nathan
>
> > ---
> >  arch/Kconfig     | 4 ----
> >  arch/x86/Kconfig | 1 -
> >  2 files changed, 5 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 0b36d74d47031..83731b858ba65 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -828,12 +828,8 @@ config AUTOFDO_CLANG
> >
> >         If unsure, say N.
> >
> > -config ARCH_SUPPORTS_PROPELLER_CLANG
> > -     bool
> > -
> >  config PROPELLER_CLANG
> >       bool "Enable Clang's Propeller build"
> > -     depends on ARCH_SUPPORTS_PROPELLER_CLANG
> >       depends on CC_IS_CLANG && CLANG_VERSION >= 190000
> >       help
> >         This option enables Clang’s Propeller build. When the Propeller
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 9363d9cc9a00a..6c633d93c6390 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -128,7 +128,6 @@ config X86
> >       select ARCH_SUPPORTS_LTO_CLANG
> >       select ARCH_SUPPORTS_LTO_CLANG_THIN
> >       select ARCH_SUPPORTS_RT
> > -     select ARCH_SUPPORTS_PROPELLER_CLANG    if X86_64
> >       select ARCH_USE_BUILTIN_BSWAP
> >       select ARCH_USE_CMPXCHG_LOCKREF         if X86_CMPXCHG64
> >       select ARCH_USE_MEMTEST
> > --
> > 2.47.0.338.g60cca15819-goog
> >

Re: [PATCH 1/2] AutoFDO: Remove the architecture specific config

[Will Deacon]

On Wed, Dec 11, 2024 at 02:29:42PM -0800, Rong Xu wrote:
> The CONFIG_AUTOFDO_CLANG option currently depends on
> ARCH_SUPPORTS_AUTOFDO_CLANG, but this dependency seems unnecessary.
> 
> Remove ARCH_SUPPORTS_AUTOFDO_CLANG and allow users to control AutoFDO
> builds solely through CONFIG_AUTOFDO_CLANG. This simplifies the kconfig
> and avoids potential confusion.
> 
> Signed-off-by: Rong Xu <xur@google.com>
> Suggested-by: Will Deacon <will@kernel.org>
> ---
>  arch/Kconfig     | 4 ----
>  arch/x86/Kconfig | 1 -
>  2 files changed, 5 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

However, you've dropped the previous hunk you had to
Documentation/dev-tools/autofdo.rst which had some text about using ETM
to collect fdo data on arm64. It seems like that might still be useful
to have?

Will

Re: [PATCH 1/2] AutoFDO: Remove the architecture specific config

[Rong Xu]

Thanks for the Acked-by.

Regarding the ETM documentation change, I haven't forgotten it. I
think it's best for Yabin to submit another patch (or revision of his
previous patch), as he has done all the ETM side work.

Best regards,

-Rong

On Thu, Dec 19, 2024 at 7:52 AM Will Deacon <will@kernel.org> wrote:
>
> On Wed, Dec 11, 2024 at 02:29:42PM -0800, Rong Xu wrote:
> > The CONFIG_AUTOFDO_CLANG option currently depends on
> > ARCH_SUPPORTS_AUTOFDO_CLANG, but this dependency seems unnecessary.
> >
> > Remove ARCH_SUPPORTS_AUTOFDO_CLANG and allow users to control AutoFDO
> > builds solely through CONFIG_AUTOFDO_CLANG. This simplifies the kconfig
> > and avoids potential confusion.
> >
> > Signed-off-by: Rong Xu <xur@google.com>
> > Suggested-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/Kconfig     | 4 ----
> >  arch/x86/Kconfig | 1 -
> >  2 files changed, 5 deletions(-)
>
> Acked-by: Will Deacon <will@kernel.org>
>
> However, you've dropped the previous hunk you had to
> Documentation/dev-tools/autofdo.rst which had some text about using ETM
> to collect fdo data on arm64. It seems like that might still be useful
> to have?
>
> Will

Re: [PATCH 1/2] AutoFDO: Remove the architecture specific config

[Yabin Cui]

Hi Rong,

Thanks for the patch! Sorry for the delay in testing it.
I will upload another patch to document steps for using ETM.

Tested-by: Yabin Cui <yabinc@google.com>

Thanks,
Yabin



On Thu, Dec 19, 2024 at 9:20 AM Rong Xu <xur@google.com> wrote:
>
> Thanks for the Acked-by.
>
> Regarding the ETM documentation change, I haven't forgotten it. I
> think it's best for Yabin to submit another patch (or revision of his
> previous patch), as he has done all the ETM side work.
>
> Best regards,
>
> -Rong
>
> On Thu, Dec 19, 2024 at 7:52 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Wed, Dec 11, 2024 at 02:29:42PM -0800, Rong Xu wrote:
> > > The CONFIG_AUTOFDO_CLANG option currently depends on
> > > ARCH_SUPPORTS_AUTOFDO_CLANG, but this dependency seems unnecessary.
> > >
> > > Remove ARCH_SUPPORTS_AUTOFDO_CLANG and allow users to control AutoFDO
> > > builds solely through CONFIG_AUTOFDO_CLANG. This simplifies the kconfig
> > > and avoids potential confusion.
> > >
> > > Signed-off-by: Rong Xu <xur@google.com>
> > > Suggested-by: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/Kconfig     | 4 ----
> > >  arch/x86/Kconfig | 1 -
> > >  2 files changed, 5 deletions(-)
> >
> > Acked-by: Will Deacon <will@kernel.org>
> >
> > However, you've dropped the previous hunk you had to
> > Documentation/dev-tools/autofdo.rst which had some text about using ETM
> > to collect fdo data on arm64. It seems like that might still be useful
> > to have?
> >
> > Will

Re: [PATCH 2/2] Propeller: Remove the architecture specific config

[Yabin Cui]

Tested-by: Yabin Cui <yabinc@google.com>

On Wed, Dec 11, 2024 at 11:48 PM Rong Xu <xur@google.com> wrote:
>
>  (o
>
> On Wed, Dec 11, 2024 at 3:06 PM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > Hi Rong,
> >
> > On Wed, Dec 11, 2024 at 02:29:43PM -0800, Rong Xu wrote:
> > > The CONFIG_PROPELLER_CLANG option currently depends on
> > > ARCH_SUPPORTS_PROPELLER_CLANG, but this dependency seems unnecessary.
> > >
> > > Remove ARCH_SUPPORTS_PROPELLER_CLANG and allow users to control
> > > Propeller builds solely through CONFIG_PROPELLER_CLANG. This simplifies
> > > the kconfig and avoids potential confusion.
> > >
> > > Signed-off-by: Rong Xu <xur@google.com>
> > > Suggested-by: Will Deacon <will@kernel.org>
> >
> > In commit d5dc95836147 ("kbuild: Add Propeller configuration for kernel
> > build"), you added .llvm_bb_addr_map to arch/x86/kernel/vmlinux.lds.S.
> > Was this to address a orphan section warning from the linker? Is that
> > same change needed in the linker scripts of the other architectures that
> > clang supports building in the kernel?
>
> The .llvm_bb_addr_map section is created when compiling with the
> -fbasic-block-address-map flag (or the older -fbasic-block-sections=labels
> option), with LLVM. We need the change in the linker script to group the
> sections together and emit in the final vmlinux.
>
> This applies to all architectures, as LLVM can generate these sections for
> any architecture when the corresponding option is used.
>
> The statement that AutoFDO and Propeller are only supported on certain
> platforms really refers to the availability of *native* profile generation:
> the native profile generation is only available on these platforms.
>
> Hope this helps,
>
> -Rong
>
> > Cheers,
> > Nathan
> >
> > > ---
> > >  arch/Kconfig     | 4 ----
> > >  arch/x86/Kconfig | 1 -
> > >  2 files changed, 5 deletions(-)
> > >
> > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > index 0b36d74d47031..83731b858ba65 100644
> > > --- a/arch/Kconfig
> > > +++ b/arch/Kconfig
> > > @@ -828,12 +828,8 @@ config AUTOFDO_CLANG
> > >
> > >         If unsure, say N.
> > >
> > > -config ARCH_SUPPORTS_PROPELLER_CLANG
> > > -     bool
> > > -
> > >  config PROPELLER_CLANG
> > >       bool "Enable Clang's Propeller build"
> > > -     depends on ARCH_SUPPORTS_PROPELLER_CLANG
> > >       depends on CC_IS_CLANG && CLANG_VERSION >= 190000
> > >       help
> > >         This option enables Clang’s Propeller build. When the Propeller
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 9363d9cc9a00a..6c633d93c6390 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -128,7 +128,6 @@ config X86
> > >       select ARCH_SUPPORTS_LTO_CLANG
> > >       select ARCH_SUPPORTS_LTO_CLANG_THIN
> > >       select ARCH_SUPPORTS_RT
> > > -     select ARCH_SUPPORTS_PROPELLER_CLANG    if X86_64
> > >       select ARCH_USE_BUILTIN_BSWAP
> > >       select ARCH_USE_CMPXCHG_LOCKREF         if X86_CMPXCHG64
> > >       select ARCH_USE_MEMTEST
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >

Re: [PATCH 1/2] AutoFDO: Remove the architecture specific config

[Kees Cook]

On Wed, Dec 11, 2024 at 02:29:42PM -0800, Rong Xu wrote:
> The CONFIG_AUTOFDO_CLANG option currently depends on
> ARCH_SUPPORTS_AUTOFDO_CLANG, but this dependency seems unnecessary.
> 
> Remove ARCH_SUPPORTS_AUTOFDO_CLANG and allow users to control AutoFDO
> builds solely through CONFIG_AUTOFDO_CLANG. This simplifies the kconfig
> and avoids potential confusion.
> 
> Signed-off-by: Rong Xu <xur@google.com>

Looks right to me!

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH 2/2] Propeller: Remove the architecture specific config

[Kees Cook]

On Wed, Dec 11, 2024 at 02:29:43PM -0800, Rong Xu wrote:
> The CONFIG_PROPELLER_CLANG option currently depends on
> ARCH_SUPPORTS_PROPELLER_CLANG, but this dependency seems unnecessary.
> 
> Remove ARCH_SUPPORTS_PROPELLER_CLANG and allow users to control
> Propeller builds solely through CONFIG_PROPELLER_CLANG. This simplifies
> the kconfig and avoids potential confusion.
> 
> Signed-off-by: Rong Xu <xur@google.com>

With Nathan's question answered, this looks good to me:

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook