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

[Ying Fang]

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.
> 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. And we are planning to add 
support for it if nobody is currently doing that.

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