[PATCH 08/16] KVM: arm64: timers: Allow userspace to set the counter offsets

[Colton Lewis]

Marc Zyngier <maz at kernel.org> 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