Patchwork [kvmarm:kvm-arm64/nv-wip-v5.0-rc1,4/75] arch/arm/kvm/../../../virt/kvm/arm/arch_timer.c:700:7: error: 'SYS_CNTP_TVAL_EL0' undeclared

login
register
mail settings
Submitter Christoffer Dall
Date Jan. 9, 2019, 10:48 a.m.
Message ID <20190109104857.GT10769@e113682-lin.lund.arm.com>
Download mbox | patch
Permalink /patch/695655/
State New
Headers show

Comments

Christoffer Dall - Jan. 9, 2019, 10:48 a.m.
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:




Thanks,

    Christoffer

Patch

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);