On 25/01/2019 12:33, Julien Thierry wrote:
> Hi,
>
> I'm wondering, could this patch be split in two? One for the
> introduction of kvm_arm_timer_read_sysreg() +
> kvm_arm_timer_write_sysreg() and the other for merging the handlers into
> a single function?
Maybe. I'm not sure it brings us much though.
>
>
>
> On 24/01/2019 14:00, Christoffer Dall wrote:
>> From: Andre Przywara <andre.przywara@xxxxxxx>
>>
>> At the moment we have separate system register emulation handlers for
>> each timer register. Actually they are quite similar, and we rely on
>> kvm_arm_timer_[gs]et_reg() for the actual emulation anyways, so let's
>> just merge all of those handlers into one function, which just marshalls
>> the arguments and then hands off to a set of common accessors.
>> This makes extending the emulation to include EL2 timers much easier.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> [Fixed 32-bit VM breakage and reduced to reworking existing code]
>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx>
>> [Fixed 32bit host, general cleanup]
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm/kvm/coproc.c | 23 +++---
>> arch/arm64/include/asm/sysreg.h | 4 +
>> arch/arm64/kvm/sys_regs.c | 80 +++++++++++---------
>> include/kvm/arm_arch_timer.h | 23 ++++++
>> virt/kvm/arm/arch_timer.c | 129 +++++++++++++++++++++++++++-----
>> 5 files changed, 196 insertions(+), 63 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 222c1635bc7a..51863364f8d1 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -293,15 +293,16 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
>> const struct coproc_params *p,
>> const struct coproc_reg *r)
>> {
>> - u64 now = kvm_phys_timer_read();
>> - u64 val;
>> + u32 val;
>>
>> if (p->is_write) {
>> val = *vcpu_reg(vcpu, p->Rt1);
>> - kvm_arm_timer_set_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL, val + now);
>> + kvm_arm_timer_write_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_TVAL, val);
>> } else {
>> - val = kvm_arm_timer_get_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL);
>> - *vcpu_reg(vcpu, p->Rt1) = val - now;
>> + val = kvm_arm_timer_read_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_TVAL);
>> + *vcpu_reg(vcpu, p->Rt1) = val;
>> }
>>
>> return true;
>> @@ -315,9 +316,11 @@ static bool access_cntp_ctl(struct kvm_vcpu *vcpu,
>>
>> if (p->is_write) {
>> val = *vcpu_reg(vcpu, p->Rt1);
>> - kvm_arm_timer_set_reg(vcpu, KVM_REG_ARM_PTIMER_CTL, val);
>> + kvm_arm_timer_write_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_CTL, val);
>> } else {
>> - val = kvm_arm_timer_get_reg(vcpu, KVM_REG_ARM_PTIMER_CTL);
>> + val = kvm_arm_timer_read_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_CTL);
>> *vcpu_reg(vcpu, p->Rt1) = val;
>> }
>>
>> @@ -333,9 +336,11 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
>> if (p->is_write) {
>> val = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>> val |= *vcpu_reg(vcpu, p->Rt1);
>> - kvm_arm_timer_set_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL, val);
>> + kvm_arm_timer_write_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_CVAL, val);
>> } else {
>> - val = kvm_arm_timer_get_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL);
>> + val = kvm_arm_timer_read_sysreg(vcpu,
>> + TIMER_PTIMER, TIMER_REG_CVAL);
>> *vcpu_reg(vcpu, p->Rt1) = val;
>> *vcpu_reg(vcpu, p->Rt2) = val >> 32;
>> }
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index 3e5650903d6d..6482e8bcf1b8 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -392,6 +392,10 @@
>> #define SYS_CNTP_CTL_EL0 sys_reg(3, 3, 14, 2, 1)
>> #define SYS_CNTP_CVAL_EL0 sys_reg(3, 3, 14, 2, 2)
>>
>> +#define SYS_AARCH32_CNTP_TVAL sys_reg(0, 0, 14, 2, 0)
>> +#define SYS_AARCH32_CNTP_CTL sys_reg(0, 0, 14, 2, 1)
>> +#define SYS_AARCH32_CNTP_CVAL sys_reg(0, 2, 0, 14, 0)
>> +
>> #define __PMEV_op2(n) ((n) & 0x7)
>> #define __CNTR_CRm(n) (0x8 | (((n) >> 3) & 0x3))
>> #define SYS_PMEVCNTRn_EL0(n) sys_reg(3, 3, 14, __CNTR_CRm(n), __PMEV_op2(n))
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 1a5bea4285e4..65ea63366c67 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -990,44 +990,51 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> { SYS_DESC(SYS_PMEVTYPERn_EL0(n)), \
>> access_pmu_evtyper, reset_unknown, (PMEVTYPER0_EL0 + n), }
>>
>> -static bool access_cntp_tval(struct kvm_vcpu *vcpu,
>> - struct sys_reg_params *p,
>> - const struct sys_reg_desc *r)
>> +static bool access_arch_timer(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> {
>> - u64 now = kvm_phys_timer_read();
>> - u64 cval;
>> + enum kvm_arch_timers tmr;
>> + enum kvm_arch_timer_regs treg;
>> + u64 reg = reg_to_encoding(r);
>>
>> - if (p->is_write) {
>> - kvm_arm_timer_set_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL,
>> - p->regval + now);
>> - } else {
>> - cval = kvm_arm_timer_get_reg(vcpu, KVM_REG_ARM_PTIMER_CVAL);
>> - p->regval = cval - now;
>> + switch (reg) {
>> + case SYS_CNTP_TVAL_EL0:
>> + case SYS_CNTP_CTL_EL0:
>> + case SYS_CNTP_CVAL_EL0:
>> + case SYS_AARCH32_CNTP_TVAL:
>> + case SYS_AARCH32_CNTP_CTL:
>> + case SYS_AARCH32_CNTP_CVAL:
>> + tmr = TIMER_PTIMER;
>> + break;
>> + default:
>> + BUG();
>> }
>>
>> - return true;
>> -}
>> + switch (reg) {
>
> I find having two consecutive switch on the same element a bit weird
> (and takes a lot of space).
>
> Either I'd merge the two since the valid cases are the same, or I'd put
> them in separate function "reg_get_timer(reg)",
> "reg_get_timer_reg(reg)". (Can probably fit those in
> include/kvm/arm_arch_timer.h)
Nah, I'll just merge them. I wrote it this way as it was just easier to
reason about one thing at a time, but in the end this is (as you
noticed) fairly pointless.
I guess I'll have more fun rebasing the NV stuff on top! ;-)
[...]
>> +static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>> + struct arch_timer_context *timer,
>> + enum kvm_arch_timer_regs treg)
>> +{
>> + u64 val;
>> +
>> + switch (treg) {
>> + case TIMER_REG_TVAL:
>> + val = kvm_phys_timer_read() - timer->cntvoff - timer->cnt_cval;
>> + break;
>> +
>> + case TIMER_REG_CTL:
>> + val = read_timer_ctl(timer);
>> + break;
>> +
>> + case TIMER_REG_CVAL:
>> + val = timer->cnt_cval;
>> + break;
>> +
>> + case TIMER_REG_CNT:
>> + val = kvm_phys_timer_read() - timer->cntvoff;
>
> Unless you really don't want people to read this register, you might
> want to add a "break;" here :) .
Timers are for wimps. Real hax0rz use a calibrated loop with interrupts
disabled! ;-)
Thanks for the heads up!
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
