On Tue, 10 Aug 2021 10:44:01 +0100,
Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> On Tue, Aug 10, 2021 at 2:35 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Wed, 04 Aug 2021 09:58:11 +0100,
> > Oliver Upton <oupton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
--
Without deviation from the norm, progress is not possible.
