Re: [PATCH v3 05/17] s390x: protvirt: Support unpack facility

[Cornelia Huck]

On Fri, 14 Feb 2020 10:16:24 -0500
Janosch Frank <address@hidden> wrote:

> When a guest has saved a ipib of type 5 and call diagnose308 with

s/call/calls/

> subcode 10, we have to setup the protected processing environment via
> Ultravisor calls. The calls are done by KVM and are exposed via an API.
> 
> The following steps are necessary:
> 1. Create a VM (register it with the Ultravisor)
> 2. Create secure CPUs for all of our current cpus
> 3. Forward the secure header to the Ultravisor (has all information on
> how to decrypt the image and VM information)
> 4. Protect image pages from the host and decrypt them
> 5. Verify the image integrity
> 
> Only after step 5 a protected VM is allowed to run.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden> [Changes
> to machine]
> ---
>  hw/s390x/Makefile.objs              |   1 +
>  hw/s390x/ipl.c                      |  32 ++++++
>  hw/s390x/ipl.h                      |   2 +
>  hw/s390x/pv.c                       | 154 ++++++++++++++++++++++++++++
>  hw/s390x/pv.h                       |  38 +++++++
>  hw/s390x/s390-virtio-ccw.c          |  79 ++++++++++++++
>  include/hw/s390x/s390-virtio-ccw.h  |   1 +
>  target/s390x/cpu.c                  |   4 +
>  target/s390x/cpu.h                  |   1 +
>  target/s390x/cpu_features_def.inc.h |   1 +
>  10 files changed, 313 insertions(+)
>  create mode 100644 hw/s390x/pv.c
>  create mode 100644 hw/s390x/pv.h

(...)

> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> new file mode 100644
> index 0000000000..5b6a26cba9
> --- /dev/null
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,154 @@
> +/*
> + * Secure execution functions
> + *
> + * Copyright IBM Corp. 2019

Update the year?

> + * 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.
> + */

(...)

> +void s390_pv_vm_destroy(void)
> +{
> +     s390_pv_cmd_exit(KVM_PV_VM_DESTROY, NULL);

Why does this exit()? Should Never Happen?

> +}
> +
> +int s390_pv_vcpu_create(CPUState *cs)
> +{
> +    int rc;
> +
> +    rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
> +    if (!rc) {
> +        S390_CPU(cs)->env.pv = true;
> +    }
> +
> +    return rc;
> +}
> +
> +void s390_pv_vcpu_destroy(CPUState *cs)
> +{
> +    s390_pv_cmd_vcpu_exit(cs, KVM_PV_VCPU_DESTROY, NULL);

dito

> +    S390_CPU(cs)->env.pv = false;
> +}

(...)

> +void s390_pv_perf_clear_reset(void)
> +{
> +    s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);

And here. Or is that because the machine should not be left around in
an undefined state?

> +}
> +
> +int s390_pv_verify(void)
> +{
> +    return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
> +}
> +
> +void s390_pv_unshare(void)
> +{
> +    s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
> +}
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> new file mode 100644
> index 0000000000..7d20bdd12e
> --- /dev/null
> +++ b/hw/s390x/pv.h
> @@ -0,0 +1,38 @@
> +/*
> + * Protected Virtualization header
> + *
> + * Copyright IBM Corp. 2019

Year++

> + * 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.
> + */
> +
> +#ifndef HW_S390_PV_H
> +#define HW_S390_PV_H
> +
> +#ifdef CONFIG_KVM
> +int s390_pv_vm_create(void);
> +void s390_pv_vm_destroy(void);
> +void s390_pv_vcpu_destroy(CPUState *cs);
> +int s390_pv_vcpu_create(CPUState *cs);
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
> +void s390_pv_perf_clear_reset(void);
> +int s390_pv_verify(void);
> +void s390_pv_unshare(void);
> +#else
> +int s390_pv_vm_create(void) { return 0; }

I'm wondering why you return 0 here (and below). These function should
not be called for !KVM, but just to help catch logic error, use -EINVAL
or so?

> +void s390_pv_vm_destroy(void) {}
> +void s390_pv_vcpu_destroy(CPUState *cs) {}
> +int s390_pv_vcpu_create(CPUState *cs) { return 0; }
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0: 
> }
> +void s390_pv_perf_clear_reset(void) {}
> +int s390_pv_verify(void) { return 0; }
> +void s390_pv_unshare(void) {}
> +#endif
> +
> +#endif /* HW_S390_PV_H */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e759eb5f83..5fa4372083 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/s390x/pv.h"
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -240,9 +241,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;

I'm wondering why you need to init this to false - isn't it already
zeroed out?

>      s390_sclp_init();
>      /* init memory + setup max page size. Required for the CPU model */
>      s390_memory_init(machine->ram_size);
> @@ -318,10 +321,58 @@ static inline void s390_do_cpu_ipl(CPUState *cs, 
> run_on_cpu_data arg)
>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
>  
> +static int s390_machine_pv_secure(S390CcwMachineState *ms)
> +{
> +    CPUState *t;
> +    int rc;
> +
> +    /* Create SE VM */
> +    rc = s390_pv_vm_create();
> +    if (rc) {
> +        return rc;
> +    }
> +
> +    CPU_FOREACH(t) {
> +        rc = s390_pv_vcpu_create(t);
> +        if (rc) {
> +            return rc;

No need to undo something on error?

> +        }
> +    }
> +
> +    ms->pv = true;
> +
> +    /* Set SE header and unpack */
> +    rc = s390_ipl_prepare_pv_header();
> +    if (rc) {
> +        return rc;

Also here.

> +    }
> +
> +    /* Decrypt image */
> +    rc = s390_ipl_pv_unpack();
> +    if (rc) {
> +        return rc;

And here.

> +    }
> +
> +    /* Verify integrity */
> +    rc = s390_pv_verify();
> +    return rc;

And here.

> +}

(...)

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8da1905485..1dbd84b9d7 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -37,6 +37,8 @@
>  #include "sysemu/hw_accel.h"
>  #include "hw/qdev-properties.h"
>  #ifndef CONFIG_USER_ONLY
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/pv.h"
>  #include "hw/boards.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
> @@ -191,6 +193,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  
>  #if !defined(CONFIG_USER_ONLY)
>      MachineState *ms = MACHINE(qdev_get_machine());
> +    S390CcwMachineState *ccw = S390_CCW_MACHINE(ms);

I find the variable name a bit confusing... maybe ccw_ms?

>      unsigned int max_cpus = ms->smp.max_cpus;
>      if (cpu->env.core_id >= max_cpus) {
>          error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
> @@ -205,6 +208,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          goto out;
>      }
>  
> +    cpu->env.pv = ccw->pv;

So, if you add a cpu, it will inherit the pv state of the machine...
doesn't it need any setup?

>      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
>      cs->cpu_index = cpu->env.core_id;
>  #endif

(...)