Re: [qemu-s390x] [PATCH v3 16/16] s390-bios: dasd-ipl: Use control unit type to customize error data

[Jason J. Herne]

On 3/4/19 12:02 PM, Eric Farman wrote:
> On 03/01/2019 01:59 PM, Jason J. Herne wrote:
> > Propagate control unit type from main through the dasd ipl call chain.
> > The control unit type can be used to determine if we are attempting to
> > boot from a real dasd device. If something goes wrong we'll want to print
> > detailed dasd sense data (for diagnostic use) but only if we're attempting
> > to boot from a real dasd device.
> >
> > Note: We also query and print the dasd sense data if we fail while
> > attempting to determine the control unit type. In this case, we don't know
> > if we're dealing with a real dasd device yet, but if we are, then the sense
> > data may be useful for figuring out what went wrong. Since determining
> > the control unit type is the very first thing we do with any real dasd device,
> > this is our most likely point of failure.
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
> > ---
> >   pc-bios/s390-ccw/cio.c      | 16 ++++++++++------
> >   pc-bios/s390-ccw/cio.h      |  6 ++++--
> >   pc-bios/s390-ccw/dasd-ipl.c | 25 +++++++++++++------------
> >   pc-bios/s390-ccw/dasd-ipl.h |  2 +-
> >   pc-bios/s390-ccw/main.c     |  2 +-
> >   pc-bios/s390-ccw/virtio.c   |  2 +-
> >   6 files changed, 30 insertions(+), 23 deletions(-)
> >
> > diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> > index c528bbf..593fb33 100644
> > --- a/pc-bios/s390-ccw/cio.c
> > +++ b/pc-bios/s390-ccw/cio.c
> > @@ -54,14 +54,15 @@ uint16_t cu_type(SubChannelId schid)
> >       sense_id_ccw.count = sizeof(sense_data);
> >       sense_id_ccw.flags |= CCW_FLAG_SLI;
> > -    if (do_cio(schid, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
> > +    if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
> >           panic("Failed to run SenseID CCw\n");
> >       }
> >       return sense_data.cu_type;
> >   }
> > -void basic_sense(SubChannelId schid, void *sense_data, uint16_t data_size)
> > +void basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
> > +                 uint16_t data_size)
> >   {
> >       Ccw1 senseCcw;
> > @@ -69,7 +70,7 @@ void basic_sense(SubChannelId schid, void *sense_data, uint16_t 
> > data_size)
> >       senseCcw.cda = ptr2u32(sense_data);
> >       senseCcw.count = data_size;
> > -    if (do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1)) {
> > +    if (do_cio(schid, cutype, ptr2u32(&senseCcw), CCW_FMT1)) {
> >           panic("Failed to run Basic Sense CCW\n");
> >       }
> >   }
> > @@ -364,7 +365,7 @@ static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, 
> > Irb *irb)
> >    *
> >    * Returns non-zero on error.
> >    */
> > -int do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt)
> > +int do_cio(SubChannelId schid, uint16_t cutype, uint32_t ccw_addr, int fmt)
> >   {
> >       Irb irb = {};
> >       SenseDataEckdDasd sd;
>
> Missed this one?  :)
This field is used in two places. 1) For an ECKD Dasd only path. 2) In an error recovery 
path where we need to issue a basic sense to clear status but we never examine the data. 
So I guess having the struct named "SenseDataEckdDasd" and using it in a potentially 
generic path *might* be slightly misleading, it seems superior to allocating a new chunk 
of memory we'll never use. I assume this is the issue you are hinting at?

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