Re: [PATCH 4/4] s390x: Beautify machine reset

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2956 bytes --]

On 11/22/19 1:25 PM, David Hildenbrand wrote:
> On 22.11.19 13:22, Janosch Frank wrote:
>> On 11/22/19 1:10 PM, Cornelia Huck wrote:
>>> On Fri, 22 Nov 2019 12:47:44 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 22.11.19 12:46, Janosch Frank wrote:
>>>>> On 11/22/19 11:59 AM, David Hildenbrand wrote:
>>>>>> On 22.11.19 08:52, Janosch Frank wrote:
>>>>>>> * Add comments that tell you which diag308 subcode caused the reset
>>>>>>> * Sort by diag308 reset subcode
>>>>>>>
>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>> ---
>>>>>>>     hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>>>>>>     1 file changed, 10 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>>> index c1d1440272..88f7758721 100644
>>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>>>>         s390_cmma_reset();
>>>>>>>     
>>>>>>>         switch (reset_type) {
>>>>>>> -    case S390_RESET_EXTERNAL:
>>>>>>> -    case S390_RESET_REIPL:
>>>>>>> -        qemu_devices_reset();
>>>>>>> -        s390_crypto_reset();
>>>>>>> -
>>>>>>> -        /* configure and start the ipl CPU only */
>>>>>>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>>>>>> -        break;
>>>>>>> -    case S390_RESET_MODIFIED_CLEAR:
>>>>>>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
>>>>>>
>>>>>> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
>>>>>>
>>>>>> I'd suggest to just document the subcodes along with the definitions, if
>>>>>> really needed, and drop this patch, at least I don't quite see the value
>>>>>> of moving code around here... or is the code shuffling of any value on
>>>>>> your prot virt patches?
>>>>>>   
>>>>>
>>>>> It keeps me from consulting the POP every time I need to change things
>>>>> in the machine resets. This is basically a 1:1 mapping of diag 308
>>>>> subcodes to machine resets, so why don't we want to make that obvious
>>>>> and order them by the subcodes?
>>>>>    
>>>>
>>>> Because it is not a 1:1 mapping: S390_RESET_EXTERNAL
>>>>
>>>
>>> Tack the explanation onto the definitions of S390_RESET_, then?
>>> Probably still quicker than consulting the POP :)
>>>
>>
>> Does it really bother you that much, that I add some explanations to the
>> things we're doing. The external reset also gets a comment so Conni
>> won't need that much coffee anymore to understand the code :-)
>>
> 
> I'm really sorry, but I fail to see how "Subcode 0" is *any* better than 
> S390_RESET_MODIFIED_CLEAR (and avoids consulting the PoP) and why the 
> order should matter at all here to make it easier to understand.
> 
> I don't NACK this, I just find it *completely* useless :)
> 

Ugh, time for some rebase conflicts...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]