>>> >>> +static void s390_machine_unprotect(S390CcwMachineState *ms) >>> +{ >>> + CPUState *t; >>> + >>> + if (!ms->pv) >>> + return; >> >> How can this ever happen? g_assert(ms->pv) ? > > Currently not, that's only used in the follow up patches with the ballon > and migration blocker
Even then, why should we unprotect when not protected. That looks wrong to me and has to be fixed in the other patches, > >> >> Also, i don't see this function getting called from anywhere else except >> when s390_machine_protect() fails. That looks wrong. This has to be >> called when going out of PV mode. > > Yes, but that's in the diag308 1-4 patch. The way patches were split up is somewhat sub-optimal for review. [...] >>> + 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); >> >> Ah, so it's not an actual exception. BUT: All other guest cpus were >> reset, can the guest deal with that? > > Well, all other CPUs should be stopped for diag308, no? > Also it's done by the bootloader and not a OS which just stops its cpus > and goes into protected mode. > >> >> (run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL) should go after the >> s390_machine_protect() I assume - the change you had in the other patch) > > That's not a good idea, I want to reset before we automatically call the > UV routines on a reset. But how can s390_machine_inject_pv_error(cs) be any effective if you already reset the CPU? -- Thanks, David / dhildenb