Re: [PATCH v2 02/21] arm64: Allow the arch timer to use the HYP timer

From: Christoffer Dall
Date: Mon Feb 01 2016 - 10:36:46 EST

On Mon, Feb 01, 2016 at 01:42:34PM +0000, Marc Zyngier wrote:
> On 01/02/16 12:29, Christoffer Dall wrote:
> > On Mon, Jan 25, 2016 at 03:53:36PM +0000, Marc Zyngier wrote:
> >> With the ARMv8.1 VHE, the kernel can run in HYP mode, and thus
> >> use the HYP timer instead of the normal guest timer in a mostly
> >> transparent way, except for the interrupt line.
> >>
> >> This patch reworks the arch timer code to allow the selection of
> >> the HYP PPI, possibly falling back to the guest timer if not
> >> available.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >> ---
> >> drivers/clocksource/arm_arch_timer.c | 96 ++++++++++++++++++++++--------------
> >> 1 file changed, 59 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >> index c64d543..ffe9d1c 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -67,7 +67,7 @@ static int arch_timer_ppi[MAX_TIMER_PPI];
> >>
> >> static struct clock_event_device __percpu *arch_timer_evt;
> >>
> >> -static bool arch_timer_use_virtual = true;
> >> +static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
> >> static bool arch_timer_c3stop;
> >> static bool arch_timer_mem_use_virtual;
> >>
> >> @@ -263,14 +263,20 @@ static void __arch_timer_setup(unsigned type,
> >> clk->name = "arch_sys_timer";
> >> clk->rating = 450;
> >> clk->cpumask = cpumask_of(smp_processor_id());
> >> - if (arch_timer_use_virtual) {
> >> - clk->irq = arch_timer_ppi[VIRT_PPI];
> >> + clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
> >> + switch (arch_timer_uses_ppi) {
> >> + case VIRT_PPI:
> >> clk->set_state_shutdown = arch_timer_shutdown_virt;
> >> clk->set_next_event = arch_timer_set_next_event_virt;
> >> - } else {
> >> - clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
> >> + break;
> >> + case PHYS_SECURE_PPI:
> >> + case PHYS_NONSECURE_PPI:
> >> + case HYP_PPI:
> >> clk->set_state_shutdown = arch_timer_shutdown_phys;
> >> clk->set_next_event = arch_timer_set_next_event_phys;
> >> + break;
> >> + default:
> >> + BUG();
> >> }
> >> } else {
> >> clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
> >> @@ -338,17 +344,20 @@ static void arch_counter_set_user_access(void)
> >> arch_timer_set_cntkctl(cntkctl);
> >> }
> >>
> >> +static bool arch_timer_has_nonsecure_ppi(void)
> >> +{
> >> + return (arch_timer_uses_ppi == PHYS_SECURE_PPI &&
> >> + arch_timer_ppi[PHYS_NONSECURE_PPI]);
> >> +}
> >> +
> >> static int arch_timer_setup(struct clock_event_device *clk)
> >> {
> >> __arch_timer_setup(ARCH_CP15_TIMER, clk);
> >>
> >> - if (arch_timer_use_virtual)
> >> - enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
> >> - else {
> >> - enable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], 0);
> >> - if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> >> - enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> >> - }
> >> + enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
> >> +
> >> + if (arch_timer_has_nonsecure_ppi())
> >> + enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> >>
> >> arch_counter_set_user_access();
> >> if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
> >> @@ -390,7 +399,7 @@ static void arch_timer_banner(unsigned type)
> >> (unsigned long)arch_timer_rate / 1000000,
> >> (unsigned long)(arch_timer_rate / 10000) % 100,
> >> type & ARCH_CP15_TIMER ?
> >> - arch_timer_use_virtual ? "virt" : "phys" :
> >> + (arch_timer_uses_ppi == VIRT_PPI) ? "virt" : "phys" :
> >> "",
> >> type == (ARCH_CP15_TIMER | ARCH_MEM_TIMER) ? "/" : "",
> >> type & ARCH_MEM_TIMER ?
> >> @@ -460,7 +469,7 @@ static void __init arch_counter_register(unsigned type)
> >>
> >> /* Register the CP15 based counter if we have one */
> >> if (type & ARCH_CP15_TIMER) {
> >> - if (IS_ENABLED(CONFIG_ARM64) || arch_timer_use_virtual)
> >> + if (IS_ENABLED(CONFIG_ARM64) || arch_timer_uses_ppi == VIRT_PPI)
> >> arch_timer_read_counter = arch_counter_get_cntvct;
> >> else
> >> arch_timer_read_counter = arch_counter_get_cntpct;
> >> @@ -490,13 +499,9 @@ static void arch_timer_stop(struct clock_event_device *clk)
> >> pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
> >> clk->irq, smp_processor_id());
> >>
> >> - if (arch_timer_use_virtual)
> >> - disable_percpu_irq(arch_timer_ppi[VIRT_PPI]);
> >> - else {
> >> - disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]);
> >> - if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> >> - disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> >> - }
> >> + disable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi]);
> >> + if (arch_timer_has_nonsecure_ppi())
> >> + disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> >>
> >> clk->set_state_shutdown(clk);
> >> }
> >> @@ -562,12 +567,14 @@ static int __init arch_timer_register(void)
> >> goto out;
> >> }
> >>
> >> - if (arch_timer_use_virtual) {
> >> - ppi = arch_timer_ppi[VIRT_PPI];
> >> + ppi = arch_timer_ppi[arch_timer_uses_ppi];
> >> + switch (arch_timer_uses_ppi) {
> >> + case VIRT_PPI:
> >> err = request_percpu_irq(ppi, arch_timer_handler_virt,
> >> "arch_timer", arch_timer_evt);
> >> - } else {
> >> - ppi = arch_timer_ppi[PHYS_SECURE_PPI];
> >> + break;
> >> + case PHYS_SECURE_PPI:
> >> + case PHYS_NONSECURE_PPI:
> >> err = request_percpu_irq(ppi, arch_timer_handler_phys,
> >> "arch_timer", arch_timer_evt);
> >> if (!err && arch_timer_ppi[PHYS_NONSECURE_PPI]) {
> >> @@ -578,6 +585,13 @@ static int __init arch_timer_register(void)
> >> free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
> >> arch_timer_evt);
> >> }
> >> + break;
> >> + case HYP_PPI:
> >> + err = request_percpu_irq(ppi, arch_timer_handler_phys,
> >> + "arch_timer", arch_timer_evt);
> >> + break;
> >> + default:
> >> + BUG();
> >> }
> >>
> >> if (err) {
> >> @@ -602,15 +616,10 @@ static int __init arch_timer_register(void)
> >> out_unreg_notify:
> >> unregister_cpu_notifier(&arch_timer_cpu_nb);
> >> out_free_irq:
> >> - if (arch_timer_use_virtual)
> >> - free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
> >> - else {
> >> - free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
> >> + free_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], arch_timer_evt);
> >> + if (arch_timer_has_nonsecure_ppi())
> >> + free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
> >> arch_timer_evt);
> >> - if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> >> - free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
> >> - arch_timer_evt);
> >> - }
> >>
> >> out_free:
> >> free_percpu(arch_timer_evt);
> >> @@ -697,12 +706,25 @@ static void __init arch_timer_init(void)
> >> *
> >> * If no interrupt provided for virtual timer, we'll have to
> >> * stick to the physical timer. It'd better be accessible...
> >> + *
> >> + * On ARMv8.1 with VH extensions, the kernel runs in HYP. VHE
> >> + * accesses to CNTP_*_EL1 registers are silently redirected to
> >> + * their CNTHP_*_EL2 counterparts, and use a different PPI
> >> + * number.
> >> */
> >> if (is_hyp_mode_available() || !arch_timer_ppi[VIRT_PPI]) {
> >> - arch_timer_use_virtual = false;
> >> + bool has_ppi;
> >> +
> >> + if (is_kernel_in_hyp_mode()) {
> >> + arch_timer_uses_ppi = HYP_PPI;
> >> + has_ppi = !!arch_timer_ppi[HYP_PPI];
> >> + } else {
> >> + arch_timer_uses_ppi = PHYS_SECURE_PPI;
> >> + has_ppi = (!!arch_timer_ppi[PHYS_SECURE_PPI] ||
> >> + !!arch_timer_ppi[PHYS_NONSECURE_PPI]);
> >
> > shouldn't this simply test the PHYS_SECURE_PPI since otherwise you could
> > potentially have the PHYS_NONSECURE_PPI but not PHYS_SECURE_PPI and
> > you'll try to request IRQ 0 for this later... ?
>
> I don't really see how you could have the non-secure PPI, but not the
> secure one, as the binding doesn't give you opportunity to do so (the
> first interrupt is the secure one, then the non-secure one...).
>
I didn't bring the DT-binding-by-heart part of my brain to work today.

You're right, thanks.

-Christoffer