On Wed, Jan 09, 2019 at 10:09:51AM +0000, Marc Zyngier wrote: > On 09/01/2019 09:13, André Przywara wrote: > > On 09/01/2019 04:40, kbuild test robot wrote: > > > > Marc, Christoffer, > > > >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git kvm-arm64/nv-wip-v5.0-rc1 > >> head: 688c386ca096f2c1f2eee386697586c88df5d5bc > >> commit: 2b1265c58a873d917e99ac762e243c1274481dbf [4/75] KVM: arm/arm64: consolidate arch timer trap handlers > >> config: arm-axm55xx_defconfig (attached as .config) > >> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 > >> reproduce: > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> git checkout 2b1265c58a873d917e99ac762e243c1274481dbf > >> # save the attached .config to linux build tree > >> GCC_VERSION=7.2.0 make.cross ARCH=arm > >> > >> All errors (new ones prefixed by >>): > > > > I was looking at this yesterday: It's a bit nasty, don't know a good > > solution beside bringing back this part of my original timer rework series. > > The problem is that those symbols contains the Aarch64 specific > > (instruction) encoding of the timer registers, plus we need the AArch32 > > encodings for 32-on-64 guests. > > Why? There is exactly one timer that needs trapping for AArch32 (the EL1 > physical timer). All we need is: > > - the SYS_AARCH32_CNTP_* encodings on 32bit > - some CPP magic to prevent the compilation from breaking > > > > > That's why I used the generic UAPI encoding for the registers, because > > we only need *some* identification for them, it doesn't need to be > > something defined by the architecture. > > I disagree. By doing so, you're conflating userspace access and > trapping, which has proved to be a bad idea in the past. For example, > you'd end-up having both CVAL and TVAL in UAPI, which is not something > I'm keen to have. On the other hand, the trapping function do need to be > able to handle these. > I think it probably makes sense to have the sysreg encoding stuff in the arch-specific files, and have an indirection in sys_regs.c and coproc.c which 'translates' from a system register to some generic arch timer define identifying a 'timer'. I think the major breakage in the previous design was to use the same *functions* for uaccess and for sysregs traps, but I don't think it was necessarily a problem to use the same definitions to identify a timer. That of couse leaves the problem of how to identify a register within a timer. Could we do something as braindead as: diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h index ce441dda412c..1d7bd452e5fd 100644 --- a/include/kvm/arm_arch_timer.h +++ b/include/kvm/arm_arch_timer.h @@ -32,6 +32,13 @@ enum kvm_arch_timers { NR_KVM_TIMERS }; +enum arch_timer_reg { + ARCH_TIMER_REG_CTL, + ARCH_TIMER_REG_CVAL, + ARCH_TIMER_REG_TVAL, + ARCH_TIMER_REG_CNT +} + struct arch_timer_context { struct kvm_vcpu *vcpu; @@ -86,8 +93,13 @@ bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu); void kvm_timer_update_run(struct kvm_vcpu *vcpu); void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu); -u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid); -int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value); +u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, + enum kvm_arch_timers timer, + enum arch_timer_reg reg); +int kvm_arm_timer_set_reg(struct kvm_vcpu *, + enum kvm_arch_timers timer, + enum arch_timer_reg reg, + u64 value); u64 kvm_arm_timer_read_sysreg(struct kvm_vcpu *vcpu, u32 sr); void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu, u32 sr, u64 val); Thanks, Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
