Re: [qemu-s390x] [PATCH v3 10/16] s390-bios: Support for running format-0/1 channel programs

[Jason J. Herne]

On 3/4/19 1:25 PM, Cornelia Huck wrote:
> On Fri,  1 Mar 2019 13:59:30 -0500
> "Jason J. Herne" <address@hidden> wrote:
>
> > Add struct for format-0 ccws. Support executing format-0 channel
> > programs and waiting for their completion before continuing execution.
>
> That sentence is a bit confusing. What about:
>
> "Introduce a library function for executing format-0 and format-1
> channel programs..."

Agreed. Fixed.

> > This will be used for real dasd ipl.
>
> But also for virtio in the follow-on patches, won't it?

True. I removed that line. It is obvious and not really useful anyway.

...
> > +    orb.fmt = fmt ;
>
> extra ' ' before ';'

Fixed.

> > +    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;
> > +
> > +    rc = ssch(schid, &orb);
> > +    if (rc == 1) {
> > +        /* Status pending, not sure why. Eat status and ask for retry. */
> > +        tsch(schid, irb);
> > +        return -1;
> > +    }
> > +    if (rc) {
> > +        print_int("ssch failed with rc=", rc);
>
> Better 'cc' than 'rc' in the message?

Fixed here and for tsch as well.

> > +        return rc;
> > +    }
> > +
> > +    consume_io_int();
> > +
> > +    /* collect status */
> > +    rc = tsch(schid, irb);
> > +    if (rc) {
> > +        print_int("tsch failed with rc=", rc);
> > +    }
>
> Hm. The whole code flow relies on the fact that not only no more than
> one cpu is enabled for I/O interrupts, but also only one subchannel.
> Otherwise, you could get an interrupt for another subchannel, which
> would be the only way you'd get cc 1 on the tsch for this subchannel
> here (no status pending). Maybe peek at the interruption information
> stored into the lowcore first?
>
> Won't be a problem with the code as it is now, though, AFAICS.
Agreed, voting to leave as is. Perhaps a comment to explain that we rely on only one 
"Active" i/o device?

> > +
> > +    return rc;
> > +}
> > +
> > +/*
> > + * 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.
> > + *
> > + * Returns non-zero on error.
> > + */
> > +int do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt)
> > +{
> > +    Irb irb = {};
> > +    SenseDataEckdDasd sd;
> > +    int rc, retries = 0;
> > +
> > +    while (true) {
> > +        rc = __do_cio(schid, ccw_addr, fmt, &irb);
> > +
> > +        if (rc == -1) {
> > +            retries++;
> > +            continue;
> > +        }
> > +        if (rc) {
> > +            /* ssch/tsch error. Message already reported by __do_cio */
>
> You might also want to consider retrying on cc 2. Not very likely,
> though.

It is easy to retry on cc=2, fixed.

> > +            break;
> > +        }
> > +
> > +        if (!irb_error(&irb)) {
> > +            break;
> > +        }
> > +
> > +        /*
> > +         * Unexpected unit check, or interface-control-check. Use sense to
> > +         * clear (unit check only) then retry.
> > +         */
> > +        if ((unit_check(&irb) || iface_ctrl_check(&irb)) && retries <= 2) {
> > +            if (unit_check(&irb)) {
> > +                basic_sense(schid, &sd, sizeof(sd));
>
> Unless I'm confused: I think you can still descend into the unit check
> rabbit hole here. basic_sense() calls do_cio(), which calls
> basic_sense(),... and we don't even get to the retries check.
>
> Maybe call __do_cio() from basic_sense() instead and make that return
> an error instead of panicking? Then you could just bump retries here
> and give up after some retries...

Yes, good point. This is now fixed.
--
-- Jason J. Herne (address@hidden)