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

[Marc Zyngier <maz@kernel.org>]

On Thu, 23 Feb 2023 22:41:13 +0000,
Colton Lewis <coltonlewis@google.com> wrote:
> 
> Marc Zyngier <maz@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.

I can't see a reason not to. The new API is VM-wide. The old one
operates on a per-vcpu basis. What sense does it make to accept
something that directly conflicts with the previous actions from
userspace?

Once userspace has bought into the new API, it should use it
consistently.  The only reason we don't reject the write with an error
is to allow userspace to keep using the vcpu register dump as an
opaque object that it doesn't have to scan and amend.

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

I do like it, as it perfectly shows in which context these #defines
are valid. This is also a common idiom used all over the existing KVM
code (just take a look at kvm_run for the canonical example).

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

All offsets are provided by the user, no matter what API they used, so
I don't think this adds much clarity. The real distinction is between
the offsets being set by writing a vcpu attribute or a VM attribute.

By this token, I'd suggest KVM_ARM_FLAG_VM_COUNTER_OFFSETS.

Thoughts?

	M.

-- 
Without deviation from the norm, progress is not possible.

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