From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:50921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gyzUM-000502-M5 for qemu-devel@nongnu.org; Wed, 27 Feb 2019 08:43:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gyzNg-00067p-CQ for qemu-devel@nongnu.org; Wed, 27 Feb 2019 08:36:17 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:45830 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gyzNe-0005sn-EH for qemu-devel@nongnu.org; Wed, 27 Feb 2019 08:36:16 -0500 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x1RDYYjB130259 for ; Wed, 27 Feb 2019 08:35:45 -0500 Received: from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149]) by mx0a-001b2d01.pphosted.com with ESMTP id 2qwuj4828d-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 27 Feb 2019 08:35:45 -0500 Received: from localhost by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 27 Feb 2019 13:35:44 -0000 Reply-To: jjherne@linux.ibm.com References: <1548768562-20007-1-git-send-email-jjherne@linux.ibm.com> <1548768562-20007-11-git-send-email-jjherne@linux.ibm.com> <735ca9ff-76e3-966f-6fbc-a72bf994b413@linux.ibm.com> <20190204121319.32915fd6.cohuck@redhat.com> <20190205111838.5fe2fc47.cohuck@redhat.com> From: "Jason J. Herne" Date: Wed, 27 Feb 2019 08:35:37 -0500 MIME-Version: 1.0 In-Reply-To: <20190205111838.5fe2fc47.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: Subject: Re: [Qemu-devel] [PATCH 10/15] s390-bios: Support for running format-0/1 channel programs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , Farhan Ali Cc: qemu-devel@nongnu.org, qemu-s390x@nongnu.org, pasic@linux.ibm.com, borntraeger@de.ibm.com On 2/5/19 5:18 AM, Cornelia Huck wrote: > On Mon, 4 Feb 2019 14:29:18 -0500 > Farhan Ali wrote: > >> On 02/04/2019 06:13 AM, Cornelia Huck wrote: >>> On Thu, 31 Jan 2019 12:31:00 -0500 >>> Farhan Ali 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 >>>>> --- >>>>> 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.... > > Eww, you're right... > > I think that the routine needs to be split: > - inner routine that does the ssch, retries if the subchannel is status > pending, and waits for a final status (regardless whether it is a > special condition or not) > - outer routine that does error handling, if needed (like retrying on > IFCC, or doing a basic sense on unit check) > > The inner routine will probably only be called by the outer routine > (and not directly by other code). > > Does that make sense? It's hopefully enough; we really don't want to > transplant the whole Linux cio state machine into the bios... > I had a hard time following what you were suggesting here. Its most likely me,not you :). That said, I did redesign it to remove the potential infinite recursion. I'll be posting v3 soon, let me know what you think. -- -- Jason J. Herne (jjherne@linux.ibm.com)