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

["Jason J. Herne" <jjherne@linux.ibm.com>]

On 3/4/19 1:25 PM, Cornelia Huck wrote:
> On Fri,  1 Mar 2019 13:59:30 -0500
> "Jason J. Herne" <jjherne@linux.ibm.com> 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 (jjherne@linux.ibm.com)