[PATCH 12/17] powerpc/vdso: Switch to generic storage implementation

[Thomas Weißschuh]

Hi Christophe,

On Wed, Dec 18, 2024 at 08:20:51AM +0100, Christophe Leroy wrote:
> Le 16/12/2024 à 15:10, Thomas Weißschuh a écrit :

[..]

> >   #ifdef CONFIG_TIME_NS
> > -static __always_inline
> > -const struct vdso_data *__arch_get_timens_vdso_data(const struct vdso_data *vd)
> > +static __always_inline const struct vdso_time_data *__ppc_get_vdso_u_timens_data(void)
> >   {
> > -	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> > +	struct vdso_time_data *time_data;
> > +
> > +	asm(
> > +		"	bcl	20, 31, .+4\n"
> > +		"0:	mflr	%0\n"
> > +		"	addis	%0, %0, (vdso_u_timens_data - 0b)@ha\n"
> > +		"	addi	%0, %0, (vdso_u_timens_data - 0b)@l\n"
> > +	: "=r" (time_data) :: "lr");
> > +
> > +	return time_data;
> 
> Please don't do that, it kills optimisation efforts done when implementing
> VDSO time. Commit ce7d8056e38b ("powerpc/vdso: Prepare for switching VDSO to
> generic C implementation.") explains why.
> 
> For time data, the bcl/mflr dance is done by get_datapage macro called by
> cvdso_call macro in gettimeofday.S, and given to
> __cvdso_clock_gettime_data() by __c_kernel_clock_gettime() in
> vgettimeofday.c . Use that information and don't redo the bcl/mflr sequence.

So instead keeping the logic of this:

static __always_inline
const struct vdso_data *__arch_get_timens_vdso_data(const struct vdso_data *vd)
{
	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
}

Makes sense.

Adding a constant value should be cheaper or just as cheap as a
PC-relative addressing for all architectures, so it can go into the
generic code, too.

[..]

> >   }
> > +#define __arch_get_vdso_u_timens_data __ppc_get_vdso_u_timens_data
> 
> There is not #ifdef __arch_get_vdso_u_timens_data anywhere, this #define is
> not needed, the function should be called __arch_get_vdso_u_timens_data()
> directly as before, unnecessary indirections reduce readability.

I'll see how this works out with the include order and conflicts with
symbols in include/vdso/datapage.h.

> >   #endif
> > -static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
> > +static inline bool vdso_clocksource_ok(const struct vdso_time_data *vd)
> >   {
> >   	return true;
> >   }


Thanks!
Thomas