Re: [Qemu-devel] [PATCH 10/15] s390-bios: Support for running format-0/1 channel programs

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 31 Jan 2019 12:31:00 -0500
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 01/29/2019 08:29 AM, Jason J. Herne wrote:
> > Add struct for format-0 ccws. Support executing format-0 channel
> > programs and waiting for their completion before continuing execution.
> > This will be used for real dasd ipl.
> > 
> > Add cu_type() to channel io library. This will be used to query control
> > unit type which is used to determine if we are booting a virtio device or a
> > real dasd device.
> > 
> > Signed-off-by: Jason J. Herne<jjherne@linux.ibm.com>
> > ---
> >   pc-bios/s390-ccw/cio.c      | 114 +++++++++++++++++++++++++++++++++++++++
> >   pc-bios/s390-ccw/cio.h      | 127 ++++++++++++++++++++++++++++++++++++++++++--
> >   pc-bios/s390-ccw/s390-ccw.h |   1 +
> >   pc-bios/s390-ccw/start.S    |  33 +++++++++++-
> >   4 files changed, 270 insertions(+), 5 deletions(-)

> > +/*
> > + * Executes a channel program at a given subchannel. The request to run the
> > + * channel program is sent to the subchannel, we then wait for the interrupt
> > + * signaling completion of the I/O operation(s) performed by the channel
> > + * program. Lastly we verify that the i/o operation completed without error and
> > + * that the interrupt we received was for the subchannel used to run the
> > + * channel program.
> > + *
> > + * Note: This function assumes it is running in an environment where no other
> > + * cpus are generating or receiving I/O interrupts. So either run it in a
> > + * single-cpu environment or make sure all other cpus are not doing I/O and
> > + * have I/O interrupts masked off.
> > + */
> > +int do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt)
> > +{
> > +    CmdOrb orb = {};
> > +    Irb irb = {};
> > +    sense_data_eckd_dasd sd;
> > +    int rc, retries = 0;
> > +
> > +    IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
> > +
> > +    /* ccw_addr must be <= 24 bits and point to at least one whole ccw. */
> > +    if (fmt == 0) {
> > +        IPL_assert(ccw_addr <= 0xFFFFFF - 8, "Invalid ccw address");
> > +    }
> > +
> > +    orb.fmt = fmt ;
> > +    orb.pfch = 1;  /* QEMU's cio implementation requires prefetch */
> > +    orb.c64 = 1;   /* QEMU's cio implementation requires 64-bit idaws */
> > +    orb.lpm = 0xFF; /* All paths allowed */
> > +    orb.cpa = ccw_addr;
> > +
> > +    while (true) {
> > +        rc = ssch(schid, &orb);
> > +        if (rc == 1) {
> > +            /* Status pending, not sure why. Let's eat the status and retry. */
> > +            tsch(schid, &irb);
> > +            retries++;
> > +            continue;
> > +        }
> > +        if (rc) {
> > +            print_int("ssch failed with rc=", rc);
> > +            break;
> > +        }
> > +
> > +        consume_io_int();
> > +
> > +        /* collect status */
> > +        rc = tsch(schid, &irb);
> > +        if (rc) {
> > +            print_int("tsch failed with rc=", rc);
> > +            break;
> > +        }
> > +
> > +        if (!irb_error(&irb)) {
> > +            break;
> > +        }
> > +
> > +        /*
> > +         * Unexpected unit check, or interface-control-check. Use sense to
> > +         * clear unit check then retry.
> > +         */
> > +        if ((unit_check(&irb) || iface_ctrl_check(&irb)) && retries <= 2) {
> > +            basic_sense(schid, &sd, sizeof(sd));  
> 
> We are using basic sense to clear any unit check or ifcc, but is it 
> possible for the basic sense to cause another unit check?
> 
> The chapter on Basic Sense in the Common I/O Device Commands 
> (http://publibz.boulder.ibm.com/support/libraryserver/FRAMESET/dz9ar501/2.1?SHELF=&DT=19920409154647&CASE=) 
>   says this:
> 
> ""
> The basic sense command initiates a sense operation  at  all  devices 
> and cannot  cause  the  command-reject,  intervention-required, 
> data-check, or overrun bit to be set to one.  If the control unit 
> detects  an  equipment malfunction  or  invalid  checking-block  code 
> (CBC) on the sense-command code, the equipment-check or bus-out-check 
> bit is set  to  one,  and  unit check is indicated in the device-status 
> byte.
> ""
> 
> If my understanding is correct, if there is an equipment malfunction 
> then the control unit can return a unit check even for a basic sense. 
> This can lead to infinite recursion in the bios.

I think the retries variable is supposed to take care of that.

What I don't understand is why we do the basic sense after an IFCC?
Wouldn't it make more sense to simply retry the original command in
that case?

> 
> 
> 
> > +            retries++;
> > +            continue;
> > +        }
> > +
> > +        break;
> > +    }
> > +
> > +    return rc;
> > +}  
>