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