| Message ID | 1542869577-32435-2-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] libtomcrypt: Import SHA512/256 approved hash algorithm | expand |
Hi Daniel, On Thu, 22 Nov 2018 at 12:23, Sumit Garg <sumit.garg@linaro.org> wrote: > > Currently there is no means to perform background housekeeping in > secure world on Synquacer platforms. Provide an (optional) periodic > timer to allow any housekeeping to be performed. > > Although it could be expanded, at present the code is fairly simple > because we expect only a single PTA to exploit the timer interrupt. > The secure timer interrupt is configured to fire every 2ms. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > core/arch/arm/include/arm64.h | 4 ++ > core/arch/arm/plat-synquacer/main.c | 30 ++++++++++-- > core/arch/arm/plat-synquacer/platform_config.h | 2 + > core/arch/arm/plat-synquacer/sub.mk | 1 + > core/arch/arm/plat-synquacer/timer_fiq.c | 67 ++++++++++++++++++++++++++ > core/arch/arm/plat-synquacer/timer_fiq.h | 13 +++++ > 6 files changed, 112 insertions(+), 5 deletions(-) > create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.c > create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.h > > diff --git a/core/arch/arm/include/arm64.h b/core/arch/arm/include/arm64.h > index 2c1fd8c..0cf14c0 100644 > --- a/core/arch/arm/include/arm64.h > +++ b/core/arch/arm/include/arm64.h > @@ -305,6 +305,10 @@ DEFINE_REG_READ_FUNC_(cntfrq, uint32_t, cntfrq_el0) > DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0) > DEFINE_REG_READ_FUNC_(cntkctl, uint32_t, cntkctl_el1) > DEFINE_REG_WRITE_FUNC_(cntkctl, uint32_t, cntkctl_el1) > +DEFINE_REG_READ_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > +DEFINE_REG_WRITE_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > +DEFINE_REG_READ_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > +DEFINE_REG_WRITE_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > > DEFINE_REG_READ_FUNC_(pmccntr, uint64_t, pmccntr_el0) > > diff --git a/core/arch/arm/plat-synquacer/main.c b/core/arch/arm/plat-synquacer/main.c > index c3aac4c..714becd 100644 > --- a/core/arch/arm/plat-synquacer/main.c > +++ b/core/arch/arm/plat-synquacer/main.c > @@ -18,6 +18,7 @@ > #include <sm/optee_smc.h> > #include <tee/entry_fast.h> > #include <tee/entry_std.h> > +#include <timer_fiq.h> > > static void main_fiq(void); > > @@ -46,7 +47,7 @@ const struct thread_handlers *generic_boot_get_handlers(void) > > static void main_fiq(void) > { > - panic(); > + gic_it_handle(&gic_data); > } > > void console_init(void) > @@ -66,12 +67,31 @@ void main_init_gic(void) > if (!gicd_base) > panic(); > > - /* Initialize GIC */ > - gic_init(&gic_data, 0, gicd_base); > + /* On ARMv8-A, GIC configuration is initialized in TF-A */ > + gic_init_base_addr(&gic_data, 0, gicd_base); > + > itr_init(&gic_data.chip); > } > > -void main_secondary_init_gic(void) > +static enum itr_return timer_itr_cb(struct itr_handler *h __unused) > +{ > + /* Reset timer for next FIQ */ > + generic_timer_handler(); > + > + return ITRR_HANDLED; > +} > + > +static struct itr_handler timer_itr = { > + .it = IT_SEC_TIMER, > + .flags = ITRF_TRIGGER_LEVEL, > + .handler = timer_itr_cb, > +}; > + > +static TEE_Result init_timer_itr(void) > { > - gic_cpu_init(&gic_data); > + itr_add(&timer_itr); > + itr_enable(IT_SEC_TIMER); > + > + return TEE_SUCCESS; > } > +driver_init(init_timer_itr); > diff --git a/core/arch/arm/plat-synquacer/platform_config.h b/core/arch/arm/plat-synquacer/platform_config.h > index 4d6d545..f9b1b40 100644 > --- a/core/arch/arm/plat-synquacer/platform_config.h > +++ b/core/arch/arm/plat-synquacer/platform_config.h > @@ -19,6 +19,8 @@ > #define CONSOLE_UART_CLK_IN_HZ 62500000 > #define CONSOLE_BAUDRATE 115200 > > +#define IT_SEC_TIMER 29 > + > #define DRAM0_BASE 0x80000000 > > /* Platform specific defines */ > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > index 8ddc2fd..cfa1dc3 100644 > --- a/core/arch/arm/plat-synquacer/sub.mk > +++ b/core/arch/arm/plat-synquacer/sub.mk > @@ -1,2 +1,3 @@ > global-incdirs-y += . > srcs-y += main.c > +srcs-y += timer_fiq.c > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > new file mode 100644 > index 0000000..c775bb9 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include <arm.h> > +#include <console.h> > +#include <drivers/gic.h> > +#include <io.h> > +#include <kernel/panic.h> > +#include <kernel/misc.h> > +#include <kernel/spinlock.h> > +#include <timer_fiq.h> > + > +static unsigned int timer_lock = SPINLOCK_UNLOCK; > +static bool timer_fiq_running; > + > +void generic_timer_start(void) > +{ > + uint64_t cval; > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&timer_lock); > + > + if (timer_fiq_running == true) > + goto exit; > + > + /* The timer will fire every 2 ms */ > + cval = read_cntpct() + (read_cntfrq() / 500); > + write_cntps_cval(cval); > + > + /* Enable the secure physical timer */ > + write_cntps_ctl(1); > + > + timer_fiq_running = true; > + > +exit: > + cpu_spin_unlock(&timer_lock); > + thread_set_exceptions(exceptions); > +} > + > +void generic_timer_stop(void) > +{ > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&timer_lock); > + > + /* Disable the timer */ > + write_cntps_ctl(0); > + > + timer_fiq_running = false; > + > + cpu_spin_unlock(&timer_lock); > + thread_set_exceptions(exceptions); > +} > + > +void generic_timer_handler(void) > +{ > + uint64_t cval; > + > + /* Ensure that the timer did assert the interrupt */ > + assert((read_cntps_ctl() >> 2)); > + > + /* Reconfigure timer to fire every 2 ms */ > + cval = read_cntpct() + (read_cntfrq() / 500); Here I have still used cntpct value to reconfigure cntps_cval as I tried to use cntps_cval but it leads to following situation: At a particular point of time cntpct value becomes greater than (cntps_cval + 2ms) due to accumulating delay leading to timer condition always true and thereby generating interrupts without any delay. -Sumit > + write_cntps_cval(cval); > +} > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > new file mode 100644 > index 0000000..4f2091a > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#ifndef __TIMER_FIQ_H > +#define __TIMER_FIQ_H > + > +void generic_timer_start(void); > +void generic_timer_stop(void); > +void generic_timer_handler(void); > + > +#endif /* __TIMER_FIQ_H */ > -- > 2.7.4 >
On Thu, 22 Nov 2018 at 12:42, Sumit Garg <sumit.garg@linaro.org> wrote: > > Hi Daniel, > > On Thu, 22 Nov 2018 at 12:23, Sumit Garg <sumit.garg@linaro.org> wrote: > > > > Currently there is no means to perform background housekeeping in > > secure world on Synquacer platforms. Provide an (optional) periodic > > timer to allow any housekeeping to be performed. > > > > Although it could be expanded, at present the code is fairly simple > > because we expect only a single PTA to exploit the timer interrupt. > > The secure timer interrupt is configured to fire every 2ms. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > core/arch/arm/include/arm64.h | 4 ++ > > core/arch/arm/plat-synquacer/main.c | 30 ++++++++++-- > > core/arch/arm/plat-synquacer/platform_config.h | 2 + > > core/arch/arm/plat-synquacer/sub.mk | 1 + > > core/arch/arm/plat-synquacer/timer_fiq.c | 67 ++++++++++++++++++++++++++ > > core/arch/arm/plat-synquacer/timer_fiq.h | 13 +++++ > > 6 files changed, 112 insertions(+), 5 deletions(-) > > create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.c > > create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.h > > > > diff --git a/core/arch/arm/include/arm64.h b/core/arch/arm/include/arm64.h > > index 2c1fd8c..0cf14c0 100644 > > --- a/core/arch/arm/include/arm64.h > > +++ b/core/arch/arm/include/arm64.h > > @@ -305,6 +305,10 @@ DEFINE_REG_READ_FUNC_(cntfrq, uint32_t, cntfrq_el0) > > DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0) > > DEFINE_REG_READ_FUNC_(cntkctl, uint32_t, cntkctl_el1) > > DEFINE_REG_WRITE_FUNC_(cntkctl, uint32_t, cntkctl_el1) > > +DEFINE_REG_READ_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > > +DEFINE_REG_WRITE_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > > +DEFINE_REG_READ_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > > +DEFINE_REG_WRITE_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > > > > DEFINE_REG_READ_FUNC_(pmccntr, uint64_t, pmccntr_el0) > > > > diff --git a/core/arch/arm/plat-synquacer/main.c b/core/arch/arm/plat-synquacer/main.c > > index c3aac4c..714becd 100644 > > --- a/core/arch/arm/plat-synquacer/main.c > > +++ b/core/arch/arm/plat-synquacer/main.c > > @@ -18,6 +18,7 @@ > > #include <sm/optee_smc.h> > > #include <tee/entry_fast.h> > > #include <tee/entry_std.h> > > +#include <timer_fiq.h> > > > > static void main_fiq(void); > > > > @@ -46,7 +47,7 @@ const struct thread_handlers *generic_boot_get_handlers(void) > > > > static void main_fiq(void) > > { > > - panic(); > > + gic_it_handle(&gic_data); > > } > > > > void console_init(void) > > @@ -66,12 +67,31 @@ void main_init_gic(void) > > if (!gicd_base) > > panic(); > > > > - /* Initialize GIC */ > > - gic_init(&gic_data, 0, gicd_base); > > + /* On ARMv8-A, GIC configuration is initialized in TF-A */ > > + gic_init_base_addr(&gic_data, 0, gicd_base); > > + > > itr_init(&gic_data.chip); > > } > > > > -void main_secondary_init_gic(void) > > +static enum itr_return timer_itr_cb(struct itr_handler *h __unused) > > +{ > > + /* Reset timer for next FIQ */ > > + generic_timer_handler(); > > + > > + return ITRR_HANDLED; > > +} > > + > > +static struct itr_handler timer_itr = { > > + .it = IT_SEC_TIMER, > > + .flags = ITRF_TRIGGER_LEVEL, > > + .handler = timer_itr_cb, > > +}; > > + > > +static TEE_Result init_timer_itr(void) > > { > > - gic_cpu_init(&gic_data); > > + itr_add(&timer_itr); > > + itr_enable(IT_SEC_TIMER); > > + > > + return TEE_SUCCESS; > > } > > +driver_init(init_timer_itr); > > diff --git a/core/arch/arm/plat-synquacer/platform_config.h b/core/arch/arm/plat-synquacer/platform_config.h > > index 4d6d545..f9b1b40 100644 > > --- a/core/arch/arm/plat-synquacer/platform_config.h > > +++ b/core/arch/arm/plat-synquacer/platform_config.h > > @@ -19,6 +19,8 @@ > > #define CONSOLE_UART_CLK_IN_HZ 62500000 > > #define CONSOLE_BAUDRATE 115200 > > > > +#define IT_SEC_TIMER 29 > > + > > #define DRAM0_BASE 0x80000000 > > > > /* Platform specific defines */ > > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > > index 8ddc2fd..cfa1dc3 100644 > > --- a/core/arch/arm/plat-synquacer/sub.mk > > +++ b/core/arch/arm/plat-synquacer/sub.mk > > @@ -1,2 +1,3 @@ > > global-incdirs-y += . > > srcs-y += main.c > > +srcs-y += timer_fiq.c > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > > new file mode 100644 > > index 0000000..c775bb9 > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > > @@ -0,0 +1,67 @@ > > +// SPDX-License-Identifier: BSD-2-Clause > > +/* > > + * Copyright (c) 2018, Linaro Limited > > + */ > > + > > +#include <arm.h> > > +#include <console.h> > > +#include <drivers/gic.h> > > +#include <io.h> > > +#include <kernel/panic.h> > > +#include <kernel/misc.h> > > +#include <kernel/spinlock.h> > > +#include <timer_fiq.h> > > + > > +static unsigned int timer_lock = SPINLOCK_UNLOCK; > > +static bool timer_fiq_running; > > + > > +void generic_timer_start(void) > > +{ > > + uint64_t cval; > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > + > > + cpu_spin_lock(&timer_lock); > > + > > + if (timer_fiq_running == true) > > + goto exit; > > + > > + /* The timer will fire every 2 ms */ > > + cval = read_cntpct() + (read_cntfrq() / 500); > > + write_cntps_cval(cval); > > + > > + /* Enable the secure physical timer */ > > + write_cntps_ctl(1); > > + > > + timer_fiq_running = true; > > + > > +exit: > > + cpu_spin_unlock(&timer_lock); > > + thread_set_exceptions(exceptions); > > +} > > + > > +void generic_timer_stop(void) > > +{ > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > + > > + cpu_spin_lock(&timer_lock); > > + > > + /* Disable the timer */ > > + write_cntps_ctl(0); > > + > > + timer_fiq_running = false; > > + > > + cpu_spin_unlock(&timer_lock); > > + thread_set_exceptions(exceptions); > > +} > > + > > +void generic_timer_handler(void) > > +{ > > + uint64_t cval; > > + > > + /* Ensure that the timer did assert the interrupt */ > > + assert((read_cntps_ctl() >> 2)); > > + > > + /* Reconfigure timer to fire every 2 ms */ > > + cval = read_cntpct() + (read_cntfrq() / 500); > > Here I have still used cntpct value to reconfigure cntps_cval as I > tried to use cntps_cval but it leads to following situation: > > At a particular point of time cntpct value becomes greater than > (cntps_cval + 2ms) due to accumulating delay leading to timer > condition always true and thereby generating interrupts without any > delay. > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 reference manual), it seems that there is another TimerValue view that looks more appropriate for our usage than CounterValue view used in this patch. So I have tried to use it as follows. It works as expected. +void generic_timer_handler(void) +{ + /* Ensure that the timer did assert the interrupt */ + assert((read_cntps_ctl() >> 2)); + + /* Disable the timer */ + write_cntps_ctl(0); + + /* Reconfigure timer to fire every 2 ms */ + write_cntps_tval(read_cntfrq() / 500); + + /* Enable the secure physical timer */ + write_cntps_ctl(1); +} Here to reconfigure the timer value, we need to disable/enable timer as per Section D7.5.13 CNTPS_TVAL_EL1, ARMv8 reference manual. Please share your views regarding this. -Sumit > -Sumit > > > + write_cntps_cval(cval); > > +} > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > new file mode 100644 > > index 0000000..4f2091a > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > @@ -0,0 +1,13 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2018, Linaro Limited > > + */ > > + > > +#ifndef __TIMER_FIQ_H > > +#define __TIMER_FIQ_H > > + > > +void generic_timer_start(void); > > +void generic_timer_stop(void); > > +void generic_timer_handler(void); > > + > > +#endif /* __TIMER_FIQ_H */ > > -- > > 2.7.4 > >
On Thu, Nov 22, 2018 at 01:31:38PM +0530, Sumit Garg wrote: > > > +void generic_timer_handler(void) > > > +{ > > > + uint64_t cval; > > > + > > > + /* Ensure that the timer did assert the interrupt */ > > > + assert((read_cntps_ctl() >> 2)); > > > + > > > + /* Reconfigure timer to fire every 2 ms */ > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > > Here I have still used cntpct value to reconfigure cntps_cval as I > > tried to use cntps_cval but it leads to following situation: > > > > At a particular point of time cntpct value becomes greater than > > (cntps_cval + 2ms) due to accumulating delay leading to timer > > condition always true and thereby generating interrupts without any > > delay. Comments are for things that are not clear from reading the code. In other words in general for review comments that are not obviously stupid it is often better to reply in comments on the suprising code than in e-mail. > > > > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 > reference manual), it seems that there is another TimerValue view that > looks more appropriate for our usage than CounterValue view used in > this patch. > > So I have tried to use it as follows. It works as expected. > > +void generic_timer_handler(void) > +{ > + /* Ensure that the timer did assert the interrupt */ > + assert((read_cntps_ctl() >> 2)); > + > + /* Disable the timer */ > + write_cntps_ctl(0); > + > + /* Reconfigure timer to fire every 2 ms */ As you mentioned above... timer does not fire every 2 ms. Here we reconfigure the time to fire 2ms from now. > + write_cntps_tval(read_cntfrq() / 500); > + > + /* Enable the secure physical timer */ > + write_cntps_ctl(1); > +} > > Here to reconfigure the timer value, we need to disable/enable timer > as per Section D7.5.13 > CNTPS_TVAL_EL1, ARMv8 reference manual. > > Please share your views regarding this. Looks like TVAL and CVAL are simply alternative views of the same counter. Agree TVAL seems more suitable. > > -Sumit > > > -Sumit > > > > > + write_cntps_cval(cval); > > > +} > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > > new file mode 100644 > > > index 0000000..4f2091a > > > --- /dev/null > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > > @@ -0,0 +1,13 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > +/* > > > + * Copyright (C) 2018, Linaro Limited > > > + */ > > > + > > > +#ifndef __TIMER_FIQ_H > > > +#define __TIMER_FIQ_H > > > + > > > +void generic_timer_start(void); > > > +void generic_timer_stop(void); > > > +void generic_timer_handler(void); > > > + > > > +#endif /* __TIMER_FIQ_H */ > > > -- > > > 2.7.4 > > >
On Thu, 22 Nov 2018 at 17:12, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Nov 22, 2018 at 01:31:38PM +0530, Sumit Garg wrote: > > > > +void generic_timer_handler(void) > > > > +{ > > > > + uint64_t cval; > > > > + > > > > + /* Ensure that the timer did assert the interrupt */ > > > > + assert((read_cntps_ctl() >> 2)); > > > > + > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > > > > Here I have still used cntpct value to reconfigure cntps_cval as I > > > tried to use cntps_cval but it leads to following situation: > > > > > > At a particular point of time cntpct value becomes greater than > > > (cntps_cval + 2ms) due to accumulating delay leading to timer > > > condition always true and thereby generating interrupts without any > > > delay. > > Comments are for things that are not clear from reading the code. In > other words in general for review comments that are not obviously stupid > it is often better to reply in comments on the suprising code than > in e-mail. > I am afraid, if I completely understand your comment here. But it seems that you are looking for surprising code. Below is the diff that causes the issue: - cval = read_cntpct() + (read_cntfrq() / 500); + cval = read_cntps_cval() + (read_cntfrq() / 500); > > > > > > > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 > > reference manual), it seems that there is another TimerValue view that > > looks more appropriate for our usage than CounterValue view used in > > this patch. > > > > So I have tried to use it as follows. It works as expected. > > > > +void generic_timer_handler(void) > > +{ > > + /* Ensure that the timer did assert the interrupt */ > > + assert((read_cntps_ctl() >> 2)); > > + > > + /* Disable the timer */ > > + write_cntps_ctl(0); > > + > > + /* Reconfigure timer to fire every 2 ms */ > > As you mentioned above... timer does not fire every 2 ms. Here we > reconfigure the time to fire 2ms from now. So basically you want to avoid the delay between the moment timer interrupt is asserted and interrupt handler, correct? If yes, then it would be so minute (maybe < 1us) to worry about in our case. > > > + write_cntps_tval(read_cntfrq() / 500); > > + > > + /* Enable the secure physical timer */ > > + write_cntps_ctl(1); > > +} > > > > Here to reconfigure the timer value, we need to disable/enable timer > > as per Section D7.5.13 > > CNTPS_TVAL_EL1, ARMv8 reference manual. > > > > Please share your views regarding this. > > Looks like TVAL and CVAL are simply alternative views of the same > counter. Agree TVAL seems more suitable. > Ok, will use TVAL then. > > > > -Sumit > > > > > -Sumit > > > > > > > + write_cntps_cval(cval); > > > > +} > > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > new file mode 100644 > > > > index 0000000..4f2091a > > > > --- /dev/null > > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > @@ -0,0 +1,13 @@ > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > +/* > > > > + * Copyright (C) 2018, Linaro Limited > > > > + */ > > > > + > > > > +#ifndef __TIMER_FIQ_H > > > > +#define __TIMER_FIQ_H > > > > + > > > > +void generic_timer_start(void); > > > > +void generic_timer_stop(void); > > > > +void generic_timer_handler(void); > > > > + > > > > +#endif /* __TIMER_FIQ_H */ > > > > -- > > > > 2.7.4 > > > >
On Thu, Nov 22, 2018 at 05:42:06PM +0530, Sumit Garg wrote: > On Thu, 22 Nov 2018 at 17:12, Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Thu, Nov 22, 2018 at 01:31:38PM +0530, Sumit Garg wrote: > > > > > +void generic_timer_handler(void) > > > > > +{ > > > > > + uint64_t cval; > > > > > + > > > > > + /* Ensure that the timer did assert the interrupt */ > > > > > + assert((read_cntps_ctl() >> 2)); > > > > > + > > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > > > > > > Here I have still used cntpct value to reconfigure cntps_cval as I > > > > tried to use cntps_cval but it leads to following situation: > > > > > > > > At a particular point of time cntpct value becomes greater than > > > > (cntps_cval + 2ms) due to accumulating delay leading to timer > > > > condition always true and thereby generating interrupts without any > > > > delay. > > > > Comments are for things that are not clear from reading the code. In > > other words in general for review comments that are not obviously stupid > > it is often better to reply in comments on the suprising code than > > in e-mail. > > > > I am afraid, if I completely understand your comment here. But it > seems that you are looking for surprising code. Below is the diff that > causes the issue: > > - cval = read_cntpct() + (read_cntfrq() / 500); > + cval = read_cntps_cval() + (read_cntfrq() / 500); I'm saying don't tell *me* in private e-mail why you have decided to read from read_cntpct(). Tell *everyone* by putting an appropriate comment into the code. > > > > > > > > > > > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 > > > reference manual), it seems that there is another TimerValue view that > > > looks more appropriate for our usage than CounterValue view used in > > > this patch. > > > > > > So I have tried to use it as follows. It works as expected. > > > > > > +void generic_timer_handler(void) > > > +{ > > > + /* Ensure that the timer did assert the interrupt */ > > > + assert((read_cntps_ctl() >> 2)); > > > + > > > + /* Disable the timer */ > > > + write_cntps_ctl(0); > > > + > > > + /* Reconfigure timer to fire every 2 ms */ > > > > As you mentioned above... timer does not fire every 2 ms. Here we > > reconfigure the time to fire 2ms from now. > > So basically you want to avoid the delay between the moment timer > interrupt is asserted and interrupt handler, correct? If yes, then it > would be so minute (maybe < 1us) to worry about in our case. I am simply saying that the comment is wrong. The code the comment applies to does *not* "Reconfigure timer to fire every 2 ms". In any case, you said in previous e-mail that sometimes when we rearm the timer is has taken more than 2ms between the timer firing and the ISR running. In such a situation then the difference is not minute. > > > + write_cntps_tval(read_cntfrq() / 500); > > > + > > > + /* Enable the secure physical timer */ > > > + write_cntps_ctl(1); > > > +} > > > > > > Here to reconfigure the timer value, we need to disable/enable timer > > > as per Section D7.5.13 > > > CNTPS_TVAL_EL1, ARMv8 reference manual. > > > > > > Please share your views regarding this. > > > > Looks like TVAL and CVAL are simply alternative views of the same > > counter. Agree TVAL seems more suitable. > > > > Ok, will use TVAL then. > > > > > > > -Sumit > > > > > > > -Sumit > > > > > > > > > + write_cntps_cval(cval); > > > > > +} > > > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > new file mode 100644 > > > > > index 0000000..4f2091a > > > > > --- /dev/null > > > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > @@ -0,0 +1,13 @@ > > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > > +/* > > > > > + * Copyright (C) 2018, Linaro Limited > > > > > + */ > > > > > + > > > > > +#ifndef __TIMER_FIQ_H > > > > > +#define __TIMER_FIQ_H > > > > > + > > > > > +void generic_timer_start(void); > > > > > +void generic_timer_stop(void); > > > > > +void generic_timer_handler(void); > > > > > + > > > > > +#endif /* __TIMER_FIQ_H */ > > > > > -- > > > > > 2.7.4 > > > > >
On Thu, 22 Nov 2018 at 18:14, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Nov 22, 2018 at 05:42:06PM +0530, Sumit Garg wrote: > > On Thu, 22 Nov 2018 at 17:12, Daniel Thompson > > <daniel.thompson@linaro.org> wrote: > > > > > > On Thu, Nov 22, 2018 at 01:31:38PM +0530, Sumit Garg wrote: > > > > > > +void generic_timer_handler(void) > > > > > > +{ > > > > > > + uint64_t cval; > > > > > > + > > > > > > + /* Ensure that the timer did assert the interrupt */ > > > > > > + assert((read_cntps_ctl() >> 2)); > > > > > > + > > > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > > > > > > > > Here I have still used cntpct value to reconfigure cntps_cval as I > > > > > tried to use cntps_cval but it leads to following situation: > > > > > > > > > > At a particular point of time cntpct value becomes greater than > > > > > (cntps_cval + 2ms) due to accumulating delay leading to timer > > > > > condition always true and thereby generating interrupts without any > > > > > delay. > > > > > > Comments are for things that are not clear from reading the code. In > > > other words in general for review comments that are not obviously stupid > > > it is often better to reply in comments on the suprising code than > > > in e-mail. > > > > > > > I am afraid, if I completely understand your comment here. But it > > seems that you are looking for surprising code. Below is the diff that > > causes the issue: > > > > - cval = read_cntpct() + (read_cntfrq() / 500); > > + cval = read_cntps_cval() + (read_cntfrq() / 500); > > I'm saying don't tell *me* in private e-mail why you have decided to > read from read_cntpct(). Tell *everyone* by putting an appropriate comment > into the code. > > > > > > > > > > > > > > > > > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 > > > > reference manual), it seems that there is another TimerValue view that > > > > looks more appropriate for our usage than CounterValue view used in > > > > this patch. > > > > > > > > So I have tried to use it as follows. It works as expected. > > > > > > > > +void generic_timer_handler(void) > > > > +{ > > > > + /* Ensure that the timer did assert the interrupt */ > > > > + assert((read_cntps_ctl() >> 2)); > > > > + > > > > + /* Disable the timer */ > > > > + write_cntps_ctl(0); > > > > + > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > > > As you mentioned above... timer does not fire every 2 ms. Here we > > > reconfigure the time to fire 2ms from now. > > > > So basically you want to avoid the delay between the moment timer > > interrupt is asserted and interrupt handler, correct? If yes, then it > > would be so minute (maybe < 1us) to worry about in our case. > > I am simply saying that the comment is wrong. The code the comment applies to > does *not* "Reconfigure timer to fire every 2 ms". > Would "Reconfigure timer to fire 2 ms from now" be appropriate? > In any case, you said in previous e-mail that sometimes when we rearm > the timer is has taken more than 2ms between the timer firing and the > ISR running. In such a situation then the difference is not minute. > Its an interesting scenario to debug (as prints wont be suitable to debug this timer problem). I am not sure what is actually causing the delay. Its an observation that heath tests starts to fail continuously when I use cntps_cval. Will try, if I can find the root cause. -Sumit > > > > > + write_cntps_tval(read_cntfrq() / 500); > > > > + > > > > + /* Enable the secure physical timer */ > > > > + write_cntps_ctl(1); > > > > +} > > > > > > > > Here to reconfigure the timer value, we need to disable/enable timer > > > > as per Section D7.5.13 > > > > CNTPS_TVAL_EL1, ARMv8 reference manual. > > > > > > > > Please share your views regarding this. > > > > > > Looks like TVAL and CVAL are simply alternative views of the same > > > counter. Agree TVAL seems more suitable. > > > > > > > Ok, will use TVAL then. > > > > > > > > > > -Sumit > > > > > > > > > -Sumit > > > > > > > > > > > + write_cntps_cval(cval); > > > > > > +} > > > > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > > new file mode 100644 > > > > > > index 0000000..4f2091a > > > > > > --- /dev/null > > > > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > > @@ -0,0 +1,13 @@ > > > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > > > +/* > > > > > > + * Copyright (C) 2018, Linaro Limited > > > > > > + */ > > > > > > + > > > > > > +#ifndef __TIMER_FIQ_H > > > > > > +#define __TIMER_FIQ_H > > > > > > + > > > > > > +void generic_timer_start(void); > > > > > > +void generic_timer_stop(void); > > > > > > +void generic_timer_handler(void); > > > > > > + > > > > > > +#endif /* __TIMER_FIQ_H */ > > > > > > -- > > > > > > 2.7.4 > > > > > >
On Thu, Nov 22, 2018 at 06:57:40PM +0530, Sumit Garg wrote: > On Thu, 22 Nov 2018 at 18:14, Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Thu, Nov 22, 2018 at 05:42:06PM +0530, Sumit Garg wrote: > > > On Thu, 22 Nov 2018 at 17:12, Daniel Thompson > > > <daniel.thompson@linaro.org> wrote: > > > > > > > > On Thu, Nov 22, 2018 at 01:31:38PM +0530, Sumit Garg wrote: > > > > > > > +void generic_timer_handler(void) > > > > > > > +{ > > > > > > > + uint64_t cval; > > > > > > > + > > > > > > > + /* Ensure that the timer did assert the interrupt */ > > > > > > > + assert((read_cntps_ctl() >> 2)); > > > > > > > + > > > > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > > > > > > > > > > Here I have still used cntpct value to reconfigure cntps_cval as I > > > > > > tried to use cntps_cval but it leads to following situation: > > > > > > > > > > > > At a particular point of time cntpct value becomes greater than > > > > > > (cntps_cval + 2ms) due to accumulating delay leading to timer > > > > > > condition always true and thereby generating interrupts without any > > > > > > delay. > > > > > > > > Comments are for things that are not clear from reading the code. In > > > > other words in general for review comments that are not obviously stupid > > > > it is often better to reply in comments on the suprising code than > > > > in e-mail. > > > > > > > > > > I am afraid, if I completely understand your comment here. But it > > > seems that you are looking for surprising code. Below is the diff that > > > causes the issue: > > > > > > - cval = read_cntpct() + (read_cntfrq() / 500); > > > + cval = read_cntps_cval() + (read_cntfrq() / 500); > > > > I'm saying don't tell *me* in private e-mail why you have decided to > > read from read_cntpct(). Tell *everyone* by putting an appropriate comment > > into the code. > > > > > > > > > > > > > > > > > > > > > > > I did explored more about ARM timers (Section D6.1.5 Timers, ARMv8 > > > > > reference manual), it seems that there is another TimerValue view that > > > > > looks more appropriate for our usage than CounterValue view used in > > > > > this patch. > > > > > > > > > > So I have tried to use it as follows. It works as expected. > > > > > > > > > > +void generic_timer_handler(void) > > > > > +{ > > > > > + /* Ensure that the timer did assert the interrupt */ > > > > > + assert((read_cntps_ctl() >> 2)); > > > > > + > > > > > + /* Disable the timer */ > > > > > + write_cntps_ctl(0); > > > > > + > > > > > + /* Reconfigure timer to fire every 2 ms */ > > > > > > > > As you mentioned above... timer does not fire every 2 ms. Here we > > > > reconfigure the time to fire 2ms from now. > > > > > > So basically you want to avoid the delay between the moment timer > > > interrupt is asserted and interrupt handler, correct? If yes, then it > > > would be so minute (maybe < 1us) to worry about in our case. > > > > I am simply saying that the comment is wrong. The code the comment applies to > > does *not* "Reconfigure timer to fire every 2 ms". > > > > Would "Reconfigure timer to fire 2 ms from now" be appropriate? Yes. > > > In any case, you said in previous e-mail that sometimes when we rearm > > the timer is has taken more than 2ms between the timer firing and the > > ISR running. In such a situation then the difference is not minute. > > > > Its an interesting scenario to debug (as prints wont be suitable to > debug this timer problem). I am not sure what is actually causing the > delay. Its an observation that heath tests starts to fail continuously > when I use cntps_cval. Will try, if I can find the root cause. > > -Sumit > > > > > > > > + write_cntps_tval(read_cntfrq() / 500); > > > > > + > > > > > + /* Enable the secure physical timer */ > > > > > + write_cntps_ctl(1); > > > > > +} > > > > > > > > > > Here to reconfigure the timer value, we need to disable/enable timer > > > > > as per Section D7.5.13 > > > > > CNTPS_TVAL_EL1, ARMv8 reference manual. > > > > > > > > > > Please share your views regarding this. > > > > > > > > Looks like TVAL and CVAL are simply alternative views of the same > > > > counter. Agree TVAL seems more suitable. > > > > > > > > > > Ok, will use TVAL then. > > > > > > > > > > > > > -Sumit > > > > > > > > > > > -Sumit > > > > > > > > > > > > > + write_cntps_cval(cval); > > > > > > > +} > > > > > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > > > new file mode 100644 > > > > > > > index 0000000..4f2091a > > > > > > > --- /dev/null > > > > > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.h > > > > > > > @@ -0,0 +1,13 @@ > > > > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > > > > +/* > > > > > > > + * Copyright (C) 2018, Linaro Limited > > > > > > > + */ > > > > > > > + > > > > > > > +#ifndef __TIMER_FIQ_H > > > > > > > +#define __TIMER_FIQ_H > > > > > > > + > > > > > > > +void generic_timer_start(void); > > > > > > > +void generic_timer_stop(void); > > > > > > > +void generic_timer_handler(void); > > > > > > > + > > > > > > > +#endif /* __TIMER_FIQ_H */ > > > > > > > -- > > > > > > > 2.7.4 > > > > > > >
diff --git a/core/arch/arm/include/arm64.h b/core/arch/arm/include/arm64.h index 2c1fd8c..0cf14c0 100644 --- a/core/arch/arm/include/arm64.h +++ b/core/arch/arm/include/arm64.h @@ -305,6 +305,10 @@ DEFINE_REG_READ_FUNC_(cntfrq, uint32_t, cntfrq_el0) DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0) DEFINE_REG_READ_FUNC_(cntkctl, uint32_t, cntkctl_el1) DEFINE_REG_WRITE_FUNC_(cntkctl, uint32_t, cntkctl_el1) +DEFINE_REG_READ_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) +DEFINE_REG_WRITE_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) +DEFINE_REG_READ_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) +DEFINE_REG_WRITE_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) DEFINE_REG_READ_FUNC_(pmccntr, uint64_t, pmccntr_el0) diff --git a/core/arch/arm/plat-synquacer/main.c b/core/arch/arm/plat-synquacer/main.c index c3aac4c..714becd 100644 --- a/core/arch/arm/plat-synquacer/main.c +++ b/core/arch/arm/plat-synquacer/main.c @@ -18,6 +18,7 @@ #include <sm/optee_smc.h> #include <tee/entry_fast.h> #include <tee/entry_std.h> +#include <timer_fiq.h> static void main_fiq(void); @@ -46,7 +47,7 @@ const struct thread_handlers *generic_boot_get_handlers(void) static void main_fiq(void) { - panic(); + gic_it_handle(&gic_data); } void console_init(void) @@ -66,12 +67,31 @@ void main_init_gic(void) if (!gicd_base) panic(); - /* Initialize GIC */ - gic_init(&gic_data, 0, gicd_base); + /* On ARMv8-A, GIC configuration is initialized in TF-A */ + gic_init_base_addr(&gic_data, 0, gicd_base); + itr_init(&gic_data.chip); } -void main_secondary_init_gic(void) +static enum itr_return timer_itr_cb(struct itr_handler *h __unused) +{ + /* Reset timer for next FIQ */ + generic_timer_handler(); + + return ITRR_HANDLED; +} + +static struct itr_handler timer_itr = { + .it = IT_SEC_TIMER, + .flags = ITRF_TRIGGER_LEVEL, + .handler = timer_itr_cb, +}; + +static TEE_Result init_timer_itr(void) { - gic_cpu_init(&gic_data); + itr_add(&timer_itr); + itr_enable(IT_SEC_TIMER); + + return TEE_SUCCESS; } +driver_init(init_timer_itr); diff --git a/core/arch/arm/plat-synquacer/platform_config.h b/core/arch/arm/plat-synquacer/platform_config.h index 4d6d545..f9b1b40 100644 --- a/core/arch/arm/plat-synquacer/platform_config.h +++ b/core/arch/arm/plat-synquacer/platform_config.h @@ -19,6 +19,8 @@ #define CONSOLE_UART_CLK_IN_HZ 62500000 #define CONSOLE_BAUDRATE 115200 +#define IT_SEC_TIMER 29 + #define DRAM0_BASE 0x80000000 /* Platform specific defines */ diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk index 8ddc2fd..cfa1dc3 100644 --- a/core/arch/arm/plat-synquacer/sub.mk +++ b/core/arch/arm/plat-synquacer/sub.mk @@ -1,2 +1,3 @@ global-incdirs-y += . srcs-y += main.c +srcs-y += timer_fiq.c diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c new file mode 100644 index 0000000..c775bb9 --- /dev/null +++ b/core/arch/arm/plat-synquacer/timer_fiq.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: BSD-2-Clause +/* + * Copyright (c) 2018, Linaro Limited + */ + +#include <arm.h> +#include <console.h> +#include <drivers/gic.h> +#include <io.h> +#include <kernel/panic.h> +#include <kernel/misc.h> +#include <kernel/spinlock.h> +#include <timer_fiq.h> + +static unsigned int timer_lock = SPINLOCK_UNLOCK; +static bool timer_fiq_running; + +void generic_timer_start(void) +{ + uint64_t cval; + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); + + cpu_spin_lock(&timer_lock); + + if (timer_fiq_running == true) + goto exit; + + /* The timer will fire every 2 ms */ + cval = read_cntpct() + (read_cntfrq() / 500); + write_cntps_cval(cval); + + /* Enable the secure physical timer */ + write_cntps_ctl(1); + + timer_fiq_running = true; + +exit: + cpu_spin_unlock(&timer_lock); + thread_set_exceptions(exceptions); +} + +void generic_timer_stop(void) +{ + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); + + cpu_spin_lock(&timer_lock); + + /* Disable the timer */ + write_cntps_ctl(0); + + timer_fiq_running = false; + + cpu_spin_unlock(&timer_lock); + thread_set_exceptions(exceptions); +} + +void generic_timer_handler(void) +{ + uint64_t cval; + + /* Ensure that the timer did assert the interrupt */ + assert((read_cntps_ctl() >> 2)); + + /* Reconfigure timer to fire every 2 ms */ + cval = read_cntpct() + (read_cntfrq() / 500); + write_cntps_cval(cval); +} diff --git a/core/arch/arm/plat-synquacer/timer_fiq.h b/core/arch/arm/plat-synquacer/timer_fiq.h new file mode 100644 index 0000000..4f2091a --- /dev/null +++ b/core/arch/arm/plat-synquacer/timer_fiq.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2018, Linaro Limited + */ + +#ifndef __TIMER_FIQ_H +#define __TIMER_FIQ_H + +void generic_timer_start(void); +void generic_timer_stop(void); +void generic_timer_handler(void); + +#endif /* __TIMER_FIQ_H */
Currently there is no means to perform background housekeeping in secure world on Synquacer platforms. Provide an (optional) periodic timer to allow any housekeeping to be performed. Although it could be expanded, at present the code is fairly simple because we expect only a single PTA to exploit the timer interrupt. The secure timer interrupt is configured to fire every 2ms. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- core/arch/arm/include/arm64.h | 4 ++ core/arch/arm/plat-synquacer/main.c | 30 ++++++++++-- core/arch/arm/plat-synquacer/platform_config.h | 2 + core/arch/arm/plat-synquacer/sub.mk | 1 + core/arch/arm/plat-synquacer/timer_fiq.c | 67 ++++++++++++++++++++++++++ core/arch/arm/plat-synquacer/timer_fiq.h | 13 +++++ 6 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.c create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.h -- 2.7.4