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

Hi Drew,

On Wed, Aug 4, 2021 at 4:05 AM Andrew Jones [off-list ref] wrote:

quoted

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

quoted

@@ -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.

quoted

+     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.

quoted

+             val &= ~CNTHCTL_EL1PCTEN;
+     else
+             val |= CNTHCTL_EL1PCTEN;
      write_sysreg(val, cnthctl_el2);
 }
--
2.32.0.605.g8dce9f2422-goog

Otherwise,

Reviewed-by: Andrew Jones <redacted>

Thanks!

--
Oliver

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