Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu

[Andrew Jones]

On Wed, Jun 10, 2020 at 09:32:06AM +0800, Ying Fang wrote:
> 
> 
> On 6/8/2020 8:49 PM, Andrew Jones wrote:
> > On Mon, Jun 08, 2020 at 08:12:43PM +0800, Ying Fang wrote:
> > > From: fangying <fangying1@huawei.com>
> > > 
> > > Virtual time adjustment was implemented for virt-5.0 machine type,
> > > but the cpu property was enabled only for host-passthrough and
> > > max cpu model. Let's add it for arm cpu which has the generic timer
> > > feature enabled.
> > > 
> > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > 
> > This isn't true. I did suggest the way to arrange the code, after
> > Peter suggested to move the kvm_arm_add_vcpu_properties() call to
> > arm_cpu_post_init(), but I didn't suggest making this change in general,
> > which is what this tag means. In fact, I've argued that it's pretty
> I'm quite sorry for adding it here.

No problem.

> > pointless to do this, since KVM users should be using '-cpu host' or
> > '-cpu max' anyway. Since I don't need credit for the code arranging,
> As discussed in thread [1], there is a situation where a 'custom' cpu mode
> is needed for us to keep instruction set compatibility so that migration can
> be done, just like x86 does.

I understand the motivation. But, as I've said, KVM doesn't work that way.

> And we are planning to add support for it if
> nobody is currently doing that.

Great! I'm looking forward to seeing the KVM patches. Especially since,
without the KVM patches, the 'custom' CPU model isn't a custom CPU model,
it's just a misleading way to use host passthrough. Indeed, I'm a bit
opposed to allowing anything other than '-cpu host' and '-cpu max' (with
features explicitly enabled/disabled, e.g. -cpu host,pmu=off) to work
until KVM actually works with CPU models. Otherwise, how do we know the
difference between a model that actually works and one that is just
misleadingly named?

Thanks,
drew

> 
> Thanks.
> Ying
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00022.html
> > please just drop the tag. Peter can maybe do that on merge though. Also,
> > despite not agreeing that we need this change today, as there's nothing
> > wrong with it and it looks good to me
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > 
> > Thanks,
> > drew
> > 
> > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > 
> > > ---
> > > v3:
> > > - set kvm-no-adjvtime property in kvm_arm_add_vcpu_properties
> > > 
> > > v2:
> > > - move kvm_arm_add_vcpu_properties into arm_cpu_post_init
> > > 
> > > v1:
> > > - initial commit
> > > - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg08518.html
> > > 
> > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > > index 32bec156f2..5b7a36b5d7 100644
> > > --- a/target/arm/cpu.c
> > > +++ b/target/arm/cpu.c
> > > @@ -1245,6 +1245,10 @@ void arm_cpu_post_init(Object *obj)
> > >       if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) {
> > >           qdev_property_add_static(DEVICE(cpu), 
> > > &arm_cpu_gt_cntfrq_property);
> > >       }
> > > +
> > > +    if (kvm_enabled()) {
> > > +        kvm_arm_add_vcpu_properties(obj);
> > > +    }
> > >   }
> > >   static void arm_cpu_finalizefn(Object *obj)
> > > @@ -2029,7 +2033,6 @@ static void arm_max_initfn(Object *obj)
> > >       if (kvm_enabled()) {
> > >           kvm_arm_set_cpu_features_from_host(cpu);
> > > -        kvm_arm_add_vcpu_properties(obj);
> > >       } else {
> > >           cortex_a15_initfn(obj);
> > > @@ -2183,7 +2186,6 @@ static void arm_host_initfn(Object *obj)
> > >       if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> > >           aarch64_add_sve_properties(obj);
> > >       }
> > > -    kvm_arm_add_vcpu_properties(obj);
> > >       arm_cpu_post_init(obj);
> > >   }
> > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > > index cbc5c3868f..778cecc2e6 100644
> > > --- a/target/arm/cpu64.c
> > > +++ b/target/arm/cpu64.c
> > > @@ -592,7 +592,6 @@ static void aarch64_max_initfn(Object *obj)
> > >       if (kvm_enabled()) {
> > >           kvm_arm_set_cpu_features_from_host(cpu);
> > > -        kvm_arm_add_vcpu_properties(obj);
> > >       } else {
> > >           uint64_t t;
> > >           uint32_t u;
> > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > > index 4bdbe6dcac..eef3bbd1cc 100644
> > > --- a/target/arm/kvm.c
> > > +++ b/target/arm/kvm.c
> > > @@ -194,17 +194,18 @@ static void kvm_no_adjvtime_set(Object *obj, bool 
> > > value, Error **errp)
> > >   /* KVM VCPU properties should be prefixed with "kvm-". */
> > >   void kvm_arm_add_vcpu_properties(Object *obj)
> > >   {
> > > -    if (!kvm_enabled()) {
> > > -        return;
> > > -    }
> > > +    ARMCPU *cpu = ARM_CPU(obj);
> > > +    CPUARMState *env = &cpu->env;
> > > -    ARM_CPU(obj)->kvm_adjvtime = true;
> > > -    object_property_add_bool(obj, "kvm-no-adjvtime", kvm_no_adjvtime_get,
> > > -                             kvm_no_adjvtime_set);
> > > -    object_property_set_description(obj, "kvm-no-adjvtime",
> > > -                                    "Set on to disable the adjustment of 
> > > "
> > > -                                    "the virtual counter. VM stopped 
> > > time "
> > > -                                    "will be counted.");
> > > +    if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> > > +        cpu->kvm_adjvtime = true;
> > > +        object_property_add_bool(obj, "kvm-no-adjvtime", 
> > > kvm_no_adjvtime_get,
> > > +                                 kvm_no_adjvtime_set);
> > > +        object_property_set_description(obj, "kvm-no-adjvtime",
> > > +                                        "Set on to disable the 
> > > adjustment of "
> > > +                                        "the virtual counter. VM stopped 
> > > time "
> > > +                                        "will be counted.");
> > > +    }
> > >   }
> > >   bool kvm_arm_pmu_supported(CPUState *cpu)
> > > -- 
> > > 2.23.0
> > > 
> > > 
> > 
> > .
> > 
>