Re: [PATCH 07/15] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4

[Cornelia Huck]

On Wed, 20 Nov 2019 06:43:26 -0500
Janosch Frank <address@hidden> 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 <address@hidden>
> ---
>  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;