On 2/20/20 11:39 AM, Cornelia Huck wrote: > On Fri, 14 Feb 2020 10:16:24 -0500 > Janosch Frank 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 >> Signed-off-by: Christian Borntraeger [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? ack. > >> + * Author(s): >> + * Janosch Frank >> + * >> + * 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? Yes, and we can't recover from this. > >> +} >> + >> +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? If it failed, we could only try again, there's no fixing the problem. So I chose to rather exit instead of looping around something which most likely will never recover after the first error. > >> +} >> + >> +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 >> + * >> + * 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? There have been changes in this area anyway, since Christian switched to one create/destroy instead of separate for vm and vcpu. I'll update the error handling in the new state and send out the patches ssonish. > >> + } >> + } >> + >> + 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 > > (...) >