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?
--
Thanks,
David / dhildenb
