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

[Farhan Ali <alifm@linux.ibm.com>]

On 02/04/2019 06:13 AM, Cornelia Huck wrote:
> 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.
> 

If I understand the code correctly, the retries variable cannot prevent 
infinite recursion. Because every time we get a unit check we do a basic 
sense which calls the do_cio function again. If that basic sense returns 
a unit check we do another basic sense....

> 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;
>>> +}
>>
> 
>