| Message ID | 1366879534-31578-1-git-send-email-anup.patel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 25, 2013 at 1:45 AM, Anup Patel <anup.patel@linaro.org> wrote: > The arch_timer irq numbers (or PPI number) are implementation dependent so, the host virtual timer irq number can be different from guest virtual timer irq number. > > Currently, we only have Cortex-A15 guest (for KVM ARMv7) and Cortex-A57 guest (for KVM ARMv8) supported. These guests have virtual timer irq number as 27. > > This patch ensures that host virtual timer irq number is read from DTB and guest virtual timer irq is always 27. I don't think this is the right fix. I prefer not hard-coding this stuff in the kernel, but let user space decide this. If we have good technical arguments not to do that (such as knowing that this is always defined per-core and not for an SoC (ARM guys?) then at least the patch should lookup the target processor and set the irq number accordingly. > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > --- > arch/arm/kvm/arch_timer.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kvm/arch_timer.c b/arch/arm/kvm/arch_timer.c > index 49a7516..376abf0 100644 > --- a/arch/arm/kvm/arch_timer.c > +++ b/arch/arm/kvm/arch_timer.c > @@ -30,10 +30,18 @@ > > static struct timecounter *timecounter; > static struct workqueue_struct *wqueue; > -static struct kvm_irq_level timer_irq = { > +static struct kvm_irq_level host_timer_irq = { > .level = 1, > }; > > +/* Guest virtual timer irq number will be based on type of guest we emulate. > + * For Cortex-A15 & Cortex-A57 guest, virtual timer irq is 27 > + */ > +static struct kvm_irq_level guest_timer_irq = { > + .irq = 27, > + .level = 1, > +}; > + > static cycle_t kvm_phys_timer_read(void) > { > return timecounter->cc->read(timecounter->cc); > @@ -163,12 +171,12 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > INIT_WORK(&timer->expired, kvm_timer_inject_irq_work); > hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > timer->timer.function = kvm_timer_expire; > - timer->irq = &timer_irq; > + timer->irq = &guest_timer_irq; > } > > static void kvm_timer_init_interrupt(void *info) > { > - enable_percpu_irq(timer_irq.irq, 0); > + enable_percpu_irq(host_timer_irq.irq, 0); > } > > > @@ -182,7 +190,7 @@ static int kvm_timer_cpu_notify(struct notifier_block *self, > break; > case CPU_DYING: > case CPU_DYING_FROZEN: > - disable_percpu_irq(timer_irq.irq); > + disable_percpu_irq(host_timer_irq.irq); > break; > } > > @@ -230,7 +238,7 @@ int kvm_timer_hyp_init(void) > goto out; > } > > - timer_irq.irq = ppi; > + host_timer_irq.irq = ppi; > > err = register_cpu_notifier(&kvm_timer_cpu_nb); > if (err) { > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 25 April 2013 19:04, Christoffer Dall <cdall@cs.columbia.edu> wrote: > I prefer not hard-coding this stuff in the kernel, but let user space > decide this. If we have good technical arguments not to do that (such > as knowing that this is always defined per-core and not for an SoC > (ARM guys?) then at least the patch should lookup the target processor > and set the irq number accordingly. Well, this is all implementation-defined. The ARM ARM mandates that the generic timers deliver a PPI, and that it must be the same PPI for all processors in an MP implementation, but not which PPI. The A15 and A7 happen to both be hardwired to ID27. You could in theory design a core which let the SoC configure the virtual timer ID (or let the guest arbitrarily program it, for that matter, I suppose), though I'm not sure why you'd want to. I think I'd take the simple approach of saying "the timer PPI is a fixed property of the guest CPU" unless somebody actually builds something where it isn't fixed... (for that CPU we could then define it as a feature argument to KVM_ARM_VCPU_INIT). thanks -- PMM
On Apr 25, 2013, at 11:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 April 2013 19:04, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> I prefer not hard-coding this stuff in the kernel, but let user space >> decide this. If we have good technical arguments not to do that (such >> as knowing that this is always defined per-core and not for an SoC >> (ARM guys?) then at least the patch should lookup the target processor >> and set the irq number accordingly. > > Well, this is all implementation-defined. The ARM ARM mandates > that the generic timers deliver a PPI, and that it must be the > same PPI for all processors in an MP implementation, but not which > PPI. The A15 and A7 happen to both be hardwired to ID27. You could > in theory design a core which let the SoC configure the virtual > timer ID (or let the guest arbitrarily program it, for that > matter, I suppose), though I'm not sure why you'd want to. > > I think I'd take the simple approach of saying "the timer PPI > is a fixed property of the guest CPU" unless somebody actually > builds something where it isn't fixed... (for that CPU we > could then define it as a feature argument to KVM_ARM_VCPU_INIT). > Ok, let's not bother with user space injection then, but instead of hard defaulting to two specific cores, please make a switch statement on the target CPU and log an error on init if it's an unknown core.
On Fri, Apr 26, 2013 at 12:21 AM, Christoffer Dall <cdall@cs.columbia.edu> wrote: > On Apr 25, 2013, at 11:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 25 April 2013 19:04, Christoffer Dall <cdall@cs.columbia.edu> wrote: >>> I prefer not hard-coding this stuff in the kernel, but let user space >>> decide this. If we have good technical arguments not to do that (such >>> as knowing that this is always defined per-core and not for an SoC >>> (ARM guys?) then at least the patch should lookup the target processor >>> and set the irq number accordingly. >> >> Well, this is all implementation-defined. The ARM ARM mandates >> that the generic timers deliver a PPI, and that it must be the >> same PPI for all processors in an MP implementation, but not which >> PPI. The A15 and A7 happen to both be hardwired to ID27. You could >> in theory design a core which let the SoC configure the virtual >> timer ID (or let the guest arbitrarily program it, for that >> matter, I suppose), though I'm not sure why you'd want to. >> >> I think I'd take the simple approach of saying "the timer PPI >> is a fixed property of the guest CPU" unless somebody actually >> builds something where it isn't fixed... (for that CPU we >> could then define it as a feature argument to KVM_ARM_VCPU_INIT). >> > > Ok, let's not bother with user space injection then, but instead of > hard defaulting to two specific cores, please make a switch statement > on the target CPU and log an error on init if it's an unknown core. I agree about determing guest ppi from KVM_TARGET_xxxx in kvm_timer_vcpu_init() but kvm_timer_vcpu_init() is called much before kvm_vcpu_set_target() so in kvm_timer_vcpu_init() we have vcpu->arch.target = -1 (unknown). To handle this issue, we need to determine vcpu timer irq number when we inject the timer irq first timer for that vcpu. > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm -Anup
diff --git a/arch/arm/kvm/arch_timer.c b/arch/arm/kvm/arch_timer.c index 49a7516..376abf0 100644 --- a/arch/arm/kvm/arch_timer.c +++ b/arch/arm/kvm/arch_timer.c @@ -30,10 +30,18 @@ static struct timecounter *timecounter; static struct workqueue_struct *wqueue; -static struct kvm_irq_level timer_irq = { +static struct kvm_irq_level host_timer_irq = { .level = 1, }; +/* Guest virtual timer irq number will be based on type of guest we emulate. + * For Cortex-A15 & Cortex-A57 guest, virtual timer irq is 27 + */ +static struct kvm_irq_level guest_timer_irq = { + .irq = 27, + .level = 1, +}; + static cycle_t kvm_phys_timer_read(void) { return timecounter->cc->read(timecounter->cc); @@ -163,12 +171,12 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) INIT_WORK(&timer->expired, kvm_timer_inject_irq_work); hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); timer->timer.function = kvm_timer_expire; - timer->irq = &timer_irq; + timer->irq = &guest_timer_irq; } static void kvm_timer_init_interrupt(void *info) { - enable_percpu_irq(timer_irq.irq, 0); + enable_percpu_irq(host_timer_irq.irq, 0); } @@ -182,7 +190,7 @@ static int kvm_timer_cpu_notify(struct notifier_block *self, break; case CPU_DYING: case CPU_DYING_FROZEN: - disable_percpu_irq(timer_irq.irq); + disable_percpu_irq(host_timer_irq.irq); break; } @@ -230,7 +238,7 @@ int kvm_timer_hyp_init(void) goto out; } - timer_irq.irq = ppi; + host_timer_irq.irq = ppi; err = register_cpu_notifier(&kvm_timer_cpu_nb); if (err) {
The arch_timer irq numbers (or PPI number) are implementation dependent so, the host virtual timer irq number can be different from guest virtual timer irq number. Currently, we only have Cortex-A15 guest (for KVM ARMv7) and Cortex-A57 guest (for KVM ARMv8) supported. These guests have virtual timer irq number as 27. This patch ensures that host virtual timer irq number is read from DTB and guest virtual timer irq is always 27. Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> --- arch/arm/kvm/arch_timer.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)