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
