Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support

[Marc Zyngier <maz@kernel.org>]

On Thu, 03 Mar 2022 12:01:10 +0000,
Sebastian Ene <sebastianene@google.com> wrote:
> 
> > > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu)
> > > +{
> > > +	int ret;
> > > +	bool has_stolen_time;
> > > +	u64 pvtime_guest_addr = ARM_PVTIME_MMIO_BASE + vcpu->cpu_id *
> > > +		ARM_PVTIME_STRUCT_SIZE;
> > > +	struct kvm_config *kvm_cfg = NULL;
> > > +	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> > > +		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
> > > +		.addr	= KVM_ARM_VCPU_PVTIME_IPA
> > > +	};
> > > +
> > > +	kvm_cfg = &vcpu->kvm->cfg;
> > > +	if (kvm_cfg->no_pvtime)
> > > +		return 0;
> > > +
> > > +	if (!pvtime_data.is_supported)
> > > +		return -ENOTSUP;
> > 
> > It is a bit odd to have this hard failure if running on a system that
> > doesn't have pvtime. It forces the user to alter their command-line,
> > which is a bit annoying. I'd rather have a soft-fail here.
> > 
> 
> The flag 'is_supported' is set to false when we support pvtime but we
> fail to configure it. We verify that we support pvtime by calling the check
> extension KVM_CAP_STEAL_TIME. I think the naming is odd here for the
> flag name. It should be : 'is_failed_cfg'.

Ah, I see. Yes, the name is misleading.

>
> > > +
> > > +	has_stolen_time = kvm__supports_extension(vcpu->kvm,
> > > +						  KVM_CAP_STEAL_TIME);
> > > +	if (!has_stolen_time)
> > > +		return 0;

Here, you could force no_pvtime to 1, and avoid checking for each vcpu
once you detected that the host is not equipped to deal with it.

Thanks,

	M.

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