Re: [PATCH v2 17/23] OvmfPkg/VirtioBlkDxe: map host address to device address

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, Brijesh Singh wrote:
> The SynchronousRequest(), programs the vring descriptor with the buffers

(1) you likely meant "The SynchronousRequest() function"

> pointed-by virtio-blk requests, status and memory that is referenced
> inside the request header.
> 
> The patch uses VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() function to map
> host address to device address and programs the vring descriptor 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/VirtioBlkDxe/VirtioBlk.h |   1 +
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 121 +++++++++++++++++---
>  2 files changed, 109 insertions(+), 13 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> index 6c402ca88ea4..612994d261bc 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> @@ -41,6 +41,7 @@ typedef struct {
>    VRING                  Ring;                 // VirtioRingInit      2
>    EFI_BLOCK_IO_PROTOCOL  BlockIo;              // VirtioBlkInit       1
>    EFI_BLOCK_IO_MEDIA     BlockIoMedia;         // VirtioBlkInit       1
> +  VOID                   *RingMapping;         // VirtioBlkInit       1
>  } VBLK_DEV;

(2) If possible, please use consistent field names between the drivers.
So this should be "RingMap" as well (to follow the pattern set in the
virtio-rng driver).

(3) My earlier comments apply -- depth should be 2, and "VirtioBlkInit"
should be replaced with "VirtioRingMap".

>  
>  #define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index bff15fe3add1..57baceb20a19 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -251,6 +251,11 @@ SynchronousRequest (
>    volatile VIRTIO_BLK_REQ Request;
>    volatile UINT8          HostStatus;
>    DESC_INDICES            Indices;
> +  VOID                    *RequestMapping;
> +  VOID                    *StatusMapping;
> +  VOID                    *BufferMapping;
> +  EFI_PHYSICAL_ADDRESS    DeviceAddress;
> +  EFI_STATUS              Status;
>  
>    BlockSize = Dev->BlockIoMedia.BlockSize;
>  
> @@ -289,14 +294,30 @@ SynchronousRequest (
>    ASSERT (Dev->Ring.QueueSize >= 3);
>  
>    //
> +  // Map virtio-blk request header
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &DeviceAddress,
> +             &RequestMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }

(4) You should return EFI_DEVICE_ERROR here, regardless of the Status
received from VirtioMapAllBytesInSharedBuffer().

Please consult the leading comment block on return values.

(5) In fact, please extend the documentation for the EFI_DEVICE_ERROR
retval -- it now includes any case when a mapping for a bus master
operation fails.

> +
> +  //
>    // virtio-blk header in first desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> +  VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof Request,
>      VRING_DESC_F_NEXT, &Indices);

(6) Please break every argument to a separate line.

(7) As mentioned earlier, please remove the (UINTN) cast from
"DeviceAddress".

(8) Please do not mix the mapping operations with the virtio ring
manipulation:

(8a) Rather than reusing DeviceAddress, please introduce three local
address variables.

(8b) Concentrate the mapping operations, for all three areas, to one
sequence, before touching the virtio ring.

For this, you might want to move the VirtioPrepare() call as well just
before the first VirtioAppendDesc() call.

(8c) If possible, the virtio ring manipulation code should *only* be
updated with the device addresses; control flow should not be changed.

Basically, the VirtioPrepare() -> VirtioAppendDesc() -> VirtioFlush()
sequence should never be interrupted (due to error conditions or
anything else). All preparations should be done earlier and all errors
should be caught earlier.

This will require quite a bit of reorganization for this patch, so my
remaining comments, for this function anyway, will be somewhat superficial:

>  
>    //
>    // data buffer for read/write in second desc
>    //
> +  BufferMapping = NULL;

(9) This is not useful. As discussed earlier, a NULL mapping can be
valid, dependent on VIRTIO_DEVICE_PROTOCOL implementation, so it
shouldn't be used for control flow.

Instead, please use the (BufferSize > 0) condition to tell a flush
request apart from read/write requests. (See the leading comment block
on the SynchronousRequest() function.)

>    if (BufferSize > 0) {
>      //
>      // From virtio-0.9.5, 2.3.2 Descriptor Table:
> @@ -309,29 +330,86 @@ SynchronousRequest (
>      ASSERT (BufferSize <= SIZE_1GB);
>  
>      //
> +    // Map data buffer
> +    //
> +    if (RequestIsWrite) {
> +      Status = VirtioMapAllBytesInSharedBuffer (
> +                 Dev->VirtIo,
> +                 VirtioOperationBusMasterRead,
> +                 (VOID *) Buffer,
> +                 BufferSize,
> +                 &DeviceAddress,
> +                 &BufferMapping
> +                 );
> +    } else {
> +      Status = VirtioMapAllBytesInSharedBuffer (
> +                 Dev->VirtIo,
> +                 VirtioOperationBusMasterWrite,
> +                 (VOID *) Buffer,
> +                 BufferSize,
> +                 &DeviceAddress,
> +                 &BufferMapping
> +                 );
> +    }
> +
> +    if (EFI_ERROR (Status)) {
> +      goto Unmap_Request_Buffer;
> +    }
> +
> +    //
>      // VRING_DESC_F_WRITE is interpreted from the host's point of view.
>      //
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
> +    VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, (UINT32) BufferSize,
>        VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
>        &Indices);
>    }

(10) Please break every argument to a separate line.

(11) Please remove the (UINTN) cast from the device address.

>  
>    //
> +  // Map virtio-blk status header
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterWrite,
> +             (VOID *) &HostStatus,
> +             sizeof HostStatus,
> +             &DeviceAddress,
> +             &StatusMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto Unmap_Data_Buffer;
> +  }
> +
> +  //
>    // host status in last (second or third) desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
> +  VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof HostStatus,
>      VRING_DESC_F_WRITE, &Indices);

(12) Please break every argument to a separate line.

(13) Please remove the (UINTN) cast from the device address.

>  
>    //
>    // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D).
>    //
> -  if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices,
> -        NULL) == EFI_SUCCESS &&
> +  if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL) != EFI_SUCCESS) {
> +    Status = EFI_DEVICE_ERROR;
> +  }
> +

(14) The original code isn't very idiomatic here, please use the
EFI_ERROR() macro instead. (This was the first virtio driver I wrote.)

> +  //
> +  // Unmap the HostStatus buffer before accessing it
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping);

(15) Please check the return status.

(16) This is where all three (or all two) mappings should be undone
(with return statuses checked), in reverse order of mapping. If any one
of them fails, you can jump to the corresponding error handling label
(which will continue to unmap stuff, but without checking error statuses).

> +
> +  if (Status != EFI_DEVICE_ERROR &&
>        HostStatus == VIRTIO_BLK_S_OK) {
> -    return EFI_SUCCESS;
> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = EFI_DEVICE_ERROR;
>    }
>  
> -  return EFI_DEVICE_ERROR;
> +Unmap_Data_Buffer:

(17) Please don't use underscores whenever the coding style requires
CamelCase -- the label should be "UnmapDataBuffer".

> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);

(18) incorrect indentation

> +Unmap_Request_Buffer:

(19) should be "UnmapRequestBuffer".

> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +  return Status;
>  }
>  
>  
> @@ -601,6 +679,7 @@ VirtioBlkInit (
>    UINT8      AlignmentOffset;
>    UINT32     OptIoSize;
>    UINT16     QueueSize;
> +  UINT64     RingBaseShift;
>  
>    PhysicalBlockExp = 0;
>    AlignmentOffset = 0;
> @@ -728,26 +807,33 @@ VirtioBlkInit (
>      goto Failed;
>    }
>  

(20) Please add a comment here:

  //
  // If anything fails from here on, we must release the ring resources.
  //

> +  Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
> +             &Dev->RingMapping);

(21) Please break each argument to a separate line.

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

(22) Please replace the word "release" with "unmap", in the above comment.

>    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;
>    }
>  
>    //
>    // step 4c -- Report GPFN (guest-physical frame number) of queue.
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring,
> +                          RingBaseShift);

(23) Please break each argument to a separate line.

>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>  
> @@ -758,7 +844,7 @@ VirtioBlkInit (
>      Features &= ~(UINT64)VIRTIO_F_VERSION_1;
>      Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>      if (EFI_ERROR (Status)) {
> -      goto ReleaseQueue;
> +      goto UnmapQueue;
>      }
>    }
>  
> @@ -768,7 +854,7 @@ VirtioBlkInit (
>    NextDevStat |= VSTAT_DRIVER_OK;
>    Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -811,7 +897,10 @@ VirtioBlkInit (
>    }
>    return EFI_SUCCESS;
>  
> +UnmapQueue:
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping);

(24) incorrect indentation

>  ReleaseQueue:
> +

(25) I think you intended to add the empty line above the "ReleaseQueue"
label, not below it.

>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
>  Failed:

(26) You forgot to call UnmapSharedBuffer() in the VirtioBlkUninit()
function. Each ring must be unmapped in three places:

- in the device init function, if there is an error and we're past
mapping the ring,
- in the device uninit function,
- in the ExitBootServices notify function.

> @@ -885,6 +974,12 @@ VirtioBlkExitBoot (
>    //
>    Dev = Context;
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> +  //
> +  // Unmap the ring buffer so that hypervisor can not get a readable data
> +  // after device is reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping);
>  }
>  
>  /**
> 
I think I'm skipping the rest of the series for now (except the last
patch, I have comments for that).

Please rework the rest of the transport-independent virtio drivers (scsi
and net) based on my comments for this (blk) and the previous (rng) patch.

Thanks!
Laszlo