Hi Drew,
On Wed, Aug 4, 2021 at 4:05 AM Andrew Jones <drjones@xxxxxxxxxx> 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@xxxxxxxxxx>
>
Thanks!
--
Oliver
