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

[Janosch Frank]

On 3/5/20 3:23 PM, David Hildenbrand wrote:
> On 05.03.20 15:20, Janosch Frank wrote:
>> 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.
> 
> Okay, so a guest (e.g., Linux) can deal with some other things getting
> reset I assume?

At this point in time only the bootloader runs, which survives a normal
reset.

signature.asc
Description: OpenPGP digital signature