From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B395321EB5279 for ; Thu, 31 Aug 2017 04:16:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 88C4781E1B; Thu, 31 Aug 2017 11:19:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 88C4781E1B Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.phx2.redhat.com [10.3.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id EBAC79C11B; Thu, 31 Aug 2017 11:19:02 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1504125903-29816-1-git-send-email-brijesh.singh@amd.com> <1504125903-29816-3-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <37ed5084-7341-7254-5108-7524196513c7@redhat.com> Date: Thu, 31 Aug 2017 13:19:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1504125903-29816-3-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 31 Aug 2017 11:19:04 +0000 (UTC) Subject: Re: [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Aug 2017 11:16:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/30/17 22:45, Brijesh Singh wrote: > When virtio request fails we return EFI_DEVICE_ERROR, as per the spec > EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required > to implement elaborated error reporting. > > The patch refactors out entire block of the code that creates the host > adapter error into a separate helper function (ReportHostAdapterError). > > Suggested-by: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 27 +++++++++++++++----- > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > index 5e72b1a24b59..cac213129409 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > @@ -387,6 +387,26 @@ ParseResponse ( > return EFI_DEVICE_ERROR; > } OK, I'm glad that you found the best place to introduce the new function (just below ParseResponse()). > +/** > + * The function can be used to create a fake host adapter error. > + * When VirtioScsiPassThru is failed due to some reasons then this function > + * can be called to contstruct a host adapter error. > + * > + */ (1) This comment block should be formatted according to the edk2 coding style, and it should document the Packet parameter, and the return value: ------- /** The function can be used to create a fake host adapter error. When VirtioScsiPassThru() is failed due to some reasons then this function can be called to construct a host adapter error. @param[out] Packet The Extended SCSI Pass Thru Protocol packet that the host adapter error shall be placed in. @retval EFI_DEVICE_ERROR The function returns this status code unconditionally, to be propagated by VirtioScsiPassThru(). **/ ------- There's no need to resubmit just because of this; if more serious updates are not needed, I can take care of it. (2) Another documentation update: pls see the following comment block (higher up in the source code): // UEFI Spec 2.3.1 + Errata C, 14.7 Extended SCSI Pass Thru Protocol specifies // the PassThru() interface. Beside returning a status code, the function must // set some fields in the EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET in/out // parameter on return. The following is a full list of those fields, for // easier validation of PopulateRequest(), ParseResponse(), and // VirtioScsiPassThru() below. After this patch, VirtioScsiPassThru() will no longer massage Packet->xxx fields directly, only via helper functions. So please replace the "VirtioScsiPassThru()" reference in the comment block with "ReportHostAdapterError()". I can take care of this as well, if v3 is not necessary otherwise. With these documentation updates, Reviewed-by: Laszlo Ersek Thanks, Laszlo > +STATIC > +EFI_STATUS > +ReportHostAdapterError ( > + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > + ) > +{ > + Packet->InTransferLength = 0; > + Packet->OutTransferLength = 0; > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > + Packet->SenseDataLength = 0; > + return EFI_DEVICE_ERROR; > +} > + > > // > // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL > @@ -472,12 +492,7 @@ VirtioScsiPassThru ( > // > if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, > &Indices, NULL) != EFI_SUCCESS) { > - Packet->InTransferLength = 0; > - Packet->OutTransferLength = 0; > - Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > - Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > - Packet->SenseDataLength = 0; > - return EFI_DEVICE_ERROR; > + return ReportHostAdapterError (Packet); > } > > return ParseResponse (Packet, &Response); >