Re: [PATCH v6 03/18] s390x: protvirt: Support unpack facility

[David Hildenbrand]

>>>  
>>> +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