| Message ID | 20230823142219.1046522-3-seiden@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Enable AP instructions for pv-guests | expand |
On Wed, Aug 23, 2023 at 04:22 PM +0200, Steffen Eiden <seiden@linux.ibm.com> wrote: > From: Janosch Frank <frankja@linux.ibm.com> > > Bound APQNs have to be reset before tearing down the secure config via > s390_machine_unprotect(). Otherwise the Ultravisor will return a error > code. > > So let's switch the ordering around to make that happen. > > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 4b36c9970e..795dd53d68 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -442,13 +442,13 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason) > switch (reset_type) { > case S390_RESET_EXTERNAL: > case S390_RESET_REIPL: > + qemu_devices_reset(reason); > + s390_crypto_reset(); > + > if (s390_is_pv()) { > s390_machine_unprotect(ms); > } > > - qemu_devices_reset(reason); > - s390_crypto_reset(); > - > /* configure and start the ipl CPU only */ > run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL); > break; > -- > 2.41.0 > Unfortunately, this breaks things for me. You can reproduce the problem easily… Start an SE guest via direct kernel boot and reboot the guest after the guest has booted. e.g. $ sudo qemu-system-s390x -smp 2 --machine accel=kvm,aes-key-wrap=off -kernel /var/lib/libvirt/images/hades/vmlinux-s390x.se.img -m 2048 -nographic -nodefaults -chardev stdio,id=c1,mux=on -device sclpconsole,chardev=c1 -append "earlyprintk" … # reboot # (console in the guest) … [ 3.966496] systemd-shutdown[1]: Syncing filesystems and block devices. [ 3.966606] systemd-shutdown[1]: Rebooting. qemu-system-s390x: CPU reset failed on CPU 0 type aec4 qemu-system-s390x: CPU reset failed on CPU 1 type aec4 qemu-system-s390x: KVM PV command 4 (KVM_PV_VERIFY) failed: header rc 102 rrc 1b IOCTL rc: -22 Protected boot has failed: 0xa02
On 8/31/23 18:21, Marc Hartmayer wrote: > On Wed, Aug 23, 2023 at 04:22 PM +0200, Steffen Eiden <seiden@linux.ibm.com> wrote: >> From: Janosch Frank <frankja@linux.ibm.com> >> >> Bound APQNs have to be reset before tearing down the secure config via >> s390_machine_unprotect(). Otherwise the Ultravisor will return a error >> code. >> >> So let's switch the ordering around to make that happen. >> >> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 4b36c9970e..795dd53d68 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -442,13 +442,13 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason) >> switch (reset_type) { >> case S390_RESET_EXTERNAL: >> case S390_RESET_REIPL: >> + qemu_devices_reset(reason); >> + s390_crypto_reset(); >> + >> if (s390_is_pv()) { >> s390_machine_unprotect(ms); >> } >> >> - qemu_devices_reset(reason); >> - s390_crypto_reset(); >> - >> /* configure and start the ipl CPU only */ >> run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL); >> break; >> -- >> 2.41.0 >> > > Unfortunately, this breaks things for me. You can reproduce the problem > easily… Start an SE guest via direct kernel boot and reboot the guest > after the guest has booted. Seems like we didn't test a direct kernel boot reboot... I'm looking into it.
On Fri, 2023-09-01 at 11:48 +0000, Janosch Frank wrote: > Bound APQNs have to be reset before tearing down the secure config via > s390_machine_unprotect(). Otherwise the Ultravisor will return a error > code. > > So let's do a subsystem_reset() which includes a AP reset before the > unprotect call. We'll do a full device_reset() afterwards which will > reset some devices twice. That's ok since we can't move the > device_reset() before the unprotect as it includes a CPU clear reset > which the Ultravisor does not expect at that point in time. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > > I'm not able to test this for the PV AP case right new, that has to > wait to early next week. However Marc told me that the non-AP PV test > works fine now. > > --- > hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 3dd0b2372d..2d75f2131f 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -438,10 +438,20 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason) > switch (reset_type) { > case S390_RESET_EXTERNAL: > case S390_RESET_REIPL: > + /* > + * Reset the subsystem which includes a AP reset. If a PV > + * guest had APQNs attached the AP reset is a prerequisite to > + * unprotecting since the UV checks if all APQNs are reset. > + */ > + subsystem_reset(); > if (s390_is_pv()) { > s390_machine_unprotect(ms); > } > > + /* > + * Device reset includes CPU clear resets so this has to be > + * done AFTER the unprotect call above. > + */ > qemu_devices_reset(reason); > s390_crypto_reset(); > I tested this with and without bound/associated APQNs both with booting from disk and with direct kernel boot. Subsequent reboots are correctly resetting the APQNs. I also successfully tested the case direct kernel boot -> chreipl -> disk boot. If you want you can add Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
Am 01.09.23 um 13:48 schrieb Janosch Frank: > Bound APQNs have to be reset before tearing down the secure config via > s390_machine_unprotect(). Otherwise the Ultravisor will return a error > code. > > So let's do a subsystem_reset() which includes a AP reset before the > unprotect call. We'll do a full device_reset() afterwards which will > reset some devices twice. That's ok since we can't move the > device_reset() before the unprotect as it includes a CPU clear reset > which the Ultravisor does not expect at that point in time. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Makes sense. Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com> > --- > > I'm not able to test this for the PV AP case right new, that has to > wait to early next week. However Marc told me that the non-AP PV test > works fine now. > > --- > hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 3dd0b2372d..2d75f2131f 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -438,10 +438,20 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason) > switch (reset_type) { > case S390_RESET_EXTERNAL: > case S390_RESET_REIPL: > + /* > + * Reset the subsystem which includes a AP reset. If a PV > + * guest had APQNs attached the AP reset is a prerequisite to > + * unprotecting since the UV checks if all APQNs are reset. > + */ > + subsystem_reset(); > if (s390_is_pv()) { > s390_machine_unprotect(ms); > } > > + /* > + * Device reset includes CPU clear resets so this has to be > + * done AFTER the unprotect call above. > + */ > qemu_devices_reset(reason); > s390_crypto_reset(); >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 4b36c9970e..795dd53d68 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -442,13 +442,13 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason) switch (reset_type) { case S390_RESET_EXTERNAL: case S390_RESET_REIPL: + qemu_devices_reset(reason); + s390_crypto_reset(); + if (s390_is_pv()) { s390_machine_unprotect(ms); } - qemu_devices_reset(reason); - s390_crypto_reset(); - /* configure and start the ipl CPU only */ run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL); break;