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

[Marc Zyngier <maz@kernel.org>]

On Mon, 04 Aug 2025 15:45:00 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> 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?

Because that's the only way KVM gives you the very much required
ability to offset the counters when NV is enabled.

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

On a system without this extension, there is no global offset at
all. So setting it to 0 or omitting the option have the exact same
outcome. Why should the user care?

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

This looks positively awful. Not to mention that on a modern system
(anything >= 8.6), there is no such thing as an "unreasonable" counter
value.

	M.

-- 
Jazz isn't dead. It just smells funny.