lkml.org 
[lkml]   [2016]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/6] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables
From
Date
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

\
 
 \ /
  Last update: 2016-03-04 12:01    [W:0.093 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site