The SynchronousRequest(), programs the vring descriptor with the buffers
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;
 
 #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;
+  }
+
+  //
   // 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);
 
   //
   // data buffer for read/write in second desc
   //
+  BufferMapping = NULL;
   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);
   }
 
   //
+  // 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);
 
   //
   // 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;
+  }
+
+  //
+  // Unmap the HostStatus buffer before accessing it
+  //
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping);
+
+  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:
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
+Unmap_Request_Buffer:
+  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;
   }
 
+  Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
+             &Dev->RingMapping);
+  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;
   }
 
   //
   // 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);
   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);
 ReleaseQueue:
+
   VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
 
 Failed:
@@ -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);
 }
 
 /**
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 8/21/17 10:24 AM, Laszlo Ersek wrote:

[Snip]...

> (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.

I will go through each of your comments and try to address them in v3
but I would like to understand this code flow a bit more. When we invoke
PciIo->UnmapBuffer(...,Map) [1], it searches for Mapping in its internal
list and if found then unmaps the buffer. So idea is each Map() should
have exactly one Unmap().

So the question: is it possible that we may get called from both
ExitBootService notify and device uninit functions ? If so, then we will
have issue because now we will end up calling Unmap() twice (once from
ExitBootServices then device uninit). In my debug statement I saw the
call to ExitBootServices but was never able to trigger code path where
the device uninit is called. I am wondering if you know any method to
trigger the device uninit code flow so that I can verify both the path.

[1]
https://github.com/tianocore/edk2/blob/master/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c#L316


>> @@ -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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/22/17 03:42, Brijesh Singh wrote:
> 
> 
> On 8/21/17 10:24 AM, Laszlo Ersek wrote:
> 
> [Snip]...
> 
>> (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.
> 
> I will go through each of your comments and try to address them in v3
> but I would like to understand this code flow a bit more. When we invoke
> PciIo->UnmapBuffer(...,Map) [1], it searches for Mapping in its internal
> list and if found then unmaps the buffer. So idea is each Map() should
> have exactly one Unmap().

(Implementation details aside,) this is correct. Exactly one Unmap() is
required.

> So the question: is it possible that we may get called from both
> ExitBootService notify and device uninit functions ?

This is a valid problem statement / question, and I verified your code
with regard to it, in the patches that I reviewed.

Let's use VirtioBlkDxe as an example:

(1) The VirtioBlkDriverBindingStart() function calls VirtioBlkInit().

(2) If, and only if, VirtioBlkInit() returns with success, then the ring
has been mapped.

(3) If, and only if, VirtioBlkInit() has returned with success, then we
create the Dev->ExitBoot event (EVT_SIGNAL_EXIT_BOOT_SERVICES) in
VirtioBlkDriverBindingStart(), the notification function being
VirtioBlkExitBoot(). The task priority level at which the notification
function will be enqueued is TPL_CALLBACK (just above TPL_APPLICATION).

(4) If the protocol interface installation fails in
VirtioBlkDriverBindingStart(), then the Dev->ExitBoot is closed first
(which unregisters the notification function), and VirtioBlkUninit() is
called second (more on this below).

(5) Side topic:

According to "Table 23. TPL Restrictions" in the UEFI spec v2.7,
ExitBootServices() may only be called at TPL_APPLICATION. This means
that the notification function will actually be executed before
ExitBootServices() returns.

I'm only mentioning this because in some cases the installation of a
protocol interface has to be done very carefully. Some other module may
have set up a protocol notify e.g. at TPL_CALLBACK. If the driver
installs the protocol interface at TPL_APPLICATION (the default), the
protocol notify in the other driver may be invoked immediately, and that
driver might call back into the first driver, in order to use the
just-installed protocol. This requires that the first driver either
raise the TPL temporarily (so that the protocol installation not result
in an immediate notification in the other driver), or make sure that the
protocol being installed be immediately usable.

With regard to ExitBootServices() however, this is irrelevant. Any
protocol install notification function invoked like above runs at
TPL_CALLBACK, or at a higher task priority level. Therefore it *must
not* call ExitBootServices().

Which, ultimately, guarantees that our BlockIo installation in
VirtioBlkDriverBindingStart() will never result in a direct callback to
VirtioBlkExitBoot().

(6) Now consider VirtioBlkUninit(). With the feature in place,
VirtioBlkUninit() both unmaps and frees the ring. It is called from two
contexts: (a) when we get "far enough" in VirtioBlkDriverBindingStart()
but then fail ultimately (see point (4), and the label "UninitDev"), and
(b) when the binding succeeds just fine, and then later the driver is
asked to unbind (disconnect from) the device -- in
VirtioBlkDriverBindingStop().

In VirtioBlkDriverBindingStop(), we close Dev->ExitBoot, before we call
VirtioBlkUninit() and unmap the vring. Closing the event deregisters the
notification function, and also clears any invocations for the notify
function originally enqueued via this event.

(FWIW, I don't see how any such invocations could be pending here,
because that would imply that some agent called *both*
ExitBootServices() and our VirtioBlkDriverBindingStop() at a TPL higher
than TPL_APPLICATION -- it would be totally invalid. Nonetheless, this
is how closing an event works.)


The upshot is that three scenarios are possible:

- VirtioBlkDriverBindingStart() fails. On return from that function, the
vring is not mapped, and the exit-boot-services callback does not exit.

- VirtioBlkDriverBindingStart() succeeds. On return from the function,
the vring is mapped, and the exit-boot-services callback is live. Then,
the driver is requested to unbind the device
(VirtioBlkDriverBindingStop()). The callback is removed, and the vring
is unmapped in VirtioBlkUninit(). If ExitBootServices() is called
sometime after this, then the callback is not there any longer.

- VirtioBlkDriverBindingStart() succeeds. On return from the function,
the vring is mapped, and the exit-boot-services callback is live. Then,
ExitBootServices() is called. VirtioBlkExitBoot() runs, resetting the
virtio device, and unmapping its vring. VirtioBlkDriverBindingStop() may
never be called after this point, because boot services will have been
exited after all the callbacks run and ExitBootServices() returns.


In general ExitBootServices() notify functions behave very differently
from BindingStop() functions. The latter tear down all the structures
nicely, in reverse order of construction, freeing memory, uninitializing
devices, and so on. In comparison, the former *must not* free memory,
only abort in-flight transfers and de-configure devices surgically,
"in-place", so that they forget previous system memory references. These
two are exclusive.

When people first write UEFI drivers that follow the UEFI driver model,
they are tempted to call (or imitate) the full BindingStop() logic in
the ExitBootServices() notify function. That's wrong. Those contexts are
different with regard to system memory ownership. In BindingStop(), the
memory is owned by the driver. In the ExitBootServices() callback, the
memory is already owned by the OS (sort of), it is only on "loan" to the
driver -- after which a call to BindingStop() is impossible.


Therefore the answer to your question is, "no, that is not possible".

> If so, then we will
> have issue because now we will end up calling Unmap() twice (once from
> ExitBootServices then device uninit). In my debug statement I saw the
> call to ExitBootServices but was never able to trigger code path where
> the device uninit is called.

If you had been able to trigger such a sequence (exit-boot-services
callback followed by BindingStop), that would imply a huge problem in edk2.

> I am wondering if you know any method to
> trigger the device uninit code flow so that I can verify both the path.

Testing repeated BindingStart / BindingStop calls is indeed a good idea
-- sort of necessary -- for any UEFI driver that follows the UEFI driver
model. It's easiest to do with the CONNECT, DISCONNECT and RECONNECT
commands in the UEFI shell. (Please see their documentation in the UEFI
shell spec.)

In order to find suitable handles for testing (driver and device
handles), I recommend the DEVICES / DRIVERS / DEVTREE / DH commands.
Personally I find DH the most useful, in particular with the "-d -v" and
"-p" options.

For example, "dh -d -v -p BlockIo" should give you a detailed list of
handles with BlockIo protocol interfaces on them, and the output should
help you find the ones installed by VirtioBlkDxe.

Sorry about the length of this email (I realize a shorter email might
have been more to the point). I hope it helps a little.

Thanks
Laszlo

> 
> [1]
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c#L316
> 
> 
>>> @@ -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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Laszlo,

On 08/22/2017 06:14 AM, Laszlo Ersek wrote:

> 
>> So the question: is it possible that we may get called from both
>> ExitBootService notify and device uninit functions ?
> 
> This is a valid problem statement / question, and I verified your code
> with regard to it, in the patches that I reviewed.
> 
> Let's use VirtioBlkDxe as an example:
> 
> (1) The VirtioBlkDriverBindingStart() function calls VirtioBlkInit().
> 
> (2) If, and only if, VirtioBlkInit() returns with success, then the ring
> has been mapped.
> 
> (3) If, and only if, VirtioBlkInit() has returned with success, then we
> create the Dev->ExitBoot event (EVT_SIGNAL_EXIT_BOOT_SERVICES) in
> VirtioBlkDriverBindingStart(), the notification function being
> VirtioBlkExitBoot(). The task priority level at which the notification
> function will be enqueued is TPL_CALLBACK (just above TPL_APPLICATION).
> 
> (4) If the protocol interface installation fails in
> VirtioBlkDriverBindingStart(), then the Dev->ExitBoot is closed first
> (which unregisters the notification function), and VirtioBlkUninit() is
> called second (more on this below).
> 
> (5) Side topic:
> 
> According to "Table 23. TPL Restrictions" in the UEFI spec v2.7,
> ExitBootServices() may only be called at TPL_APPLICATION. This means
> that the notification function will actually be executed before
> ExitBootServices() returns.
> 
> I'm only mentioning this because in some cases the installation of a
> protocol interface has to be done very carefully. Some other module may
> have set up a protocol notify e.g. at TPL_CALLBACK. If the driver
> installs the protocol interface at TPL_APPLICATION (the default), the
> protocol notify in the other driver may be invoked immediately, and that
> driver might call back into the first driver, in order to use the
> just-installed protocol. This requires that the first driver either
> raise the TPL temporarily (so that the protocol installation not result
> in an immediate notification in the other driver), or make sure that the
> protocol being installed be immediately usable.
> 
> With regard to ExitBootServices() however, this is irrelevant. Any
> protocol install notification function invoked like above runs at
> TPL_CALLBACK, or at a higher task priority level. Therefore it *must
> not* call ExitBootServices().
> 
> Which, ultimately, guarantees that our BlockIo installation in
> VirtioBlkDriverBindingStart() will never result in a direct callback to
> VirtioBlkExitBoot().
> 
> (6) Now consider VirtioBlkUninit(). With the feature in place,
> VirtioBlkUninit() both unmaps and frees the ring. It is called from two
> contexts: (a) when we get "far enough" in VirtioBlkDriverBindingStart()
> but then fail ultimately (see point (4), and the label "UninitDev"), and
> (b) when the binding succeeds just fine, and then later the driver is
> asked to unbind (disconnect from) the device -- in
> VirtioBlkDriverBindingStop().
> 
> In VirtioBlkDriverBindingStop(), we close Dev->ExitBoot, before we call
> VirtioBlkUninit() and unmap the vring. Closing the event deregisters the
> notification function, and also clears any invocations for the notify
> function originally enqueued via this event.
> 
> (FWIW, I don't see how any such invocations could be pending here,
> because that would imply that some agent called *both*
> ExitBootServices() and our VirtioBlkDriverBindingStop() at a TPL higher
> than TPL_APPLICATION -- it would be totally invalid. Nonetheless, this
> is how closing an event works.)
> 
> 
> The upshot is that three scenarios are possible:
> 
> - VirtioBlkDriverBindingStart() fails. On return from that function, the
> vring is not mapped, and the exit-boot-services callback does not exit.
> 
> - VirtioBlkDriverBindingStart() succeeds. On return from the function,
> the vring is mapped, and the exit-boot-services callback is live. Then,
> the driver is requested to unbind the device
> (VirtioBlkDriverBindingStop()). The callback is removed, and the vring
> is unmapped in VirtioBlkUninit(). If ExitBootServices() is called
> sometime after this, then the callback is not there any longer.
> 
> - VirtioBlkDriverBindingStart() succeeds. On return from the function,
> the vring is mapped, and the exit-boot-services callback is live. Then,
> ExitBootServices() is called. VirtioBlkExitBoot() runs, resetting the
> virtio device, and unmapping its vring. VirtioBlkDriverBindingStop() may
> never be called after this point, because boot services will have been
> exited after all the callbacks run and ExitBootServices() returns.
> 
> 
> In general ExitBootServices() notify functions behave very differently
> from BindingStop() functions. The latter tear down all the structures
> nicely, in reverse order of construction, freeing memory, uninitializing
> devices, and so on. In comparison, the former *must not* free memory,
> only abort in-flight transfers and de-configure devices surgically,
> "in-place", so that they forget previous system memory references. These
> two are exclusive.
> 
> When people first write UEFI drivers that follow the UEFI driver model,
> they are tempted to call (or imitate) the full BindingStop() logic in
> the ExitBootServices() notify function. That's wrong. Those contexts are
> different with regard to system memory ownership. In BindingStop(), the
> memory is owned by the driver. In the ExitBootServices() callback, the
> memory is already owned by the OS (sort of), it is only on "loan" to the
> driver -- after which a call to BindingStop() is impossible.
> 
> 
> Therefore the answer to your question is, "no, that is not possible".
> 
>> If so, then we will
>> have issue because now we will end up calling Unmap() twice (once from
>> ExitBootServices then device uninit). In my debug statement I saw the
>> call to ExitBootServices but was never able to trigger code path where
>> the device uninit is called.
> 
> If you had been able to trigger such a sequence (exit-boot-services
> callback followed by BindingStop), that would imply a huge problem in edk2.
> 
>> I am wondering if you know any method to
>> trigger the device uninit code flow so that I can verify both the path.
> 
> Testing repeated BindingStart / BindingStop calls is indeed a good idea
> -- sort of necessary -- for any UEFI driver that follows the UEFI driver
> model. It's easiest to do with the CONNECT, DISCONNECT and RECONNECT
> commands in the UEFI shell. (Please see their documentation in the UEFI
> shell spec.)
> 
> In order to find suitable handles for testing (driver and device
> handles), I recommend the DEVICES / DRIVERS / DEVTREE / DH commands.
> Personally I find DH the most useful, in particular with the "-d -v" and
> "-p" options.
> 
> For example, "dh -d -v -p BlockIo" should give you a detailed list of
> handles with BlockIo protocol interfaces on them, and the output should
> help you find the ones installed by VirtioBlkDxe.
> 
> Sorry about the length of this email (I realize a shorter email might
> have been more to the point). I hope it helps a little.
> 


Thanks for detail explanation, I was trying driver unload and reload from
the UEFI shell but that did not trigger the BindStop hence I was not sure
how it all works. But your explanation makes sense. I will try connect and
disconnect to trigger the code path and make sure that all logical code path
have been tested before we commit the patch :)

-Brijesh

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/22/17 16:29, Brijesh Singh wrote:

> Thanks for detail explanation, I was trying driver unload and reload
> from the UEFI shell but that did not trigger the BindStop hence I was
> not sure how it all works.

*Unload* is a different thing. Clearly a driver can only be unloaded
after all its currently bound devices are unbound, but there is no
dependency in the other direction -- a UEFI_DRIVER module can perfectly
well unbind devices *without* being unloadable.

An edk2 driver module that is unloadable specifies the UNLOAD_IMAGE
define in its INF file. Pretty few drivers do this. (None of the virtio
drivers.)

The "Driver Writer’s Guide for UEFI 2.3.1" writes about unloading in
"7.6 Adding the Unload Feature" (and possibly elsewhere).

> But your explanation makes sense. I will try connect and disconnect
> to trigger the code path and make sure that all logical code path 
> have been tested before we commit the patch :)

Sounds good, thanks!

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel