Re: [RFC PATCH 2/2] KVM: arm64: export cntvoff in debugfs

[Marc Zyngier]

On Fri, 19 Nov 2021 10:21:18 +0000,
Nicolas Saenz Julienne <nsaenzju@xxxxxxxxxx> wrote:
> 
> While using cntvct as the raw clock for tracing, it's possible to
> synchronize host/guest traces just by knowing the virtual offset applied
> to the guest's virtual counter.
> 
> This is also the case on x86 when TSC is available. The offset is
> exposed in debugfs as 'tsc-offset' on a per vcpu basis. So let's
> implement the same for arm64.

How does this work with NV, where the guest hypervisor is in control
of the virtual offset? How does userspace knows which vcpu to pick so
that it gets the right offset?

I also wonder why we need this when userspace already has direct
access to that information without any extra kernel support (read the
CNTVCT view of the vcpu using the ONEREG API, subtract it from the
host view of the counter, job done).

> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@xxxxxxxxxx>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/Makefile           |  2 +-
>  arch/arm64/kvm/arch_timer.c       |  2 +-
>  arch/arm64/kvm/debugfs.c          | 25 +++++++++++++++++++++++++
>  include/kvm/arm_arch_timer.h      |  3 +++
>  5 files changed, 31 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm64/kvm/debugfs.c
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2a5f7f38006f..130534c9079e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -29,6 +29,7 @@
>  #include <asm/thread_info.h>
>  
>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> +#define __KVM_HAVE_ARCH_VCPU_DEBUGFS
>  
>  #define KVM_HALT_POLL_NS_DEFAULT 500000
>  
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 989bb5dad2c8..17be7cf770f2 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -14,7 +14,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
>  	 $(KVM)/vfio.o $(KVM)/irqchip.o $(KVM)/binary_stats.o \
>  	 arm.o mmu.o mmio.o psci.o perf.o hypercalls.o pvtime.o \
>  	 inject_fault.o va_layout.o handle_exit.o \
> -	 guest.o debug.o reset.o sys_regs.o \
> +	 guest.o debug.o debugfs.o reset.o sys_regs.o \
>  	 vgic-sys-reg-v3.o fpsimd.o pmu.o \
>  	 arch_timer.o trng.o\
>  	 vgic/vgic.o vgic/vgic-init.o \
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 3df67c127489..ee69387f7fb6 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -82,7 +82,7 @@ u64 timer_get_cval(struct arch_timer_context *ctxt)
>  	}
>  }
>  
> -static u64 timer_get_offset(struct arch_timer_context *ctxt)
> +u64 timer_get_offset(struct arch_timer_context *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = ctxt->vcpu;
>  
> diff --git a/arch/arm64/kvm/debugfs.c b/arch/arm64/kvm/debugfs.c
> new file mode 100644
> index 000000000000..f0f5083ea8d4
> --- /dev/null
> +++ b/arch/arm64/kvm/debugfs.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Red Hat Inc.
> + */
> +
> +#include <linux/kvm_host.h>
> +#include <linux/debugfs.h>
> +
> +#include <kvm/arm_arch_timer.h>
> +
> +static int vcpu_get_cntv_offset(void *data, u64 *val)
> +{
> +	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
> +
> +	*val = timer_get_offset(vcpu_vtimer(vcpu));
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(vcpu_cntvoff_fops, vcpu_get_cntv_offset, NULL, "%lld\n");
> +
> +void kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu, struct dentry *debugfs_dentry)
> +{
> +	debugfs_create_file("cntvoff", 0444, debugfs_dentry, vcpu, &vcpu_cntvoff_fops);
> +}

This should be left in arch_timer.c until we actually need it for
multiple subsystems. When (and if) that happens, we will expose
per-subsystem debugfs initialisers instead of exposing the guts of the
timer code.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.