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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 29 Jan 2019 08:29:18 -0500
"Jason J. Herne" <jjherne@linux.ibm.com> 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:
> 
> vfio-ccw device I/O error - 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=: 0x0000000001e67098
>     prev_ccw=: 0x0000000000000000
>     this_ccw=: 0x0000000000000000
> 
> Sense Data (fmt 32-bytes):
>     Sense Condition Flags : [Equipment-Check]
>     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

Looks cool.

> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> 
> # Conflicts:
> #	pc-bios/s390-ccw/cio.c

You probably don't want to keep that :)

> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/cio.c  | 225 ++++++++++++++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/libc.h |  11 +++
>  2 files changed, 236 insertions(+)
> 

(...)

> +static void print_irb_err(Irb *irb)
> +{
> +    Ccw0 *this_ccw = u32toptr(irb->scsw.cpa);
> +    Ccw0 *prev_ccw = u32toptr(irb->scsw.cpa - 8);

I don't think you can cast this conditionally to format 0 -- I'd pass
in the format from do_cio and handle it accordingly.

> +    char msgline[256];
> +
> +    sclp_print("vfio-ccw device I/O error - Interrupt Response Block Data:\n");

If you call this from the generic function, you shouldn't talk about
vfio-ccw here; but it might make sense to print subchannel/devno and
the cu type.

(...)

> @@ -148,6 +370,9 @@ int do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt)
>              continue;
>          }
>  
> +        print_irb_err(&irb);
> +        basic_sense(schid, &sd, sizeof(sd));
> +        print_eckd_dasd_sense_data(&sd);

I think this should only be printed for actual dasds (and maybe only
print it if there is actually sense data available)?

>          break;
>      }
>