| Message ID | 20200603020208.2089-1-fangying1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/arm/cpu: adjust virtual time for arm cpu | expand |
On 6/2/20 7:02 PM, 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 "generic" > timer feature enabled. > > > Signed-off-by: Ying Fang <fangying1@huawei.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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 kvm-steal-time soon, and despite it also having 'time' in its name, it shouldn't depend on ARM_FEATURE_GENERIC_TIMER. How about the below [untested] patch instead? 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(); + } } 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 > > >
On Wed, 3 Jun 2020 at 09:26, Andrew Jones <drjones@redhat.com> wrote: > 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 > kvm-steal-time soon, and despite it also having 'time' in its name, it > shouldn't depend on ARM_FEATURE_GENERIC_TIMER. > > How about the below [untested] patch instead? Your patch is the code arrangement I had in mind, yes. -- PMM
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 >> >> >> > > > . >
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); } } @@ -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;
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(-)