| Message ID | 20210804085819.846610-14-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add idempotent controls for migrating system counter state | expand |
On Wed, Aug 04, 2021 at 08:58:11AM +0000, Oliver Upton wrote: > Allow userspace to access the guest's virtual counter-timer offset > through the ONE_REG interface. The value read or written is defined to > be an offset from the guest's physical counter-timer. Add some > documentation to clarify how a VMM should use this and the existing > CNTVCT_EL0. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > Documentation/virt/kvm/api.rst | 10 ++++++++++ > arch/arm64/include/uapi/asm/kvm.h | 1 + > arch/arm64/kvm/arch_timer.c | 11 +++++++++++ > arch/arm64/kvm/guest.c | 6 +++++- > include/kvm/arm_arch_timer.h | 1 + > 5 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 8d4a3471ad9e..28a65dc89985 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -2487,6 +2487,16 @@ arm64 system registers have the following id bit patterns:: > derived from the register encoding for CNTV_CVAL_EL0. As this is > API, it must remain this way. > > +.. warning:: > + > + The value of KVM_REG_ARM_TIMER_OFFSET is defined as an offset from > + the guest's view of the physical counter-timer. > + > + Userspace should use either KVM_REG_ARM_TIMER_OFFSET or > + KVM_REG_ARM_TIMER_CVAL to pause and resume a guest's virtual > + counter-timer. Mixed use of these registers could result in an > + unpredictable guest counter value. > + > arm64 firmware pseudo-registers have the following bit pattern:: > > 0x6030 0000 0014 <regno:16> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index b3edde68bc3e..949a31bc10f0 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -255,6 +255,7 @@ struct kvm_arm_copy_mte_tags { > #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) > #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) > #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) > +#define KVM_REG_ARM_TIMER_OFFSET ARM64_SYS_REG(3, 4, 14, 0, 3) > > /* KVM-as-firmware specific pseudo-registers */ > #define KVM_REG_ARM_FW (0x0014 << KVM_REG_ARM_COPROC_SHIFT) > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index 4c2b763a8849..a8815b09da3e 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -868,6 +868,10 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value) > timer = vcpu_vtimer(vcpu); > kvm_arm_timer_write(vcpu, timer, TIMER_REG_CVAL, value); > break; > + case KVM_REG_ARM_TIMER_OFFSET: > + timer = vcpu_vtimer(vcpu); > + update_vtimer_cntvoff(vcpu, value); > + break; > case KVM_REG_ARM_PTIMER_CTL: > timer = vcpu_ptimer(vcpu); > kvm_arm_timer_write(vcpu, timer, TIMER_REG_CTL, value); > @@ -912,6 +916,9 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid) > case KVM_REG_ARM_TIMER_CVAL: > return kvm_arm_timer_read(vcpu, > vcpu_vtimer(vcpu), TIMER_REG_CVAL); > + case KVM_REG_ARM_TIMER_OFFSET: > + return kvm_arm_timer_read(vcpu, > + vcpu_vtimer(vcpu), TIMER_REG_OFFSET); > case KVM_REG_ARM_PTIMER_CTL: > return kvm_arm_timer_read(vcpu, > vcpu_ptimer(vcpu), TIMER_REG_CTL); > @@ -949,6 +956,10 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu, > val = kvm_phys_timer_read() - timer_get_offset(timer); > break; > > + case TIMER_REG_OFFSET: > + val = timer_get_offset(timer); > + break; > + > default: > BUG(); > } > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 1dfb83578277..17fc06e2b422 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -591,7 +591,7 @@ static unsigned long num_core_regs(const struct kvm_vcpu *vcpu) > * ARM64 versions of the TIMER registers, always available on arm64 > */ > > -#define NUM_TIMER_REGS 3 > +#define NUM_TIMER_REGS 4 > > static bool is_timer_reg(u64 index) > { > @@ -599,6 +599,7 @@ static bool is_timer_reg(u64 index) > case KVM_REG_ARM_TIMER_CTL: > case KVM_REG_ARM_TIMER_CNT: > case KVM_REG_ARM_TIMER_CVAL: > + case KVM_REG_ARM_TIMER_OFFSET: > return true; > } > return false; > @@ -614,6 +615,9 @@ static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > uindices++; > if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices)) > return -EFAULT; > + uindices++; > + if (put_user(KVM_REG_ARM_TIMER_OFFSET, uindices)) > + return -EFAULT; > > return 0; > } > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index 9d65d4a29f81..615f9314f6a5 100644 > --- a/include/kvm/arm_arch_timer.h > +++ b/include/kvm/arm_arch_timer.h > @@ -21,6 +21,7 @@ enum kvm_arch_timer_regs { > TIMER_REG_CVAL, > TIMER_REG_TVAL, > TIMER_REG_CTL, > + TIMER_REG_OFFSET, > }; > > struct arch_timer_context { > -- > 2.32.0.605.g8dce9f2422-goog > Reviewed-by: Andrew Jones <drjones@redhat.com>
On Wed, 04 Aug 2021 09:58:11 +0100, Oliver Upton <oupton@google.com> wrote: > > Allow userspace to access the guest's virtual counter-timer offset > through the ONE_REG interface. The value read or written is defined to > be an offset from the guest's physical counter-timer. Add some > documentation to clarify how a VMM should use this and the existing > CNTVCT_EL0. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > Documentation/virt/kvm/api.rst | 10 ++++++++++ > arch/arm64/include/uapi/asm/kvm.h | 1 + > arch/arm64/kvm/arch_timer.c | 11 +++++++++++ > arch/arm64/kvm/guest.c | 6 +++++- > include/kvm/arm_arch_timer.h | 1 + > 5 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 8d4a3471ad9e..28a65dc89985 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -2487,6 +2487,16 @@ arm64 system registers have the following id bit patterns:: > derived from the register encoding for CNTV_CVAL_EL0. As this is > API, it must remain this way. > > +.. warning:: > + > + The value of KVM_REG_ARM_TIMER_OFFSET is defined as an offset from > + the guest's view of the physical counter-timer. > + > + Userspace should use either KVM_REG_ARM_TIMER_OFFSET or > + KVM_REG_ARM_TIMER_CVAL to pause and resume a guest's virtual You probably mean KVM_REG_ARM_TIMER_CNT here, despite the broken encoding. > + counter-timer. Mixed use of these registers could result in an > + unpredictable guest counter value. > + > arm64 firmware pseudo-registers have the following bit pattern:: > > 0x6030 0000 0014 <regno:16> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index b3edde68bc3e..949a31bc10f0 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -255,6 +255,7 @@ struct kvm_arm_copy_mte_tags { > #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) > #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) > #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) > +#define KVM_REG_ARM_TIMER_OFFSET ARM64_SYS_REG(3, 4, 14, 0, 3) I don't think we can use the encoding for CNTPOFF_EL2 here, as it will eventually clash with a NV guest using the same feature for its own purpose. We don't want this offset to overlap with any of the existing features. I actually liked your previous proposal of controlling the physical offset via a device property, as it clearly indicated that you were dealing with non-architectural state. Thanks, M.
On Tue, Aug 10, 2021 at 2:35 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 04 Aug 2021 09:58:11 +0100, > Oliver Upton <oupton@google.com> wrote: > > > > Allow userspace to access the guest's virtual counter-timer offset > > through the ONE_REG interface. The value read or written is defined to > > be an offset from the guest's physical counter-timer. Add some > > documentation to clarify how a VMM should use this and the existing > > CNTVCT_EL0. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > Documentation/virt/kvm/api.rst | 10 ++++++++++ > > arch/arm64/include/uapi/asm/kvm.h | 1 + > > arch/arm64/kvm/arch_timer.c | 11 +++++++++++ > > arch/arm64/kvm/guest.c | 6 +++++- > > include/kvm/arm_arch_timer.h | 1 + > > 5 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > > index 8d4a3471ad9e..28a65dc89985 100644 > > --- a/Documentation/virt/kvm/api.rst > > +++ b/Documentation/virt/kvm/api.rst > > @@ -2487,6 +2487,16 @@ arm64 system registers have the following id bit patterns:: > > derived from the register encoding for CNTV_CVAL_EL0. As this is > > API, it must remain this way. > > > > +.. warning:: > > + > > + The value of KVM_REG_ARM_TIMER_OFFSET is defined as an offset from > > + the guest's view of the physical counter-timer. > > + > > + Userspace should use either KVM_REG_ARM_TIMER_OFFSET or > > + KVM_REG_ARM_TIMER_CVAL to pause and resume a guest's virtual > > You probably mean KVM_REG_ARM_TIMER_CNT here, despite the broken > encoding. Indeed I do! > > > + counter-timer. Mixed use of these registers could result in an > > + unpredictable guest counter value. > > + > > arm64 firmware pseudo-registers have the following bit pattern:: > > > > 0x6030 0000 0014 <regno:16> > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > > index b3edde68bc3e..949a31bc10f0 100644 > > --- a/arch/arm64/include/uapi/asm/kvm.h > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > @@ -255,6 +255,7 @@ struct kvm_arm_copy_mte_tags { > > #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) > > #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) > > #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) > > +#define KVM_REG_ARM_TIMER_OFFSET ARM64_SYS_REG(3, 4, 14, 0, 3) > > I don't think we can use the encoding for CNTPOFF_EL2 here, as it will > eventually clash with a NV guest using the same feature for its own > purpose. We don't want this offset to overlap with any of the existing > features. > > I actually liked your previous proposal of controlling the physical > offset via a device property, as it clearly indicated that you were > dealing with non-architectural state. That's actually exactly what I did here :) That said, the macro name is horribly obfuscated from CNTVOFF_EL2. I did this for the sake of symmetry with other virtual counter-timer registers above, though this may warrant special casing given the fact that we have a similarly named device attribute to handle the physical offset. -- Thanks, Oliver
On Tue, 10 Aug 2021 10:44:01 +0100, Oliver Upton <oupton@google.com> wrote: > > On Tue, Aug 10, 2021 at 2:35 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 04 Aug 2021 09:58:11 +0100, > > Oliver Upton <oupton@google.com> wrote: > > > > > > Allow userspace to access the guest's virtual counter-timer offset > > > through the ONE_REG interface. The value read or written is defined to > > > be an offset from the guest's physical counter-timer. Add some > > > documentation to clarify how a VMM should use this and the existing > > > CNTVCT_EL0. > > > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > > --- > > > Documentation/virt/kvm/api.rst | 10 ++++++++++ > > > arch/arm64/include/uapi/asm/kvm.h | 1 + > > > arch/arm64/kvm/arch_timer.c | 11 +++++++++++ > > > arch/arm64/kvm/guest.c | 6 +++++- > > > include/kvm/arm_arch_timer.h | 1 + > > > 5 files changed, 28 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > > > index 8d4a3471ad9e..28a65dc89985 100644 > > > --- a/Documentation/virt/kvm/api.rst > > > +++ b/Documentation/virt/kvm/api.rst > > > @@ -2487,6 +2487,16 @@ arm64 system registers have the following id bit patterns:: > > > derived from the register encoding for CNTV_CVAL_EL0. As this is > > > API, it must remain this way. > > > > > > +.. warning:: > > > + > > > + The value of KVM_REG_ARM_TIMER_OFFSET is defined as an offset from > > > + the guest's view of the physical counter-timer. > > > + > > > + Userspace should use either KVM_REG_ARM_TIMER_OFFSET or > > > + KVM_REG_ARM_TIMER_CVAL to pause and resume a guest's virtual > > > > You probably mean KVM_REG_ARM_TIMER_CNT here, despite the broken > > encoding. > > Indeed I do! > > > > > > + counter-timer. Mixed use of these registers could result in an > > > + unpredictable guest counter value. > > > + > > > arm64 firmware pseudo-registers have the following bit pattern:: > > > > > > 0x6030 0000 0014 <regno:16> > > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > > > index b3edde68bc3e..949a31bc10f0 100644 > > > --- a/arch/arm64/include/uapi/asm/kvm.h > > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > > @@ -255,6 +255,7 @@ struct kvm_arm_copy_mte_tags { > > > #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) > > > #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) > > > #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) > > > +#define KVM_REG_ARM_TIMER_OFFSET ARM64_SYS_REG(3, 4, 14, 0, 3) > > > > I don't think we can use the encoding for CNTPOFF_EL2 here, as it will > > eventually clash with a NV guest using the same feature for its own > > purpose. We don't want this offset to overlap with any of the existing > > features. > > > > I actually liked your previous proposal of controlling the physical > > offset via a device property, as it clearly indicated that you were > > dealing with non-architectural state. > > That's actually exactly what I did here :) That said, the macro name > is horribly obfuscated from CNTVOFF_EL2. I did this for the sake of > symmetry with other virtual counter-timer registers above, though this > may warrant special casing given the fact that we have a similarly > named device attribute to handle the physical offset. Gah, you are of course right. Ignore my rambling. The name is fine (or at least in keeping with existing quality level of the making). For the physical offset, something along the lines of KVM_ARM_VCPU_TIMER_PHYS_OFFSET is probably right (but feel free to be creative, I'm terrible at this stuff [1]). Thanks, M. [1] https://twitter.com/codinghorror/status/506010907021828096?lang=en
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 8d4a3471ad9e..28a65dc89985 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -2487,6 +2487,16 @@ arm64 system registers have the following id bit patterns:: derived from the register encoding for CNTV_CVAL_EL0. As this is API, it must remain this way. +.. warning:: + + The value of KVM_REG_ARM_TIMER_OFFSET is defined as an offset from + the guest's view of the physical counter-timer. + + Userspace should use either KVM_REG_ARM_TIMER_OFFSET or + KVM_REG_ARM_TIMER_CVAL to pause and resume a guest's virtual + counter-timer. Mixed use of these registers could result in an + unpredictable guest counter value. + arm64 firmware pseudo-registers have the following bit pattern:: 0x6030 0000 0014 <regno:16> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index b3edde68bc3e..949a31bc10f0 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -255,6 +255,7 @@ struct kvm_arm_copy_mte_tags { #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) #define KVM_REG_ARM_TIMER_CVAL ARM64_SYS_REG(3, 3, 14, 0, 2) #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) +#define KVM_REG_ARM_TIMER_OFFSET ARM64_SYS_REG(3, 4, 14, 0, 3) /* KVM-as-firmware specific pseudo-registers */ #define KVM_REG_ARM_FW (0x0014 << KVM_REG_ARM_COPROC_SHIFT) diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c index 4c2b763a8849..a8815b09da3e 100644 --- a/arch/arm64/kvm/arch_timer.c +++ b/arch/arm64/kvm/arch_timer.c @@ -868,6 +868,10 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value) timer = vcpu_vtimer(vcpu); kvm_arm_timer_write(vcpu, timer, TIMER_REG_CVAL, value); break; + case KVM_REG_ARM_TIMER_OFFSET: + timer = vcpu_vtimer(vcpu); + update_vtimer_cntvoff(vcpu, value); + break; case KVM_REG_ARM_PTIMER_CTL: timer = vcpu_ptimer(vcpu); kvm_arm_timer_write(vcpu, timer, TIMER_REG_CTL, value); @@ -912,6 +916,9 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid) case KVM_REG_ARM_TIMER_CVAL: return kvm_arm_timer_read(vcpu, vcpu_vtimer(vcpu), TIMER_REG_CVAL); + case KVM_REG_ARM_TIMER_OFFSET: + return kvm_arm_timer_read(vcpu, + vcpu_vtimer(vcpu), TIMER_REG_OFFSET); case KVM_REG_ARM_PTIMER_CTL: return kvm_arm_timer_read(vcpu, vcpu_ptimer(vcpu), TIMER_REG_CTL); @@ -949,6 +956,10 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu, val = kvm_phys_timer_read() - timer_get_offset(timer); break; + case TIMER_REG_OFFSET: + val = timer_get_offset(timer); + break; + default: BUG(); } diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index 1dfb83578277..17fc06e2b422 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -591,7 +591,7 @@ static unsigned long num_core_regs(const struct kvm_vcpu *vcpu) * ARM64 versions of the TIMER registers, always available on arm64 */ -#define NUM_TIMER_REGS 3 +#define NUM_TIMER_REGS 4 static bool is_timer_reg(u64 index) { @@ -599,6 +599,7 @@ static bool is_timer_reg(u64 index) case KVM_REG_ARM_TIMER_CTL: case KVM_REG_ARM_TIMER_CNT: case KVM_REG_ARM_TIMER_CVAL: + case KVM_REG_ARM_TIMER_OFFSET: return true; } return false; @@ -614,6 +615,9 @@ static int copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) uindices++; if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices)) return -EFAULT; + uindices++; + if (put_user(KVM_REG_ARM_TIMER_OFFSET, uindices)) + return -EFAULT; return 0; } diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h index 9d65d4a29f81..615f9314f6a5 100644 --- a/include/kvm/arm_arch_timer.h +++ b/include/kvm/arm_arch_timer.h @@ -21,6 +21,7 @@ enum kvm_arch_timer_regs { TIMER_REG_CVAL, TIMER_REG_TVAL, TIMER_REG_CTL, + TIMER_REG_OFFSET, }; struct arch_timer_context {
Allow userspace to access the guest's virtual counter-timer offset through the ONE_REG interface. The value read or written is defined to be an offset from the guest's physical counter-timer. Add some documentation to clarify how a VMM should use this and the existing CNTVCT_EL0. Signed-off-by: Oliver Upton <oupton@google.com> --- Documentation/virt/kvm/api.rst | 10 ++++++++++ arch/arm64/include/uapi/asm/kvm.h | 1 + arch/arm64/kvm/arch_timer.c | 11 +++++++++++ arch/arm64/kvm/guest.c | 6 +++++- include/kvm/arm_arch_timer.h | 1 + 5 files changed, 28 insertions(+), 1 deletion(-)