Re: [PATCH v2 2/6] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

[Julien Grall <julien.grall@arm.com>]

Hi Christoffer,

On 03/03/16 19:38, Christoffer Dall wrote:
> On Thu, Feb 11, 2016 at 03:33:20PM +0000, Julien Grall wrote:

[...]

>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 6eb2c5d..3cdbefd 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -451,6 +451,8 @@ static struct arch_timer_kvm_info arch_timer_kvm_info;
>>
>>   struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
>>   {
>> +	arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
>> +
>
> it feels weird that we set this member on every retrieval of the pointer
> to the struct?
>
> wouldn't it be more natural to intialize the struct somewhere during
> init and then this function just returns a pointer to the struct?

True, I will move the initialization into arch_timer_common_init.

>
>>   	return &arch_timer_kvm_info;
>>   }
>>

[...]

>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index a669c6a..e4de549 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -17,7 +17,6 @@
>>    */
>>
>>   #include <linux/cpu.h>
>> -#include <linux/of_irq.h>
>>   #include <linux/kvm.h>
>>   #include <linux/kvm_host.h>
>>   #include <linux/interrupt.h>
>> @@ -375,45 +374,28 @@ static struct notifier_block kvm_timer_cpu_nb = {
>>   	.notifier_call = kvm_timer_cpu_notify,
>>   };
>>
>> -static const struct of_device_id arch_timer_of_match[] = {
>> -	{ .compatible	= "arm,armv7-timer",	},
>> -	{ .compatible	= "arm,armv8-timer",	},
>> -	{},
>> -};
>> -
>>   int kvm_timer_hyp_init(void)
>>   {
>> -	struct device_node *np;
>> -	unsigned int ppi;
>>   	struct arch_timer_kvm_info *info;
>>   	int err;
>>
>>   	info = arch_timer_get_kvm_info();
>>   	timecounter = &info->timecounter;
>>
>> -	np = of_find_matching_node(NULL, arch_timer_of_match);
>> -	if (!np) {
>> -		kvm_err("kvm_arch_timer: can't find DT node\n");
>> +	host_vtimer_irq = info->virtual_irq;
>> +	if (host_vtimer_irq <= 0) {
>> +		kvm_err("kvm_arch_timer: can't find virtual timer info or config virtual timer interrupt\n");
>
> I don't understand the difference between the two cases in this error
> message.
>
> How about: "Invalid virtual timer IRQ: %d\n" ?

I will update the error message.

>
>>   		return -ENODEV;
>>   	}
>>
>> -	ppi = irq_of_parse_and_map(np, 2);
>> -	if (!ppi) {
>> -		kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
>> -		err = -EINVAL;
>> -		goto out;
>> -	}
>> -
>> -	err = request_percpu_irq(ppi, kvm_arch_timer_handler,
>> +	err = request_percpu_irq(host_vtimer_irq, kvm_arch_timer_handler,
>>   				 "kvm guest timer", kvm_get_running_vcpus());
>>   	if (err) {
>>   		kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
>> -			ppi, err);
>> +			host_vtimer_irq, err);
>>   		goto out;
>>   	}
>>
>> -	host_vtimer_irq = ppi;
>> -
>>   	err = __register_cpu_notifier(&kvm_timer_cpu_nb);
>>   	if (err) {
>>   		kvm_err("Cannot register timer CPU notifier\n");
>> @@ -426,14 +408,13 @@ int kvm_timer_hyp_init(void)
>>   		goto out_free;
>>   	}
>>
>> -	kvm_info("%s IRQ%d\n", np->name, ppi);
>> +	kvm_info("vtimer IRQ%d\n", host_vtimer_irq);
>
> is "vtimer" a known concept anywhere?  If not, can you use some known
> terms intead?

I will use "virtual timer".

>
>>   	on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
>>
>>   	goto out;
>>   out_free:
>> -	free_percpu_irq(ppi, kvm_get_running_vcpus());
>> +	free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
>>   out:
>> -	of_node_put(np);
>>   	return err;
>>   }

Regards,

-- 
Julien Grall