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

[Marc Zyngier <marc.zyngier@arm.com>]

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

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...