| Message ID | 20200723025657.644724-9-bauerman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generalize start-powered-off property from ARM | expand |
On Wed, 22 Jul 2020 23:56:57 -0300 Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the > start-powered-off property which makes cpu_common_reset() initialize it > to 1 in common code. > > Note that this changes behavior by setting cs->halted to 1 on reset, which > didn't happen before. I think that should be fine, as we change the cpu state to STOPPED in the reset function, which sets halted to 1. > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > --- > target/s390x/cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > NB: I was only able to test that this patch builds. I wasn't able to > run it. No noticeable difference under kvm, but running under tcg seems a bit more sluggish than usual, and I saw some pausing on reboot (after the bios handover to the kernel). Not sure if it were just flukes on my laptop, would appreciate if someone else could give it a go. > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index 08eb674d22..73d7d6007e 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -291,7 +291,7 @@ static void s390_cpu_initfn(Object *obj) > S390CPU *cpu = S390_CPU(obj); > > cpu_set_cpustate_pointers(cpu); > - cs->halted = 1; > + cs->start_powered_off = true; > cs->exception_index = EXCP_HLT; > #if !defined(CONFIG_USER_ONLY) > object_property_add(obj, "crash-information", "GuestPanicInformation", >
Hi, Cornelia Huck <cohuck@redhat.com> writes: > On Wed, 22 Jul 2020 23:56:57 -0300 > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > >> Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the >> start-powered-off property which makes cpu_common_reset() initialize it >> to 1 in common code. >> >> Note that this changes behavior by setting cs->halted to 1 on reset, which >> didn't happen before. > > I think that should be fine, as we change the cpu state to STOPPED in > the reset function, which sets halted to 1. Nice, thanks for checking. >> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> --- >> target/s390x/cpu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> NB: I was only able to test that this patch builds. I wasn't able to >> run it. > > No noticeable difference under kvm, but running under tcg seems a bit > more sluggish than usual, and I saw some pausing on reboot (after the > bios handover to the kernel). Not sure if it were just flukes on my > laptop, would appreciate if someone else could give it a go. I tried today setting up a TCG guest, but didn't have success yet. Will try some more tomorrow.
On Tue, 28 Jul 2020 21:51:33 -0300 Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > Hi, > > Cornelia Huck <cohuck@redhat.com> writes: > > > On Wed, 22 Jul 2020 23:56:57 -0300 > > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > > > >> Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the > >> start-powered-off property which makes cpu_common_reset() initialize it > >> to 1 in common code. > >> > >> Note that this changes behavior by setting cs->halted to 1 on reset, which > >> didn't happen before. > > > > I think that should be fine, as we change the cpu state to STOPPED in > > the reset function, which sets halted to 1. > > Nice, thanks for checking. > > >> > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > >> --- > >> target/s390x/cpu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> NB: I was only able to test that this patch builds. I wasn't able to > >> run it. > > > > No noticeable difference under kvm, but running under tcg seems a bit > > more sluggish than usual, and I saw some pausing on reboot (after the > > bios handover to the kernel). Not sure if it were just flukes on my > > laptop, would appreciate if someone else could give it a go. Experimented a bit with it again. There's a pause when switching from the bios to the kernel (after the load reset normal has been done, I guess), which is always there, but seems to get more noticeable with this patch (varying wildly, but seems longer on average.) Hard to pin down, and I don't really see a reason why that should happen, as we should end up with halted == 1 in any case. Might still be a fluke, even though I see it both on my laptop and on an LPAR (when running under tcg; not seen under kvm, which is much faster anyway.) > > I tried today setting up a TCG guest, but didn't have success yet. > Will try some more tomorrow. > I'm also looking a bit at the other s390 folks :)
On Thu, 30 Jul 2020 11:45:41 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 28 Jul 2020 21:51:33 -0300 > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > > > Hi, > > > > Cornelia Huck <cohuck@redhat.com> writes: > > > > > On Wed, 22 Jul 2020 23:56:57 -0300 > > > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > > > > > >> Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the > > >> start-powered-off property which makes cpu_common_reset() initialize it > > >> to 1 in common code. > > >> > > >> Note that this changes behavior by setting cs->halted to 1 on reset, which > > >> didn't happen before. > > > > > > I think that should be fine, as we change the cpu state to STOPPED in > > > the reset function, which sets halted to 1. > > > > Nice, thanks for checking. > > > > >> > > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > >> --- > > >> target/s390x/cpu.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> NB: I was only able to test that this patch builds. I wasn't able to > > >> run it. > > > > > > No noticeable difference under kvm, but running under tcg seems a bit > > > more sluggish than usual, and I saw some pausing on reboot (after the > > > bios handover to the kernel). Not sure if it were just flukes on my > > > laptop, would appreciate if someone else could give it a go. > > Experimented a bit with it again. There's a pause when switching from > the bios to the kernel (after the load reset normal has been done, I > guess), which is always there, but seems to get more noticeable with > this patch (varying wildly, but seems longer on average.) Hard to pin > down, and I don't really see a reason why that should happen, as we > should end up with halted == 1 in any case. Might still be a fluke, > even though I see it both on my laptop and on an LPAR (when running > under tcg; not seen under kvm, which is much faster anyway.) Tried again, the pause now seems comparable to the pause prior to this series. Might depend on the phase of the moon. I ran kvm unit tests on it, and it looks good. So, I'm reasonable confident that this is fine, really just seems to be a fluke. Acked-by: Cornelia Huck <cohuck@redhat.com>
Cornelia Huck <cohuck@redhat.com> writes: > On Thu, 30 Jul 2020 11:45:41 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Tue, 28 Jul 2020 21:51:33 -0300 >> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: >> >> > Hi, >> > >> > Cornelia Huck <cohuck@redhat.com> writes: >> > >> > > On Wed, 22 Jul 2020 23:56:57 -0300 >> > > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: >> > > >> > >> Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the >> > >> start-powered-off property which makes cpu_common_reset() initialize it >> > >> to 1 in common code. >> > >> >> > >> Note that this changes behavior by setting cs->halted to 1 on reset, which >> > >> didn't happen before. >> > > >> > > I think that should be fine, as we change the cpu state to STOPPED in >> > > the reset function, which sets halted to 1. >> > >> > Nice, thanks for checking. >> > >> > >> >> > >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> > >> --- >> > >> target/s390x/cpu.c | 2 +- >> > >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> >> > >> NB: I was only able to test that this patch builds. I wasn't able to >> > >> run it. >> > > >> > > No noticeable difference under kvm, but running under tcg seems a bit >> > > more sluggish than usual, and I saw some pausing on reboot (after the >> > > bios handover to the kernel). Not sure if it were just flukes on my >> > > laptop, would appreciate if someone else could give it a go. >> >> Experimented a bit with it again. There's a pause when switching from >> the bios to the kernel (after the load reset normal has been done, I >> guess), which is always there, but seems to get more noticeable with >> this patch (varying wildly, but seems longer on average.) Hard to pin >> down, and I don't really see a reason why that should happen, as we >> should end up with halted == 1 in any case. Might still be a fluke, >> even though I see it both on my laptop and on an LPAR (when running >> under tcg; not seen under kvm, which is much faster anyway.) > > Tried again, the pause now seems comparable to the pause prior to this > series. Might depend on the phase of the moon. > > I ran kvm unit tests on it, and it looks good. So, I'm reasonable > confident that this is fine, really just seems to be a fluke. > > Acked-by: Cornelia Huck <cohuck@redhat.com> Ah, that's a relief. Thank you very much for looking into this!
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 08eb674d22..73d7d6007e 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -291,7 +291,7 @@ static void s390_cpu_initfn(Object *obj) S390CPU *cpu = S390_CPU(obj); cpu_set_cpustate_pointers(cpu); - cs->halted = 1; + cs->start_powered_off = true; cs->exception_index = EXCP_HLT; #if !defined(CONFIG_USER_ONLY) object_property_add(obj, "crash-information", "GuestPanicInformation",
Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the start-powered-off property which makes cpu_common_reset() initialize it to 1 in common code. Note that this changes behavior by setting cs->halted to 1 on reset, which didn't happen before. Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> --- target/s390x/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) NB: I was only able to test that this patch builds. I wasn't able to run it.