On 3/5/20 3:15 PM, David Hildenbrand wrote: >>>> >>>> +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,
I fixed it this morning :) > >> >>> >>> 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. Thanks I'll try to find a better split up of the code > > [...] > >>>> + 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? > Because a normal cpu reset does not clear out the registers.
signature.asc
Description: OpenPGP digital signature