Re: [PATCH v8 2/2] s390x: protvirt: Support unpack facility

[David Hildenbrand]

On 10.03.20 09:32, Janosch Frank wrote:
> The unpack facility provides the means to setup a protected guest. A
> protected guest can not be introspected by the hypervisor or any
> user/administrator of the machine it is running on.
> 
> Protected guests are encrypted at rest and need a special boot
> mechanism via diag308 subcode 8 and 10.
> 
> Code 8 sets the PV specific IPLB which is retained seperately from
> those set via code 5.
> 
> Code 10 is used to unpack the VM into protected memory, verify its
> integrity and start it.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> Co-developed-by: Christian Borntraeger <address@hidden> [Changes
> to machine]

[...]


> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> new file mode 100644
> index 0000000000..ba6409246e
> --- /dev/null
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,104 @@
> +/*
> + * Secure execution functions

Protected virtualization ;)

> + *
> + * Copyright IBM Corp. 2020
> + * Author(s):
> + *  Janosch Frank <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */

[...]

>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -238,9 +240,11 @@ static void s390_create_sclpconsole(const char *type, 
> Chardev *chardev)
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>      VirtualCssBus *css_bus;
>      DeviceState *dev;
>  
> +    ms->pv = false;

As discussed, not needed.

>      s390_sclp_init();
>      /* init memory + setup max page size. Required for the CPU model */
>      s390_memory_init(machine->ram);
> @@ -316,10 +320,88 @@ 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_machine_unprotect(S390CcwMachineState *ms)
> +{
> +    CPUState *t;
> +
> +    s390_pv_vm_disable();
> +    CPU_FOREACH(t) {
> +        S390_CPU(t)->env.pv = false;
> +    }

See below, would be great if we can get rid of env.pv IMHO.

[...]

> +    case S390_RESET_PV: /* Subcode 10 */
> +        subsystem_reset();
> +        s390_crypto_reset();
> +
> +        CPU_FOREACH(t) {
> +            if (t == cs) {
> +                continue;
> +            }
> +            run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
> +        }
> +        run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
> +
> +        if (s390_machine_protect(ms)) {
> +            s390_machine_inject_pv_error(cs);
> +            /*
> +             * Continue after the diag308 so the guest knows something
> +             * went wrong.
> +             */
> +            s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> +            return;

Didn't you want to squash in that hunk from the other patch? (I remember
seeing a goto)

> +        }
> +
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
>      default:

[...]

>  
> +#if !defined(CONFIG_USER_ONLY)
> +static bool machine_is_pv(MachineState *ms)
> +{
> +    static S390CcwMachineState *ccw;
> +    Object *obj;
> +
> +    if (ccw)
> +         return ccw->pv;

missing {}

> +
> +    /* we have to bail out for the "none" machine */
> +    obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
> +     if (!obj) {
> +        return false;
> +    }
> +    ccw = S390_CCW_MACHINE(obj);
> +    return ccw->pv;
> +}
> +#endif

Now that we talked about cached values, what about

#if !defined(CONFIG_USER_ONLY)
static bool s390_is_pv(void)
{
    static S390CcwMachineState *ccw;
    Object *obj;

    if (ccw) {
        return ccw->pv;
    }

    /* we have to bail out for the "none" machine */
    obj = object_dynamic_cast(qdev_get_machine(),
                              TYPE_S390_CCW_MACHINE);
    if (!obj) {
        return false;
    }
    ccw = S390_CCW_MACHINE(obj);
    return ccw->pv;
}
#endif

and drop all env->pv checks, replacing them by s390_is_pv(). (sorry,
should have recommended that earlier)

> +
>  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -205,6 +226,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          goto out;
>      }
>  
> +    cpu->env.pv = machine_is_pv(ms);
>      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
>      cs->cpu_index = cpu->env.core_id;
>  #endif
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 1d17709d6e..7e4d9d267c 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -114,6 +114,7 @@ struct CPUS390XState {
>  
>      /* Fields up to this point are cleared by a CPU reset */
>      struct {} end_reset_fields;
> +    bool pv; /* protected virtualization */
>  
>  #if !defined(CONFIG_USER_ONLY)
>      uint32_t core_id; /* PoP "CPU address", same as cpu_index */
> diff --git a/target/s390x/cpu_features_def.inc.h 
> b/target/s390x/cpu_features_def.inc.h
> index 31dff0d84e..60db28351d 100644
> --- a/target/s390x/cpu_features_def.inc.h
> +++ b/target/s390x/cpu_features_def.inc.h
> @@ -107,6 +107,7 @@ DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, 
> "Deflate-conversion facility (
>  DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxpdeh", STFL, 152, 
> "Vector-Packed-Decimal-Enhancement Facility")
>  DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155, 
> "Message-security-assist-extension-9 facility (excluding subfunctions)")
>  DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
> +DEF_FEAT(UNPACK, "unpack", STFL, 161, "Unpack facility")

Random thought: The naming of that facility could have been improved to
something that gives users/readers an idea what it's actually doing.


[...]

> @@ -128,17 +142,31 @@ out:
>          g_free(iplb);
>          return;
>      case DIAG308_STORE:
> +    case DIAG308_PV_STORE:
>          if (diag308_parm_check(env, r1, addr, ra, true)) {
>              return;
>          }
> -        iplb = s390_ipl_get_iplb();
> +        if (subcode == DIAG308_PV_STORE) {
> +            iplb = s390_ipl_get_iplb_pv();
> +        } else {
> +            iplb = s390_ipl_get_iplb();
> +        }
>          if (iplb) {
>              cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>              env->regs[r1 + 1] = DIAG_308_RC_OK;
>          } else {
>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>          }
> -        return;
> +        break;

return->break is unrelated, but we do have a wild mixture already.

> +    case DIAG308_PV_START:
> +        iplb = s390_ipl_get_iplb_pv();
> +        if (!iplb) {
> +            env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
> +            return;
> +        }
> +
> +        s390_ipl_reset_request(cs, S390_RESET_PV);
> +        break;
>      default:
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          break;
> 


-- 
Thanks,

David / dhildenb