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

[Janosch Frank]

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