On Tue, Sep 21, 2021, Reiji Watanabe wrote:
> Hi Oliver,
>
> On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton <oupton@xxxxxxxxxx> wrote:
> > +static void timer_set_guest_offset(struct arch_timer_context *ctxt, u64 offset)
> > +{
> > + struct kvm_vcpu *vcpu = ctxt->vcpu;
> > +
> > + switch (arch_timer_ctx_index(ctxt)) {
> > + case TIMER_VTIMER: {
> > + u64 host_offset = timer_get_offset(ctxt);
> > +
> > + host_offset += offset - __vcpu_sys_reg(vcpu, CNTVOFF_EL2);
> > + __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset;
> > + timer_set_offset(ctxt, host_offset);
Really getting into nitpicking territory, but it maybe name this
timer_set_virtual_offset() (assuming v=virtual and p=physical). Based on the
name, I expected this to set a variable literally named guest_offset, but it
reads and writes host_offset. Maintaining the virtual vs. physical terminology
all the way down avoids having direct host vs. guest naming conflicts, i.e.
virtual and host aren't generally though of as mutually exclusive.
> > + break;
> > + }
> > + default:
> > + WARN_ONCE(offset, "timer %ld\n", arch_timer_ctx_index(ctxt));
> > + }
> > +}
> > +
> > u64 kvm_phys_timer_read(void)
> > {
> > return timecounter->cc->read(timecounter->cc);
> > @@ -749,7 +763,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
> >
> > /* Make offset updates for all timer contexts atomic */
> > static void update_timer_offset(struct kvm_vcpu *vcpu,
> > - enum kvm_arch_timers timer, u64 offset)
> > + enum kvm_arch_timers timer, u64 offset,
> > + bool guest_visible)
>
> The name 'guest_visible' looks confusing to me because it also
> affects the type of the 'offset' that its caller needs to specify.
> (The 'offset' must be an offset from the guest's physical counter
> if 'guest_visible' == true, and an offset from the host's virtual
> counter otherwise.)
> Having said that, I don't have a good alternative name for it though...
> IMHO, 'is_host_offset' would be less confusing because it indicates
> what the caller needs to specify.
I'd say ditch the param altogether and just have two separate helpers. Even in
the final code, the callers all pass explicit 'true' or 'false', i.e. the callers
can just as easily call a different function.
Despite the near-identical code, smushing guest and host into the same function
doesn't actually save much, just the function prototype and the local variables.
That'd also avoid having to document/comment what 'true' and 'false' means at the
call sites.
> > {
> > int i;
> > struct kvm *kvm = vcpu->kvm;
> > @@ -758,13 +773,20 @@ static void update_timer_offset(struct kvm_vcpu *vcpu,
> > lockdep_assert_held(&kvm->lock);
> >
> > kvm_for_each_vcpu(i, tmp, kvm)
This needs braces if you keep it as is.
> > - timer_set_offset(vcpu_get_timer(tmp, timer), offset);
> > + if (guest_visible)
> > + timer_set_guest_offset(vcpu_get_timer(tmp, timer),
> > + offset);
Let this poke out, 84 chars isn't the end of the world.
> > + else
> > + timer_set_offset(vcpu_get_timer(tmp, timer), offset);
> >
> > /*
> > * When called from the vcpu create path, the CPU being created is not
> > * included in the loop above, so we just set it here as well.
> > */
Any reason this can't be called from kvm_arch_vcpu_postcreate()? That'd eliminate
the need for the extra handling. The vCPU is technically visible to userspace, but
userspace would have to very intentionally do the wrong thing to cause problems,
and I don't see any obviosu danger to the host.
> > - timer_set_offset(vcpu_get_timer(vcpu, timer), offset);
> > + if (guest_visible)
> > + timer_set_guest_offset(vcpu_get_timer(vcpu, timer), offset);
> > + else
> > + timer_set_offset(vcpu_get_timer(vcpu, timer), offset);
> > }
> >
