Re: [PATCH] arm64: Restore trapless ptimer access

[Andrew Jones]

On Mon, Sep 04, 2023 at 10:18:05AM +0200, Claudio Fontana wrote:
> Hi,
> 
> I think this discussion from ~2015 could potentially be be historically 
> relevant for context,
> at the time we had the problem with CNTVOFF IIRC so KVM_REG_ARM_TIMER_CNT 
> being read and rewritten causing time warps in the guest:
> 
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/1435157697-28579-1-git-send-email-marc.zyngier@arm.com/
> 
> I could not remember or find if/where the problem was fixed in the end in 
> QEMU,

It's most likely commit 4b7a6bf402bd ("target-arm: kvm: Differentiate
registers based on write-back levels")

Thanks,
drew

> 
> Ciao,
> 
> Claudio
> 
> On 9/1/23 21:23, Colton Lewis wrote:
> > On Fri, Sep 01, 2023 at 09:35:47AM +0200, Andrew Jones wrote:
> >> On Thu, Aug 31, 2023 at 07:00:52PM +0000, Colton Lewis wrote:
> >>> Due to recent KVM changes, QEMU is setting a ptimer offset resulting
> >>> in unintended trap and emulate access and a consequent performance
> >>> hit. Filter out the PTIMER_CNT register to restore trapless ptimer
> >>> access.
> >>>
> >>> Quoting Andrew Jones:
> >>>
> >>> Simply reading the CNT register and writing back the same value is
> >>> enough to set an offset, since the timer will have certainly moved
> >>> past whatever value was read by the time it's written.  QEMU
> >>> frequently saves and restores all registers in the get-reg-list array,
> >>> unless they've been explicitly filtered out (with Linux commit
> >>> 680232a94c12, KVM_REG_ARM_PTIMER_CNT is now in the array). So, to
> >>> restore trapless ptimer accesses, we need a QEMU patch to filter out
> >>> the register.
> >>>
> >>> See
> >>> https://lore.kernel.org/kvmarm/gsntttsonus5.fsf@coltonlewis-kvm.c.googlers.com/T/#m0770023762a821db2a3f0dd0a7dc6aa54e0d0da9
> >>
> >> The link can be shorter with
> >>
> >> https://lore.kernel.org/all/20230823200408.1214332-1-coltonlewis@google.com/
> > 
> > I will keep that in mind next time.
> > 
> >>> for additional context.
> >>>
> >>> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> >>
> >> Thanks for the testing and posting, Colton. Please add your s-o-b and a
> >> Tested-by tag as well.
> > 
> > Assuming it is sufficient to add here instead of reposting the whole patch:
> > 
> > Signed-off-by: Colton Lewis <coltonlewis@google.com>
> > Tested-by: Colton Lewis <coltonlewis@google.com>
> > 
> >>> ---
> >>>  target/arm/kvm64.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> >>> index 4d904a1d11..2dd46e0a99 100644
> >>> --- a/target/arm/kvm64.c
> >>> +++ b/target/arm/kvm64.c
> >>> @@ -672,6 +672,7 @@ typedef struct CPRegStateLevel {
> >>>   */
> >>>  static const CPRegStateLevel non_runtime_cpregs[] = {
> >>>      { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE },
> >>> +    { KVM_REG_ARM_PTIMER_CNT, KVM_PUT_FULL_STATE },
> >>>  };
> >>>
> >>>  int kvm_arm_cpreg_level(uint64_t regidx)
> >>> --
> >>> 2.42.0.283.g2d96d420d3-goog
> >>>
> > 
>