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

[Marc Zyngier <maz@kernel.org>]

On Mon, 10 Apr 2023 16:34:41 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Thu, Mar 30, 2023 at 06:47:45PM +0100, Marc Zyngier wrote:

[...]

> > +	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".

[...]

> > +	{ 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).

> 
> 
> >  	{ 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.

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?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.