Re: [PATCH v3 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address

[Laszlo Ersek <lersek@redhat.com>]

On 08/31/17 17:01, Brijesh Singh wrote:
> The patch updates VirtioScsiDxe to use IOMMU-like member functions to map
> the system physical address to device address for buffers (including vring,
> device specific request and response pointed by vring descriptor, and any
> furter memory reference by those request and response).
>
> 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>
>
> Repo: https://github.com/codomania/edk2
> Branch: virtio-scsi-3
>
> Changes since v2:
>  * changes to address v2 feedback
>
> Brijesh Singh (4):
>   OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap()
>   OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error
>   OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers
>   OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
>
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.h |   1 +
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 306 +++++++++++++++++---
>  2 files changed, 273 insertions(+), 34 deletions(-)
>

Okay, so these are the updates (cumulatively) that I implemented here:

> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 313b4f66c7f8..7b8c3d22c8de 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -387,19 +387,23 @@ ParseResponse (
>    return EFI_DEVICE_ERROR;
>  }
>
> +
>  /**
> - * 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.
> - *
> - * @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().
> - */
> +
> +  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().
> +
> +**/
>  STATIC
>  EFI_STATUS
>  ReportHostAdapterError (
> @@ -490,14 +494,15 @@ VirtioScsiPassThru (
>      //  * we perform the request fine
>      //  * but we fail to unmap the "InDataMapping"
>      //
> -    //  In that case simply returing the EFI_DEVICE_ERROR is not sufficient.
> -    //  In addition to the error code we also need to update Packet fields
> -    //  accordingly so that we report the full loss of the incoming transfer.
> -    //  We allocate a temporary buffer and map it with BusMasterCommonBuffer.
> -    //  If the Virtio request is successful then we copy the data from
> -    //  temporary buffer into Packet->InDataBuffer.
> +    // In that case simply returing the EFI_DEVICE_ERROR is not sufficient. In
> +    // addition to the error code we also need to update Packet fields
> +    // accordingly so that we report the full loss of the incoming transfer.
>      //
> -    InDataNumPages = (UINTN)EFI_SIZE_TO_PAGES (Packet->InTransferLength);
> +    // We allocate a temporary buffer and map it with BusMasterCommonBuffer. If
> +    // the Virtio request is successful then we copy the data from temporary
> +    // buffer into Packet->InDataBuffer.
> +    //
> +    InDataNumPages = EFI_SIZE_TO_PAGES ((UINTN)Packet->InTransferLength);
>      Status = Dev->VirtIo->AllocateSharedPages (
>                              Dev->VirtIo,
>                              InDataNumPages,

To patch #2, I added the following editing remark:

[lersek@redhat.com: fix style & typo in ReportHostAdapterError() comment]

To patch #3, I added the following editing remarks:

[lersek@redhat.com: restore lost sentence/paragraph in commit message]
[lersek@redhat.com: reindent/reflow "InDataBuffer" comment block]
[lersek@redhat.com: cast arg, not result, of EFI_SIZE_TO_PAGES() to UINTN]

With the above, the series is now fully Reviewed-by me.

I tested this series as follows:

                    virtio-mmio  legacy PCI        modern PCI
                    -----------  ----------------- --------------------------
  test scenario     AARCH64      IA32     X64      IA32     X64/SEV- X64/SEV+
  ----------------  -----------  -------- -------- -------- -------- --------
  R/W text editor   PASS         PASS     PASS     PASS     PASS     PASS

  shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS

  OS boot from it   PASS         PASS     PASS     PASS     PASS     PASS

Therefore, for the series:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Pushed as commit range fefeb416e63b..17cbf7359f04.

Thanks,
Laszlo