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

Andre Przywara
2025-09-16T12:17:36Z

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