Re: [PATCH v6 19/21] KVM: arm64: Emulate physical counter offsetting on non-ECV systems

[Oliver Upton <oupton@google.com>]

Hi Drew,

On Wed, Aug 4, 2021 at 4:05 AM Andrew Jones <drjones@redhat.com> wrote:
> > +static bool ptimer_emulation_required(struct kvm_vcpu *vcpu)
> > +{
> > +     return timer_get_offset(vcpu_ptimer(vcpu)) &&
> > +                     !cpus_have_const_cap(ARM64_ECV);
>
> Whenever I see a static branch check and something else in the same
> condition, I always wonder if we could trim a few instructions for
> the static branch is false case by testing it first.

Good point, I'll reclaim those few cycles in the next spin ;-)

> > @@ -1539,11 +1551,8 @@ int kvm_arm_timer_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >       switch (attr->attr) {
> >       case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
> >       case KVM_ARM_VCPU_TIMER_IRQ_PTIMER:
> > -             return 0;
> >       case KVM_ARM_VCPU_TIMER_OFFSET:
> > -             if (cpus_have_const_cap(ARM64_ECV))
> > -                     return 0;
> > -             break;
> > +             return 0;
>
> So now, if userspace wants to know when they're using an emulated
> TIMER_OFFSET vs. ECV, then they'll need to check the HWCAP. I guess
> that's fair. We should update the selftest to report what it's testing
> when the HWCAP is available.
>

Hmm...

I hadn't yet wired up the ECV cpufeature bits to an ELF HWCAP, but
this point is a bit interesting. I can see the argument being made
that we shouldn't have two ELF HWCAP bits for ECV (depending on
partial or full ECV support). ECV=0x1 is most certainly of interest to
userspace, since self-synchronized views of the counter are then
available. However, ECV=0x2 is purely of interest to EL2.

What if we only had only one ELF HWCAP bit for ECV >= 0x1? We could
let userspace read ID_AA64MMFR0_EL1.ECV if it really needs to know
about ECV = 0x2.

> > +     if (vcpu_ptimer(vcpu)->host_offset && !cpus_have_const_cap(ARM64_ECV))
>
> Shouldn't we expose and reuse ptimer_emulation_required() here?
>

Agreed, makes it much cleaner.

> > +             val &= ~CNTHCTL_EL1PCTEN;
> > +     else
> > +             val |= CNTHCTL_EL1PCTEN;
> >       write_sysreg(val, cnthctl_el2);
> >  }
> > --
> > 2.32.0.605.g8dce9f2422-goog
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>

Thanks!

--
Oliver