>> >>> + CPUS390XState *env; >>> >>> /* get the reset parameters, reset them once done */ >>> s390_ipl_get_reset_request(&cs, &reset_type); >>> @@ -327,9 +411,16 @@ static void s390_machine_reset(MachineState *machine) >>> /* all CPUs are paused and synchronized at this point */ >>> s390_cmma_reset(); >>> >>> + cpu = S390_CPU(cs); >>> + env = &cpu->env; >> >> Can you just pass "cpu" to s390_pv_prepare_reset() and handle it in there? > > Wouldn't it make more sense to test the machine state here anyway?
Whatever you prefer. Just saying, that introducing "CPUS390XState *env" in this function can be avoided. > >> >>> + >>> switch (reset_type) { >>> case S390_RESET_EXTERNAL: >>> case S390_RESET_REIPL: >>> + if (ms->pv) { >>> + s390_machine_unprotect(ms); >>> + } >>> + >>> qemu_devices_reset(); >>> s390_crypto_reset(); >>> >>> @@ -337,22 +428,52 @@ static void s390_machine_reset(MachineState *machine) >>> run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL); >>> break; >>> case S390_RESET_MODIFIED_CLEAR: >>> + /* >>> + * Susbsystem reset needs to be done before we unshare memory >>> + * and loose access to VIRTIO structures in guest memory. >>> + */ >>> + 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(); >>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >>> break; >>> case S390_RESET_LOAD_NORMAL: >>> + /* >>> + * Susbsystem reset needs to be done before we unshare memory >>> + * and loose access to VIRTIO structures in guest memory. >>> + */ >>> + 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_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); >>> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >>> + return; >>> + } >>> + >>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); >> >> [...] >> >>> >>> +#if !defined(CONFIG_USER_ONLY) >>> +static bool machine_is_pv(MachineState *ms) >>> +{ >>> + Object *obj; >>> + >>> + /* we have to bail out for the "none" machine */ >>> + obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE); >>> + if (!obj) { >>> + return false; >>> + } >>> + return S390_CCW_MACHINE(obj)->pv; >> >> Maybe you want to cache the machine, so you can avoid the >> lookup+conversion on every new CPU. > > Christian has provided me with a fix to this code, I'll squash it. > >> >>> +} >>> +#endif >> >> [...] >>> 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 -1; >>> @@ -93,6 +101,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, >>> uint64_t r3, uintptr_t ra) >>> return; >>> } >>> >>> + if (subcode > 7 && !s390_has_feat(S390_FEAT_UNPACK)) { >> >>> = DIAG308_PV_SET > > ? Instead of the magic number seven, check against "subcode >= DIAG308_PV_SET" -- Thanks, David / dhildenb