Re: [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  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 <lersek@redhat.com>

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);
>