| Message ID | 20210916181510.963449-3-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add idempotent controls to migrate guest counter | expand |
Hi Oliver, On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton <oupton@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@google.com> > Reviewed-by: Andrew Jones <drjones@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@google.com> Thanks, Reiji
On Tue, Sep 21, 2021, Reiji Watanabe wrote: > Hi Oliver, > > On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton <oupton@google.com> 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); > > } > >
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@google.com> > Reviewed-by: Andrew Jones <drjones@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) > { > 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 {
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; }; struct timer_map {