[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu
From: |
Andrew Jones |
Subject: |
Re: [PATCH v3] target/arm/cpu: adjust virtual time for arm cpu |
Date: |
Wed, 10 Jun 2020 09:40:26 +0200 |
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 <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.
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 <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
> > >
> > >
> >
> > .
> >
>