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

[Christoffer Dall]

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... ?

> +		}
>  
> -		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
> -		    !arch_timer_ppi[PHYS_NONSECURE_PPI]) {
> +		if (!has_ppi) {
>  			pr_warn("arch_timer: No interrupt available, giving up\n");
>  			return;
>  		}
> @@ -735,7 +757,7 @@ static void __init arch_timer_of_init(struct device_node *np)
>  	 */
>  	if (IS_ENABLED(CONFIG_ARM) &&
>  	    of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
> -			arch_timer_use_virtual = false;
> +		arch_timer_uses_ppi = PHYS_SECURE_PPI;
>  
>  	arch_timer_init();
>  }
> -- 
> 2.1.4
> 

Thanks,
-Christoffer