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

[Laszlo Ersek <lersek@redhat.com>]

(1) should have noticed it in my previous review: the subject should be
changed like this ("pkg" -> "Pkg"):

-Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers
+OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers

No need to repost just because of this.

On 08/30/17 22:45, 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.
> 
> - 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."

I can update 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 | 225 ++++++++++++++++++--
>  1 file changed, 209 insertions(+), 16 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index cac213129409..3e04097ddd11 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -429,26 +429,161 @@ 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                   InDataBufferIsMapped;
> +  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;
> +  InDataBufferIsMapped = FALSE;
> +  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-xxx fields

(3) Please replace "Packet-xxx" with just "Packet"

> +    //  accordingly so that we report the full loss of the incoming transfer.
> +    //
> +    //  We allocate a temporary buffer and map it with BusMasterCommon. If the

(4) s/BusMasterCommon/BusMasterCommonBuffer/

Please don't forget to rewrap the text to 79 or 80 columns.

> +    //  Virtio request is successful then we copy the data from temporary
> +    //  buffer into Packet->InDataBuffer.
> +    //

(5) The last two paragraphs in the comment should be un-indented one column.

> +    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).

> +    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;
> +    }
> +
> +    InDataBufferIsMapped = TRUE;
> +  }
> +
> +  //
> +  // 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
> @@ -459,31 +594,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.
> @@ -492,10 +645,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 (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) {
> +    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
> +  }

(7) The comment is exactly right, but the condition that you check after
is incorrect.

The right thing to do is to call CopyMem() *unconditionally*.

Namely, at this point we are past ParseResponse(). As I wrote before,
ParseResponse() updates the Packet->... fields in every case, even if it
reports an EFI_STATUS that is different from EFI_SUCCESS. And whatever
we expose to the caller through "Packet->InTransferLength" *must* be
reflected in "Packet->InDataBuffer" regardless of return status.

Therefore the Status check must be dropped. And then we need not check
(Packet->InTransferLength>0) either, because the CopyMem() will deal
with it internally.

Think of it like this: the "worst" that can happen, on error, is that
"Packet->InTransferLength" is unchanged from its "input" value, and we
overwrite the caller's "Packet->InDataBuffer" entirely. What is the data
we are going to put there? It's all zeroes, from your

  ZeroMem (InDataBuffer, Packet->InTransferLength);

higher up.

So, again, this CopyMem() needs to be unconditional -- as the comment
says, if the *virtio* request was successful (== we talked to the
virtio-scsi adapter), then we have to copy the data, even if the *SCSI*
request produced an error status in ParseResponse.

> +
> +UnmapResponseBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 ResponseBuffer
> +                 );
> +
> +UnmapOutDataBuffer:
> +  if (OutDataBufferIsMapped == TRUE) {

(8) Just say "if (OutDataBufferIsMapped)"

> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (InDataBufferIsMapped == TRUE) {

(9) The "InDataBufferIsMapped" variable can be eliminated safely.
Instead, just say

  if (InDataBuffer != NULL)

Because, if you reach this error label, then InDataBuffer!=NULL
guarantees that you have mapped it as well, and InDataBuffer==NULL
guarantees that you have not mapped it either.

Put differently, if InDataBuffer!=NULL, then we must have attempted to
map it (because Packet->InTransferLength was positive on input). If we
succeeded in mapping it, then we have to unmap it. If we failed to map
it, then we're not here! Because we jumped to FreeInDataBuffer, just below.

> +    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;
>  }
>  
>  
> 

Otherwise, the logic is very good in this patch; all the updates I'm
requesting are localized tweaks, and the only control flow change is
point (7) -- which is also very tight.

For the updates above, I must ask you for v3, but I trust that v3 is
going to be the final version.

Thank you!
Laszlo