Hi Oliver,
I don't understand what this patch is trying to achieve, so I'm just going to ask
some high level questions before I go through the code.
On 9/16/21 19:15, Oliver Upton 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.
I find this description very hard to parse. What do you mean by the "register
presented to the guest or the VMM through said guest's architectural state"?
If I understand the code correctly, what the patch does is to create another copy
of __vcpu_sys_reg(CNTVOFF_EL2) in vcpu_vtimer(vcpu)->host_offset in a very
roundabout manner, in the function timer_set_guest_offset() (please correct me if
I'm wrong). The commit doesn't explain why that is done at all, except for this
part: "In some instances, a VMM may want to update the guest's counter-timer
offset in a transparent manner", which looks very cryptic, at least to me.
In the cover letter, you mention adding support for a physical timer offset. I
think it would make the commits clearer to follow if there was a better
distinction between changes to the virtual timer offset and physical timer offsets.
>
> Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
> Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>
> ---
> 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)
> {
> 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;
I find the name and the comment very confusing. The name makes me think it
represents the host's virtual timer offset, but that is always 0. Judging from the
code, host_offset refers to the guest's virtual timer offset. The comment refers
to the host's physical counter-timer, which makes me believe the opposite, that
it's the offset from the physical timer.
Thanks,
Alex
> };
>
> struct timer_map {
