linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: do not descend to vdso directories twice
@ 2020-12-18  2:45 Masahiro Yamada
  2020-12-21 14:39 ` Vincenzo Frascino
  2021-01-20 13:01 ` Will Deacon
  0 siblings, 2 replies; 5+ messages in thread
From: Masahiro Yamada @ 2020-12-18  2:45 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, linux-arm-kernel
  Cc: Michael Ellerman, Masahiro Yamada, linux-kernel

arm64 descends into each vdso directory twice; first in vdso_prepare,
second during the ordinary build process.

PPC mimicked it and uncovered a problem [1]. In the first descend,
Kbuild directly visits the vdso directories, therefore it does not
inherit subdir-ccflags-y from upper directories.

This means the command line parameters may differ between the two.
If it happens, the offset values in the generated headers might be
different from real offsets of vdso.so in the kernel.

This potential danger should be avoided. The vdso directories are
built in the vdso_prepare stage, so the second descend is unneeded.

[1]: https://lore.kernel.org/linux-kbuild/CAK7LNARAkJ3_-4gX0VA2UkapbOftuzfSTVMBbgbw=HD8n7N+7w@mail.gmail.com/T/#ma10dcb961fda13f36d42d58fa6cb2da988b7e73a

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 arch/arm64/Makefile                                | 10 ++++++----
 arch/arm64/kernel/Makefile                         |  5 +++--
 arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S}     |  0
 arch/arm64/kernel/vdso/Makefile                    |  1 -
 arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} |  0
 arch/arm64/kernel/vdso32/Makefile                  |  1 -
 6 files changed, 9 insertions(+), 8 deletions(-)
 rename arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S} (100%)
 rename arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} (100%)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 6a87d592bd00..f18d20a68170 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -179,10 +179,12 @@ ifeq ($(KBUILD_EXTMOD),)
 # this hack.
 prepare: vdso_prepare
 vdso_prepare: prepare0
-	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
-	$(if $(CONFIG_COMPAT_VDSO),$(Q)$(MAKE) \
-		$(build)=arch/arm64/kernel/vdso32  \
-		include/generated/vdso32-offsets.h)
+	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso \
+	include/generated/vdso-offsets.h arch/arm64/kernel/vdso/vdso.so
+ifdef CONFIG_COMPAT_VDSO
+	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso32 \
+	include/generated/vdso32-offsets.h arch/arm64/kernel/vdso32/vdso.so
+endif
 endif
 
 define archhelp
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 86364ab6f13f..42f6ad2c7eac 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -59,9 +59,10 @@ obj-$(CONFIG_CRASH_CORE)		+= crash_core.o
 obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
 obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
 obj-$(CONFIG_ARM64_MTE)			+= mte.o
+obj-y					+= vdso-wrap.o
+obj-$(CONFIG_COMPAT_VDSO)		+= vdso32-wrap.o
 
-obj-y					+= vdso/ probes/
-obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
+obj-y					+= probes/
 head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds
 
diff --git a/arch/arm64/kernel/vdso/vdso.S b/arch/arm64/kernel/vdso-wrap.S
similarity index 100%
rename from arch/arm64/kernel/vdso/vdso.S
rename to arch/arm64/kernel/vdso-wrap.S
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index a8f8e409e2bf..85222f64f394 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -45,7 +45,6 @@ endif
 # Disable gcov profiling for VDSO code
 GCOV_PROFILE := n
 
-obj-y += vdso.o
 targets += vdso.lds
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
diff --git a/arch/arm64/kernel/vdso32/vdso.S b/arch/arm64/kernel/vdso32-wrap.S
similarity index 100%
rename from arch/arm64/kernel/vdso32/vdso.S
rename to arch/arm64/kernel/vdso32-wrap.S
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index a1e0f91e6cea..789ad420f16b 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -155,7 +155,6 @@ c-obj-vdso-gettimeofday := $(addprefix $(obj)/, $(c-obj-vdso-gettimeofday))
 asm-obj-vdso := $(addprefix $(obj)/, $(asm-obj-vdso))
 obj-vdso := $(c-obj-vdso) $(c-obj-vdso-gettimeofday) $(asm-obj-vdso)
 
-obj-y += vdso.o
 targets += vdso.lds
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: do not descend to vdso directories twice
  2020-12-18  2:45 [PATCH] arm64: do not descend to vdso directories twice Masahiro Yamada
@ 2020-12-21 14:39 ` Vincenzo Frascino
  2020-12-21 16:03   ` Masahiro Yamada
  2021-01-20 13:01 ` Will Deacon
  1 sibling, 1 reply; 5+ messages in thread
From: Vincenzo Frascino @ 2020-12-21 14:39 UTC (permalink / raw)
  To: Masahiro Yamada, Catalin Marinas, Will Deacon, linux-arm-kernel
  Cc: Michael Ellerman, linux-kernel

Hi Masahiro,

On 12/18/20 2:45 AM, Masahiro Yamada wrote:
> arm64 descends into each vdso directory twice; first in vdso_prepare,
> second during the ordinary build process.
> 
> PPC mimicked it and uncovered a problem [1]. In the first descend,
> Kbuild directly visits the vdso directories, therefore it does not
> inherit subdir-ccflags-y from upper directories.
> 
> This means the command line parameters may differ between the two.
> If it happens, the offset values in the generated headers might be
> different from real offsets of vdso.so in the kernel.
> 
> This potential danger should be avoided. The vdso directories are
> built in the vdso_prepare stage, so the second descend is unneeded.
> 
> [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARAkJ3_-4gX0VA2UkapbOftuzfSTVMBbgbw=HD8n7N+7w@mail.gmail.com/T/#ma10dcb961fda13f36d42d58fa6cb2da988b7e73a
> 

I could not reproduce the problem you are reporting on arm64. Could you please
provide some steps?

In my case the vDSO library is not rebuilt as a result of the procedure reported
in the email you linked at [1].

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  arch/arm64/Makefile                                | 10 ++++++----
>  arch/arm64/kernel/Makefile                         |  5 +++--
>  arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S}     |  0
>  arch/arm64/kernel/vdso/Makefile                    |  1 -
>  arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} |  0
>  arch/arm64/kernel/vdso32/Makefile                  |  1 -
>  6 files changed, 9 insertions(+), 8 deletions(-)
>  rename arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S} (100%)
>  rename arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} (100%)
> 
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 6a87d592bd00..f18d20a68170 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -179,10 +179,12 @@ ifeq ($(KBUILD_EXTMOD),)
>  # this hack.
>  prepare: vdso_prepare
>  vdso_prepare: prepare0
> -	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
> -	$(if $(CONFIG_COMPAT_VDSO),$(Q)$(MAKE) \
> -		$(build)=arch/arm64/kernel/vdso32  \
> -		include/generated/vdso32-offsets.h)
> +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso \
> +	include/generated/vdso-offsets.h arch/arm64/kernel/vdso/vdso.so
> +ifdef CONFIG_COMPAT_VDSO
> +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso32 \
> +	include/generated/vdso32-offsets.h arch/arm64/kernel/vdso32/vdso.so
> +endif
>  endif

The reason why it is currently done in two phases (a bit hacky as per comment)
is because vdso-offsets.h is required to be generated before compiling kernel/.
Please refer to the comment in arch/arm64/Makefile.

Could you explain how your change satisfies the dependency?

>  
>  define archhelp
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 86364ab6f13f..42f6ad2c7eac 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -59,9 +59,10 @@ obj-$(CONFIG_CRASH_CORE)		+= crash_core.o
>  obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
>  obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
>  obj-$(CONFIG_ARM64_MTE)			+= mte.o
> +obj-y					+= vdso-wrap.o
> +obj-$(CONFIG_COMPAT_VDSO)		+= vdso32-wrap.o
>  
> -obj-y					+= vdso/ probes/
> -obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
> +obj-y					+= probes/
>  head-y					:= head.o
>  extra-y					+= $(head-y) vmlinux.lds
>  
> diff --git a/arch/arm64/kernel/vdso/vdso.S b/arch/arm64/kernel/vdso-wrap.S
> similarity index 100%
> rename from arch/arm64/kernel/vdso/vdso.S
> rename to arch/arm64/kernel/vdso-wrap.S
> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> index a8f8e409e2bf..85222f64f394 100644
> --- a/arch/arm64/kernel/vdso/Makefile
> +++ b/arch/arm64/kernel/vdso/Makefile
> @@ -45,7 +45,6 @@ endif
>  # Disable gcov profiling for VDSO code
>  GCOV_PROFILE := n
>  
> -obj-y += vdso.o
>  targets += vdso.lds
>  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
>  
> diff --git a/arch/arm64/kernel/vdso32/vdso.S b/arch/arm64/kernel/vdso32-wrap.S
> similarity index 100%
> rename from arch/arm64/kernel/vdso32/vdso.S
> rename to arch/arm64/kernel/vdso32-wrap.S
> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> index a1e0f91e6cea..789ad420f16b 100644
> --- a/arch/arm64/kernel/vdso32/Makefile
> +++ b/arch/arm64/kernel/vdso32/Makefile
> @@ -155,7 +155,6 @@ c-obj-vdso-gettimeofday := $(addprefix $(obj)/, $(c-obj-vdso-gettimeofday))
>  asm-obj-vdso := $(addprefix $(obj)/, $(asm-obj-vdso))
>  obj-vdso := $(c-obj-vdso) $(c-obj-vdso-gettimeofday) $(asm-obj-vdso)
>  
> -obj-y += vdso.o
>  targets += vdso.lds
>  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
>  
> 

-- 
Regards,
Vincenzo

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: do not descend to vdso directories twice
  2020-12-21 14:39 ` Vincenzo Frascino
@ 2020-12-21 16:03   ` Masahiro Yamada
  2020-12-22 14:34     ` Vincenzo Frascino
  0 siblings, 1 reply; 5+ messages in thread
From: Masahiro Yamada @ 2020-12-21 16:03 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Michael Ellerman,
	Linux Kernel Mailing List

On Mon, Dec 21, 2020 at 11:36 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Hi Masahiro,
>
> On 12/18/20 2:45 AM, Masahiro Yamada wrote:
> > arm64 descends into each vdso directory twice; first in vdso_prepare,
> > second during the ordinary build process.
> >
> > PPC mimicked it and uncovered a problem [1]. In the first descend,
> > Kbuild directly visits the vdso directories, therefore it does not
> > inherit subdir-ccflags-y from upper directories.
> >
> > This means the command line parameters may differ between the two.
> > If it happens, the offset values in the generated headers might be
> > different from real offsets of vdso.so in the kernel.
> >
> > This potential danger should be avoided. The vdso directories are
> > built in the vdso_prepare stage, so the second descend is unneeded.
> >
> > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARAkJ3_-4gX0VA2UkapbOftuzfSTVMBbgbw=HD8n7N+7w@mail.gmail.com/T/#ma10dcb961fda13f36d42d58fa6cb2da988b7e73a
> >
>
> I could not reproduce the problem you are reporting on arm64. Could you please
> provide some steps?


As far as I see in arm64 Makefiles, you cannot reproduce it.
So, this is a _potential_ problem.

When somebody adds subdir-ccflags-y to arch/arm64/Kbuild or
arch/arm64/kernel/Makefile,
the real issue will suddenly come up.


arm64 is the first arch that implemented vdso-offsets.h

arch maintainers tend to copy Makefiles from existing ones.

Now PPC is the second with almost the same Makefile implementation.

This time, powerpc hit the problem because arch/powerpc/Kbuild
has subdir-ccflags-y.

I do not want to be bothered every time arch maintainers hit this issue.
So, I want to fix arm64 first,
then powerpc will eventually copy the correct Makefiles.




> In my case the vDSO library is not rebuilt as a result of the procedure reported
> in the email you linked at [1].
>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  arch/arm64/Makefile                                | 10 ++++++----
> >  arch/arm64/kernel/Makefile                         |  5 +++--
> >  arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S}     |  0
> >  arch/arm64/kernel/vdso/Makefile                    |  1 -
> >  arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} |  0
> >  arch/arm64/kernel/vdso32/Makefile                  |  1 -
> >  6 files changed, 9 insertions(+), 8 deletions(-)
> >  rename arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S} (100%)
> >  rename arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} (100%)
> >
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index 6a87d592bd00..f18d20a68170 100644
> > --- a/arch/arm64/Makefile
> > +++ b/arch/arm64/Makefile
> > @@ -179,10 +179,12 @@ ifeq ($(KBUILD_EXTMOD),)
> >  # this hack.
> >  prepare: vdso_prepare
> >  vdso_prepare: prepare0
> > -     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
> > -     $(if $(CONFIG_COMPAT_VDSO),$(Q)$(MAKE) \
> > -             $(build)=arch/arm64/kernel/vdso32  \
> > -             include/generated/vdso32-offsets.h)
> > +     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso \
> > +     include/generated/vdso-offsets.h arch/arm64/kernel/vdso/vdso.so
> > +ifdef CONFIG_COMPAT_VDSO
> > +     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso32 \
> > +     include/generated/vdso32-offsets.h arch/arm64/kernel/vdso32/vdso.so
> > +endif
> >  endif
>
> The reason why it is currently done in two phases (a bit hacky as per comment)
> is because vdso-offsets.h is required to be generated before compiling kernel/.
> Please refer to the comment in arch/arm64/Makefile.

Yes, I know.

>
> Could you explain how your change satisfies the dependency?

vdso_prepare completes before Kbuild starts ordinary descending.
So *.so files and vdso-offsets.h exist
when Kbuild builds arch/arm64/kernel/.






> >
> >  define archhelp
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index 86364ab6f13f..42f6ad2c7eac 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -59,9 +59,10 @@ obj-$(CONFIG_CRASH_CORE)           += crash_core.o
> >  obj-$(CONFIG_ARM_SDE_INTERFACE)              += sdei.o
> >  obj-$(CONFIG_ARM64_PTR_AUTH)         += pointer_auth.o
> >  obj-$(CONFIG_ARM64_MTE)                      += mte.o
> > +obj-y                                        += vdso-wrap.o
> > +obj-$(CONFIG_COMPAT_VDSO)            += vdso32-wrap.o
> >
> > -obj-y                                        += vdso/ probes/
> > -obj-$(CONFIG_COMPAT_VDSO)            += vdso32/
> > +obj-y                                        += probes/
> >  head-y                                       := head.o
> >  extra-y                                      += $(head-y) vmlinux.lds
> >
> > diff --git a/arch/arm64/kernel/vdso/vdso.S b/arch/arm64/kernel/vdso-wrap.S
> > similarity index 100%
> > rename from arch/arm64/kernel/vdso/vdso.S
> > rename to arch/arm64/kernel/vdso-wrap.S
> > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> > index a8f8e409e2bf..85222f64f394 100644
> > --- a/arch/arm64/kernel/vdso/Makefile
> > +++ b/arch/arm64/kernel/vdso/Makefile
> > @@ -45,7 +45,6 @@ endif
> >  # Disable gcov profiling for VDSO code
> >  GCOV_PROFILE := n
> >
> > -obj-y += vdso.o
> >  targets += vdso.lds
> >  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
> >
> > diff --git a/arch/arm64/kernel/vdso32/vdso.S b/arch/arm64/kernel/vdso32-wrap.S
> > similarity index 100%
> > rename from arch/arm64/kernel/vdso32/vdso.S
> > rename to arch/arm64/kernel/vdso32-wrap.S
> > diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> > index a1e0f91e6cea..789ad420f16b 100644
> > --- a/arch/arm64/kernel/vdso32/Makefile
> > +++ b/arch/arm64/kernel/vdso32/Makefile
> > @@ -155,7 +155,6 @@ c-obj-vdso-gettimeofday := $(addprefix $(obj)/, $(c-obj-vdso-gettimeofday))
> >  asm-obj-vdso := $(addprefix $(obj)/, $(asm-obj-vdso))
> >  obj-vdso := $(c-obj-vdso) $(c-obj-vdso-gettimeofday) $(asm-obj-vdso)
> >
> > -obj-y += vdso.o
> >  targets += vdso.lds
> >  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
> >
> >
>
> --
> Regards,
> Vincenzo



--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: do not descend to vdso directories twice
  2020-12-21 16:03   ` Masahiro Yamada
@ 2020-12-22 14:34     ` Vincenzo Frascino
  0 siblings, 0 replies; 5+ messages in thread
From: Vincenzo Frascino @ 2020-12-22 14:34 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Michael Ellerman,
	Linux Kernel Mailing List



On 12/21/20 4:03 PM, Masahiro Yamada wrote:
> On Mon, Dec 21, 2020 at 11:36 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>
>> Hi Masahiro,
>>
>> On 12/18/20 2:45 AM, Masahiro Yamada wrote:
>>> arm64 descends into each vdso directory twice; first in vdso_prepare,
>>> second during the ordinary build process.
>>>
>>> PPC mimicked it and uncovered a problem [1]. In the first descend,
>>> Kbuild directly visits the vdso directories, therefore it does not
>>> inherit subdir-ccflags-y from upper directories.
>>>
>>> This means the command line parameters may differ between the two.
>>> If it happens, the offset values in the generated headers might be
>>> different from real offsets of vdso.so in the kernel.
>>>
>>> This potential danger should be avoided. The vdso directories are
>>> built in the vdso_prepare stage, so the second descend is unneeded.
>>>
>>> [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARAkJ3_-4gX0VA2UkapbOftuzfSTVMBbgbw=HD8n7N+7w@mail.gmail.com/T/#ma10dcb961fda13f36d42d58fa6cb2da988b7e73a
>>>
>>
>> I could not reproduce the problem you are reporting on arm64. Could you please
>> provide some steps?
> 
> 
> As far as I see in arm64 Makefiles, you cannot reproduce it.
> So, this is a _potential_ problem.
> 
> When somebody adds subdir-ccflags-y to arch/arm64/Kbuild or
> arch/arm64/kernel/Makefile,
> the real issue will suddenly come up.
> 
> 
> arm64 is the first arch that implemented vdso-offsets.h
> 
> arch maintainers tend to copy Makefiles from existing ones.
> 
> Now PPC is the second with almost the same Makefile implementation.
> 
> This time, powerpc hit the problem because arch/powerpc/Kbuild
> has subdir-ccflags-y.
> 
> I do not want to be bothered every time arch maintainers hit this issue.
> So, I want to fix arm64 first,
> then powerpc will eventually copy the correct Makefiles.
> 
>

Ok this seems a problem related to a top down approach in the ccflags
propagation, which as it stands does not affect arm64, hence IMHO we should not
fix what is not broken, but I do not have a strong opinion on this.

> 
> 
>> In my case the vDSO library is not rebuilt as a result of the procedure reported
>> in the email you linked at [1].
>>
>>> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>>> ---
>>>
>>>  arch/arm64/Makefile                                | 10 ++++++----
>>>  arch/arm64/kernel/Makefile                         |  5 +++--
>>>  arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S}     |  0
>>>  arch/arm64/kernel/vdso/Makefile                    |  1 -
>>>  arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} |  0
>>>  arch/arm64/kernel/vdso32/Makefile                  |  1 -
>>>  6 files changed, 9 insertions(+), 8 deletions(-)
>>>  rename arch/arm64/kernel/{vdso/vdso.S => vdso-wrap.S} (100%)
>>>  rename arch/arm64/kernel/{vdso32/vdso.S => vdso32-wrap.S} (100%)
>>>
>>> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
>>> index 6a87d592bd00..f18d20a68170 100644
>>> --- a/arch/arm64/Makefile
>>> +++ b/arch/arm64/Makefile
>>> @@ -179,10 +179,12 @@ ifeq ($(KBUILD_EXTMOD),)
>>>  # this hack.
>>>  prepare: vdso_prepare
>>>  vdso_prepare: prepare0
>>> -     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
>>> -     $(if $(CONFIG_COMPAT_VDSO),$(Q)$(MAKE) \
>>> -             $(build)=arch/arm64/kernel/vdso32  \
>>> -             include/generated/vdso32-offsets.h)
>>> +     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso \
>>> +     include/generated/vdso-offsets.h arch/arm64/kernel/vdso/vdso.so
>>> +ifdef CONFIG_COMPAT_VDSO
>>> +     $(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso32 \
>>> +     include/generated/vdso32-offsets.h arch/arm64/kernel/vdso32/vdso.so
>>> +endif
>>>  endif
>>
>> The reason why it is currently done in two phases (a bit hacky as per comment)
>> is because vdso-offsets.h is required to be generated before compiling kernel/.
>> Please refer to the comment in arch/arm64/Makefile.
> 
> Yes, I know.
> 
>>
>> Could you explain how your change satisfies the dependency?
> 
> vdso_prepare completes before Kbuild starts ordinary descending.
> So *.so files and vdso-offsets.h exist
> when Kbuild builds arch/arm64/kernel/.
> 
> 

This is not immediately obvious, could you please mention it in the commit message?

Nit: Could you please re-organize the code as follows in order to limit the
changes (untested)?

$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h \
	arch/arm64/kernel/vdso/vdso.so
$(if $(CONFIG_COMPAT_VDSO),$(Q)$(MAKE) \
	$(build)=arch/arm64/kernel/vdso32  \
	include/generated/vdso32-offsets.h \
	arch/arm64/kernel/vdso32/vdso.so)

> 
> 
> 
> 
>>>
>>>  define archhelp
>>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>>> index 86364ab6f13f..42f6ad2c7eac 100644
>>> --- a/arch/arm64/kernel/Makefile
>>> +++ b/arch/arm64/kernel/Makefile
>>> @@ -59,9 +59,10 @@ obj-$(CONFIG_CRASH_CORE)           += crash_core.o
>>>  obj-$(CONFIG_ARM_SDE_INTERFACE)              += sdei.o
>>>  obj-$(CONFIG_ARM64_PTR_AUTH)         += pointer_auth.o
>>>  obj-$(CONFIG_ARM64_MTE)                      += mte.o
>>> +obj-y                                        += vdso-wrap.o
>>> +obj-$(CONFIG_COMPAT_VDSO)            += vdso32-wrap.o
>>>
>>> -obj-y                                        += vdso/ probes/
>>> -obj-$(CONFIG_COMPAT_VDSO)            += vdso32/
>>> +obj-y                                        += probes/
>>>  head-y                                       := head.o
>>>  extra-y                                      += $(head-y) vmlinux.lds
>>>
>>> diff --git a/arch/arm64/kernel/vdso/vdso.S b/arch/arm64/kernel/vdso-wrap.S
>>> similarity index 100%
>>> rename from arch/arm64/kernel/vdso/vdso.S
>>> rename to arch/arm64/kernel/vdso-wrap.S
>>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
>>> index a8f8e409e2bf..85222f64f394 100644
>>> --- a/arch/arm64/kernel/vdso/Makefile
>>> +++ b/arch/arm64/kernel/vdso/Makefile
>>> @@ -45,7 +45,6 @@ endif
>>>  # Disable gcov profiling for VDSO code
>>>  GCOV_PROFILE := n
>>>
>>> -obj-y += vdso.o
>>>  targets += vdso.lds
>>>  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
>>>
>>> diff --git a/arch/arm64/kernel/vdso32/vdso.S b/arch/arm64/kernel/vdso32-wrap.S
>>> similarity index 100%
>>> rename from arch/arm64/kernel/vdso32/vdso.S
>>> rename to arch/arm64/kernel/vdso32-wrap.S
>>> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
>>> index a1e0f91e6cea..789ad420f16b 100644
>>> --- a/arch/arm64/kernel/vdso32/Makefile
>>> +++ b/arch/arm64/kernel/vdso32/Makefile
>>> @@ -155,7 +155,6 @@ c-obj-vdso-gettimeofday := $(addprefix $(obj)/, $(c-obj-vdso-gettimeofday))
>>>  asm-obj-vdso := $(addprefix $(obj)/, $(asm-obj-vdso))
>>>  obj-vdso := $(c-obj-vdso) $(c-obj-vdso-gettimeofday) $(asm-obj-vdso)
>>>
>>> -obj-y += vdso.o
>>>  targets += vdso.lds
>>>  CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
>>>
>>>
>>
>> --
>> Regards,
>> Vincenzo
> 
> 
> 
> --
> Best Regards
> Masahiro Yamada
> 

-- 
Regards,
Vincenzo

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: do not descend to vdso directories twice
  2020-12-18  2:45 [PATCH] arm64: do not descend to vdso directories twice Masahiro Yamada
  2020-12-21 14:39 ` Vincenzo Frascino
@ 2021-01-20 13:01 ` Will Deacon
  1 sibling, 0 replies; 5+ messages in thread
From: Will Deacon @ 2021-01-20 13:01 UTC (permalink / raw)
  To: Catalin Marinas, Masahiro Yamada, linux-arm-kernel
  Cc: kernel-team, Will Deacon, Michael Ellerman, linux-kernel

On Fri, 18 Dec 2020 11:45:40 +0900, Masahiro Yamada wrote:
> arm64 descends into each vdso directory twice; first in vdso_prepare,
> second during the ordinary build process.
> 
> PPC mimicked it and uncovered a problem [1]. In the first descend,
> Kbuild directly visits the vdso directories, therefore it does not
> inherit subdir-ccflags-y from upper directories.
> 
> [...]

Applied to arm64 (for-next/vdso), thanks!

[1/1] arm64: do not descend to vdso directories twice
      https://git.kernel.org/arm64/c/a5b8ca97fbf8

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-20 14:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18  2:45 [PATCH] arm64: do not descend to vdso directories twice Masahiro Yamada
2020-12-21 14:39 ` Vincenzo Frascino
2020-12-21 16:03   ` Masahiro Yamada
2020-12-22 14:34     ` Vincenzo Frascino
2021-01-20 13:01 ` Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).