Re: [PATCH v4 05/20] KVM: arm64: timers: Allow physical offset without CNTPOFF_EL2

[Reiji Watanabe <reijiw@google.com>]

> > > +	assign_clear_set_bit(tpt, CNTHCTL_EL1PCEN << 10, set, clr);
> > > +	assign_clear_set_bit(tpc, CNTHCTL_EL1PCTEN << 10, set, clr);
> > 
> > Nit: IMHO the way the code specifies the 'set' and 'clr' arguments for
> > the macro might be a bit confusing ('set' is for '_clr', and 'clr' is
> > for '_set')?
> 
> I don't disagree, but we end-up with bits of different polarity once
> NV is fully in, see:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/arch/arm64/kvm/arch_timer.c?h=kvm-arm64/nv-6.4-WIP#n861
> 
> > Perhaps changing the parameter names of assign_clear_set_bit() like
> > below or flipping the condition (i.e. Specify !tpt or no_tpt instead
> > of tpt) might be less confusing?
> > 
> > #define assign_clear_set_bit(_pred, _bit, _t_val, _f_val)	\
> > do {								\
> > 	if (_pred)						\
> > 		(_t_val) |= (_bit);				\
> > 	else							\
> > 		(_f_val) |= (_bit);				\
> > } while (0)
> 
> See the pointer above. We need a good way to specify bits that have
> one polarity or another, and compute the result given the high-level
> constraints that are provided by the emulation code.
> 
> So far, I haven't been able to work out something "nice".

Thank you for the pointer.  Understood.
I don't have a batter idea though :)

> 
> [...]
> 
> > > +	{ SYS_DESC(SYS_CNTPCT_EL0), access_arch_timer },
> > > +	{ SYS_DESC(SYS_CNTPCTSS_EL0), access_arch_timer },
> > >  	{ SYS_DESC(SYS_CNTP_TVAL_EL0), access_arch_timer },
> > >  	{ SYS_DESC(SYS_CNTP_CTL_EL0), access_arch_timer },
> > >  	{ SYS_DESC(SYS_CNTP_CVAL_EL0), access_arch_timer },
> > > @@ -2525,6 +2533,7 @@ static const struct sys_reg_desc cp15_64_regs[] = {
> > >  	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR0_EL1 },
> > >  	{ CP15_PMU_SYS_REG(DIRECT, 0, 0, 9, 0), .access = access_pmu_evcntr },
> > >  	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
> > > +	{ SYS_DESC(SYS_AARCH32_CNTPCT),	      access_arch_timer },
> > 
> > Shouldn't KVM also emulate CNTPCTSS (Aarch32) when its trapping is
> > enabled on the host with ECV_CNTPOFF ?
> 
> Oh, well spotted. I'll queue something on top of the series to that
> effect (I'd rather not respin it as it has been in -next for some
> time, and the merge window is approaching).

Understood.


> 
> > 
> > 
> > >  	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR1_EL1 },
> > >  	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
> > >  	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > 
> > Nit: In emulating reading physical counter/timer for direct_ptimer
> > (poffset != 0 on VHE without ECV_CNTPOFF), it appears that
> > kvm_arm_timer_read_sysreg() unnecessarily calls
> > timer_{save,restore}_state(), and kvm_arm_timer_write_sysreg()
> > unnecessarily calls timer_save_state().  Couldn't we skip those
> > unnecessary calls ? (I didn't check all the following patches, but
> > the current code would be more convenient in the following patches ?)
> 
> Well, it depends how you look at it. We still perform "some" level of
> emulation (such as offsetting CVAL), and it allows us to share some
> code with the full emulation.

Even if we skip calling timer_save_state() and/or timer_restore_state(),
I would think we could still share some code with the full emulation.
What I meant was something like below (a diff from the patch-5).
Or Am I misunderstanding something?


--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1003,7 +1003,7 @@ u64 kvm_arm_timer_read_sysreg(struct kvm_vcpu *vcpu,
        get_timer_map(vcpu, &map);
        timer = vcpu_get_timer(vcpu, tmr);
 
-       if (timer == map.emul_ptimer)
+       if (timer == map.emul_ptimer || timer == map.direct_ptimer)
                return kvm_arm_timer_read(vcpu, timer, treg);
 
        preempt_disable();
@@ -1056,7 +1056,9 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
                timer_emulate(timer);
        } else {
                preempt_disable();
-               timer_save_state(timer);
+               if (timer != map.direct_ptimer)
+                       timer_save_state(timer);
+
                kvm_arm_timer_write(vcpu, timer, treg, val);
                timer_restore_state(timer);
                preempt_enable();


> On top of that, we already fast-track CNTPCT_EL0, which is the main
> user, and has a visible benefit with NV. If anything, I'd rather add a
> similar fast-tracking for the read side of CNTP_CVAL_EL0 and
> CNTP_CTL_EL0. We could then leave that code for 32bit only, which
> nobody gives a toss about.
> 
> What do you think?

Ah, I didn't check that fast-track CNTPCT_EL0.
Yes, that would be nicer.

Thank you!
Reiji