On Fri, Oct 13, 2017 at 03:21:59PM +0100, Marc Zyngier wrote:
> On 13/10/17 15:15, Dave Martin wrote:
> > On Thu, Oct 12, 2017 at 12:28:32PM +0100, Marc Zyngier wrote:
> >> On 12/10/17 12:04, Dave Martin wrote:
> >>> On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote:
> >>>> [+ Christoffer]
> >>>>
> >>>> On 10/10/17 19:38, Dave Martin wrote:
[...]
> >>>> Hmmm. How does this work if...
> >>>
> >>> !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so
> >>> if CONFIG_ARM64_SVE is not set, the call is optimised away.
> >>>
> >>> [...]
> >>>
> >>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> >>>>> index a9cb794..6ae3703 100644
> >>>>> --- a/arch/arm64/kernel/fpsimd.c
> >>>>> +++ b/arch/arm64/kernel/fpsimd.c
> >>>>> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t)
> >>>
> >>> [...]
> >>>
> >>>>> +#ifdef CONFIG_ARM64_SVE
> >>>>> +void sve_flush_cpu_state(void)
> >>>>> +{
> >>>>> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state);
> >>>>> + struct task_struct *tsk;
> >>>>> +
> >>>>> + if (!fpstate)
> >>>>> + return;
> >>>>> +
> >>>>> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state);
> >>>>> + if (test_tsk_thread_flag(tsk, TIF_SVE))
> >>>>> + fpsimd_flush_cpu_state();
> >>>>> +}
> >>>>> +#endif /* CONFIG_ARM64_SVE */
> >>>>
> >>>> ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of
> >>>> moving the #ifdef/#endif inside the function...
> >>>
> >>> Because sve_flush_cpu_state() is not in the same compilation unit it
> >>> can't be static, and that means the compiler won't remove it
> >>> automatically if it's unused -- hence the #ifdef.
> >>>
> >>> Because the call site is optimised away, there is no link failure.
> >>>
> >>> Don't we rely on this sort of thing all over the place?
> >> Dunno. It just feels weird. But if you are sure that it won't break,
> >> fine by me. I guess we'll find out pretty quickly how this fares,
> >> specially with older toolchains.
> >
> > I thought this was why the kernel doesn't support building with -O0.
> > There are many instances of this in the series, not just here.
> >
> > Let me know if you feel this isn't good enough though.
>
> That's OK to me. As I said, we'll find out pretty quickly if anything
> breaks unexpectedly.
Yup
>
> > Do you have any other comments on this patch?
> None. You can add my:
>
> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
OK, thanks for the input.
Cheers
---Dave
