Re: [PATCH v5 07/13] ARM: smp: Add initialization of CNTVOFF

From: MylÃne Josserand
Date: Wed Apr 04 2018 - 09:59:28 EST


Hi Marc,

Thank you for the review.

On Wed, 4 Apr 2018 14:01:48 +0100
Marc Zyngier <marc.zyngier@xxxxxxx> wrote:

> Hi MylÃne,
>
> On 03/04/18 07:18, MylÃne Josserand wrote:
> > The CNTVOFF register from arch timer is uninitialized.
> > It should be done by the bootloader but it is currently not the case,
> > even for boot CPU because this SoC is booting in secure mode.
> > It leads to an random offset value meaning that each CPU will have a
> > different time, which isn't working very well.
> >
> > Add assembly code used for boot CPU and secondary CPU cores to make
> > sure that the CNTVOFF register is initialized. Because this code can
> > be used by different platforms, add this assembly file in ARM's common
> > folder.
> >
> > Signed-off-by: MylÃne Josserand <mylene.josserand@xxxxxxxxxxx>
> > ---
> > arch/arm/common/Makefile | 1 +
> > arch/arm/common/smp_cntvoff.S | 35 +++++++++++++++++++++++++++++++++++
> > arch/arm/include/asm/smp_cntvoff.h | 8 ++++++++
> > 3 files changed, 44 insertions(+)
> > create mode 100644 arch/arm/common/smp_cntvoff.S
> > create mode 100644 arch/arm/include/asm/smp_cntvoff.h
> >
> > diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
> > index 70b4a14ed993..83117deb86c8 100644
> > --- a/arch/arm/common/Makefile
> > +++ b/arch/arm/common/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_DMABOUNCE) += dmabounce.o
> > obj-$(CONFIG_SHARP_LOCOMO) += locomo.o
> > obj-$(CONFIG_SHARP_PARAM) += sharpsl_param.o
> > obj-$(CONFIG_SHARP_SCOOP) += scoop.o
> > +obj-$(CONFIG_SMP) += smp_cntvoff.o
> > obj-$(CONFIG_PCI_HOST_ITE8152) += it8152.o
> > obj-$(CONFIG_MCPM) += mcpm_head.o mcpm_entry.o mcpm_platsmp.o vlock.o
> > CFLAGS_REMOVE_mcpm_entry.o = -pg
> > diff --git a/arch/arm/common/smp_cntvoff.S b/arch/arm/common/smp_cntvoff.S
> > new file mode 100644
> > index 000000000000..65ed199a50fe
> > --- /dev/null
> > +++ b/arch/arm/common/smp_cntvoff.S
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Chen-Yu Tsai
> > + * Copyright (c) 2018 Bootlin
> > + *
> > + * Chen-Yu Tsai <wens@xxxxxxxx>
> > + * MylÃne Josserand <mylene.josserand@xxxxxxxxxxx>
>
> Given that this is literally lifted from shmobile_init_cntvoff, the
> whole attribution is a bit dubious.

Yes, sorry, I will fix that.

>
> > + *
> > + * SMP support for sunxi based systems with Cortex A7/A15
>
> That's not restricted to sunxi, is it?

Nope, I will update this line too (bad copy-paste from
sunxi/headsmp.S...)

>
> > + *
> > + */
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/assembler.h>
> > +
> > +ENTRY(smp_init_cntvoff)
>
> The name doesn't quite reflect the usage constraints. This will only
> work if used from secure, and is UNPREDICTABLE otherwise (see the CPS
> instruction). Also, the "smp" prefix is quite misleading, as it only
> affects the current CPU, and not any other.
>
> How about secure_cntvoff_init instead? Same thing for the file name.

Sure, this name is fine for me.

>
> > + .arch armv7-a
> > + /*
> > + * CNTVOFF has to be initialized either from non-secure Hypervisor
> > + * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> > + * then it should be handled by the secure code
> > + */
> > + cps #MON_MODE
> > + mrc p15, 0, r1, c1, c1, 0 /* Get Secure Config */
> > + orr r0, r1, #1
> > + mcr p15, 0, r0, c1, c1, 0 /* Set Non Secure bit */
> > + isb
> > + mov r0, #0
> > + mcrr p15, 4, r0, r0, c14 /* CNTVOFF = 0 */
> > + isb
> > + mcr p15, 0, r1, c1, c1, 0 /* Set Secure bit */
> > + isb
> > + cps #SVC_MODE
> > + ret lr
> > +ENDPROC(smp_init_cntvoff)
> > diff --git a/arch/arm/include/asm/smp_cntvoff.h b/arch/arm/include/asm/smp_cntvoff.h
> > new file mode 100644
> > index 000000000000..59a95f7604ee
> > --- /dev/null
> > +++ b/arch/arm/include/asm/smp_cntvoff.h
> > @@ -0,0 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#ifndef __ASMARM_ARCH_CNTVOFF_H
> > +#define __ASMARM_ARCH_CNTVOFF_H
> > +
> > +extern void smp_init_cntvoff(void);
> > +
> > +#endif
> >
>
> It'd be good to take this opportunity to refactor the shmobile code.

I can do it in this series but I do not have any shmobile platforms so
I will not be able to test my modifications (only compilation).

If someone can test it for me (who?), it is okay for me to refactor this
code :)

Best regards,

--
MylÃne Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com