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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
