From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web10.47120.1585564230124498354 for ; Mon, 30 Mar 2020 03:30:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Tv0oeRss; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585564229; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8nFpRx1NOVhlChVdx4jB9EQKkYQa/3H0uzjXaetEd7M=; b=Tv0oeRsseCkQILLG/9uLvndcIsxqsTuQq9YYHSbwiz6hyRSBm1U1ZmUqRCWtrQkWj2KqFe 91ySWCyPNOY6Po/DgQJDA2V79F7eO7gfMUFMai/R/PgZOJ8XZWKJUmiuWAfS37xEhhIp+X A6O+o83pDdVuITXHgzQSJpRQUrp4G1w= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-266-9aHgAg74Mo-k7Txcwkz1Fg-1; Mon, 30 Mar 2020 06:30:20 -0400 X-MC-Unique: 9aHgAg74Mo-k7Txcwkz1Fg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 54B5E800D6C; Mon, 30 Mar 2020 10:30:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-191.ams2.redhat.com [10.36.112.191]) by smtp.corp.redhat.com (Postfix) with ESMTP id 63D58165D3; Mon, 30 Mar 2020 10:30:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response To: Liran Alon , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-16-liran.alon@oracle.com> <6f32838a-d136-050e-7a05-f817da7954a8@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 30 Mar 2020 12:30:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 03/27/20 23:04, Liran Alon wrote: >=20 > On 28/03/2020 0:05, Laszlo Ersek wrote: >> On 03/27/20 14:04, Liran Alon wrote: >>> On 27/03/2020 14:26, Laszlo Ersek wrote: >>>> On 03/25/20 17:10, Liran Alon wrote: >>>>> +=A0=A0=A0=A0=A0 Packet->HostAdapterStatus =3D >>>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 >>>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN; >>>>> +=A0=A0=A0=A0=A0 return EFI_BAD_BUFFER_SIZE; >>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI >>>> spec, >>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed". >>>> But that's not the case here -- we do have a partially completed >>>> transfer. >>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU >>> in MdePkg/Include/Protocol/ScsiPassThru.h: >>> >>> =A0=A0 @retval EFI_BAD_BUFFER_SIZE=A0=A0=A0=A0=A0=A0 The SCSI Request P= acket was >>> executed, but the >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 entire DataBuffer could not be >>> transferred. >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The actual number of bytes >>> transferred is returned >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in TransferLength. See >>> HostAdapterStatus, >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 TargetStatus, SenseDataLength, and >>> SenseData in >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 that order for additional status >>> information. >>> >>> So I don't know who to believe... It does seem to me that this >>> documentation in the code makes more sense >>> and then my current code is correct. What do you think? >> You are looking at the wrong protocol header file. > Oops. You are right. >> The top of this >> header file bears the comment >> >> =A0=A0 SCSI Pass Through protocol as defined in EFI 1.1. >> >> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it >> only refers to Mantis ticket 845 >> > > with subject >> "EFI_SCSI_PASS_THRU_PROTOCOL replacement". >> >> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in >> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the >> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The >> SCSI Request Packet was not executed"). >=20 > Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and > ScsiPassThruExt.h header file. It doesn't make sense. I agree there is a bug in the UEFI spec (but I do think the intent is mostly clear). More below. > In the new header file, there is no way for PassThru() to indicate to > caller that a partial data-transfer have occurred. > Which seems to be a quite standard functionality. Almost all SCSI > devices are able to return information on partial data-transfer > completions. My understanding from the spec is that EFI_BAD_BUFFER_SIZE is for the case when the driver can determine up-front -- without talking to the device for the specific request's sake -- that the request is over-sized. Otherwise (=3D it makes sense to submit the request to the device), a short transfer is normal / expected. If there is no other issue with the transfer, then InTransferLength / OutTransferLength should be updated on output, and EFI_SUCCESS should be returned. >=20 > Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete() > implementation, we can see how it handles partial data-transfer > completions. > It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual > accordingly. > Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will > be handled by updating Packet->InTransferLength and > Packet->OutTransferLength based on Response->Residual, > but then setting Packet->HostAdapterStatus to > EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS. > (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in > this case). This is what I wrote, verbatim: '(9) Thus I feel we should use a "break" here.' Note: "feel". VirtioScsiDxe returns EFI_SUCCESS (and modifies InTransferLength/OutTransferLength, per "residual") when the device reports VIRTIO_SCSI_S_OK. That's a top-level success report from the device= . PVSCSI does not return a top-level success code in this case (apparently). I have never before worked with PVSCSI, and I don't know what exactly its return codes stand for. If "PvScsiBtStatDatarun" and "PvScsiBtStatDataUnderrun" mean "normal partial transfer", then I fully agree with you that EFI_DEVICE_ERROR would not be correct. > The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS > specifies: >=20 > =A0 @retval EFI_SUCCESS=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The SCSI Request Pa= cket was sent by the > host. For bi-directional > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 commands, InTransferLength bytes were > transferred from > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 InDataBuffer. For write and > bi-directional commands, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 OutTransferLength bytes were transferred by > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 OutDataBuffer. >=20 > But it seems to reference the InTransferLength and OutTransferLength > *before* the call to PassThru(). If you look at the individual "InTransferLength" and "OutTransferLength" parameter (i.e., Packet member) specifications, in the spec (not the header file), they do clarify "on input" vs. "on output". They make sense to me, in connection with the EFI_SUCCESS return status. (There *is* a bug, IMO, in the documentation of EFI_BAD_BUFFER_SIZE, in the spec.) Quote: ----v---- InTransferLength On Input, the size, in bytes, of InDataBuffer. On output, the number of bytes transferred between the SCSI controller and the SCSI device. If InTransferLength is larger than the SCSI controller can handle, no data will be transferred, InTransferLength will be updated to contain the number of bytes that the SCSI controller is able to transfer, and EFI_BAD_BUFFER_SIZE will be returned. OutTransferLength On Input, the size, in bytes of OutDataBuffer. On Output, the Number of bytes transferred between SCSI Controller and the SCSI device. If OutTransferLength is larger than the SCSI controller can handle, no data will be transferred, OutTransferLength will be updated to contain the number of bytes that the SCSI controller is able to transfer, and EFI_BAD_BUFFER_SIZE will be returned. If the data buffer described by InDataBuffer and InTransferLength is too big to be transferred in a single command, then no data is transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes that can be transferred in a single command are returned in InTransferLength. If the data buffer described by OutDataBuffer and OutTransferLength is too big to be transferred in a single command, then no data is transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes that can be transferred in a single command are returned in OutTransferLength. EFI_SUCCESS The SCSI Request Packet was sent by the host. For bi-directional commands, InTransferLength bytes were transferred from InDataBuffer. For write and bi-directional commands, OutTransferLength bytes were transferred by OutDataBuffer. See HostAdapterStatus, TargetStatus, SenseDataLength, and SenseData in that order for additional status information. EFI_BAD_BUFFER_SIZE The SCSI Request Packet was not executed. The number of bytes that could be transferred is returned in InTransferLength. For write and bi-directional commands, OutTransferLength bytes were transferred by OutDataBuffer. See HostAdapterStatus, TargetStatus, and in that order for additional status information. ----^---- The spec bug (IMO) is the penultimate sentence of the "EFI_BAD_BUFFER_SIZE" documentation -- "were transferred" seems to make no sense, given that "The SCSI Request Packet was not executed". Regarding why VirtioScsiDxe sets HostAdapterStatus to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK when reporting an (otherwise successful) short transfer: I can list two reasons. (1) Paolo had reviewed this code at least on two separate occasions -- once in October 2012 and another time in September 2015. (2) To me, EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN seemed much closer to VIRTIO_SCSI_S_OVERRUN, which -- per virtio spec -- stands for something very different: VIRTIO_SCSI_S_OVERRUN if the content of the CDB (such as the allocation length, parameter length or transfer size) requires more data than is available in the datain and dataout buffers. Given that this part of VirtioScsiDxe has not caused any problems in the past 7.5 years (it goes back to commit 37078a63b1911), i.e. setting EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK on a partial (but otherwise OK) transfer, I would like to change it (to what?) only if: - Paolo recommends that too, - there is an actual industry standard or spec that indicates the current host status is wrong. To summarize my (updated) opinion on your handling of PvScsiBtStatDatarun / PvScsiBtStatDataUnderrun: - I don't know what the HostAdapterStatus should be (on output), or whether you should report EFI_SUCCESS vs. EFI_DEVICE_ERROR. (If those PVSCSI response codes stand for "normal, just short, transfer", then I would suggest to follow VirtioScsiDxe's handling -- but I don't insist at all.) - Either way, I am quite convinced that returning EFI_BAD_BUFFER_SIZE is wrong, because the packet *was* submitted to the device. > In contrast to old header file, ScsiPassThru.h, which for > EFI_BAD_BUFFER_SIZE specifies explicitly: >=20 > =A0@retval EFI_BAD_BUFFER_SIZE=A0=A0=A0=A0=A0=A0 The SCSI Request Packet = was executed, > but the > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 entire DataBuffer could not be > transferred. > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 The actual number of bytes > transferred is returned > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in TransferLength. See > HostAdapterStatus, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 TargetStatus, SenseDataLength, and > SenseData in > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 that order for additional status > information. >=20 > In addition, looking at EDK2 iSCSI ScsiPassThru driver > (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use > EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET > (Which matches the new header file ScsiPassThruExt.h and not the old > ScsiPassThru.h). > If you would have a look at how IScsiOnScsiRspRcvd() handles > SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW bits, > it seems to always > set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating > Packet->InTransferLength / Packet->OutTransferLength. Great example! And yes, I do feel that returning EFI_BAD_BUFFER_SIZE, after updating InTransferLength / OutTransferLength from a field called "ResidualCount", is wrong. "ResidualCount" is coming back in a response structure, and its name ("residual") indicates the device transfer was actually initiated on the remote end. To me EFI_BAD_BUFFER_SIZE just has a different "spirit". It generally means "I couldn't begin to act upon the request due to the wrong buffer size. Adjust the buffer size, and try again". A partial / short transfer is different from that. > What do you think? Do you have any further insights? This is quite > confusing to me. I agree that it's very confusing. I'm not sure if this is covered by the UEFI SCT (self conformance test). Thanks, Laszlo