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

[Sebastian Ene <sebastianene@google.com>]

On Thu, Mar 03, 2022 at 05:51:36PM +0000, Marc Zyngier wrote:
> On Thu, 03 Mar 2022 12:01:10 +0000,
> Sebastian Ene <sebastianene@google.com> wrote:

Hi,

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

I will update the name for this to: 'is_failed_cfg'.

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

Good point ! I will do this.

Thanks,
Sebastian

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