KVM/ARM Architected Timers support
Subsystems (2)
v4 cover KVM/ARM Architected Timers support
The following series implements support for the architected generic timers for KVM/ARM.
This is an unmodified repost of the previously submitted series.
This patch series can also be pulled from:
git://github.com/virtualopensystems/linux-kvm-arm.git
branch: kvm-arm-v13-vgic-timers
---
Marc Zyngier (5):
ARM: arch_timers: switch to physical timers if HYP mode is available
ARM: KVM: arch_timers: Add minimal infrastructure
ARM: KVM: arch_timers: Add guest timer core support
ARM: KVM: arch_timers: Add timer world switch
ARM: KVM: arch_timers: Wire the init code and config option
arch/arm/include/asm/kvm_arch_timer.h | 100 ++++++++++++++++ arch/arm/include/asm/kvm_host.h | 5 + arch/arm/kernel/arch_timer.c | 7 + arch/arm/kernel/asm-offsets.c | 8 + arch/arm/kvm/Kconfig | 7 + arch/arm/kvm/Makefile | 1 arch/arm/kvm/arm.c | 14 ++ arch/arm/kvm/interrupts.S | 2 arch/arm/kvm/interrupts_head.S | 60 ++++++++++ arch/arm/kvm/reset.c | 9 + arch/arm/kvm/timer.c | 204 +++++++++++++++++++++++++++++++++ arch/arm/kvm/vgic.c | 1 12 files changed, 417 insertions(+), 1 deletion(-) create mode 100644 arch/arm/include/asm/kvm_arch_timer.h create mode 100644 arch/arm/kvm/timer.c
PATCH v4 1/5 ARM: arch_timers: switch to physical timers if HYP mode is available
From: Marc Zyngier <[email protected]>
If we're booted in HYP mode, it is possible that we'll run some kind of virtualized environment. In this case, it is a better to switch to the physical timers, and leave the virtual timers to guests.
--- arch/arm/kernel/arch_timer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm/kernel/arch_timer.c +6 -1
@@ -26,6 +26,7 @@#include <asm/arch_timer.h> #include <asm/system_info.h> #include <asm/sched_clock.h> +#include <asm/virt.h> static unsigned long arch_timer_rate;
@@ -489,10 +490,14 @@ int __init arch_timer_of_register(void) arch_timer_ppi[i] =irq_of_parse_and_map (np, i);/* +* If HYP mode is available, we know that the physical timer +* has been configured to be accessible from PL1. Use it, so +* that a guest can use the virtual timer instead. +* * If no interrupt provided for virtual timer, we'll have to * stick to the physical timer. It'd better be accessible... */ -if (!arch_timer_ppi[VIRT_PPI ]) { +if (is_hyp_mode_available () || !arch_timer_ppi[VIRT_PPI ]) { arch_timer_use_virtual = false; if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
Re: [PATCH v4 1/5] ARM: arch_timers: switch to physical timers if HYP mode is available
Quoted 8 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
If we're booted in HYP mode, it is possible that we'll run some kind of virtualized environment. In this case, it is a better to switch to the physical timers, and leave the virtual timers to guests.
Signed-off-by Marc Zyngier <[email protected]>
Seems simple enough.
Will
PATCH v4 2/5 ARM: KVM: arch_timers: Add minimal infrastructure
In reply to Christoffer DallFrom: Marc Zyngier <[email protected]>
Add some very minimal architected timer related infrastructure. For the moment, we just provide empty structures, and enable/disable access to the physical timer across world switch.
--- arch/arm/include/asm/kvm_arch_timer.h | 45 +++++++++++++++++++++++++++++++++ arch/arm/include/asm/kvm_host.h | 5 ++++ arch/arm/kvm/interrupts.S | 2 + arch/arm/kvm/interrupts_head.S | 19 ++++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 arch/arm/include/asm/kvm_arch_timer.h
arch/arm/include/asm/kvm_arch_timer.h new file +45 -0
@@ -0,0 +1,45 @@ +/* +* Copyright (C) 2012 ARM Ltd. +* Author: Marc Zyngier <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#ifndef __ASM_ARM_KVM_ARCH_TIMER_H +#define __ASM_ARM_KVM_ARCH_TIMER_H + +struct arch_timer_kvm { +}; + +struct arch_timer_cpu { +}; + +#ifndef CONFIG_KVM_ARM_TIMER +static inline int kvm_timer_hyp_init (void ) +{ +return 0 ; +}; + +static inline int kvm_timer_init (struct kvm *kvm) +{ +return 0 ; +} + +static inline void kvm_timer_vcpu_init (struct kvm_vcpu *vcpu) {} +static inline void kvm_timer_sync_to_cpu (struct kvm_vcpu *vcpu) {} +static inline void kvm_timer_sync_from_cpu (struct kvm_vcpu *vcpu) {} +static inline void kvm_timer_vcpu_terminate (struct kvm_vcpu *vcpu) {} +#endif + +#endif
arch/arm/include/asm/kvm_host.h +5 -0
@@ -24,6 +24,7 @@#include <asm/fpstate.h> #include <asm/kvm_decode.h> #include <asm/kvm_vgic.h> +#include <asm/kvm_arch_timer.h> #define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS#define KVM_MEMORY_SLOTS 32
@@ -48,6 +49,9 @@ struct kvm_arch {/* VTTBR value associated with below pgd and vmid */ u64 vttbr; +/* Timer */ +struct arch_timer_kvm timer; +/* * Anything that is not used directly from assembly code goes * here.
@@ -98,6 +102,7 @@ struct kvm_vcpu_arch {/* VGIC state */ struct vgic_cpu vgic_cpu; +struct arch_timer_cpu timer_cpu;/* * Anything that is not used directly from assembly code goes
arch/arm/kvm/interrupts.S +2 -0
@@ -92,6 +92,7 @@ ENTRY(__kvm_vcpu_run) save_host_regs restore_vgic_state r0 + restore_timer_state r0 @ Store hardware CP15 state and load guest state read_cp15_state
@@ -186,6 +187,7 @@ after_vfp_restore: read_cp15_state 1, r1 write_cp15_state + save_timer_state r1 save_vgic_state r1 restore_host_regs
arch/arm/kvm/interrupts_head.S +19 -0
@@ -418,6 +418,25 @@ #endif .endm +#define CNTHCTL_PL1PCTEN (1 << 0) +#define CNTHCTL_PL1PCEN (1 << 1) + +.macro save_timer_state vcpup + @ Allow physical timer/counter access for the host + mrc p15, 4, r2, c14, c1, 0 @ CNTHCTL + orr r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN) + mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL +.endm + +.macro restore_timer_state vcpup + @ Disallow physical timer access for the guest + @ Physical counter access is allowed + mrc p15, 4, r2, c14, c1, 0 @ CNTHCTL + orr r2, r2, #CNTHCTL_PL1PCTEN + bic r2, r2, #CNTHCTL_PL1PCEN + mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL +.endm + /* Configures the HSTR (Hyp System Trap Register) on entry/return * (hardware reset value is 0) */ .macro set_hstr entry
Re: [PATCH v4 2/5] ARM: KVM: arch_timers: Add minimal infrastructure
Quoted 8 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
Add some very minimal architected timer related infrastructure. For the moment, we just provide empty structures, and enable/disable access to the physical timer across world switch.
Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>
Is it even worth having this patch? It doesn't seem to do much other than stub everything out, so I'd be inclined to fold it in with the next one.
Will
Re: [PATCH v4 2/5] ARM: KVM: arch_timers: Add minimal infrastructure
Quoted 7 lines · Will Deacon
Quoted 6 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
Add some very minimal architected timer related infrastructure. For the moment, we just provide empty structures, and enable/disable access to the physical timer across world switch.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>Is it even worth having this patch? It doesn't seem to do much other than stub everything out, so I'd be inclined to fold it in with the next one.
done,
-Christoffer
PATCH v4 3/5 ARM: KVM: arch_timers: Add guest timer core support
In reply to Christoffer DallFrom: Marc Zyngier <[email protected]>
We can inject a timer interrupt into the guest as a result of three possible events:
- The virtual timer interrupt has fired while we were still executing the guest
- The timer interrupt hasn't fired, but it expired while we were doing the world switch
- A hrtimer we programmed earlier has fired
--- arch/arm/include/asm/kvm_arch_timer.h | 57 +++++++++ arch/arm/kvm/reset.c | 9 + arch/arm/kvm/timer.c | 204 +++++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 arch/arm/kvm/timer.c
arch/arm/include/asm/kvm_arch_timer.h +56 -1
@@ -19,13 +19,68 @@ #ifndef __ASM_ARM_KVM_ARCH_TIMER_H #define __ASM_ARM_KVM_ARCH_TIMER_H +#include <linux/clocksource.h> +#include <linux/hrtimer.h> +#include <linux/workqueue.h> + struct arch_timer_kvm { +#ifdef CONFIG_KVM_ARM_TIMER + /* Is the timer enabled */ + bool enabled; + + /* + * Virtual offset (kernel access it through cntvoff, HYP code + * access it as two 32bit values). + */ + union { + cycle_t cntvoff; + struct { + u32 low; /* Restored only */ + u32 high; /* Restored only */ + } cntvoff32; + }; +#endif }; struct arch_timer_cpu { +#ifdef CONFIG_KVM_ARM_TIMER + /* Registers: control register, timer value */ + u32 cntv_ctl; /* Saved/restored */ + union { + cycle_t cntv_cval; + struct { + u32 low; /* Saved/restored */ + u32 high; /* Saved/restored */ + } cntv_cval32; + }; + + /* + * Anything that is not used directly from assembly code goes + * here. + */ + + /* Background timer used when the guest is not running */ + struct hrtimer timer; + + /* Work queued with the above timer expires */ + struct work_struct expired; + + /* Background timer active */ + bool armed; + + /* Timer IRQ */ + const struct kvm_irq_level *irq; +#endif }; -#ifndef CONFIG_KVM_ARM_TIMER +#ifdef CONFIG_KVM_ARM_TIMER +int kvm_timer_hyp_init(void); +int kvm_timer_init(struct kvm *kvm); +void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu); +void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu); +void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu); +void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu); +#else static inline int kvm_timer_hyp_init(void) { return 0;
arch/arm/kvm/reset.c +9 -0
@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, }; +#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq =27 },/* irq: A7/A15 specific */ + .level =1 /* level */ +}; +#endif /******************************************************************************* * Exported reset function
@@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)return -EINVAL ; cpu_reset = &a15_regs_reset; vcpu->arch.midr =read_cpuid_id (); +#ifdef CONFIG_KVM_ARM_TIMER + vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi; +#endif break ;default :return -ENODEV ;
arch/arm/kvm/timer.c new file +204 -0
204 lines not on this page · raw message
Re: [PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support
Quoted 38 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
We can inject a timer interrupt into the guest as a result of three possible events:
- The virtual timer interrupt has fired while we were still executing the guest
- The timer interrupt hasn't fired, but it expired while we were doing the world switch
- A hrtimer we programmed earlier has firedSigned-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_arch_timer.h | 57 +++++++++ arch/arm/kvm/reset.c | 9 + arch/arm/kvm/timer.c | 204 +++++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 arch/arm/kvm/timer.carch/arm/include/asm/kvm_arch_timer.h +10
-0@@ -19,13 +19,68 @@ #ifndef __ASM_ARM_KVM_ARCH_TIMER_H #define __ASM_ARM_KVM_ARCH_TIMER_H +#include <linux/clocksource.h> +#include <linux/hrtimer.h> +#include <linux/workqueue.h> + struct arch_timer_kvm { +#ifdef CONFIG_KVM_ARM_TIMER + /* Is the timer enabled */ + bool enabled; + + /* + * Virtual offset (kernel access it through cntvoff, HYP code
arch/arm/include/asm/kvm_arch_timer.h line 42
Quoted 10 lines
arch/arm/include/asm/kvm_arch_timer.h +10
-0+* access it as two 32bit values). +*/ +union { +cycle_t cntvoff; +struct { +u32 low;/* Restored only */ +u32 high;/* Restored only */ + } cntvoff32; + }; +#endif
Endianness?
arch/arm/include/asm/kvm_arch_timer.h line 55
Quoted 13 lines
arch/arm/include/asm/kvm_arch_timer.h +10
-0};struct arch_timer_cpu { +#ifdef CONFIG_KVM_ARM_TIMER +/* Registers: control register, timer value */ +u32 cntv_ctl;/* Saved/restored */ +union { +cycle_t cntv_cval; +struct { +u32 low;/* Saved/restored */ +u32 high;/* Saved/restored */ + } cntv_cval32; + };
Similarly.
Quoted 36 lines
arch/arm/include/asm/kvm_arch_timer.h +25
-1+/* +* Anything that is not used directly from assembly code goes +* here. +*/ + +/* Background timer used when the guest is not running */ +struct hrtimer timer; + +/* Work queued with the above timer expires */ +struct work_struct expired; + +/* Background timer active */ +bool armed; + +/* Timer IRQ */ +const struct kvm_irq_level *irq; +#endif }; -#ifndef CONFIG_KVM_ARM_TIMER +#ifdef CONFIG_KVM_ARM_TIMER +int kvm_timer_hyp_init (void ); +int kvm_timer_init (struct kvm *kvm); +void kvm_timer_vcpu_init (struct kvm_vcpu *vcpu); +void kvm_timer_sync_to_cpu (struct kvm_vcpu *vcpu); +void kvm_timer_sync_from_cpu (struct kvm_vcpu *vcpu); +void kvm_timer_vcpu_terminate (struct kvm_vcpu *vcpu); +#else static inline int kvm_timer_hyp_init (void ) {return 0 ;arch/arm/kvm/reset.c +0
-0
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = { · line 42
Quoted 6 lines
arch/arm/kvm/reset.c +3
-0@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, }; +#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq = 27 }, /* irq: A7/A15 specific */
This should be parameterised by the vCPU type.
arch/arm/kvm/timer.c
Quoted 21 lines
arch/arm/kvm/reset.c +6
-0+ .level = 1 /* level */ +}; +#endif /******************************************************************************* * Exported reset function @@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)return -EINVAL ; cpu_reset = &a15_regs_reset; vcpu->arch.midr =read_cpuid_id (); +#ifdef CONFIG_KVM_ARM_TIMER + vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi; +#endif break ;default :return -ENODEV ;arch/arm/kvm/timer.c new file +0
-0
Maybe kvm/arch_timer.c since it seems to be specific to that device and matches the naming for the main driver under kernel/? If we get new virtual timer devices in the future, I guess this would need to be split up so the arch-timer-specific bits are separate from generic hrtimer and kvm code.
Quoted 32 lines
arch/arm/kvm/timer.c new file +31
-0@@ -0,0 +1,204 @@ +/* +* Copyright (C) 2012 ARM Ltd. +* Author: Marc Zyngier <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#include <linux/of_irq.h> +#include <linux/kvm.h> +#include <linux/kvm_host.h> +#include <linux/interrupt.h> + +#include <asm/arch_timer.h> + +#include <asm/kvm_vgic.h> +#include <asm/kvm_arch_timer.h> + +static struct timecounter *timecounter; +static struct workqueue_struct *wqueue; +
arch/arm/kvm/timer.c line 41
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+static cycle_t kvm_phys_timer_read (void ) +{ +return timecounter->cc->read (timecounter->cc); +} + +static void kvm_timer_inject_irq (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + + timer->cntv_ctl |=1 <<1 ;/* Mask the interrupt in the guest */
This is ARCH_TIMER_CTRL_IT_MASK right? We should make that definition available if it is needed here.
Quoted 7 lines
arch/arm/kvm/timer.c new file +7
-0+kvm_vgic_inject_irq (vcpu->kvm, vcpu->vcpu_id, + vcpu->arch.timer_cpu.irq->irq, + vcpu->arch.timer_cpu.irq->level); +} + +static irqreturn_t kvm_arch_timer_handler (int irq,void *dev_id) +{
arch/arm/kvm/timer.c line 58
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; + +/* +* We disable the timer in the world switch and let it be +* handled by kvm_timer_sync_from_cpu(). Getting a timer +* interrupt at this point is a sure sign of some major +* breakage. +*/ +pr_warn ("Unexpected interrupt %d on vcpu %p\n" , irq, vcpu); +return IRQ_HANDLED ;
IRQ_NONE?
Quoted 24 lines
arch/arm/kvm/timer.c new file +24
-0+} + +static void kvm_timer_inject_irq_work (struct work_struct *work) +{ +struct kvm_vcpu *vcpu; + + vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired); + vcpu->arch.timer_cpu.armed = false; +kvm_timer_inject_irq (vcpu); +} + +static enum hrtimer_restart kvm_timer_expire (struct hrtimer *hrt) +{ +struct arch_timer_cpu *timer; + timer = container_of(hrt, struct arch_timer_cpu, timer); +queue_work (wqueue, &timer->expired); +return HRTIMER_NORESTART ; +} + +void kvm_timer_sync_to_cpu (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + +/*
arch/arm/kvm/timer.c line 92
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+* We're about to run this vcpu again, so there is no need to +* keep the background timer running, as we're about to +* populate the CPU timer again. +*/ +if (timer->armed) { +hrtimer_cancel (&timer->timer); +cancel_work_sync (&timer->expired); + timer->armed = false; + } +}
I think some helper functions like timer_is_armed, timer_arm and timer_disarm would make this more readable (resisting arm_timer, which confuses things more!).
arch/arm/kvm/timer.c line 102
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+ +void kvm_timer_sync_from_cpu (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; +cycle_t cval, now; +u64 ns; + +/* Check if the timer is enabled and unmasked first */ +if ((timer->cntv_ctl &3 ) !=1 ) +return ;
Again, we should create/use ARCH_TIMER #defines for this hardware bits.
arch/arm/kvm/timer.c line 105
Quoted 2 lines
arch/arm/kvm/timer.c new file +2
-0+ cval = timer->cntv_cval; + now =kvm_phys_timer_read () - vcpu->kvm->arch.timer.cntvoff;
We probably want to add an isb() *before* the mrrc in arch_timer_counter_read if you want to do things like this, because the counter can be speculated in advance.
arch/arm/kvm/timer.c line 116
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+BUG_ON (timer->armed); + +if (cval <= now) { +/* +* Timer has already expired while we were not +* looking. Inject the interrupt and carry on. +*/ +kvm_timer_inject_irq (vcpu); +return ; + }
Does this buy you much? You still have to cope with the timer expiring here anyway.
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+ timer->armed = true; + ns =cyclecounter_cyc2ns (timecounter->cc, cval - now); +hrtimer_start (&timer->timer,ktime_add_ns (ktime_get (), ns), +HRTIMER_MODE_ABS ); +} + +void kvm_timer_vcpu_init (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; +
arch/arm/kvm/timer.c line 137
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+INIT_WORK (&timer->expired, kvm_timer_inject_irq_work); +hrtimer_init (&timer->timer,CLOCK_MONOTONIC ,HRTIMER_MODE_ABS ); + timer->timer.function = kvm_timer_expire; +} + +static void kvm_timer_init_interrupt (void *info) +{ +unsigned int *irqp = info; + +enable_percpu_irq (*irqp,0 );
IRQ_TYPE_NONE
Quoted 36 lines
arch/arm/kvm/timer.c new file +36
-0+} + + +static const struct of_device_id arch_timer_of_match[] = { + { .compatible ="arm,armv7-timer" , }, + {}, +}; + +int kvm_timer_hyp_init (void ) +{ +struct device_node *np; +unsigned int ppi; +int err; + + timecounter =arch_timer_get_timecounter (); +if (!timecounter) +return -ENODEV ; + + np =of_find_matching_node (NULL , arch_timer_of_match); +if (!np) { +kvm_err ("kvm_arch_timer: can't find DT node\n" ); +return -ENODEV ; + } + + ppi =irq_of_parse_and_map (np,2 ); +if (!ppi) { +kvm_err ("kvm_arch_timer: no virtual timer interrupt\n" ); +return -EINVAL ; + } + + err =request_percpu_irq (ppi, 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); +return err;
arch/arm/kvm/timer.c line 183
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+ } + + wqueue =create_singlethread_workqueue ("kvm_arch_timer" ); +if (!wqueue) { +free_percpu_irq (ppi,kvm_get_running_vcpus ()); +return -ENOMEM ; + } + +kvm_info ("%s IRQ%d\n" , np->name, ppi); +on_each_cpu (kvm_timer_init_interrupt, &ppi,1 );
on_each_cpu currently just returns 0, but you could use that as your return value for good measure anyway.
Quoted 5 lines
arch/arm/kvm/timer.c new file +5
-0+return 0 ; +} + +void kvm_timer_vcpu_terminate (struct kvm_vcpu *vcpu) +{
arch/arm/kvm/timer.c line 199
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + +hrtimer_cancel (&timer->timer); +cancel_work_sync (&timer->expired); +} + +int kvm_timer_init (struct kvm *kvm) +{ +if (timecounter && wqueue) { + kvm->arch.timer.cntvoff =kvm_phys_timer_read ();
Shouldn't this be initialised to 0 and then updated on world switch?
Will
Re: [PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support
Quoted 43 lines · Will Deacon
Quoted 42 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
We can inject a timer interrupt into the guest as a result of three possible events:
- The virtual timer interrupt has fired while we were still executing the guest
- The timer interrupt hasn't fired, but it expired while we were doing the world switch
- A hrtimer we programmed earlier has firedSigned-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_arch_timer.h | 57 +++++++++ arch/arm/kvm/reset.c | 9 + arch/arm/kvm/timer.c | 204 +++++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 arch/arm/kvm/timer.carch/arm/include/asm/kvm_arch_timer.h +14
-0@@ -19,13 +19,68 @@ #ifndef __ASM_ARM_KVM_ARCH_TIMER_H #define __ASM_ARM_KVM_ARCH_TIMER_H +#include <linux/clocksource.h> +#include <linux/hrtimer.h> +#include <linux/workqueue.h> + struct arch_timer_kvm { +#ifdef CONFIG_KVM_ARM_TIMER + /* Is the timer enabled */ + bool enabled; + + /* + * Virtual offset (kernel access it through cntvoff, HYP code + * access it as two 32bit values). + */ + union { + cycle_t cntvoff;
Quoted 8 lines
arch/arm/include/asm/kvm_arch_timer.h line 42
Quoted 6 lines
arch/arm/include/asm/kvm_arch_timer.h +6
-0+struct { +u32 low;/* Restored only */ +u32 high;/* Restored only */ + } cntvoff32; + }; +#endif Endianness?
Yup. I'll fix that.
Quoted 14 lines
arch/arm/include/asm/kvm_arch_timer.h line 55
Quoted 13 lines
arch/arm/include/asm/kvm_arch_timer.h +10
-0};struct arch_timer_cpu { +#ifdef CONFIG_KVM_ARM_TIMER +/* Registers: control register, timer value */ +u32 cntv_ctl;/* Saved/restored */ +union { +cycle_t cntv_cval; +struct { +u32 low;/* Saved/restored */ +u32 high;/* Saved/restored */ + } cntv_cval32; + };
Quoted 1 line
Similarly.
Ditto.
Quoted 40 lines
Quoted 40 lines
arch/arm/include/asm/kvm_arch_timer.h +25
-1+/* +* Anything that is not used directly from assembly code goes +* here. +*/ + +/* Background timer used when the guest is not running */ +struct hrtimer timer; + +/* Work queued with the above timer expires */ +struct work_struct expired; + +/* Background timer active */ +bool armed; + +/* Timer IRQ */ +const struct kvm_irq_level *irq; +#endif }; -#ifndef CONFIG_KVM_ARM_TIMER +#ifdef CONFIG_KVM_ARM_TIMER +int kvm_timer_hyp_init (void ); +int kvm_timer_init (struct kvm *kvm); +void kvm_timer_vcpu_init (struct kvm_vcpu *vcpu); +void kvm_timer_sync_to_cpu (struct kvm_vcpu *vcpu); +void kvm_timer_sync_from_cpu (struct kvm_vcpu *vcpu); +void kvm_timer_vcpu_terminate (struct kvm_vcpu *vcpu); +#else static inline int kvm_timer_hyp_init (void ) {return 0 ;arch/arm/kvm/reset.c +0
-0@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, };
Quoted 5 lines
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = { · line 42
Quoted 3 lines
arch/arm/kvm/reset.c +3
-0+#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq = 27 }, /* irq: A7/A15 specific */This should be parameterised by the vCPU type.
This is already A15 specific, and assigned in an A15 specific code section below.
Quoted 18 lines
Quoted 18 lines
arch/arm/kvm/reset.c +6
-0+ .level = 1 /* level */ +}; +#endif /******************************************************************************* * Exported reset function @@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)return -EINVAL ; cpu_reset = &a15_regs_reset; vcpu->arch.midr =read_cpuid_id (); +#ifdef CONFIG_KVM_ARM_TIMER + vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi; +#endif break ;default :return -ENODEV ;arch/arm/kvm/timer.c new file +0
-0
Quoted 8 lines
arch/arm/kvm/timer.c
Quoted 3 lines
index 0000000..a241298
arch/arm/kvm/timer.c new file +0
-0Maybe
kvm/arch_timer.csince it seems to be specific to that device and matches the naming for the main driver underkernel/? If we get new virtual timer devices in the future, I guess this would need to be split up so the arch-timer-specific bits are separate from generic hrtimer and kvm code.
Sure.
Quoted 37 lines
Quoted 37 lines
arch/arm/kvm/timer.c new file +36
-0@@ -0,0 +1,204 @@ +/* +* Copyright (C) 2012 ARM Ltd. +* Author: Marc Zyngier <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#include <linux/of_irq.h> +#include <linux/kvm.h> +#include <linux/kvm_host.h> +#include <linux/interrupt.h> + +#include <asm/arch_timer.h> + +#include <asm/kvm_vgic.h> +#include <asm/kvm_arch_timer.h> + +static struct timecounter *timecounter; +static struct workqueue_struct *wqueue; + +static cycle_t kvm_phys_timer_read (void ) +{ +return timecounter->cc->read (timecounter->cc); +} +
Quoted 8 lines
arch/arm/kvm/timer.c line 41
Quoted 5 lines
arch/arm/kvm/timer.c new file +5
-0+static void kvm_timer_inject_irq (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + + timer->cntv_ctl |=1 <<1 ;/* Mask the interrupt in the guest */ This is
ARCH_TIMER_CTRL_IT_MASKright? We should make that definition available if it is needed here.
OK.
Quoted 11 lines
Quoted 11 lines
arch/arm/kvm/timer.c new file +11
-0+kvm_vgic_inject_irq (vcpu->kvm, vcpu->vcpu_id, + vcpu->arch.timer_cpu.irq->irq, + vcpu->arch.timer_cpu.irq->level); +} + +static irqreturn_t kvm_arch_timer_handler (int irq,void *dev_id) +{ +struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; + +/* +* We disable the timer in the world switch and let it be
Quoted 8 lines
arch/arm/kvm/timer.c line 58
Quoted 6 lines
arch/arm/kvm/timer.c new file +6
-0+* handled by kvm_timer_sync_from_cpu(). Getting a timer +* interrupt at this point is a sure sign of some major +* breakage. +*/ +pr_warn ("Unexpected interrupt %d on vcpu %p\n" , irq, vcpu); +return IRQ_HANDLED ;
IRQ_NONE?
I don't think so. We're actually handling the interrupt (admittedly in a very basic way), and as it is a per-cpu interrupt, there will be noone else to take care of it.
Quoted 30 lines
Quoted 30 lines
arch/arm/kvm/timer.c new file +30
-0+} + +static void kvm_timer_inject_irq_work (struct work_struct *work) +{ +struct kvm_vcpu *vcpu; + + vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired); + vcpu->arch.timer_cpu.armed = false; +kvm_timer_inject_irq (vcpu); +} + +static enum hrtimer_restart kvm_timer_expire (struct hrtimer *hrt) +{ +struct arch_timer_cpu *timer; + timer = container_of(hrt, struct arch_timer_cpu, timer); +queue_work (wqueue, &timer->expired); +return HRTIMER_NORESTART ; +} + +void kvm_timer_sync_to_cpu (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + +/* +* We're about to run this vcpu again, so there is no need to +* keep the background timer running, as we're about to +* populate the CPU timer again. +*/ +if (timer->armed) { +hrtimer_cancel (&timer->timer);
Quoted 8 lines
arch/arm/kvm/timer.c line 92
Quoted 4 lines
arch/arm/kvm/timer.c new file +4
-0+cancel_work_sync (&timer->expired); + timer->armed = false; + } +}I think some helper functions like
timer_is_armed,timer_armandtimer_disarmwould make this more readable (resistingarm_timer, which confuses things more!).
OK.
Quoted 13 lines
arch/arm/kvm/timer.c line 102
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+ +void kvm_timer_sync_from_cpu (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; +cycle_t cval, now; +u64 ns; + +/* Check if the timer is enabled and unmasked first */ +if ((timer->cntv_ctl &3 ) !=1 ) +return ;Again, we should create/use
ARCH_TIMER#defines for this hardware bits.
Quoted 6 lines
arch/arm/kvm/timer.c line 105
Quoted 2 lines
arch/arm/kvm/timer.c new file +2
-0+ cval = timer->cntv_cval; + now =kvm_phys_timer_read () - vcpu->kvm->arch.timer.cntvoff;We probably want to add an
isb()*before* the mrrc inarch_timer_counter_readif you want to do things like this, because the counter can be speculated in advance.
Indeed.
Quoted 11 lines
arch/arm/kvm/timer.c line 116
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+BUG_ON (timer->armed); + +if (cval <= now) { +/* +* Timer has already expired while we were not +* looking. Inject the interrupt and carry on. +*/ +kvm_timer_inject_irq (vcpu); +return ; + }
Quoted 2 lines
Does this buy you much? You still have to cope with the timer expiring here anyway.
It definitely does from a latency point of view. Programming a timer that will expire right away, calling the interrupt handler, queuing the work queue, waiting for the workqueue to be scheduled and finally delivering the interrupt... If we can catch a few of these early (and we do), it is worth it.
Quoted 64 lines
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+ timer->armed = true; + ns =cyclecounter_cyc2ns (timecounter->cc, cval - now); +hrtimer_start (&timer->timer,ktime_add_ns (ktime_get (), ns), +HRTIMER_MODE_ABS ); +} + +void kvm_timer_vcpu_init (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; +arch/arm/kvm/timer.c line 137
Quoted 10 lines
arch/arm/kvm/timer.c new file +10
-0+INIT_WORK (&timer->expired, kvm_timer_inject_irq_work); +hrtimer_init (&timer->timer,CLOCK_MONOTONIC ,HRTIMER_MODE_ABS ); + timer->timer.function = kvm_timer_expire; +} + +static void kvm_timer_init_interrupt (void *info) +{ +unsigned int *irqp = info; + +enable_percpu_irq (*irqp,0 );
IRQ_TYPE_NONEQuoted 41 lines
arch/arm/kvm/timer.c new file +41
-0+} + + +static const struct of_device_id arch_timer_of_match[] = { + { .compatible ="arm,armv7-timer" , }, + {}, +}; + +int kvm_timer_hyp_init (void ) +{ +struct device_node *np; +unsigned int ppi; +int err; + + timecounter =arch_timer_get_timecounter (); +if (!timecounter) +return -ENODEV ; + + np =of_find_matching_node (NULL , arch_timer_of_match); +if (!np) { +kvm_err ("kvm_arch_timer: can't find DT node\n" ); +return -ENODEV ; + } + + ppi =irq_of_parse_and_map (np,2 ); +if (!ppi) { +kvm_err ("kvm_arch_timer: no virtual timer interrupt\n" ); +return -EINVAL ; + } + + err =request_percpu_irq (ppi, 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); +return err; + } + + wqueue =create_singlethread_workqueue ("kvm_arch_timer" ); +if (!wqueue) { +free_percpu_irq (ppi,kvm_get_running_vcpus ());
Quoted 8 lines
arch/arm/kvm/timer.c line 183
Quoted 5 lines
arch/arm/kvm/timer.c new file +5
-0+return -ENOMEM ; + } + +kvm_info ("%s IRQ%d\n" , np->name, ppi); +on_each_cpu (kvm_timer_init_interrupt, &ppi,1 );
on_each_cpucurrently just returns 0, but you could use that as your return value for good measure anyway.
Sure.
Quoted 9 lines
Quoted 9 lines
arch/arm/kvm/timer.c new file +9
-0+return 0 ; +} + +void kvm_timer_vcpu_terminate (struct kvm_vcpu *vcpu) +{ +struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; + +hrtimer_cancel (&timer->timer); +cancel_work_sync (&timer->expired);
Quoted 8 lines
arch/arm/kvm/timer.c line 199
Quoted 6 lines
arch/arm/kvm/timer.c new file +6
-0+} + +int kvm_timer_init (struct kvm *kvm) +{ +if (timecounter && wqueue) { + kvm->arch.timer.cntvoff =kvm_phys_timer_read ();Shouldn't this be initialised to 0 and then updated on world switch?
No. You do not want your virtual offset to drift. Otherwise you'll observe something like time dilatation, and your clocks will drift. Plus, you really want all your vcpus to be synchronized. Allowing them to drift apart could be an interesting experience... ;-)
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support
Quoted 9 lines · Marc Zyngier
Quoted 8 lines · Will Deacon
Quoted 8 lines
arch/arm/kvm/reset.c +0
-0@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, };
Quoted 8 lines
Quoted 5 lines
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = { · line 42
Quoted 3 lines
arch/arm/kvm/reset.c +3
-0+#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq = 27 }, /* irq: A7/A15 specific */This should be parameterised by the vCPU type.
This is already A15 specific, and assigned in an A15 specific code section below.
Right, but we can take the interrupt number from the device-tree, like we do for the host anyway.
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
arch/arm/kvm/reset.c +6
-0+static irqreturn_t kvm_arch_timer_handler (int irq,void *dev_id) +{ +struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; + +/* +* We disable the timer in the world switch and let it be Quoted 8 lines
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = {
Quoted 6 lines
arch/arm/kvm/reset.c +6
-0+* handled by kvm_timer_sync_from_cpu(). Getting a timer +* interrupt at this point is a sure sign of some major +* breakage. +*/ +pr_warn ("Unexpected interrupt %d on vcpu %p\n" , irq, vcpu); +return IRQ_HANDLED ;
IRQ_NONE?
Quoted 3 lines
I don't think so. We're actually handling the interrupt (admittedly in a very basic way), and as it is a per-cpu interrupt, there will be noone else to take care of it.
For an SPI, returning IRQ_NONE would (eventually) silence a screaming interrupt because the generic IRQ bits would disable it. I'm not sure if that applies to PPIs or not but if it does, I'd say that's a good reason to use it.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
arch/arm/kvm/reset.c +10
-0+BUG_ON (timer->armed); + +if (cval <= now) { +/* +* Timer has already expired while we were not +* looking. Inject the interrupt and carry on. +*/ +kvm_timer_inject_irq (vcpu); +return ; + }
Quoted 8 lines
Quoted 2 lines
Does this buy you much? You still have to cope with the timer expiring here anyway.
It definitely does from a latency point of view. Programming a timer that will expire right away, calling the interrupt handler, queuing the work queue, waiting for the workqueue to be scheduled and finally delivering the interrupt... If we can catch a few of these early (and we do), it is worth it.
Ok, interesting. I wasn't sure how often that happened in practice.
Quoted 11 lines
Quoted 6 lines
arch/arm/kvm/reset.c
Quoted 4 lines
arch/arm/kvm/reset.c +4
-0+int kvm_timer_init (struct kvm *kvm) +{ +if (timecounter && wqueue) { + kvm->arch.timer.cntvoff =kvm_phys_timer_read ();Shouldn't this be initialised to 0 and then updated on world switch?
No. You do not want your virtual offset to drift. Otherwise you'll observe something like time dilatation, and your clocks will drift. Plus, you really want all your vcpus to be synchronized. Allowing them to drift apart could be an interesting experience... ;-)
In which case, why do we initialise it to the physical timer in the first place? Surely the value doesn't matter, as long as everybody agrees on what it is?
Will
Re: [PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support
Quoted 16 lines · Will Deacon
Quoted 15 lines · Marc Zyngier
Quoted 13 lines · Will Deacon
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = { · line 42
Quoted 11 lines
arch/arm/kvm/reset.c +3
-0@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, }; +#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq = 27 }, /* irq: A7/A15 specific */This should be parameterised by the vCPU type.
Quoted 5 lines
Quoted 2 lines
This is already A15 specific, and assigned in an A15 specific code section below.
Right, but we can take the interrupt number from the device-tree, like we do for the host anyway.
Certainly. I'll update this bit.
Quoted 53 lines
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
arch/arm/kvm/reset.c +6
-0+static irqreturn_t kvm_arch_timer_handler (int irq,void *dev_id) +{ +struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; + +/* +* We disable the timer in the world switch and let it be Quoted 8 lines
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = {
Quoted 6 lines
arch/arm/kvm/reset.c +6
-0+* handled by kvm_timer_sync_from_cpu(). Getting a timer +* interrupt at this point is a sure sign of some major +* breakage. +*/ +pr_warn ("Unexpected interrupt %d on vcpu %p\n" , irq, vcpu); +return IRQ_HANDLED ;
IRQ_NONE?Quoted 3 lines
I don't think so. We're actually handling the interrupt (admittedly in a very basic way), and as it is a per-cpu interrupt, there will be noone else to take care of it.
For an SPI, returning
IRQ_NONEwould (eventually) silence a screaming interrupt because the generic IRQ bits would disable it. I'm not sure if that applies to PPIs or not but if it does, I'd say that's a good reason to use it.Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
arch/arm/kvm/reset.c +10
-0+BUG_ON (timer->armed); + +if (cval <= now) { +/* +* Timer has already expired while we were not +* looking. Inject the interrupt and carry on. +*/ +kvm_timer_inject_irq (vcpu); +return ; + }Quoted 8 lines
Quoted 2 lines
Does this buy you much? You still have to cope with the timer expiring here anyway.
It definitely does from a latency point of view. Programming a timer that will expire right away, calling the interrupt handler, queuing the work queue, waiting for the workqueue to be scheduled and finally delivering the interrupt... If we can catch a few of these early (and we do), it is worth it.
Ok, interesting. I wasn't sure how often that happened in practice.
Quoted 7 lines
Quoted 6 lines
arch/arm/kvm/reset.c
Quoted 4 lines
arch/arm/kvm/reset.c +4
-0+int kvm_timer_init (struct kvm *kvm) +{ +if (timecounter && wqueue) { + kvm->arch.timer.cntvoff =kvm_phys_timer_read ();Shouldn't this be initialised to 0 and then updated on world switch?
Quoted 8 lines
Quoted 4 lines
No. You do not want your virtual offset to drift. Otherwise you'll observe something like time dilatation, and your clocks will drift. Plus, you really want all your vcpus to be synchronized. Allowing them to drift apart could be an interesting experience... ;-)
In which case, why do we initialise it to the physical timer in the first place? Surely the value doesn't matter, as long as everybody agrees on what it is?
This ensures that the VM gets its virtual counter starting as closely as possible to 0. As good a convention as any.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support
Quoted 17 lines · Marc Zyngier
Quoted 16 lines · Will Deacon
Quoted 15 lines · Marc Zyngier
Quoted 13 lines · Will Deacon
arch/arm/kvm/reset.c static struct kvm_regs a15_regs_reset = { · line 42
Quoted 11 lines
arch/arm/kvm/reset.c +3
-0@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = { .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT, }; +#ifdef CONFIG_KVM_ARM_TIMER +static const struct kvm_irq_level a15_virt_timer_ppi = { + { .irq = 27 }, /* irq: A7/A15 specific */This should be parameterised by the vCPU type.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This is already A15 specific, and assigned in an A15 specific code section below.
Right, but we can take the interrupt number from the device-tree, like we do for the host anyway.
Certainly. I'll update this bit.
So we lock ourselves to always only support emulating the same CPU on the guest as on the host?
(I know we assume this elsewhere and that even the A7 uses the same IRQ number, but these doesn't necessarily have to be the same in the future, and there's no need to make such a task more difficult, and we are not really making things mode stable/simple by taking it from the DT - are we?)
-Christoffer
PATCH v4 4/5 ARM: KVM: arch_timers: Add timer world switch
In reply to Christoffer DallFrom: Marc Zyngier <[email protected]>
Do the necessary save/restore dance for the timers in the world switch code. In the process, allow the guest to read the physical counter, which is useful for its own clock_event_device.
--- arch/arm/kernel/asm-offsets.c | 8 ++++++++ arch/arm/kvm/arm.c | 3 +++ arch/arm/kvm/interrupts_head.S | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+)
arch/arm/kernel/asm-offsets.c +8 -0
@@ -177,6 +177,14 @@ int main(void)DEFINE (VGIC_CPU_APR , offsetof(struct vgic_cpu , vgic_apr));DEFINE (VGIC_CPU_LR , offsetof(struct vgic_cpu , vgic_lr));DEFINE (VGIC_CPU_NR_LR , offsetof(struct vgic_cpu , nr_lr)); +#ifdef CONFIG_KVM_ARM_TIMER + DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); + DEFINE(VCPU_TIMER_CNTV_CVALH, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high)); + DEFINE(VCPU_TIMER_CNTV_CVALL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low)); + DEFINE(KVM_TIMER_CNTVOFF_H, offsetof(struct kvm, arch.timer.cntvoff32.high)); + DEFINE(KVM_TIMER_CNTVOFF_L, offsetof(struct kvm, arch.timer.cntvoff32.low)); + DEFINE(KVM_TIMER_ENABLED, offsetof(struct kvm, arch.timer.enabled)); +#endif DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base));#endif DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr));
arch/arm/kvm/arm.c +3 -0
@@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)update_vttbr (vcpu->kvm);kvm_vgic_sync_to_cpu (vcpu); +kvm_timer_sync_to_cpu (vcpu);local_irq_disable ();
@@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)if (ret <=0 ||need_new_vmid_gen (vcpu->kvm)) {local_irq_enable (); +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu);continue ; }
@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)* Back from guest *************************************************************/ +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu); ret =handle_exit (vcpu, run, ret);
arch/arm/kvm/interrupts_head.S +41 -0
53 lines not on this page · raw message
Re: [PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch
Quoted 67 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
Do the necessary save/restore dance for the timers in the world switch code. In the process, allow the guest to read the physical counter, which is useful for its own
clock_event_device.Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/kernel/asm-offsets.c | 8 ++++++++ arch/arm/kvm/arm.c | 3 +++ arch/arm/kvm/interrupts_head.S | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+)arch/arm/kernel/asm-offsets.c +8
-0@@ -177,6 +177,14 @@ int main(void)DEFINE (VGIC_CPU_APR , offsetof(struct vgic_cpu , vgic_apr));DEFINE (VGIC_CPU_LR , offsetof(struct vgic_cpu , vgic_lr));DEFINE (VGIC_CPU_NR_LR , offsetof(struct vgic_cpu , nr_lr)); +#ifdef CONFIG_KVM_ARM_TIMER + DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); + DEFINE(VCPU_TIMER_CNTV_CVALH, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high)); + DEFINE(VCPU_TIMER_CNTV_CVALL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low)); + DEFINE(KVM_TIMER_CNTVOFF_H, offsetof(struct kvm, arch.timer.cntvoff32.high)); + DEFINE(KVM_TIMER_CNTVOFF_L, offsetof(struct kvm, arch.timer.cntvoff32.low)); + DEFINE(KVM_TIMER_ENABLED, offsetof(struct kvm, arch.timer.enabled)); +#endif DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base));#endif DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr));arch/arm/kvm/arm.c +3
-0@@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)update_vttbr (vcpu->kvm);kvm_vgic_sync_to_cpu (vcpu); +kvm_timer_sync_to_cpu (vcpu);local_irq_disable ();@@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)if (ret <=0 ||need_new_vmid_gen (vcpu->kvm)) {local_irq_enable (); +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu);continue ; }@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)* Back from guest *************************************************************/ +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu); ret =handle_exit (vcpu, run, ret);arch/arm/kvm/interrupts_head.S +0
-0@@ -422,6 +422,25 @@ #define CNTHCTL_PL1PCEN (1 << 1)
arch/arm/kvm/interrupts_head.S line 432
Quoted 10 lines
arch/arm/kvm/interrupts_head.S +8
-0.macro save_timer_state vcpup +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL + and r2, #3
Why do you need this and (you do the masking on the restore path)?
Quoted 26 lines
arch/arm/kvm/interrupts_head.S +19
-0+ str r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + bic r2, #1 @ Clear ENABLE + mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb + + mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL + str r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + str r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL] + +1: +#endif @ Allow physical timer/counter access for the host mrc p15, 4, r2, c14, c1, 0 @ CNTHCTL orr r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)@@ -435,6 +454,28 @@ orr r2, r2, #CNTHCTL_PL1PCTEN bic r2, r2, #CNTHCTL_PL1PCEN mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL + +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + ldr r3, [r4, #KVM_TIMER_CNTVOFF_H]
arch/arm/kvm/interrupts_head.S line 474
Quoted 10 lines
arch/arm/kvm/interrupts_head.S +10
-0+ ldr r2, [r4, #KVM_TIMER_CNTVOFF_L] + mcrr p15, 4, r2, r3, c14 @ CNTVOFF + isb + + ldr r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL] + mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL + + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + and r2, #3
Please use the long form syntax for these instructions (and r2, r2, #3) as toolchains have historically been picky about accepting these when not targetting Thumb and it also makes it consistent with the rest of arch/arm/.
arch/arm/kvm/interrupts_head.S line 443
Quoted 4 lines
arch/arm/kvm/interrupts_head.S +4
-0+ mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb +1: +#endif
It's pretty horrible to take a macro argument (vcpup) and then open-code all the other register numbers without moving the argument somewhere nice. Are callers just expected to understand that you clobber {r2,r3,r4} and avoid them? If so, why not just use the PCS so that people don't have to learn a new convention?
Will
Re: [PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch
Quoted 69 lines · Will Deacon
Quoted 68 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
Do the necessary save/restore dance for the timers in the world switch code. In the process, allow the guest to read the physical counter, which is useful for its own
clock_event_device.Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/kernel/asm-offsets.c | 8 ++++++++ arch/arm/kvm/arm.c | 3 +++ arch/arm/kvm/interrupts_head.S | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+)arch/arm/kernel/asm-offsets.c +8
-0@@ -177,6 +177,14 @@ int main(void)DEFINE (VGIC_CPU_APR , offsetof(struct vgic_cpu , vgic_apr));DEFINE (VGIC_CPU_LR , offsetof(struct vgic_cpu , vgic_lr));DEFINE (VGIC_CPU_NR_LR , offsetof(struct vgic_cpu , nr_lr)); +#ifdef CONFIG_KVM_ARM_TIMER + DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); + DEFINE(VCPU_TIMER_CNTV_CVALH, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high)); + DEFINE(VCPU_TIMER_CNTV_CVALL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low)); + DEFINE(KVM_TIMER_CNTVOFF_H, offsetof(struct kvm, arch.timer.cntvoff32.high)); + DEFINE(KVM_TIMER_CNTVOFF_L, offsetof(struct kvm, arch.timer.cntvoff32.low)); + DEFINE(KVM_TIMER_ENABLED, offsetof(struct kvm, arch.timer.enabled)); +#endif DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base));#endif DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr));arch/arm/kvm/arm.c +3
-0@@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)update_vttbr (vcpu->kvm);kvm_vgic_sync_to_cpu (vcpu); +kvm_timer_sync_to_cpu (vcpu);local_irq_disable ();@@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)if (ret <=0 ||need_new_vmid_gen (vcpu->kvm)) {local_irq_enable (); +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu);continue ; }@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)* Back from guest *************************************************************/ +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu); ret =handle_exit (vcpu, run, ret);arch/arm/kvm/interrupts_head.S +0
-0@@ -422,6 +422,25 @@ #define CNTHCTL_PL1PCEN (1 << 1)
Quoted 11 lines
arch/arm/kvm/interrupts_head.S line 432
Quoted 9 lines
arch/arm/kvm/interrupts_head.S +8
-0.macro save_timer_state vcpup +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL + and r2, #3Why do you need this and (you do the masking on the restore path)?
Probably not necessary. We could just clear the enable below.
Quoted 33 lines
Quoted 33 lines
arch/arm/kvm/interrupts_head.S +25
-0+ str r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + bic r2, #1 @ Clear ENABLE + mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb + + mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL + str r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + str r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL] + +1: +#endif @ Allow physical timer/counter access for the host mrc p15, 4, r2, c14, c1, 0 @ CNTHCTL orr r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)@@ -435,6 +454,28 @@ orr r2, r2, #CNTHCTL_PL1PCTEN bic r2, r2, #CNTHCTL_PL1PCEN mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL + +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + ldr r3, [r4, #KVM_TIMER_CNTVOFF_H] + ldr r2, [r4, #KVM_TIMER_CNTVOFF_L] + mcrr p15, 4, r2, r3, c14 @ CNTVOFF + isb + + ldr r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
Quoted 8 lines
arch/arm/kvm/interrupts_head.S line 474
Quoted 4 lines
arch/arm/kvm/interrupts_head.S +4
-0+ mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL + + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + and r2, #3Please use the long form syntax for these instructions (and r2, r2, #3) as toolchains have historically been picky about accepting these when not targetting Thumb and it also makes it consistent with the rest of
arch/arm/.
OK.
Quoted 11 lines
arch/arm/kvm/interrupts_head.S line 443
Quoted 5 lines
arch/arm/kvm/interrupts_head.S +4
-0+ mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb +1: +#endifIt's pretty horrible to take a macro argument (vcpup) and then open-code all the other register numbers without moving the argument somewhere nice. Are callers just expected to understand that you clobber {r2,r3,r4} and avoid them? If so, why not just use the PCS so that people don't have to learn a new convention?
I'm happy to remove this vcpup and stick to r0/r1 (depending on the path). All macros have to know about the side effects anyway, as we switch all the registers.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch
Quoted 140 lines · Marc Zyngier
Quoted 69 lines · Will Deacon
Quoted 68 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
Do the necessary save/restore dance for the timers in the world switch code. In the process, allow the guest to read the physical counter, which is useful for its own
clock_event_device.Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/kernel/asm-offsets.c | 8 ++++++++ arch/arm/kvm/arm.c | 3 +++ arch/arm/kvm/interrupts_head.S | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+)arch/arm/kernel/asm-offsets.c +8
-0@@ -177,6 +177,14 @@ int main(void)DEFINE (VGIC_CPU_APR , offsetof(struct vgic_cpu , vgic_apr));DEFINE (VGIC_CPU_LR , offsetof(struct vgic_cpu , vgic_lr));DEFINE (VGIC_CPU_NR_LR , offsetof(struct vgic_cpu , nr_lr)); +#ifdef CONFIG_KVM_ARM_TIMER + DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); + DEFINE(VCPU_TIMER_CNTV_CVALH, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high)); + DEFINE(VCPU_TIMER_CNTV_CVALL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low)); + DEFINE(KVM_TIMER_CNTVOFF_H, offsetof(struct kvm, arch.timer.cntvoff32.high)); + DEFINE(KVM_TIMER_CNTVOFF_L, offsetof(struct kvm, arch.timer.cntvoff32.low)); + DEFINE(KVM_TIMER_ENABLED, offsetof(struct kvm, arch.timer.enabled)); +#endif DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base));#endif DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr));arch/arm/kvm/arm.c +3
-0@@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)update_vttbr (vcpu->kvm);kvm_vgic_sync_to_cpu (vcpu); +kvm_timer_sync_to_cpu (vcpu);local_irq_disable ();@@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)if (ret <=0 ||need_new_vmid_gen (vcpu->kvm)) {local_irq_enable (); +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu);continue ; }@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)* Back from guest *************************************************************/ +kvm_timer_sync_from_cpu (vcpu);kvm_vgic_sync_from_cpu (vcpu); ret =handle_exit (vcpu, run, ret);arch/arm/kvm/interrupts_head.S +0
-0@@ -422,6 +422,25 @@ #define CNTHCTL_PL1PCEN (1 << 1)Quoted 11 lines
arch/arm/kvm/interrupts_head.S line 432
Quoted 9 lines
arch/arm/kvm/interrupts_head.S +8
-0.macro save_timer_state vcpup +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL + and r2, #3Why do you need this and (you do the masking on the restore path)?
Probably not necessary. We could just clear the enable below.
Quoted 33 lines
Quoted 33 lines
arch/arm/kvm/interrupts_head.S +25
-0+ str r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + bic r2, #1 @ Clear ENABLE + mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb + + mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL + str r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + str r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL] + +1: +#endif @ Allow physical timer/counter access for the host mrc p15, 4, r2, c14, c1, 0 @ CNTHCTL orr r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)@@ -435,6 +454,28 @@ orr r2, r2, #CNTHCTL_PL1PCTEN bic r2, r2, #CNTHCTL_PL1PCEN mcr p15, 4, r2, c14, c1, 0 @ CNTHCTL + +#ifdef CONFIG_KVM_ARM_TIMER + ldr r4, [\vcpup, #VCPU_KVM] + ldr r2, [r4, #KVM_TIMER_ENABLED] + cmp r2, #0 + beq 1f + + ldr r3, [r4, #KVM_TIMER_CNTVOFF_H] + ldr r2, [r4, #KVM_TIMER_CNTVOFF_L] + mcrr p15, 4, r2, r3, c14 @ CNTVOFF + isb + + ldr r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH] + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]Quoted 8 lines
arch/arm/kvm/interrupts_head.S line 474
Quoted 4 lines
arch/arm/kvm/interrupts_head.S +4
-0+ mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL + + ldr r2, [\vcpup, #VCPU_TIMER_CNTV_CTL] + and r2, #3Please use the long form syntax for these instructions (and r2, r2, #3) as toolchains have historically been picky about accepting these when not targetting Thumb and it also makes it consistent with the rest of
arch/arm/.OK.
Quoted 11 lines
arch/arm/kvm/interrupts_head.S line 443
Quoted 5 lines
arch/arm/kvm/interrupts_head.S +4
-0+ mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL + isb +1: +#endifIt's pretty horrible to take a macro argument (vcpup) and then open-code all the other register numbers without moving the argument somewhere nice. Are callers just expected to understand that you clobber {r2,r3,r4} and avoid them? If so, why not just use the PCS so that people don't have to learn a new convention?
Quoted 4 lines
I'm happy to remove this vcpup and stick to r0/r1 (depending on the path). All macros have to know about the side effects anyway, as we switch all the registers.
Using the PCS is not going to help us, because we need a lot of registers for a lot of these operations and we don't want to push/pop extra stuff to the stack here - this is in the super critical path.
The nature of this code is not one where you have random callers that need to work under certain assumptions - you need to scrutinize every line here.
What we could do is pass all temporary variables to the macros, but it becomes ugly when they take 5+ temporary regs, and it doesn't make sense for register save/restore that also makes assumptions about which register is used for the vcpu pointer.
Alternatively, and more attainable, we could make it a convention that all the macros in this file take r0 as the vcpu pointer. Actually, let's use r0 for the cpu pointer all over. Why not.
Let me craft a patch and send that out right away!
-Christoffer
PATCH v4 5/5 ARM: KVM: arch_timers: Wire the init code and config option
In reply to Christoffer DallFrom: Marc Zyngier <[email protected]>
It is now possible to select CONFIG_KVM_ARM_TIMER to enable the KVM architected timer support.
--- arch/arm/kvm/Kconfig | 7 +++++++ arch/arm/kvm/Makefile | 1 + arch/arm/kvm/arm.c | 11 +++++++++++ arch/arm/kvm/vgic.c | 1 + 4 files changed, 20 insertions(+)
arch/arm/kvm/Kconfig +7 -0
@@ -58,6 +58,13 @@ config KVM_ARM_VGIC ---help--- Adds support for a hardware assisted, in-kernel GIC emulation. +config KVM_ARM_TIMER + bool "KVM support for Architected Timers" + depends on KVM_ARM_VGIC && ARM_ARCH_TIMER + select HAVE_KVM_IRQCHIP + ---help--- + Adds support for the Architected Timers in virtual machines + source drivers/virtio/Kconfig endif # VIRTUALIZATION
arch/arm/kvm/Makefile +1 -0
@@ -21,3 +21,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += $(addprefix ../../../virt/kvm/, kvm_main.o coalesc obj-$(CONFIG_KVM_ARM_HOST) += arm.o guest.o mmu.o emulate.o reset.o obj-$(CONFIG_KVM_ARM_HOST) += coproc.o coproc_a15.o mmio.o decode.o obj-$(CONFIG_KVM_ARM_VGIC) += vgic.o +obj-$(CONFIG_KVM_ARM_TIMER) += timer.o
arch/arm/kvm/arm.c +11 -0
@@ -286,6 +286,7 @@ out:void kvm_arch_vcpu_free (struct kvm_vcpu *vcpu) {kvm_mmu_free_memory_caches (vcpu); +kvm_timer_vcpu_terminate (vcpu);kmem_cache_free (kvm_vcpu_cache, vcpu); }
@@ -323,6 +324,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)/* Set up VGIC */ kvm_vgic_vcpu_init (vcpu); +/* Set up the timer */ +kvm_timer_vcpu_init (vcpu); +return 0 ; }
@@ -1048,6 +1052,13 @@ static int init_hyp_mode(void)if (!err) vgic_present = true; +/* +* Init HYP architected timer support +*/ + err =kvm_timer_hyp_init (); +if (err) + goto out_free_mappings; +return 0 ; out_free_vfp:free_percpu (kvm_host_vfp_state);
arch/arm/kvm/vgic.c +1 -0
@@ -1168,6 +1168,7 @@ int kvm_vgic_init(struct kvm *kvm)for (i =32 ; i <VGIC_NR_IRQS ; i +=4 )vgic_set_target_reg (kvm,0 , i); +kvm_timer_init (kvm); kvm->arch.vgic.ready = true; out:mutex_unlock (&kvm->lock);
Re: [PATCH v4 5/5] ARM: KVM: arch_timers: Wire the init code and config option
Quoted 19 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
It is now possible to select
CONFIG_KVM_ARM_TIMERto enable the KVM architected timer support.Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/kvm/Kconfig | 7 +++++++ arch/arm/kvm/Makefile | 1 + arch/arm/kvm/arm.c | 11 +++++++++++ arch/arm/kvm/vgic.c | 1 + 4 files changed, 20 insertions(+)arch/arm/kvm/Kconfig +0
-0
arch/arm/kvm/Kconfig config KVM_ARM_VGIC · line 67
Quoted 10 lines
arch/arm/kvm/Kconfig +7
-0@@ -58,6 +58,13 @@ config KVM_ARM_VGIC ---help--- Adds support for a hardware assisted, in-kernel GIC emulation. +config KVM_ARM_TIMER + bool "KVM support for Architected Timers" + depends on KVM_ARM_VGIC && ARM_ARCH_TIMER + select HAVE_KVM_IRQCHIP + ---help--- + Adds support for the Architected Timers in virtual machines +
This should probably be default y
Will
Re: [PATCH v4 5/5] ARM: KVM: arch_timers: Wire the init code and config option
Quoted 23 lines · Will Deacon
Quoted 22 lines · Christoffer Dall
From: Marc Zyngier <[email protected]>
It is now possible to select
CONFIG_KVM_ARM_TIMERto enable the KVM architected timer support.Signed-off-by Marc Zyngier <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/kvm/Kconfig | 7 +++++++ arch/arm/kvm/Makefile | 1 + arch/arm/kvm/arm.c | 11 +++++++++++ arch/arm/kvm/vgic.c | 1 + 4 files changed, 20 insertions(+)arch/arm/kvm/Kconfig +0
-0@@ -58,6 +58,13 @@ config KVM_ARM_VGIC ---help--- Adds support for a hardware assisted, in-kernel GIC emulation.
Quoted 10 lines
arch/arm/kvm/Kconfig config KVM_ARM_VGIC · line 67
Quoted 7 lines
arch/arm/kvm/Kconfig +7
-0+config KVM_ARM_TIMER + bool "KVM support for Architected Timers" + depends on KVM_ARM_VGIC && ARM_ARCH_TIMER + select HAVE_KVM_IRQCHIP + ---help--- + Adds support for the Architected Timers in virtual machines +This should probably be default y
yes, good idea, they all should, like this:
arch/arm/kvm/Kconfig +3 -0
@@ -21,6 +21,7 @@ config KVM select PREEMPT_NOTIFIERS select ANON_INODES select KVM_MMIO + select KVM_ARM_HOST depends on ARM_VIRT_EXT && ARM_LPAE ---help--- Support hosting virtualized guest machines. You will also
@@ -54,6 +55,7 @@ config KVM_ARM_VGIC bool "KVM support for Virtual GIC" depends on KVM_ARM_HOST && OF select HAVE_KVM_IRQCHIP + default y ---help--- Adds support for a hardware assisted, in-kernel GIC emulation.
@@ -61,6 +63,7 @@ config KVM_ARM_TIMER bool "KVM support for Architected Timers" depends on KVM_ARM_VGIC && ARM_ARCH_TIMER select HAVE_KVM_IRQCHIP + default y ---help--- Adds support for the Architected Timers in virtual machines
--
Thanks,
-Christoffer