On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > FEAT_ECV provides a guest physical counter-timer offset register
> > (CNTPOFF_EL2), but ECV-enabled hardware is nonexistent at the time of
> > writing so support for it was elided for the sake of the author :)
>
> You seem to have done most the work for that, and there are SW models
> out there that implement ECV. If anything, the ECV support should come
> first, and its emulation only as a poor man's workaround.
>
> It is also good to show silicon vendors that they suck at providing
> useful features, and pointing them to the code that would use these
> features *is* an incentive.
Lol, then so be it. I'll add ECV support to this series. What can I
test with, though? I don't recall QEMU supporting ECV last I checked..
> I really dislike the fallback to !vhe here. I'd rather you
> special-case the emulated ptimer in the VHE case:
>
> if (has_vhe()) {
> map->direct_vtimer = vcpu_vtimer(vcpu);
> if (!timer_get_offset(vcpu_ptimer(vcpu)))
> map->direct_ptimer = vcpu_ptimer(vcpu);
> map->emul_ptimer = NULL;
> } else {
> map->direct_ptimer = NULL;
> map->emul_ptimer = vcpu_ptimer(vcpu);
> }
> } else {
Ack.
> and you can drop the timer_emulation_required() helper which doesn't
> serve any purpose.
Especially if I add ECV to this series, I'd prefer to carry it than
open-code the check for CNTPOFF && !ECV in get_timer_map(). Do you
concur? I can tighten it to ptimer_emulation_required() and avoid
taking an arch_timer context if you prefer.
> > +static inline bool __hyp_handle_counter(struct kvm_vcpu *vcpu)
> > +{
> > + u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu));
> > + int rt = kvm_vcpu_sys_get_rt(vcpu);
> > + u64 rv;
> > +
> > + if (sysreg != SYS_CNTPCT_EL0)
>
> That's feels really optimistic. You don't check for the exception
> class, and pray that the bits will align? ;-)
Doh! Missed the EC check.
> Please don't add more small includes like this. It makes things hard
> to read and hides bugs
Ack.
> the counter read can (and will) be speculated,
> so you definitely need an ISB before the access. Please also look at
> __arch_counter_get_cntpct() and arch_counter_enforce_ordering().
Wouldn't it be up to the guest to decide if it wants the counter to be
speculated or not? I debated this a bit myself in the implementation,
as we would trap both ordered and un-ordered reads. Well, no way to
detect it :)
> >
> > -/*
> > - * Should only be called on non-VHE systems.
> > - * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe().
> > - */
>
> This comment still applies. this function *is* nVHE specific.
>
Ack.
>
> You now pay the price of reprogramming CNTHCTL_EL2 on every entry for
> VHE, and that's not right. On VHE, it should be enough to perform the
> programming on vcpu_load() only.
>
True. I'll rework the programming in the next series.
> Overall, this patch needs a bit of splitting up (userspace interface,
> HV rework), re-optimise VHE, and it *definitely* wants the ECV support
> for the physical timer.
>
Sure thing, thanks for the review Marc!
--
Best,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
