Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323238

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

Show all headers | View raw

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@arm.com>
> >> ---
> >>  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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH v2 02/21] arm64: Allow the arch timer to use the HYP timer Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 13:30 +0100
  Re: [PATCH v2 02/21] arm64: Allow the arch timer to use the HYP timer Marc Zyngier <marc.zyngier@arm.com> - 2016-02-01 14:50 +0100
    Re: [PATCH v2 02/21] arm64: Allow the arch timer to use the HYP timer Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 16:40 +0100

csiph-web