| Message ID | 20200722035016.469075-2-bauerman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generalize start-powered-off property from ARM | expand |
Hi Thiago, On 7/22/20 5:50 AM, Thiago Jung Bauermann wrote: > There are other platforms which also have CPUs that start powered off, so > generalize the start-powered-off property so that it can be used by them. > > Note that ARMv7MState also has a property of the same name but this patch > doesn't change it because that class isn't a subclass of CPUState so it > wouldn't be a trivial change. > > This change should not cause any change in behavior. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> As I participated in reviewing your v1, I'd have appreciated being Cc'ed for v2. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > exec.c | 1 + > include/hw/core/cpu.h | 4 ++++ > target/arm/cpu.c | 5 ++--- > target/arm/cpu.h | 3 --- > target/arm/kvm32.c | 2 +- > target/arm/kvm64.c | 2 +- > 6 files changed, 9 insertions(+), 8 deletions(-)
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > Hi Thiago, > > On 7/22/20 5:50 AM, Thiago Jung Bauermann wrote: >> There are other platforms which also have CPUs that start powered off, so >> generalize the start-powered-off property so that it can be used by them. >> >> Note that ARMv7MState also has a property of the same name but this patch >> doesn't change it because that class isn't a subclass of CPUState so it >> wouldn't be a trivial change. >> >> This change should not cause any change in behavior. >> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > As I participated in reviewing your v1, I'd have appreciated > being Cc'ed for v2. I'm sorry about this. I fixed the Cc list for the next version. > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thank you very much for your prompt review, and suggestions. I will post a new version addressing your comments shortly. >> --- >> exec.c | 1 + >> include/hw/core/cpu.h | 4 ++++ >> target/arm/cpu.c | 5 ++--- >> target/arm/cpu.h | 3 --- >> target/arm/kvm32.c | 2 +- >> target/arm/kvm64.c | 2 +- >> 6 files changed, 9 insertions(+), 8 deletions(-) -- Thiago Jung Bauermann IBM Linux Technology Center
On Wed, Jul 22, 2020 at 12:50:08AM -0300, Thiago Jung Bauermann wrote: > There are other platforms which also have CPUs that start powered off, so > generalize the start-powered-off property so that it can be used by them. > > Note that ARMv7MState also has a property of the same name but this patch > doesn't change it because that class isn't a subclass of CPUState so it > wouldn't be a trivial change. > > This change should not cause any change in behavior. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > exec.c | 1 + > include/hw/core/cpu.h | 4 ++++ > target/arm/cpu.c | 5 ++--- > target/arm/cpu.h | 3 --- > target/arm/kvm32.c | 2 +- > target/arm/kvm64.c | 2 +- > 6 files changed, 9 insertions(+), 8 deletions(-) > > NB: I was only able to test that this patch builds. I wasn't able to > run it. > > diff --git a/exec.c b/exec.c > index 6f381f98e2..82e82fab09 100644 > --- a/exec.c > +++ b/exec.c > @@ -899,6 +899,7 @@ Property cpu_common_props[] = { > DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION, > MemoryRegion *), > #endif > + DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 8f145733ce..9fc2696db5 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -374,6 +374,10 @@ struct CPUState { > bool created; > bool stop; > bool stopped; > + > + /* Should CPU start in powered-off state? */ > + bool start_powered_off; > + > bool unplug; > bool crash_occurred; > bool exit_request; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 111579554f..ec65c7653f 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -174,8 +174,8 @@ static void arm_cpu_reset(DeviceState *dev) > env->vfp.xregs[ARM_VFP_MVFR1] = cpu->isar.mvfr1; > env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2; > > - cpu->power_state = cpu->start_powered_off ? PSCI_OFF : PSCI_ON; > - s->halted = cpu->start_powered_off; > + cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON; > + s->halted = s->start_powered_off; > > if (arm_feature(env, ARM_FEATURE_IWMMXT)) { > env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q'; > @@ -2182,7 +2182,6 @@ static const ARMCPUInfo arm_cpus[] = { > }; > > static Property arm_cpu_properties[] = { > - DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), > DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), > DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0), > DEFINE_PROP_UINT64("mp-affinity", ARMCPU, > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 9e8ed423ea..a925d26996 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -810,9 +810,6 @@ struct ARMCPU { > */ > uint32_t psci_version; > > - /* Should CPU start in PSCI powered-off state? */ > - bool start_powered_off; > - > /* Current power state, access guarded by BQL */ > ARMPSCIState power_state; > > diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c > index 0af46b41c8..1f2b8f8b7a 100644 > --- a/target/arm/kvm32.c > +++ b/target/arm/kvm32.c > @@ -218,7 +218,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > > /* Determine init features for this CPU */ > memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features)); > - if (cpu->start_powered_off) { > + if (cs->start_powered_off) { > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF; > } > if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) { > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 1169237905..f8a6d905fb 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -775,7 +775,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > > /* Determine init features for this CPU */ > memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features)); > - if (cpu->start_powered_off) { > + if (cs->start_powered_off) { > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF; > } > if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) { >
Hello David, David Gibson <david@gibson.dropbear.id.au> writes: > On Wed, Jul 22, 2020 at 12:50:08AM -0300, Thiago Jung Bauermann wrote: >> There are other platforms which also have CPUs that start powered off, so >> generalize the start-powered-off property so that it can be used by them. >> >> Note that ARMv7MState also has a property of the same name but this patch >> doesn't change it because that class isn't a subclass of CPUState so it >> wouldn't be a trivial change. >> >> This change should not cause any change in behavior. >> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Thank you very much for your review! Unfortunately I apparently had a minor email mishap and only got your reviews after I sent the v3 patches, so I wasn't able to put your Reviewed-by's in them. Sorry about that.
diff --git a/exec.c b/exec.c index 6f381f98e2..82e82fab09 100644 --- a/exec.c +++ b/exec.c @@ -899,6 +899,7 @@ Property cpu_common_props[] = { DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION, MemoryRegion *), #endif + DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 8f145733ce..9fc2696db5 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -374,6 +374,10 @@ struct CPUState { bool created; bool stop; bool stopped; + + /* Should CPU start in powered-off state? */ + bool start_powered_off; + bool unplug; bool crash_occurred; bool exit_request; diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 111579554f..ec65c7653f 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -174,8 +174,8 @@ static void arm_cpu_reset(DeviceState *dev) env->vfp.xregs[ARM_VFP_MVFR1] = cpu->isar.mvfr1; env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2; - cpu->power_state = cpu->start_powered_off ? PSCI_OFF : PSCI_ON; - s->halted = cpu->start_powered_off; + cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON; + s->halted = s->start_powered_off; if (arm_feature(env, ARM_FEATURE_IWMMXT)) { env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q'; @@ -2182,7 +2182,6 @@ static const ARMCPUInfo arm_cpus[] = { }; static Property arm_cpu_properties[] = { - DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0), DEFINE_PROP_UINT64("mp-affinity", ARMCPU, diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 9e8ed423ea..a925d26996 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -810,9 +810,6 @@ struct ARMCPU { */ uint32_t psci_version; - /* Should CPU start in PSCI powered-off state? */ - bool start_powered_off; - /* Current power state, access guarded by BQL */ ARMPSCIState power_state; diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c index 0af46b41c8..1f2b8f8b7a 100644 --- a/target/arm/kvm32.c +++ b/target/arm/kvm32.c @@ -218,7 +218,7 @@ int kvm_arch_init_vcpu(CPUState *cs) /* Determine init features for this CPU */ memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features)); - if (cpu->start_powered_off) { + if (cs->start_powered_off) { cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF; } if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) { diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 1169237905..f8a6d905fb 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -775,7 +775,7 @@ int kvm_arch_init_vcpu(CPUState *cs) /* Determine init features for this CPU */ memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features)); - if (cpu->start_powered_off) { + if (cs->start_powered_off) { cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF; } if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
There are other platforms which also have CPUs that start powered off, so generalize the start-powered-off property so that it can be used by them. Note that ARMv7MState also has a property of the same name but this patch doesn't change it because that class isn't a subclass of CPUState so it wouldn't be a trivial change. This change should not cause any change in behavior. Suggested-by: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> --- exec.c | 1 + include/hw/core/cpu.h | 4 ++++ target/arm/cpu.c | 5 ++--- target/arm/cpu.h | 3 --- target/arm/kvm32.c | 2 +- target/arm/kvm64.c | 2 +- 6 files changed, 9 insertions(+), 8 deletions(-) NB: I was only able to test that this patch builds. I wasn't able to run it.