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 <fran...@linux.ibm.com> > Co-developed-by: Christian Borntraeger <borntrae...@de.ibm.com> [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 <fran...@linux.ibm.com> > + * > + * 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