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

[Janosch Frank]

On 11/22/19 1:10 PM, Cornelia Huck wrote:
> On Fri, 22 Nov 2019 12:47:44 +0100
> David Hildenbrand <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>    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 :-)

signature.asc
Description: OpenPGP digital signature