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

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 4 Aug 2025 15:45:00 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> 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.

That's a much longer wording for something meant to be a very concise
description of the option, with the same meaning. So while "defaulting to
0" might not be 100% correct when it comes to how it's *implemented*, from
the user's point of view the effect is the same. And this is a user facing
message.

> >  	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.

On the other hand rejecting "--counter-offset 0" even when it's the
default behaviour and would work is even more cumbersome, I'd say. And I'd
argue that in general "offset 0" being a special case is well understood,
so I wouldn't be too confused about that.

If you really feel that needs detailed explanation, maybe we should add
that to the documentation?

Cheers,
Andre

> 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
> >