Re: [PATCH v3 3/4] OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Laszlo Ersek <lersek@redhat.com>]

On 08/31/17 17:01, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of virtio request, response and any memory referenced by those
> request/response to the bus master.
>
> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to
> map request and response buffers system physical address to the device
> address.
>
> - If the buffer need to be accessed by both the processor and a bus
>   master then map with BusMasterCommonBuffer.
>
>   However, after a BusMasterWrite Unmap() failure, error reporting via
>   EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET would be very complex,
>   therefore we map such buffers too with BusMasterCommonBuffer.

(1) In my v2 review, I wrote (emphasis added now):

On 08/31/17 15:23, Laszlo Ersek wrote:
> On 08/30/17 22:45, Brijesh Singh wrote:
>> - If the buffer need to be accessed for a write operation by a bus master
>>   then map with BusMasterWrite.
>
> (2) We no longer use BusMasterWrite in this patch -- as I requested, so
> that's good --, so I suggest that we *continue* this paragraph as follows:
>
> "However, after a BusMasterWrite Unmap() failure, error reporting via
> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET would be very complex,
> therefore we map such buffers too with BusMasterCommonBuffer."

You didn't *continue* the paragraph in v3, you removed it. And now the
"However,..." sentence appears to refer to the BusMasterCommonBuffer
paragraph. That's incorrect.

I'll fix up the commit message.

> - If the buffer need to be accessed for a read  operation by a bus master
>   then map with BusMasterRead.
>
> 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 | 220 ++++++++++++++++++--
>  1 file changed, 204 insertions(+), 16 deletions(-)
>
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e977c636a0a..337fb4b2f1e0 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -436,26 +436,156 @@ VirtioScsiPassThru (
>    UINT16                    TargetValue;
>    EFI_STATUS                Status;
>    volatile VIRTIO_SCSI_REQ  Request;
> -  volatile VIRTIO_SCSI_RESP Response;
> +  volatile VIRTIO_SCSI_RESP *Response;
> +  VOID                      *ResponseBuffer;
>    DESC_INDICES              Indices;
> +  VOID                      *RequestMapping;
> +  VOID                      *ResponseMapping;
> +  VOID                      *InDataMapping;
> +  VOID                      *OutDataMapping;
> +  EFI_PHYSICAL_ADDRESS      RequestDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      ResponseDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      InDataDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      OutDataDeviceAddress;
> +  VOID                      *InDataBuffer;
> +  UINTN                     InDataNumPages;
> +  BOOLEAN                   OutDataBufferIsMapped;
>
>    ZeroMem ((VOID*) &Request, sizeof (Request));
> -  ZeroMem ((VOID*) &Response, sizeof (Response));
>
>    Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
>    CopyMem (&TargetValue, Target, sizeof TargetValue);
>
> +  InDataBuffer = NULL;
> +  OutDataBufferIsMapped = FALSE;
> +  InDataNumPages = 0;
> +
>    Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>
> -  VirtioPrepare (&Dev->Ring, &Indices);
> +  //
> +  // Map the virtio-scsi Request header buffer
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping);
> +  if (EFI_ERROR (Status)) {
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  //
> +  // Map the input buffer
> +  //
> +  if (Packet->InTransferLength > 0) {
> +    //
> +    // Allocate a intermediate input buffer. This is mainly to handle the
> +    // following case:
> +    //  * caller submits a bi-directional request
> +    //  * 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.
> +    //

(2) In my v2 review, point (5), I asked, "The last two paragraphs in the
comment should be un-indented one column.".

You forgot to do that, I'll fix it up.

> +    InDataNumPages = (UINTN)EFI_SIZE_TO_PAGES (Packet->InTransferLength);

(3) In my v2 review, I wrote (emphasis added now):

On 08/31/17 15:23, Laszlo Ersek wrote:
> On 08/30/17 22:45, Brijesh Singh wrote:
>> +    InDataNumPages = EFI_SIZE_TO_PAGES (Packet->InTransferLength);
>
> (6) "Packet->InTransferLength" has type (UINT32), but
> EFI_SIZE_TO_PAGES() takes UINTN.
>
> Please cast the *argument* to (UINTN).

In v3, you didn't cast the *argument* to UINTN, you cast the result.

I'll fix it up.

The rest is good.

After I apply the above changes:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +    Status = Dev->VirtIo->AllocateSharedPages (
> +                            Dev->VirtIo,
> +                            InDataNumPages,
> +                            &InDataBuffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto UnmapRequestBuffer;
> +    }
> +
> +    ZeroMem (InDataBuffer, Packet->InTransferLength);
> +
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterCommonBuffer,
> +               InDataBuffer,
> +               Packet->InTransferLength,
> +               &InDataDeviceAddress,
> +               &InDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto FreeInDataBuffer;
> +    }
> +  }
> +
> +  //
> +  // Map the output buffer
> +  //
> +  if (Packet->OutTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterRead,
> +               Packet->OutDataBuffer,
> +               Packet->OutTransferLength,
> +               &OutDataDeviceAddress,
> +               &OutDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto UnmapInDataBuffer;
> +    }
> +
> +    OutDataBufferIsMapped = TRUE;
> +  }
> +
> +  //
> +  // Response header is bi-direction (we preset with host status and expect
> +  // the device to update it). Allocate a response buffer which can be mapped
> +  // to access equally by both processor and device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *Response),
> +                          &ResponseBuffer
> +                          );
> +  if (EFI_ERROR (Status)) {
> +    Status = ReportHostAdapterError (Packet);
> +    goto UnmapOutDataBuffer;
> +  }
> +
> +  Response = ResponseBuffer;
> +
> +  ZeroMem ((VOID *)Response, sizeof (*Response));
>
>    //
>    // preset a host status for ourselves that we do not accept as success
>    //
> -  Response.Response = VIRTIO_SCSI_S_FAILURE;
> +  Response->Response = VIRTIO_SCSI_S_FAILURE;
> +
> +  //
> +  // Map the response buffer with BusMasterCommonBuffer so that response
> +  // buffer can be accessed by both host and device.
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             ResponseBuffer,
> +             sizeof (*Response),
> +             &ResponseDeviceAddress,
> +             &ResponseMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = ReportHostAdapterError (Packet);
> +    goto FreeResponseBuffer;
> +  }
> +
> +  VirtioPrepare (&Dev->Ring, &Indices);
>
>    //
>    // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -466,31 +596,49 @@ VirtioScsiPassThru (
>    //
>    // enqueue Request
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> -    VRING_DESC_F_NEXT, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    RequestDeviceAddress,
> +    sizeof Request,
> +    VRING_DESC_F_NEXT,
> +    &Indices
> +    );
>
>    //
>    // enqueue "dataout" if any
>    //
>    if (Packet->OutTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
> -      Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      OutDataDeviceAddress,
> +      Packet->OutTransferLength,
> +      VRING_DESC_F_NEXT,
> +      &Indices
> +      );
>    }
>
>    //
>    // enqueue Response, to be written by the host
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
> -    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
> -                          VRING_DESC_F_NEXT : 0),
> -    &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    ResponseDeviceAddress,
> +    sizeof *Response,
> +    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
> +    &Indices
> +    );
>
>    //
>    // enqueue "datain" if any, to be written by the host
>    //
>    if (Packet->InTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
> -      Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      InDataDeviceAddress,
> +      Packet->InTransferLength,
> +      VRING_DESC_F_WRITE,
> +      &Indices
> +      );
>    }
>
>    // If kicking the host fails, we must fake a host adapter error.
> @@ -499,10 +647,50 @@ VirtioScsiPassThru (
>    //
>    if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>          &Indices, NULL) != EFI_SUCCESS) {
> -    return ReportHostAdapterError (Packet);
> +    Status = ReportHostAdapterError (Packet);
> +    goto UnmapResponseBuffer;
>    }
>
> -  return ParseResponse (Packet, &Response);
> +  Status = ParseResponse (Packet, Response);
> +
> +  //
> +  // If virtio request was successful and it was a CPU read request then we
> +  // have used an intermediate buffer. Copy the data from intermediate buffer
> +  // to the final buffer.
> +  //
> +  if (InDataBuffer != NULL) {
> +    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
> +  }
> +
> +UnmapResponseBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 ResponseBuffer
> +                 );
> +
> +UnmapOutDataBuffer:
> +  if (OutDataBufferIsMapped) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (InDataBuffer != NULL) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> +  }
> +
> +FreeInDataBuffer:
> +  if (InDataBuffer != NULL) {
> +    Dev->VirtIo->FreeSharedPages (Dev->VirtIo, InDataNumPages, InDataBuffer);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +  return Status;
>  }
>
>
>