| Message ID | 6fc4b6faf718f56c4ca0ffb109fe80774299f558.1424091289.git.jan.kiszka@siemens.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: > We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to > happen for all cores. > > Fixing this resolves problems of KVM with emulating the generic > timer/counter. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S > index b7501fb..119c246 100644 > --- a/arch/arm/cpu/armv7/tegra-common/psci.S > +++ b/arch/arm/cpu/armv7/tegra-common/psci.S > @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) > > mrc p15, 0, r4, c0, c0, 5 @ MPIDR > and r4, r4, #7 @ number of CPUs in cluster > + > + adr r5, _sys_clock_freq > + cmp r4, #0 > + > + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 > + streq r7, [r5] > + > + ldrne r7, [r5] > + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 Is it not possible to have a hook that uses the same variable as arch_timer_init rather than doing a here copy? It seems a shame to duplicate the effort. Thanks, Mark. > + > bl psci_get_cpu_stack_top > mov sp, r5 > > bx r6 > ENDPROC(psci_arch_init) > > +_sys_clock_freq: > + .word > + > ENTRY(psci_cpu_off) > bl psci_cpu_off_common > > -- > 2.1.4 > >
On 2015-02-16 14:37, Mark Rutland wrote: > On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: >> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to >> happen for all cores. >> >> Fixing this resolves problems of KVM with emulating the generic >> timer/counter. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S >> index b7501fb..119c246 100644 >> --- a/arch/arm/cpu/armv7/tegra-common/psci.S >> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S >> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) >> >> mrc p15, 0, r4, c0, c0, 5 @ MPIDR >> and r4, r4, #7 @ number of CPUs in cluster >> + >> + adr r5, _sys_clock_freq >> + cmp r4, #0 >> + >> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 >> + streq r7, [r5] >> + >> + ldrne r7, [r5] >> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 > > Is it not possible to have a hook that uses the same variable as > arch_timer_init rather than doing a here copy? It seems a shame to > duplicate the effort. The problem is related to the different address spaces. Here we run in the secure monitor, arch_timer_init - to my understanding - in non-secure mode. Didn't find a pattern so far how to transfer data (and that shouldn't be more complex than the above code). Jan
On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote: > On 2015-02-16 14:37, Mark Rutland wrote: > > On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: > >> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to > >> happen for all cores. > >> > >> Fixing this resolves problems of KVM with emulating the generic > >> timer/counter. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> --- > >> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S > >> index b7501fb..119c246 100644 > >> --- a/arch/arm/cpu/armv7/tegra-common/psci.S > >> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S > >> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) > >> > >> mrc p15, 0, r4, c0, c0, 5 @ MPIDR > >> and r4, r4, #7 @ number of CPUs in cluster > >> + > >> + adr r5, _sys_clock_freq > >> + cmp r4, #0 > >> + > >> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 > >> + streq r7, [r5] > >> + > >> + ldrne r7, [r5] > >> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 > > > > Is it not possible to have a hook that uses the same variable as > > arch_timer_init rather than doing a here copy? It seems a shame to > > duplicate the effort. > > The problem is related to the different address spaces. Here we run in > the secure monitor, arch_timer_init - to my understanding - in > non-secure mode. Didn't find a pattern so far how to transfer data (and > that shouldn't be more complex than the above code). Surely arch_timer_init must be run in a secure mode in order to be allowed to write to CNTFRQ? If this is simply the easiest way of moving the data around then there's no real problem with it; it's just a shame that it only happens in the PSCI case. Thanks, Mark.
On 2015-02-16 14:51, Mark Rutland wrote: > On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote: >> On 2015-02-16 14:37, Mark Rutland wrote: >>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: >>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to >>>> happen for all cores. >>>> >>>> Fixing this resolves problems of KVM with emulating the generic >>>> timer/counter. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S >>>> index b7501fb..119c246 100644 >>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S >>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S >>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) >>>> >>>> mrc p15, 0, r4, c0, c0, 5 @ MPIDR >>>> and r4, r4, #7 @ number of CPUs in cluster >>>> + >>>> + adr r5, _sys_clock_freq >>>> + cmp r4, #0 >>>> + >>>> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 >>>> + streq r7, [r5] >>>> + >>>> + ldrne r7, [r5] >>>> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 >>> >>> Is it not possible to have a hook that uses the same variable as >>> arch_timer_init rather than doing a here copy? It seems a shame to >>> duplicate the effort. >> >> The problem is related to the different address spaces. Here we run in >> the secure monitor, arch_timer_init - to my understanding - in >> non-secure mode. Didn't find a pattern so far how to transfer data (and >> that shouldn't be more complex than the above code). > > Surely arch_timer_init must be run in a secure mode in order to be > allowed to write to CNTFRQ? Ah, right. > > If this is simply the easiest way of moving the data around then there's > no real problem with it; it's just a shame that it only happens in the > PSCI case. OK, I'll check again. Maybe it's easier than I thought. Jan
On 2015-02-16 15:02, Jan Kiszka wrote: > On 2015-02-16 14:51, Mark Rutland wrote: >> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote: >>> On 2015-02-16 14:37, Mark Rutland wrote: >>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: >>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to >>>>> happen for all cores. >>>>> >>>>> Fixing this resolves problems of KVM with emulating the generic >>>>> timer/counter. >>>>> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>> --- >>>>> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S >>>>> index b7501fb..119c246 100644 >>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S >>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S >>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) >>>>> >>>>> mrc p15, 0, r4, c0, c0, 5 @ MPIDR >>>>> and r4, r4, #7 @ number of CPUs in cluster >>>>> + >>>>> + adr r5, _sys_clock_freq >>>>> + cmp r4, #0 >>>>> + >>>>> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 >>>>> + streq r7, [r5] >>>>> + >>>>> + ldrne r7, [r5] >>>>> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 >>>> >>>> Is it not possible to have a hook that uses the same variable as >>>> arch_timer_init rather than doing a here copy? It seems a shame to >>>> duplicate the effort. >>> >>> The problem is related to the different address spaces. Here we run in >>> the secure monitor, arch_timer_init - to my understanding - in >>> non-secure mode. Didn't find a pattern so far how to transfer data (and >>> that shouldn't be more complex than the above code). >> >> Surely arch_timer_init must be run in a secure mode in order to be >> allowed to write to CNTFRQ? > > Ah, right. > >> >> If this is simply the easiest way of moving the data around then there's >> no real problem with it; it's just a shame that it only happens in the >> PSCI case. > > OK, I'll check again. Maybe it's easier than I thought. It isn't: the variable we would have to write - conditionally, i.e. not for the SPL build - is not yet where it will finally be. The monitor is copied later on, but the symbol points to the destination already. One can account for that, but the result won't get simpler and cleaner IMHO. Jan
On Tue, Feb 17, 2015 at 07:01:57AM +0000, Jan Kiszka wrote: > On 2015-02-16 15:02, Jan Kiszka wrote: > > On 2015-02-16 14:51, Mark Rutland wrote: > >> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote: > >>> On 2015-02-16 14:37, Mark Rutland wrote: > >>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: > >>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to > >>>>> happen for all cores. > >>>>> > >>>>> Fixing this resolves problems of KVM with emulating the generic > >>>>> timer/counter. > >>>>> > >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >>>>> --- > >>>>> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ > >>>>> 1 file changed, 13 insertions(+) > >>>>> > >>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S > >>>>> index b7501fb..119c246 100644 > >>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S > >>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S > >>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) > >>>>> > >>>>> mrc p15, 0, r4, c0, c0, 5 @ MPIDR > >>>>> and r4, r4, #7 @ number of CPUs in cluster > >>>>> + > >>>>> + adr r5, _sys_clock_freq > >>>>> + cmp r4, #0 > >>>>> + > >>>>> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 > >>>>> + streq r7, [r5] > >>>>> + > >>>>> + ldrne r7, [r5] > >>>>> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 > >>>> > >>>> Is it not possible to have a hook that uses the same variable as > >>>> arch_timer_init rather than doing a here copy? It seems a shame to > >>>> duplicate the effort. > >>> > >>> The problem is related to the different address spaces. Here we run in > >>> the secure monitor, arch_timer_init - to my understanding - in > >>> non-secure mode. Didn't find a pattern so far how to transfer data (and > >>> that shouldn't be more complex than the above code). > >> > >> Surely arch_timer_init must be run in a secure mode in order to be > >> allowed to write to CNTFRQ? > > > > Ah, right. > > > >> > >> If this is simply the easiest way of moving the data around then there's > >> no real problem with it; it's just a shame that it only happens in the > >> PSCI case. > > > > OK, I'll check again. Maybe it's easier than I thought. > > It isn't: the variable we would have to write - conditionally, i.e. not > for the SPL build - is not yet where it will finally be. The monitor is > copied later on, but the symbol points to the destination already. One > can account for that, but the result won't get simpler and cleaner IMHO. Fair enough. As I mentioned earlier there's no real problem with the above, it just seemed a shame that this was only done in the PSCI case. I take it for the quoted sequence above that the primary CPU runs through this path before sending the secondaries through (and that either there's a DSB somewhere between the write and waking up secondaries or memory accesses are strongly ordered at this point). Thanks, Mark.
On 2015-02-17 11:21, Mark Rutland wrote: > On Tue, Feb 17, 2015 at 07:01:57AM +0000, Jan Kiszka wrote: >> On 2015-02-16 15:02, Jan Kiszka wrote: >>> On 2015-02-16 14:51, Mark Rutland wrote: >>>> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote: >>>>> On 2015-02-16 14:37, Mark Rutland wrote: >>>>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote: >>>>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to >>>>>>> happen for all cores. >>>>>>> >>>>>>> Fixing this resolves problems of KVM with emulating the generic >>>>>>> timer/counter. >>>>>>> >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> --- >>>>>>> arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ >>>>>>> 1 file changed, 13 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S >>>>>>> index b7501fb..119c246 100644 >>>>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S >>>>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S >>>>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) >>>>>>> >>>>>>> mrc p15, 0, r4, c0, c0, 5 @ MPIDR >>>>>>> and r4, r4, #7 @ number of CPUs in cluster >>>>>>> + >>>>>>> + adr r5, _sys_clock_freq >>>>>>> + cmp r4, #0 >>>>>>> + >>>>>>> + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 >>>>>>> + streq r7, [r5] >>>>>>> + >>>>>>> + ldrne r7, [r5] >>>>>>> + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 >>>>>> >>>>>> Is it not possible to have a hook that uses the same variable as >>>>>> arch_timer_init rather than doing a here copy? It seems a shame to >>>>>> duplicate the effort. >>>>> >>>>> The problem is related to the different address spaces. Here we run in >>>>> the secure monitor, arch_timer_init - to my understanding - in >>>>> non-secure mode. Didn't find a pattern so far how to transfer data (and >>>>> that shouldn't be more complex than the above code). >>>> >>>> Surely arch_timer_init must be run in a secure mode in order to be >>>> allowed to write to CNTFRQ? >>> >>> Ah, right. >>> >>>> >>>> If this is simply the easiest way of moving the data around then there's >>>> no real problem with it; it's just a shame that it only happens in the >>>> PSCI case. >>> >>> OK, I'll check again. Maybe it's easier than I thought. >> >> It isn't: the variable we would have to write - conditionally, i.e. not >> for the SPL build - is not yet where it will finally be. The monitor is >> copied later on, but the symbol points to the destination already. One >> can account for that, but the result won't get simpler and cleaner IMHO. > > Fair enough. As I mentioned earlier there's no real problem with the > above, it just seemed a shame that this was only done in the PSCI case. > > I take it for the quoted sequence above that the primary CPU runs > through this path before sending the secondaries through (and that > either there's a DSB somewhere between the write and waking up > secondaries or memory accesses are strongly ordered at this point). Yes to both: The primary performs this init before starting the target OS, and the dsb is at latest in psci_cpu_on before kicking off a secondary CPU. Jan
diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S index b7501fb..119c246 100644 --- a/arch/arm/cpu/armv7/tegra-common/psci.S +++ b/arch/arm/cpu/armv7/tegra-common/psci.S @@ -51,12 +51,25 @@ ENTRY(psci_arch_init) mrc p15, 0, r4, c0, c0, 5 @ MPIDR and r4, r4, #7 @ number of CPUs in cluster + + adr r5, _sys_clock_freq + cmp r4, #0 + + mrceq p15, 0, r7, c14, c0, 0 @ read CNTFRQ from CPU0 + streq r7, [r5] + + ldrne r7, [r5] + mcrne p15, 0, r7, c14, c0, 0 @ write CNTFRQ to CPU1..3 + bl psci_get_cpu_stack_top mov sp, r5 bx r6 ENDPROC(psci_arch_init) +_sys_clock_freq: + .word + ENTRY(psci_cpu_off) bl psci_cpu_off_common
We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to happen for all cores. Fixing this resolves problems of KVM with emulating the generic timer/counter. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++ 1 file changed, 13 insertions(+)