Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset

From: Oliver Upton <oupton@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Peter Shier <pshier@google.com>,
	Jim Mattson <jmattson@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <Alexandru.Elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Andrew Jones <drjones@redhat.com>
Subject: Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
Date: Fri, 30 Jul 2021 08:22:01 -0700	[thread overview]
Message-ID: <CAOQ_QsgCrEWQqakicR3Peu_c8oCMeq8Cok+CK8vJVURUwAdG0A@mail.gmail.com> (raw)
In-Reply-To: <875yws2h5w.wl-maz@kernel.org>

On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier <maz@kernel.org> 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