Re: [Qemu-devel] [PATCH v3 11/16] s390-bios: cio error handling

[Jason J. Herne]

On 3/4/19 1:35 PM, Cornelia Huck wrote:
> On Fri,  1 Mar 2019 13:59:31 -0500
> "Jason J. Herne" <address@hidden> wrote:
>
> > Add verbose error output for when unexpected i/o errors happen. This eases the
> > burden of debugging and reporting i/o errors. No error information is printed
> > in the success case, here is an example of what is output on error:
> >
> > cio device error
> >    ssid  : 0x0000000000000000
> >    cssid : 0x0000000000000000
> >    sch_no: 0x0000000000000000
> >
> > Interrupt Response Block Data:
> >      Function Ctrl : [Start]
> >      Activity Ctrl : [Start-Pending]
> >      Status Ctrl : [Alert] [Primary] [Secondary] [Status-Pending]
> >      Device Status : [Unit-Check]
> >      Channel Status :
> >      cpa=: 0x000000007f8d6038
> >      prev_ccw=: 0x0000000000000000
> >      this_ccw=: 0x0000000000000000
> > Eckd Dasd Sense Data (fmt 32-bytes):
> >      Sense Condition Flags :
> >      Residual Count     =: 0x0000000000000000
> >      Phys Drive ID      =: 0x000000000000009e
> >      low cyl address    =: 0x0000000000000000
> >      head addr & hi cyl =: 0x0000000000000000
> >      format/message     =: 0x0000000000000008
> >      fmt-dependent[0-7] =: 0x0000000000000004
> >      fmt-dependent[8-15]=: 0xe561282305082fff
> >      prog action code   =: 0x0000000000000016
> >      Configuration info =: 0x00000000000040e0
> >      mcode / hi-cyl     =: 0x0000000000000000
> >      cyl & head addr [0]=: 0x0000000000000000
> >      cyl & head addr [1]=: 0x0000000000000000
> >      cyl & head addr [2]=: 0x0000000000000000
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
> > ---
> >   pc-bios/s390-ccw/cio.c  | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   pc-bios/s390-ccw/libc.h |  11 +++
> >   2 files changed, 241 insertions(+)
> >
> > diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> > index e61cfd3..c528bbf 100644
> > --- a/pc-bios/s390-ccw/cio.c
> > +++ b/pc-bios/s390-ccw/cio.c
> > @@ -82,6 +82,228 @@ static bool irb_error(Irb *irb)
> >       return irb->scsw.dstat != (SCSW_DSTAT_DEVEND | SCSW_DSTAT_CHEND);
> >   }
> >   
> > +static void print_eckd_dasd_sense_data(SenseDataEckdDasd *sd)
> > +{
> > +    char msgline[512];
> > +
> > +    if (sd->config_info & 0x8000) {
> > +        sclp_print("Eckd Dasd Sense Data (fmt 24-bytes):\n");
> > +    } else {
> > +        sclp_print("Eckd Dasd Sense Data (fmt 32-bytes):\n");
> > +    }
> > +
> > +    strcat(msgline, "    Sense Condition Flags :");
> > +    if (sd->status[0] & SNS_STAT0_CMD_REJECT) {
> > +        strcat(msgline, " [Cmd-Reject]");
> > +    }
> > +    if (sd->status[0] & SNS_STAT0_INTERVENTION_REQ) {
> > +        strcat(msgline, " [Intervention-Required]");
> > +    }
> > +    if (sd->status[0] & SNS_STAT0_BUS_OUT_CHECK) {
> > +        strcat(msgline, " [Bus-Out-Parity-Check]");
> > +    }
> > +    if (sd->status[0] & SNS_STAT0_EQUIPMENT_CHECK) {
> > +        strcat(msgline, " [Equipment-Check]");
> > +    }
> > +    if (sd->status[0] & SNS_STAT0_DATA_CHECK) {
> > +        strcat(msgline, " [Data-Check]");
>
> I'm wondering whether it would make sense to factor the common bits
> out. Might be overkill, though.

I'm not huge fan of this error code myself. It looks nice in output and may be useful for 
debugging which is why I decided to submit it. I did think about ways to make it look 
cleaner but didn't come up with anything great. I'd be open to suggestions, but I also 
think going too deep is overkill.

...
> Maybe do basic_sense + print sense data only if there's actually a unit
> check?
>
> (Also, I'm not sure if you can even do a basic_sense in case e.g. of
> unexpected busy.)
>
> > +        basic_sense(schid, &sd, sizeof(sd));
> > +        print_eckd_dasd_sense_data(&sd);
> >           rc = -1;
> >           break;
> >       }

I've made a change that should address both issues. Preview:

        if (cutype == CU_TYPE_DASD_3990 || cutype == CU_TYPE_UNKNOWN) {
            if (!basic_sense(schid, cutype, &sd, sizeof(sd))) {
                print_eckd_dasd_sense_data(&sd);
            }
        }

Now we only print sense data if basic sense works. So if it fails because of a busy status 
(or any reason) we won't try to print.

--
-- Jason J. Herne (address@hidden)