Re: [PATCH kvmtool v3 4/6] arm64: add counter offset control

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Andre,

You might want to capitalize the first letter of the subject line (add->Add).

On Tue, Jul 29, 2025 at 10:57:43AM +0100, Andre Przywara wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
> KVM allows the offsetting of the global counter in order to help with
> migration of a VM. This offset applies cumulatively with the offsets
> provided by the architecture.
> 
> Although kvmtool doesn't provide a way to migrate a VM, controlling
> this offset is useful to test the timer subsystem.
> 
> Add the command line option --counter-offset to allow setting this value
> when creating a VM.

Out of curiosity, how is this related to nested virtualization?

> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm64/include/kvm/kvm-config-arch.h |  3 +++
>  arm64/kvm.c                         | 17 +++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arm64/include/kvm/kvm-config-arch.h b/arm64/include/kvm/kvm-config-arch.h
> index a1dac28e6..44c43367b 100644
> --- a/arm64/include/kvm/kvm-config-arch.h
> +++ b/arm64/include/kvm/kvm-config-arch.h
> @@ -14,6 +14,7 @@ struct kvm_config_arch {
>  	u64		kaslr_seed;
>  	enum irqchip_type irqchip;
>  	u64		fw_addr;
> +	u64		counter_offset;
>  	unsigned int	sve_max_vq;
>  	bool		no_pvtime;
>  };
> @@ -59,6 +60,8 @@ int sve_vl_parser(const struct option *opt, const char *arg, int unset);
>  		     irqchip_parser, NULL),					\
>  	OPT_U64('\0', "firmware-address", &(cfg)->fw_addr,			\
>  		"Address where firmware should be loaded"),			\
> +	OPT_U64('\0', "counter-offset", &(cfg)->counter_offset,			\
> +		"Specify the counter offset, defaulting to 0"),			\

I'm having a hard time parsing this - if it's zero, then kvmtool leaves it
unset, how is the default value 0? Maybe you want to say that if left unset,
the counters behaves as if the global offset is zero.

>  	OPT_BOOLEAN('\0', "nested", &(cfg)->nested_virt,			\
>  		    "Start VCPUs in EL2 (for nested virt)"),
>  
> diff --git a/arm64/kvm.c b/arm64/kvm.c
> index 23b4dab1f..6e971dd78 100644
> --- a/arm64/kvm.c
> +++ b/arm64/kvm.c
> @@ -119,6 +119,22 @@ static void kvm__arch_enable_mte(struct kvm *kvm)
>  	pr_debug("MTE capability enabled");
>  }
>  
> +static void kvm__arch_set_counter_offset(struct kvm *kvm)
> +{
> +	struct kvm_arm_counter_offset offset = {
> +		.counter_offset = kvm->cfg.arch.counter_offset,
> +	};
> +
> +	if (!kvm->cfg.arch.counter_offset)
> +		return;
> +
> +	if (!kvm__supports_extension(kvm, KVM_CAP_COUNTER_OFFSET))
> +		die("No support for global counter offset");

What happens when the user sets --counter-offset 0 and KVM doesn't support
the capability? Looks to me like instead of getting an error, kvmtool is happy
to proceed without actually setting the counter offset to 0. User might then be
fooled into thinking that KVM supports KVM_CAP_COUNTER_OFFSET, and when the same
user does --counter-offset x, they will get an error saying that there's no
support for it in KVM. I would be extremely confused by that.

If this is something that you want to address, you can do it similar to
ram_addr: initialize the offset to something unreasonable before parsing the
command line parameters, and then bail early in kvm__arch_set_counter_offset().

Thanks,
Alex

> +
> +	if (ioctl(kvm->vm_fd, KVM_ARM_SET_COUNTER_OFFSET, &offset))
> +		die_perror("KVM_ARM_SET_COUNTER_OFFSET");
> +}
> +
>  void kvm__arch_init(struct kvm *kvm)
>  {
>  	/* Create the virtual GIC. */
> @@ -126,6 +142,7 @@ void kvm__arch_init(struct kvm *kvm)
>  		die("Failed to create virtual GIC");
>  
>  	kvm__arch_enable_mte(kvm);
> +	kvm__arch_set_counter_offset(kvm);
>  }
>  
>  static u64 kvm__arch_get_payload_region_size(struct kvm *kvm)
> -- 
> 2.25.1
>