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

[Ying Fang]

On 6/3/2020 4:26 PM, Andrew Jones wrote:
> On Wed, Jun 03, 2020 at 10:02:08AM +0800, Ying Fang wrote:
> > 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 gernic
> > timer feature enabled.
> >
> >
> > Signed-off-by: Ying Fang <fangying1@huawei.com>
> >
> > ---
> > 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
> >
> > ---
> >   target/arm/cpu.c   | 3 +--
> >   target/arm/cpu64.c | 1 -
> >   2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 32bec156f2..1e9b7a51f2 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1244,6 +1244,7 @@ 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);
> > +        kvm_arm_add_vcpu_properties(obj);
>
> The name 'kvm_arm_add_vcpu_properties' says nothing about being specific
> to the timer. So this is either the wrong place for this function, or the
> function is named wrong. I'd say it's the wrong place, because, as the
> comment above kvm_arm_add_vcpu_properties() implies, the function is
> for all 'kvm-*' prefixed properties, not just timer related ones. It's
> true we don't have any others yet, but we will. I have plans to post
can't agree more.

> kvm-steal-time soon, and despite it also having 'time' in its name, it
> shouldn't depend on ARM_FEATURE_GENERIC_TIMER.
yes, that would be nice.

> How about the below [untested] patch instead?
this patch is much more reasonable for me, thanks for your help.
I've tested it and it works fine.

> Thanks,
> drew
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 32bec156f2d4..e9084f98ef10 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()

should be 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 cbc5c3868fce..778cecc2e6ca 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 4bdbe6dcac07..eef3bbd1cc2c 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)
>
>
> >       }
> >   }
> >   
> > @@ -2029,7 +2030,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 +2183,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;
> > --
> > 2.23.0
>
>
> .