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

[Cornelia Huck <cohuck@redhat.com>]

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 :)