On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier [off-list ref] wrote:

quoted

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.

quoted

+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 :)

quoted

-/*
- * 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel