[viostor]: Use the Status table to hold the command header as well

When a pending request gets completed by the driver before it is handled by the host, it may happen that the host have not read the command header yet. This means a read from a physical memory no longer owned by the driver. Moving the physical memory holding the command header to the Status table as well enforces that the physical memory is always owned by the driver when the host code is reading from it.

Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>

Author: Martin Drab

viostor/virtio_status_table.c

@@ -30,6 +30,7 @@
  * SUCH DAMAGE.
  */
 #include <ntddk.h>
+#include "virtio_stor.h"
 #include "virtio_stor_trace.h"
 #include "virtio_status_table.h"
 #if defined(EVENT_TRACING)
@@ -65,12 +66,12 @@ void StatusTableInit(PSTATUS_TABLE Table)
     return;
 }
 
-unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id)
+PSTATUS_TABLE_ENTRY StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id)
 {
     size_t index = 0;
     size_t attempt = 0;
     size_t nextIndex = 0;
-    unsigned char *ret = NULL;
+    PSTATUS_TABLE_ENTRY ret = NULL;
     PSTATUS_TABLE_ENTRY entry = NULL;
 
     while (InterlockedCompareExchange(&Table->Lock, 1, 0))
@@ -88,7 +89,7 @@ unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id)
             entry->Present = 1;
             entry->Deleted = 0;
             entry->Id = Id;
-            ret = &entry->Status;
+            ret = entry;
             break;
         }
 

viostor/virtio_status_table.h

@@ -31,25 +31,10 @@
 #define __VIRTIO_STATUS_TABLE_H__
 
 #include <ntddk.h>
-
-typedef struct _STATUS_TABLE_ENTRY
-{
-    unsigned char Status;
-    ULONG64 Id;
-    int Present : 1;
-    int Deleted : 1;
-} STATUS_TABLE_ENTRY, *PSTATUS_TABLE_ENTRY;
-
-#define VIRTIO_STATUS_TABLE_SIZE 769
-
-typedef struct _STATUS_TABLE
-{
-    STATUS_TABLE_ENTRY Entries[VIRTIO_STATUS_TABLE_SIZE];
-    volatile LONG Lock;
-} STATUS_TABLE, *PSTATUS_TABLE;
+#include "virtio_stor.h"
 
 void StatusTableInit(PSTATUS_TABLE Table);
-unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id);
+PSTATUS_TABLE_ENTRY StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id);
 void StatusTableDelete(PSTATUS_TABLE Table, ULONG64 Id, unsigned char *Status);
 
 #endif

viostor/virtio_stor.c

@@ -30,6 +30,7 @@
  * SUCH DAMAGE.
  */
 #include "virtio_stor.h"
+#include "virtio_status_table.h"
 #if defined(EVENT_TRACING)
 #include "virtio_stor.tmh"
 #endif

viostor/virtio_stor.h

@@ -41,7 +41,6 @@
 #include "virtio_ring.h"
 #include "virtio_stor_utils.h"
 #include "virtio_stor_hw_helper.h"
-#include "virtio_status_table.h"
 
 typedef struct VirtIOBufferDescriptor VIO_SG, *PVIO_SG;
 
@@ -207,6 +206,23 @@ typedef struct _REQUEST_LIST
     ULONG srb_cnt;
 } REQUEST_LIST, *PREQUEST_LIST;
 
+typedef struct _STATUS_TABLE_ENTRY
+{
+    blk_outhdr OutHdr;
+    unsigned char Status;
+    ULONG64 Id;
+    int Present : 1;
+    int Deleted : 1;
+} STATUS_TABLE_ENTRY, * PSTATUS_TABLE_ENTRY;
+
+#define VIRTIO_STATUS_TABLE_SIZE 769
+
+typedef struct _STATUS_TABLE
+{
+    STATUS_TABLE_ENTRY Entries[VIRTIO_STATUS_TABLE_SIZE];
+    volatile LONG Lock;
+} STATUS_TABLE, * PSTATUS_TABLE;
+
 typedef struct _ADAPTER_EXTENSION
 {
     VirtIODevice vdev;

viostor/virtio_stor_hw_helper.c

@@ -68,7 +68,7 @@ static BOOLEAN _SendSRB(PVOID DeviceExtension,
                         ULONGLONG PA)
 {
     ULONG dummy = 1;
-    unsigned char *pStatus = NULL;
+    PSTATUS_TABLE_ENTRY opEntry = NULL;
     PADAPTER_EXTENSION adaptExt = NULL;
     PSRB_EXTENSION srbExt = NULL;
     PREQUEST_LIST element;
@@ -79,20 +79,34 @@ static BOOLEAN _SendSRB(PVOID DeviceExtension,
 
     adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
     srbExt = SRB_EXTENSION(Srb);
-    pStatus = StatusTableInsert(&adaptExt->StatusTable, srbExt->id);
-    if (pStatus == NULL)
+    opEntry = StatusTableInsert(&adaptExt->StatusTable, srbExt->id);
+    if (opEntry == NULL)
     {
         RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to allocate space for holding status for Srb 0x%p\n", Srb);
         return FALSE;
     }
 
+    opEntry->OutHdr = srbExt->vbr.out_hdr;
+    srbExt->sg[0].physAddr = StorPortGetPhysicalAddress(DeviceExtension,
+                                                        NULL,
+                                                        &opEntry->OutHdr,
+                                                        &dummy);
+    srbExt->sg[0].length = sizeof(opEntry->OutHdr);
+    if (srbExt->sg[0].physAddr.QuadPart == 0)
+    {
+        RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to translate address 0x%p, Srb 0x%p\n", &opEntry->OutHdr, Srb);
+        StatusTableDelete(&adaptExt->StatusTable, srbExt->id, NULL);
+        return FALSE;
+    }
+
     srbExt->sg[srbExt->in + srbExt->out - 1].physAddr = StorPortGetPhysicalAddress(DeviceExtension,
                                                                                    NULL,
-                                                                                   pStatus,
+                                                                                   &opEntry->Status,
                                                                                    &dummy);
+    srbExt->sg[srbExt->in + srbExt->out - 1].length = sizeof(opEntry->Status);
     if (srbExt->sg[srbExt->in + srbExt->out - 1].physAddr.QuadPart == 0)
     {
-        RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to translate address 0x%p, Srb 0x%p\n", pStatus, Srb);
+        RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to translate address 0x%p, Srb 0x%p\n", &opEntry->Status, Srb);
         StatusTableDelete(&adaptExt->StatusTable, srbExt->id, NULL);
         return FALSE;
     }
@@ -146,7 +160,6 @@ RhelDoFlush(PVOID DeviceExtension, PSRB_TYPE Srb, BOOLEAN resend, BOOLEAN bIsr)
 {
     PADAPTER_EXTENSION adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
     PSRB_EXTENSION srbExt = SRB_EXTENSION(Srb);
-    ULONG fragLen = 0UL;
     PVOID va = NULL;
     ULONGLONG pa = 0ULL;
 
@@ -197,11 +210,6 @@ RhelDoFlush(PVOID DeviceExtension, PSRB_TYPE Srb, BOOLEAN resend, BOOLEAN bIsr)
     srbExt->out = 1;
     srbExt->in = 1;
 
-    srbExt->sg[0].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.out_hdr, &fragLen);
-    srbExt->sg[0].length = sizeof(srbExt->vbr.out_hdr);
-    srbExt->sg[1].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.status, &fragLen);
-    srbExt->sg[1].length = sizeof(srbExt->vbr.status);
-
     return _SendSRB(DeviceExtension, Srb, QueueNumber, MessageId, resend, va, pa);
 }
 
@@ -332,12 +340,8 @@ RhelDoUnMap(IN PVOID DeviceExtension, IN PSRB_TYPE Srb)
     srbExt->out = 2;
     srbExt->in = 1;
 
-    srbExt->sg[0].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.out_hdr, &fragLen);
-    srbExt->sg[0].length = sizeof(srbExt->vbr.out_hdr);
     srbExt->sg[1].physAddr = MmGetPhysicalAddress(&adaptExt->blk_discard[0]);
     srbExt->sg[1].length = sizeof(blk_discard_write_zeroes) * BlockDescrCount;
-    srbExt->sg[2].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.status, &fragLen);
-    srbExt->sg[2].length = sizeof(srbExt->vbr.status);
 
     if (adaptExt->num_queues > 1)
     {
@@ -427,12 +431,8 @@ RhelGetSerialNumber(IN PVOID DeviceExtension, IN PSRB_TYPE Srb)
     srbExt->out = 1;
     srbExt->in = 2;
 
-    srbExt->sg[0].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.out_hdr, &fragLen);
-    srbExt->sg[0].length = sizeof(srbExt->vbr.out_hdr);
     srbExt->sg[1].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &adaptExt->sn[0], &fragLen);
     srbExt->sg[1].length = sizeof(adaptExt->sn);
-    srbExt->sg[2].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.status, &fragLen);
-    srbExt->sg[2].length = sizeof(srbExt->vbr.status);
 
     return _SendSRB(DeviceExtension, Srb, QueueNumber, MessageId, FALSE, va, pa);
 }