Re: [PATCH v2 18/23] OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors

[Laszlo Ersek <lersek@redhat.com>]

After all, I've taken a look here as well. I'm not going to point out
all the earlier remarks (please do address them here anyway, because
they certainly apply), I'll just say what I feel is specific to this patch:

On 08/14/17 13:36, Brijesh Singh wrote:
> The VirtioScsiPassThru(), programs the vring descriptor using the host
> addresses pointed-by virtio-scsi request, response and memory that is
> referenced inside the request and response header.
> 
> The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
> function to map system memory to device address and programs the vring
> descriptors with device addresses.
> 
> 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.h |   1 +
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 148 +++++++++++++++++---
>  2 files changed, 133 insertions(+), 16 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> index 6d00567e8cb8..bb1c5c70ef74 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> @@ -60,6 +60,7 @@ typedef struct {
>    VRING                           Ring;           // VirtioRingInit      2
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;       // VirtioScsiInit      1
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;   // VirtioScsiInit      1
> +  VOID                            *RingBufMapping;// VirtioScsiInit      1
>  } VSCSI_DEV;
>  
>  #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index a983b3df7b9c..65e9bda0827a 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -409,8 +409,13 @@ VirtioScsiPassThru (
>    UINT16                    TargetValue;
>    EFI_STATUS                Status;
>    volatile VIRTIO_SCSI_REQ  Request;
> -  volatile VIRTIO_SCSI_RESP Response;
> +  VIRTIO_SCSI_RESP          *Response;

(1) Please don't drop the volatile qualification. If you want to turn it
into a pointer, that might be OK, but the target of the pointer should
remain volatile.

>    DESC_INDICES              Indices;
> +  VOID                      *RequestMapping;
> +  VOID                      *ResponseMapping;
> +  VOID                      *InDataMapping;
> +  VOID                      *OutDataMapping;
> +  EFI_PHYSICAL_ADDRESS      DeviceAddress;
>  
>    ZeroMem ((VOID*) &Request, sizeof (Request));
>    ZeroMem ((VOID*) &Response, sizeof (Response));

(2) Hmm, this ZeroMem() now nulls the pointer. Probably not what you
intended.

> @@ -418,9 +423,41 @@ VirtioScsiPassThru (
>    Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
>    CopyMem (&TargetValue, Target, sizeof TargetValue);
>  
> +  Response = NULL;
> +  ResponseMapping = NULL;
> +  RequestMapping = NULL;
> +  InDataMapping = NULL;
> +  OutDataMapping = NULL;
> +
> +  //
> +  // 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.
> +  //

Good point!

(3) In that case however, please do the same for "HostStatus" in the
VirtioBlkDxe driver as well. We also pre-set that value for ourselves.

> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *Response),
> +                          (VOID *) &Response
> +                          );

(4) In general I abhor this kind of pointer type punning (it is
undefined behavior in standard C), but it is so pervasive in edk2, when
looking up protocol interfaces, that I guess we can live with it.

However, the type that we cast &Response to should be (VOID **), not
(VOID *). The compiler didn't complain to you because (VOID *) silently
converts to and from other object pointer types -- assuming same const /
volatile qualification.

The cleanest solution for this would be, IMO:

  volatile VIRTIO_SCSI_RESP *Response;
  VOID                      *ResponseBuffer;

  Status = Dev->VirtIo->AllocateSharedPages (
                          Dev->VirtIo,
                          EFI_SIZE_TO_PAGES (sizeof *Response),
                          &ResponseBuffer
                          );
  ...
  Response = ResponseBuffer;

... I think this is all I wanted to say about this patch for now, on top
of my earlier comments for virtio-rng and virtio-blk, which also apply here.

Thanks,
Laszlo

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             (VOID *) Response,
> +             sizeof (*Response),
> +             &DeviceAddress,
> +             &ResponseMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto Free_Response_Buffer;
> +  }
> +
>    Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
>    if (EFI_ERROR (Status)) {
> -    return Status;
> +    goto Unmap_Response_Buffer;
>    }
>  
>    VirtioPrepare (&Dev->Ring, &Indices);
> @@ -428,7 +465,7 @@ VirtioScsiPassThru (
>    //
>    // 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;
>  
>    //
>    // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -437,23 +474,51 @@ VirtioScsiPassThru (
>    ASSERT (Dev->Ring.QueueSize >= 4);
>  
>    //
> +  // Map the scsi-blk request header HostAddress to DeviceAddress
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &DeviceAddress,
> +             &RequestMapping);
> +  if (EFI_ERROR (Status)) {
> +    goto Unmap_Response_Buffer;
> +  }
> +
> +  //
>    // enqueue Request
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> +  VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof Request,
>      VRING_DESC_F_NEXT, &Indices);
>  
>    //
>    // enqueue "dataout" if any
>    //
>    if (Packet->OutTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
> +    //
> +    // Map the buffer address to a device address
> +    //
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterRead,
> +               Packet->OutDataBuffer,
> +               Packet->OutTransferLength,
> +               &DeviceAddress,
> +               &OutDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      goto Unmap_Request_Buffer;
> +    }
> +    VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress,
>        Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
>    }
>  
>    //
>    // enqueue Response, to be written by the host
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
> +  VirtioAppendDesc (&Dev->Ring, (UINTN) Response, sizeof *Response,
>      VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
>                            VRING_DESC_F_NEXT : 0),
>      &Indices);
> @@ -462,7 +527,22 @@ VirtioScsiPassThru (
>    // enqueue "datain" if any, to be written by the host
>    //
>    if (Packet->InTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
> +    //
> +    // Map the buffer address to a device address
> +    //
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterWrite,
> +               Packet->InDataBuffer,
> +               Packet->InTransferLength,
> +               &DeviceAddress,
> +               &InDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      goto Unmap_Out_Buffer;
> +    }
> +
> +    VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress,
>        Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
>    }
>  
> @@ -480,7 +560,29 @@ VirtioScsiPassThru (
>      return EFI_DEVICE_ERROR;
>    }
>  
> -  return ParseResponse (Packet, &Response);
> +  Status = ParseResponse (Packet, Response);
> +
> +  if (Packet->InTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> +  }
> +
> +Unmap_Out_Buffer:
> +  if (Packet->OutTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +Unmap_Request_Buffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +Unmap_Response_Buffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +Free_Response_Buffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 (VOID *) Response
> +                 );
> +
> +  return Status;
>  }
>  
>  
> @@ -707,7 +809,7 @@ VirtioScsiInit (
>  {
>    UINT8      NextDevStat;
>    EFI_STATUS Status;
> -
> +  UINT64     RingBaseShift;
>    UINT64     Features;
>    UINT16     MaxChannel; // for validation only
>    UINT32     NumQueues;  // for validation only
> @@ -838,18 +940,24 @@ VirtioScsiInit (
>      goto Failed;
>    }
>  
> +  Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
> +             &Dev->RingBufMapping);
> +  if (EFI_ERROR (Status)) {
> +    goto ReleaseQueue;
> +  }
> +
>    //
>    // Additional steps for MMIO: align the queue appropriately, and set the
>    // size. If anything fails from here on, we must release the ring resources.
>    //
>    Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -857,7 +965,7 @@ VirtioScsiInit (
>    //
>    Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -867,7 +975,7 @@ VirtioScsiInit (
>      Features &= ~(UINT64)VIRTIO_F_VERSION_1;
>      Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>      if (EFI_ERROR (Status)) {
> -      goto ReleaseQueue;
> +      goto UnmapQueue;
>      }
>    }
>  
> @@ -877,11 +985,11 @@ VirtioScsiInit (
>    //
>    Status = VIRTIO_CFG_WRITE (Dev, CdbSize, VIRTIO_SCSI_CDB_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>    Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -890,7 +998,7 @@ VirtioScsiInit (
>    NextDevStat |= VSTAT_DRIVER_OK;
>    Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -926,6 +1034,8 @@ VirtioScsiInit (
>  
>    return EFI_SUCCESS;
>  
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingBufMapping);
>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
> @@ -995,6 +1105,12 @@ VirtioScsiExitBoot (
>    //
>    Dev = Context;
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> +  //
> +  // If Ring buffer mapping exist then unmap it so that hypervisor can
> +  // not get readable data after device reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingBufMapping);
>  }
>  
>  
>