On Fri, May 31, 2013 at 10:04:35AM +0100, Marc Zyngier wrote:
> On 31/05/13 07:10, Christoffer Dall wrote:
> > On Thu, May 30, 2013 at 09:17:53PM +0200, Andre Przywara wrote:
> >> Christoffer, Geoff,
> >>
> >> below the timer state save patch.
> >> I am using the ONE_REG interface of the kernel to export the virtual
> >> timer state, which is actually fully per (V)CPU.
> >> Briefly on ONE_REG: There is an ioctl which can enumerate all
> >> available registers (by unique numbers) and then each of the
> >> registers is saved/restored via the [sg]et_one_reg ioctl. The
> >> userland currently just saves/restores every register advertised by
> >> the kernel, without using any knowledge about their specific
> >> semantics. So by just including the timer state registers in the list
> >> we actually get the userland almost for free.
> >>
> >> We handle three timer registers:
> >> - the control register, basically holding the enable bit
> >> - the counter register
> >> - the compare register, holding the value when the next interrupt
> >> should be triggered
> >>
> >> In case you wonder, the magic numbers for the registers are an
> >> encoding of the coproc parameters (CRn,CRm,Op1,Op2) together with
> >> some bits for the ARM architecture, 32/64 bit value size and
> >> coproc #15.
> >>
> >> If you have question, feel free to ask. I will make a proper commit
> >> message tomorrow.
> >>
> >> Regards,
> >> Andre.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> >> ---
> >> arch/arm/include/asm/kvm_asm.h | 7 ++++++-
> >> arch/arm/include/asm/kvm_host.h | 3 +++
> >> arch/arm/include/uapi/asm/kvm.h | 4 ++++
> >> arch/arm/kvm/arch_timer.c | 32 ++++++++++++++++++++++++++++++++
> >> arch/arm/kvm/coproc.c | 30 ++++++++++++++++++++++++++++++
> >> 5 files changed, 75 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> >> index 18d5032..a732b17 100644
> >> --- a/arch/arm/include/asm/kvm_asm.h
> >> +++ b/arch/arm/include/asm/kvm_asm.h
> >> @@ -46,7 +46,12 @@
> >> #define c13_TID_URO 24 /* Thread ID, User R/O */
> >> #define c13_TID_PRIV 25 /* Thread ID, Privileged */
> >> #define c14_CNTKCTL 26 /* Timer Control Register (PL1) */
> >> -#define NR_CP15_REGS 27 /* Number of regs (incl. invalid) */
> >> +#define c14_CNTV_CTL 27 /* Virtual Timer Control Register */
> >> +#define c14_CNTVCT 28 /* Virtual Timer Counter Register */
> >> +#define c14_CNTVCT_high 29 /* Virtual Timer Counter Register */
> >> +#define c14_CNTV_CVAL 30 /* Virtual Timer Counter Register */
> >> +#define c14_CNTV_CVAL_high 31 /* Virtual Timer Counter Register */
> >> +#define NR_CP15_REGS 32 /* Number of regs (incl. invalid) */
>
> So there is one thing that strikes me here:
> We have reserved storage these registers in the CP15 file, but we're
> actually not using that storage at all, as all the data is stored in a
> separate structure.
>
> >>
> >> #define ARM_EXCEPTION_RESET 0
> >> #define ARM_EXCEPTION_UNDEFINED 1
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 57cb786..80714ab 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -224,4 +224,7 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
> >> int kvm_perf_init(void);
> >> int kvm_perf_teardown(void);
> >>
> >> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, const struct kvm_one_reg *);
> >> +void kvm_arm_timer_set_reg(struct kvm_vcpu *, const struct kvm_one_reg *, u64);
> >> +
> >> #endif /* __ARM_KVM_HOST_H__ */
> >> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> >> index c1ee007..796e736 100644
> >> --- a/arch/arm/include/uapi/asm/kvm.h
> >> +++ b/arch/arm/include/uapi/asm/kvm.h
> >> @@ -118,6 +118,10 @@ struct kvm_arch_memory_slot {
> >> #define KVM_REG_ARM_32_CRN_MASK 0x0000000000007800
> >> #define KVM_REG_ARM_32_CRN_SHIFT 11
> >>
> >> +#define KVM_REG_ARM_TIMER_CTL 0x40200000000F7181
> >> +#define KVM_REG_ARM_TIMER_CNT 0x40300000000F0708
> >> +#define KVM_REG_ARM_TIMER_CVAL 0x40300000000F0718
> >> +
> >
> > You should add this to Documentation/virtual/kvm/api.txt
> >
> >> /* Normal registers are mapped as coprocessor 16. */
> >> #define KVM_REG_ARM_CORE (0x0010 << KVM_REG_ARM_COPROC_SHIFT)
> >> #define KVM_REG_ARM_CORE_REG(name) (offsetof(struct kvm_regs, name) / 4)
> >> diff --git a/arch/arm/kvm/arch_timer.c b/arch/arm/kvm/arch_timer.c
> >> index c55b608..64a8920b 100644
> >> --- a/arch/arm/kvm/arch_timer.c
> >> +++ b/arch/arm/kvm/arch_timer.c
> >> @@ -171,6 +171,38 @@ static void kvm_timer_init_interrupt(void *info)
> >> enable_percpu_irq(timer_irq.irq, 0);
> >> }
> >>
> >> +void kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu,
> >> + const struct kvm_one_reg *reg, u64 val)
> >> +{
> >> + struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >> +
> >> + switch (reg->id) {
> >> + case KVM_REG_ARM_TIMER_CTL:
> >> + timer->cntv_ctl = val;
> >> + break;
> >> + case KVM_REG_ARM_TIMER_CNT:
> >> + vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;
> >> + break;
> >> + case KVM_REG_ARM_TIMER_CVAL:
> >> + timer->cntv_cval = val;
> >> + break;
> >> + }
> >
> > Isn't there a case here when you restore a timer, where if/when this VM were
> > left running on the original host there was potentially a soft timer
> > schedules, which we'll not catch now?
>
> cntv_cval reflects the time when the timer is expected to fire.
>
> > Or are we relying on the fact that we always run the VCPU again after a
> > restore of the VM, and we'll write this state back onto the hardware,
> > and the hardware will cause a timer interrupt? But, hmmm, then the time
> > could have passed and we wouldn't pick it up until the next exit from
> > the guest.
>
> The interrupt will fire immediately, as the condition will be met
> (cntvct >= cntv_cval).
>
ok, so we're relying on the fact that we'll never have a VCPU be blocked
after migrating the VM, I thought of the cntv_cval as firing when it
equaled cntvct, which was obviously not correct.
> > I guess on a tickless system this could potentially look a little bad,
> > unintentionally leaving the guest running in a wait loop for a long
> > time, no?
>
> Not sure I understand what you mean here. Care to elaborate?
>
See above, I was under the impression that restoring the registers as
they were after the counter value had expired, would not cause a
hardware interrupt, flawed on my side.
> > If this is all bogus (quite possible) then at least lets have a comment
> > here explaining why it's all fine and dandy.
> >
I still think a comment as to why we don't need a timer after migrating
the VM makes sense.
> >> +}
> >> +
> >> +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >> +{
> >> + struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >> +
> >> + switch (reg->id) {
> >> + case KVM_REG_ARM_TIMER_CTL:
> >> + return timer->cntv_ctl;
> >> + case KVM_REG_ARM_TIMER_CNT:
> >> + return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> >> + case KVM_REG_ARM_TIMER_CVAL:
> >> + return timer->cntv_cval;
> >> + }
> >> + return 0;
> >> +}
> >>
> >> static int kvm_timer_cpu_notify(struct notifier_block *self,
> >> unsigned long action, void *cpu)
> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index 8eea97b..6bd29c4 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -156,6 +156,8 @@ static const struct coproc_reg cp15_regs[] = {
> >> /* TTBR0/TTBR1: swapped by interrupt.S. */
> >> { CRm( 2), Op1( 0), is64, NULL, reset_unknown64, c2_TTBR0 },
> >> { CRm( 2), Op1( 1), is64, NULL, reset_unknown64, c2_TTBR1 },
> >> + { CRm(14), Op1( 1), is64, NULL, reset_unknown64, c14_CNTVCT},
> >> + { CRm(14), Op1( 3), is64, NULL, reset_unknown64, c14_CNTV_CVAL},
> >>
> >> /* TTBCR: swapped by interrupt.S. */
> >> { CRn( 2), CRm( 0), Op1( 0), Op2( 2), is32,
> >> @@ -226,6 +228,8 @@ static const struct coproc_reg cp15_regs[] = {
> >> /* CNTKCTL: swapped by interrupt.S. */
> >> { CRn(14), CRm( 1), Op1( 0), Op2( 0), is32,
> >> NULL, reset_val, c14_CNTKCTL, 0x00000000 },
> >> + { CRn(14), CRm( 3), Op1( 0), Op2( 1), is32,
> >> + NULL, reset_unknown, c14_CNTV_CTL},
>
> So to solve the above problem, I'd be tempted to turn the reg field into
> some kind of offset into the vcpu structure... Or find a way to express
> the fact that the value is stored outside of the cp15 array.
>
> We could also do the opposite (change the timer code to use the cp15
> array), but that may become a bit messy with the timer code being shared
> with the arm64 port, and I like it to be relatively clean and
> architecture agnostic (sort of).
>
> > the reset value on an A15 guarantees that bit[0] = 0b0
> >
> >> };
> >>
> >> /* Target specific emulation tables */
> >> @@ -823,10 +827,22 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
> >> }
> >> #endif /* !CONFIG_VFPv3 */
> >>
> >> +static bool is_timer_reg(u64 index)
> >> +{
> >> + switch (index) {
> >> + case KVM_REG_ARM_TIMER_CTL:
> >> + case KVM_REG_ARM_TIMER_CNT:
> >> + case KVM_REG_ARM_TIMER_CVAL:
> >> + return true;
> >> + }
> >> + return false;
> >> +}
> >> +
> >> int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >> {
> >> const struct coproc_reg *r;
> >> void __user *uaddr = (void __user *)(long)reg->addr;
> >> + u64 val;
> >>
> >> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
> >> return demux_c15_get(reg->id, uaddr);
> >> @@ -834,6 +850,11 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_VFP)
> >> return vfp_get_reg(vcpu, reg->id, uaddr);
> >>
> >> + if (is_timer_reg(reg->id)) {
> >> + val = kvm_arm_timer_get_reg(vcpu, reg);
> >> + return reg_to_user(uaddr, &val, reg->id);
> >> + }
> >> +
> >> r = index_to_coproc_reg(vcpu, reg->id);
> >> if (!r)
> >> return get_invariant_cp15(reg->id, uaddr);
> >> @@ -846,6 +867,8 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >> {
> >> const struct coproc_reg *r;
> >> void __user *uaddr = (void __user *)(long)reg->addr;
> >> + u64 val;
> >> + int ret;
> >>
> >> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
> >> return demux_c15_set(reg->id, uaddr);
> >> @@ -853,6 +876,13 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_VFP)
> >> return vfp_set_reg(vcpu, reg->id, uaddr);
> >>
> >> + if (is_timer_reg(reg->id)) {
> >> + ret = reg_from_user(&val, uaddr, reg->id);
> >> + if (ret == 0)
> >> + kvm_arm_timer_set_reg(vcpu, reg, val);
> >> + return ret;
> >> + }
> >> +
> >> r = index_to_coproc_reg(vcpu, reg->id);
> >> if (!r)
> >> return set_invariant_cp15(reg->id, uaddr);
> >> --
> >> 1.7.12.1
> >>
> >
> > Otherwise, this looks pretty good.
> >
> > -Christoffer
>
> Side comment: why isn't all this on the mailing list?
>
I was told by Andre that he had decided with you and Peter that you
would review this code initially outside of the list, which also
surprised me quite a bit, but I probably just misunderstood.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
