Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
On 6/10/2020 3:40 PM, Andrew Jones wrote: 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 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 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? Yes you are right here. My colleague zhanghailiang and me are now working on it. We will post the patch set soon. 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 Thanks, drew Signed-off-by: Ying Fang --- 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_de
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
On Mon, 8 Jun 2020 at 13:12, Ying Fang wrote: > > From: fangying > > 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 > Signed-off-by: Ying Fang Applied to target-arm.next, thanks (with a minor commit message wording tweak, and the suggested-by tag removed). -- PMM
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
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 > > > > > > 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 > > > > 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 > > > > Thanks, > > drew > > > > > Signed-off-by: Ying Fang > > > > > > --- > > > 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 > > > " > > > -
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
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 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 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 Thanks, drew Signed-off-by: Ying Fang --- 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 .
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
On Mon, Jun 08, 2020 at 08:12:43PM +0800, Ying Fang wrote: > From: fangying > > 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 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 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, 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 Thanks, drew > Signed-off-by: Ying Fang > > --- > 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 > >
[PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
From: fangying 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 Signed-off-by: Ying Fang --- 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