Marc Zyngier <maz@xxxxxxxxxx> writes:
Once this new API is used, there is no going back, and the counters cannot be written to to set the offsets implicitly (the writes are instead ignored).
Why do this? I can't see a reason for disabling the other API the first time this one is used.
In keeping with the architecture, the offsets are expressed as a delta that is substracted from the physical counter value.
^ nit: subtracted
+/* + * Counter/Timer offset structure. Describe the virtual/physical offsets. + * To be used with KVM_ARM_SET_CNT_OFFSETS. + */ +struct kvm_arm_counter_offsets { + __u64 virtual_offset; + __u64 physical_offset; + +#define KVM_COUNTER_SET_VOFFSET_FLAG (1UL << 0) +#define KVM_COUNTER_SET_POFFSET_FLAG (1UL << 1) + + __u64 flags; + __u64 reserved; +}; +
It looks weird to have the #defines in the middle of the struct like that. I think it would be easier to read with the #defines before the struct.
@@ -852,9 +852,11 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) ptimer->vcpu = vcpu; ptimer->offset.vm_offset = &vcpu->kvm->arch.offsets.poffset;
- /* Synchronize cntvoff across all vtimers of a VM. */ - timer_set_offset(vtimer, kvm_phys_timer_read()); - timer_set_offset(ptimer, 0); + /* Synchronize offsets across timers of a VM if not already provided */ + if (!test_bit(KVM_ARCH_FLAG_COUNTER_OFFSETS, &vcpu->kvm->arch.flags)) { + timer_set_offset(vtimer, kvm_phys_timer_read()); + timer_set_offset(ptimer, 0); + }
hrtimer_init(&timer->bg_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD); timer->bg_timer.function = kvm_bg_timer_expire;
The code says "assign the offsets if the KVM_ARCH_FLAG_COUNTER_OFFSETS flag is not on". The flag name is confusing and made it hard for me to understand the intent. I think the intent is to only assign the offsets if the user has not called the API to provide some offsets (that would have been assigned in the API call along with flipping the flag on). With that in mind, I would prefer the flag name reference the user. KVM_ARCH_FLAG_USER_OFFSETS