| Message ID | 20191120114334.2287-8-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On Wed, 20 Nov 2019 06:43:26 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Now that we know the protection state off the cpus, we can start > handling all diag 308 subcodes in the protected state. "As we now have access to the protection state of the cpus, we can implement special handling of diag 308 subcodes for cpus in the protected state." ? > > For subcodes 0 and 1 we need to unshare all pages before continuing, > so the guest doesn't accidently expose data when dumping. > > For subcode 3/4 we tear down the protected VM and reboot into > unprotected mode. We do not provide a secure reboot. > > Before we can do the unshare calls, we need to mark all cpus as > stopped. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++++++++--- > target/s390x/diag.c | 4 ++++ > 2 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 7262453616..6fd50b4c42 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -319,11 +319,26 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > } > > +static void s390_pv_prepare_reset(CPUS390XState *env) > +{ > + CPUState *cs; > + > + if (!env->pv) { > + return; > + } > + CPU_FOREACH(cs) { > + s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs)); > + } > + s390_pv_unshare(); > + s390_pv_perf_clear_reset(); > +} > + > static void s390_machine_reset(MachineState *machine) > { > enum s390_reset reset_type; > CPUState *cs, *t; > S390CPU *cpu; > + CPUS390XState *env; > > /* get the reset parameters, reset them once done */ > s390_ipl_get_reset_request(&cs, &reset_type); > @@ -332,29 +347,38 @@ static void s390_machine_reset(MachineState *machine) > s390_cmma_reset(); > > cpu = S390_CPU(cs); > + env = &cpu->env; > > switch (reset_type) { > case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */ > + subsystem_reset(); > + s390_crypto_reset(); > + s390_pv_prepare_reset(env); > CPU_FOREACH(t) { > run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL); > } > - subsystem_reset(); > - s390_crypto_reset(); This also switches the order in which different parts are reset for normal guests. I presume that order doesn't matter here? > run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > break; > case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ missing blank before */ (introduced in a previous patch) > + subsystem_reset(); > + s390_pv_prepare_reset(env); > CPU_FOREACH(t) { > if (t == cs) { > continue; > } > run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); > } > - subsystem_reset(); > run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); > run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > break; > case S390_RESET_EXTERNAL: Annotate this with the subcode as well? (in the patch introducing it) > case S390_RESET_REIPL: /* Subcode 4 */ > + if (env->pv) { > + CPU_FOREACH(t) { > + s390_pv_vcpu_destroy(t); > + } > + s390_pv_vm_destroy(); > + } > qemu_devices_reset(); > s390_crypto_reset(); > /* configure and start the ipl CPU only */ > diff --git a/target/s390x/diag.c b/target/s390x/diag.c > index 32049bb4ee..db6d79cef3 100644 > --- a/target/s390x/diag.c > +++ b/target/s390x/diag.c > @@ -68,6 +68,10 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3) > static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr, > uintptr_t ra, bool write) > { > + /* Handled by the Ultravisor */ > + if (env->pv) { > + return 0; > + } > if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) { > s390_program_interrupt(env, PGM_SPECIFICATION, ra); > return -EINVAL;
On 11/21/19 2:50 PM, Cornelia Huck wrote: > On Wed, 20 Nov 2019 06:43:26 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Now that we know the protection state off the cpus, we can start >> handling all diag 308 subcodes in the protected state. > > "As we now have access to the protection state of the cpus, we can > implement special handling of diag 308 subcodes for cpus in the > protected state." > > ? Sure > >> >> For subcodes 0 and 1 we need to unshare all pages before continuing, >> so the guest doesn't accidently expose data when dumping. >> >> For subcode 3/4 we tear down the protected VM and reboot into >> unprotected mode. We do not provide a secure reboot. >> >> Before we can do the unshare calls, we need to mark all cpus as >> stopped. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++++++++--- >> target/s390x/diag.c | 4 ++++ >> 2 files changed, 31 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 7262453616..6fd50b4c42 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -319,11 +319,26 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> } >> >> +static void s390_pv_prepare_reset(CPUS390XState *env) >> +{ >> + CPUState *cs; >> + >> + if (!env->pv) { >> + return; >> + } >> + CPU_FOREACH(cs) { >> + s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs)); >> + } >> + s390_pv_unshare(); >> + s390_pv_perf_clear_reset(); >> +} >> + >> static void s390_machine_reset(MachineState *machine) >> { >> enum s390_reset reset_type; >> CPUState *cs, *t; >> S390CPU *cpu; >> + CPUS390XState *env; >> >> /* get the reset parameters, reset them once done */ >> s390_ipl_get_reset_request(&cs, &reset_type); >> @@ -332,29 +347,38 @@ static void s390_machine_reset(MachineState *machine) >> s390_cmma_reset(); >> >> cpu = S390_CPU(cs); >> + env = &cpu->env; >> >> switch (reset_type) { >> case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */ >> + subsystem_reset(); >> + s390_crypto_reset(); >> + s390_pv_prepare_reset(env); >> CPU_FOREACH(t) { >> run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL); >> } >> - subsystem_reset(); >> - s390_crypto_reset(); > > This also switches the order in which different parts are reset for > normal guests. I presume that order doesn't matter here? I don't think that the order is specified in the POP, it is however specified for protected VMs... > >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >> break; >> case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ > > missing blank before */ (introduced in a previous patch) Will fix > > >> + subsystem_reset(); >> + s390_pv_prepare_reset(env); >> CPU_FOREACH(t) { >> if (t == cs) { >> continue; >> } >> run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); >> } >> - subsystem_reset(); >> run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >> break; >> case S390_RESET_EXTERNAL: > > Annotate this with the subcode as well? (in the patch introducing it) Sure > >> case S390_RESET_REIPL: /* Subcode 4 */ >> + if (env->pv) { >> + CPU_FOREACH(t) { >> + s390_pv_vcpu_destroy(t); >> + } >> + s390_pv_vm_destroy(); >> + } >> qemu_devices_reset(); >> s390_crypto_reset(); >> /* configure and start the ipl CPU only */ >> diff --git a/target/s390x/diag.c b/target/s390x/diag.c >> index 32049bb4ee..db6d79cef3 100644 >> --- a/target/s390x/diag.c >> +++ b/target/s390x/diag.c >> @@ -68,6 +68,10 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3) >> static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr, >> uintptr_t ra, bool write) >> { >> + /* Handled by the Ultravisor */ >> + if (env->pv) { >> + return 0; >> + } >> if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) { >> s390_program_interrupt(env, PGM_SPECIFICATION, ra); >> return -EINVAL; >
On 11/21/19 2:50 PM, Cornelia Huck wrote: > On Wed, 20 Nov 2019 06:43:26 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >> break; >> case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ > > missing blank before */ (introduced in a previous patch) > > >> + subsystem_reset(); >> + s390_pv_prepare_reset(env); >> CPU_FOREACH(t) { >> if (t == cs) { >> continue; >> } >> run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); >> } >> - subsystem_reset(); >> run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >> break; >> case S390_RESET_EXTERNAL: > > Annotate this with the subcode as well? (in the patch introducing it) I think this has no diag 308 subcode and is triggered by qemu
On Thu, 21 Nov 2019 15:04:31 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 11/21/19 2:50 PM, Cornelia Huck wrote: > > On Wed, 20 Nov 2019 06:43:26 -0500 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > > >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > >> break; > >> case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ > > > > missing blank before */ (introduced in a previous patch) > > > > > >> + subsystem_reset(); > >> + s390_pv_prepare_reset(env); > >> CPU_FOREACH(t) { > >> if (t == cs) { > >> continue; > >> } > >> run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); > >> } > >> - subsystem_reset(); > >> run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); > >> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > >> break; > >> case S390_RESET_EXTERNAL: > > > > Annotate this with the subcode as well? (in the patch introducing it) > > I think this has no diag 308 subcode and is triggered by qemu -ENOCOFFE But even more reason to annotate this with the fact that this is triggered by QEMU :)
On 11/21/19 3:17 PM, Cornelia Huck wrote: > On Thu, 21 Nov 2019 15:04:31 +0100 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 11/21/19 2:50 PM, Cornelia Huck wrote: >>> On Wed, 20 Nov 2019 06:43:26 -0500 >>> Janosch Frank <frankja@linux.ibm.com> wrote: >> >>> >>>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >>>> break; >>>> case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ >>> >>> missing blank before */ (introduced in a previous patch) >>> >>> >>>> + subsystem_reset(); >>>> + s390_pv_prepare_reset(env); >>>> CPU_FOREACH(t) { >>>> if (t == cs) { >>>> continue; >>>> } >>>> run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); >>>> } >>>> - subsystem_reset(); >>>> run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); >>>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >>>> break; >>>> case S390_RESET_EXTERNAL: >>> >>> Annotate this with the subcode as well? (in the patch introducing it) >> >> I think this has no diag 308 subcode and is triggered by qemu > > -ENOCOFFE > > But even more reason to annotate this with the fact that this is > triggered by QEMU :) > You're too late with that idea :) I just split out the reordering and the annotation into a new commit: https://github.com/frankjaa/qemu/commit/8c53d5c8a6bbcc53496c7a2877c7cbffc435b708
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 7262453616..6fd50b4c42 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -319,11 +319,26 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); } +static void s390_pv_prepare_reset(CPUS390XState *env) +{ + CPUState *cs; + + if (!env->pv) { + return; + } + CPU_FOREACH(cs) { + s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs)); + } + s390_pv_unshare(); + s390_pv_perf_clear_reset(); +} + static void s390_machine_reset(MachineState *machine) { enum s390_reset reset_type; CPUState *cs, *t; S390CPU *cpu; + CPUS390XState *env; /* get the reset parameters, reset them once done */ s390_ipl_get_reset_request(&cs, &reset_type); @@ -332,29 +347,38 @@ static void s390_machine_reset(MachineState *machine) s390_cmma_reset(); cpu = S390_CPU(cs); + env = &cpu->env; switch (reset_type) { case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */ + subsystem_reset(); + s390_crypto_reset(); + s390_pv_prepare_reset(env); CPU_FOREACH(t) { run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL); } - subsystem_reset(); - s390_crypto_reset(); run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); break; case S390_RESET_LOAD_NORMAL: /* Subcode 1*/ + subsystem_reset(); + s390_pv_prepare_reset(env); CPU_FOREACH(t) { if (t == cs) { continue; } run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); } - subsystem_reset(); run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL); run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); break; case S390_RESET_EXTERNAL: case S390_RESET_REIPL: /* Subcode 4 */ + if (env->pv) { + CPU_FOREACH(t) { + s390_pv_vcpu_destroy(t); + } + s390_pv_vm_destroy(); + } qemu_devices_reset(); s390_crypto_reset(); /* configure and start the ipl CPU only */ diff --git a/target/s390x/diag.c b/target/s390x/diag.c index 32049bb4ee..db6d79cef3 100644 --- a/target/s390x/diag.c +++ b/target/s390x/diag.c @@ -68,6 +68,10 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3) static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr, uintptr_t ra, bool write) { + /* Handled by the Ultravisor */ + if (env->pv) { + return 0; + } if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) { s390_program_interrupt(env, PGM_SPECIFICATION, ra); return -EINVAL;
Now that we know the protection state off the cpus, we can start handling all diag 308 subcodes in the protected state. For subcodes 0 and 1 we need to unshare all pages before continuing, so the guest doesn't accidently expose data when dumping. For subcode 3/4 we tear down the protected VM and reboot into unprotected mode. We do not provide a secure reboot. Before we can do the unshare calls, we need to mark all cpus as stopped. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++++++++--- target/s390x/diag.c | 4 ++++ 2 files changed, 31 insertions(+), 3 deletions(-)