Re: [PATCH v2 1/2] arm64: Move handling of erratum 1418040 into C code

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Jul 31, 2020 at 11:00:34AM -0700, Stephen Boyd wrote:
> Quoting Will Deacon (2020-07-31 04:32:28)
> > On Fri, Jul 31, 2020 at 12:14:40PM +0100, Marc Zyngier wrote:
> > > On 2020-07-31 11:41, Will Deacon wrote:
> > > > On Fri, Jul 31, 2020 at 09:33:57AM +0100, Marc Zyngier wrote:
> > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > > > index 6089638c7d43..8bbf066224ab 100644
> > > > > --- a/arch/arm64/kernel/process.c
> > > > > +++ b/arch/arm64/kernel/process.c
> > > > > @@ -515,6 +515,40 @@ static void entry_task_switch(struct
> > > > > task_struct *next)
> > > > >   __this_cpu_write(__entry_task, next);
> > > > >  }
> > > > > 
> > > > > +/*
> > > > > + * ARM erratum 1418040 handling, affecting the 32bit view of CNTVCT.
> > > > > + * Assuming the virtual counter is enabled at the beginning of times:
> > > > > + *
> > > > > + * - disable access when switching from a 64bit task to a 32bit task
> > > > > + * - enable access when switching from a 32bit task to a 64bit task
> > > > > + */
> > > > > +static __always_inline
> > > > 
> > > > Do we need the __always_inline? None of the other calls from
> > > > __switch_to()
> > > > have it.
> > > 
> > > Suggestion from Stephen. In my experience, it doesn't change much as
> > > most things get inlined anyway. Happy to drop it.
> > 
> > Yes, please. We can add it back if it's shown to be a problem.
> 
> Just for my own edification, why is __always_inline undesirable? Should
> there be an always inline version of the function that has the static
> key so that the erratum path is kept out of the switch_to() default
> path?

It's rather unnecessary, so it just makes the code a bit more
unreadable. I usually go by two questions:

1. Is the function required to be inlined for correct execution?

2. Is there a visible performance benefit by using __always_inline?
   (e.g. the compiler does a bad job)

For a static function called only in one place, the compiler usually
inlines it.

I can't speak on behalf of Will but from my perspective I'm against
adding __always_inline just because it's harmless (and occasionally I
edit it out of patches I apply manually).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel