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

[Ying Fang <fangying1@huawei.com>]

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