[PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values

[Reiji Watanabe]

Hi Oliver,

On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton <oupton at google.com> wrote:
>
> In some instances, a VMM may want to update the guest's counter-timer
> offset in a transparent manner, meaning that changes to the hardware
> value do not affect the synthetic register presented to the guest or the
> VMM through said guest's architectural state. Lay the groundwork to
> separate guest offset register writes from the hardware values utilized
> by KVM.
>
> Signed-off-by: Oliver Upton <oupton at google.com>
> Reviewed-by: Andrew Jones <drjones at redhat.com>
> ---
>  arch/arm64/kvm/arch_timer.c  | 42 +++++++++++++++++++++++++++---------
>  include/kvm/arm_arch_timer.h |  3 +++
>  2 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index c0101db75ad4..cf2f4a034dbe 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -84,11 +84,9 @@ u64 timer_get_cval(struct arch_timer_context *ctxt)
>
>  static u64 timer_get_offset(struct arch_timer_context *ctxt)
>  {
> -       struct kvm_vcpu *vcpu = ctxt->vcpu;
> -
>         switch(arch_timer_ctx_index(ctxt)) {
>         case TIMER_VTIMER:
> -               return __vcpu_sys_reg(vcpu, CNTVOFF_EL2);
> +               return ctxt->host_offset;
>         default:
>                 return 0;
>         }
> @@ -128,17 +126,33 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval)
>
>  static void timer_set_offset(struct arch_timer_context *ctxt, u64 offset)
>  {
> -       struct kvm_vcpu *vcpu = ctxt->vcpu;
> -
>         switch(arch_timer_ctx_index(ctxt)) {
>         case TIMER_VTIMER:
> -               __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset;
> +               ctxt->host_offset = offset;
>                 break;
>         default:
>                 WARN(offset, "timer %ld\n", arch_timer_ctx_index(ctxt));
>         }
>  }
>
> +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);
> +               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.


>  {
>         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)
> -               timer_set_offset(vcpu_get_timer(tmp, timer), offset);
> +               if (guest_visible)
> +                       timer_set_guest_offset(vcpu_get_timer(tmp, timer),
> +                                              offset);
> +               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.
>          */
> -       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);
>  }
>
>  static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
> @@ -772,7 +794,7 @@ static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
>         struct kvm *kvm = vcpu->kvm;
>
>         mutex_lock(&kvm->lock);
> -       update_timer_offset(vcpu, TIMER_VTIMER, cntvoff);
> +       update_timer_offset(vcpu, TIMER_VTIMER, cntvoff, true);
>         mutex_unlock(&kvm->lock);
>  }
>
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 51c19381108c..9d65d4a29f81 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -42,6 +42,9 @@ struct arch_timer_context {
>         /* Duplicated state from arch_timer.c for convenience */
>         u32                             host_timer_irq;
>         u32                             host_timer_irq_flags;
> +
> +       /* offset relative to the host's physical counter-timer */
> +       u64                             host_offset;
>  };

Just out of curiosity, have you considered having 'host_offset'
in one place (in arch_timer_cpu or somewhere ?) as physical offset
rather than having them separately for each arch_timer_context ?
I would think that could simplify the offset calculation code
and update_ptimer_cntpoff() doesn't need to call update_timer_offset()
for TIMER_VTIMER.  It would require extra memory accesses for
timer_get_offset(TIMER_VTIMER) though...

Otherwise,
Reviewed-by: Reiji Watanabe <reijiw at google.com>

Thanks,
Reiji