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

From: David Hildenbrand <david@redhat.com>
To: Janosch Frank <frankja@linux.ibm.com>, qemu-devel@nongnu.org
Cc: borntraeger@de.ibm.com, qemu-s390x@nongnu.org, cohuck@redhat.com
Subject: Re: [PATCH v6 03/18] s390x: protvirt: Support unpack facility
Date: Thu, 5 Mar 2020 15:23:01 +0100	[thread overview]
Message-ID: <5afa8775-5382-f128-7bfd-39643a02c454@redhat.com> (raw)
In-Reply-To: <91a4bd8b-6c88-a9d8-42a4-154df21a0c08@linux.ibm.com>

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