Skip to content

Commit

Permalink
Nutanix-ENG-741981 [vioscsi] Fix NMI in crashdump/hibernation pathway…
Browse files Browse the repository at this point in the history 
… (regression)

Credit to Nutanix and in particular @MartinCHarvey-Nutanix for his work in PR virtio-win#1293.

Background: We previously ignored calls for a spinlock with isr=TRUE in
VioScsiVQLock() and VioScsiVQUnlock(). This was replaced with a call to
InterruptLock in the (!IsCrashDumpMode && adaptExt->dpc_ok) = FALSE pathway.
In testing, suspend/resume/hibernate did not use this pathway but instead
issued DPCs. The InterruptLock was presumed to be used when IsCrashDumpMode=TRUE.
Also, using PVOID LockContext = NULL, and / or then setting LockContext
to &adaptExt->dpc[vq_req_idx], appears to cause a HCK Flush Test failure.

Created new overloaded enumeration called CUSTOM_STOR_SPINLOCK which adds some
new (invalid) spinlock types such as Skip_Locking and No_Lock. Also provides InvalidLock
for builds prior to NTDDI_WIN11_GE (Windows 11, version 24H2, build 26100) via Invalid_Lock.
In similar vein, Dpc_Lock = DpcLock, StartIo_Lock = StartIoLock, Interrupt_Lock =
InterruptLock, ThreadedDpc_Lock = ThreadedDpcLock & ThreadedDpc_Lock = ThreadedDpcLock.

This fix has two components:

1. Only DpcLock type spinlocks are processed in ProcessBuffer() with all other
   types presently being ignored ; and
2. The (PVOID)LockContext is no longer used, with calls to StorPortAcquireSpinLock()
   for DpcLock type spinlocks using &adaptExt->dpc[vq_req_idx] directly.

Note: Use of InvalidLock requires Win11 and both InvalidLock and DpcLevelLock
      require using StorPortAcquireSpinLockEx. Consider for future use.

Signed-off-by: benyamin-codez <[email protected]>
  • Loading branch information
@benyamin-codez
benyamin-codez committed Feb 22, 2025
1 parent 2bcf04f commit 09adb8d
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 20 deletions.
4 changes: 1 addition & 3 deletions vioscsi/helper.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ VOID SendSRB(IN PVOID DeviceExtension, IN PSRB_TYPE Srb)
ULONG QueueNumber = VIRTIO_SCSI_REQUEST_QUEUE_0;
BOOLEAN notify = FALSE;
STOR_LOCK_HANDLE LockHandle = {0};
PVOID LockContext;
ULONG status = STOR_STATUS_SUCCESS;
UCHAR ScsiStatus = SCSISTAT_GOOD;
INT add_buffer_req_status = VQ_ADD_BUFFER_SUCCESS;
Expand Down Expand Up @@ -107,7 +106,6 @@ VOID SendSRB(IN PVOID DeviceExtension, IN PSRB_TYPE Srb)
}

vq_req_idx = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0;
LockContext = &adaptExt->dpc[vq_req_idx];

if (adaptExt->reset_in_progress)
{
Expand All @@ -117,7 +115,7 @@ VOID SendSRB(IN PVOID DeviceExtension, IN PSRB_TYPE Srb)
return;
}

StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
StorPortAcquireSpinLock(DeviceExtension, DpcLock, &adaptExt->dpc[vq_req_idx], &LockHandle);
SET_VA_PA();
add_buffer_req_status = virtqueue_add_buf(adaptExt->vq[QueueNumber],
srbExt->psgl,
Expand Down
2 changes: 1 addition & 1 deletion vioscsi/helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ SynchronizedKickEventRoutine(IN PVOID DeviceExtension, IN PVOID Context);

VOID VioScsiCompleteDpcRoutine(IN PSTOR_DPC Dpc, IN PVOID Context, IN PVOID SystemArgument1, IN PVOID SystemArgument2);

VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN STOR_SPINLOCK LockMode);
VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN CUSTOM_STOR_SPINLOCK LockMode);

VOID
// FORCEINLINE
Expand Down
30 changes: 14 additions & 16 deletions vioscsi/vioscsi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ VioScsiInterrupt(IN PVOID DeviceExtension)
}
else
{
ProcessBuffer(DeviceExtension, QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0), InterruptLock);
ProcessBuffer(DeviceExtension, QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0), Skip_Locking);
}
}

Expand Down Expand Up @@ -1243,14 +1243,12 @@ VioScsiUnitControl(IN PVOID DeviceExtension, IN SCSI_UNIT_CONTROL_TYPE ControlTy
ULONG vq_req_idx;
PREQUEST_LIST element;
STOR_LOCK_HANDLE LockHandle = {0};
PVOID LockContext = NULL; // sanity check for LockMode = InterruptLock or StartIoLock
PSTOR_ADDR_BTL8 stor_addr = (PSTOR_ADDR_BTL8)Parameters;

for (vq_req_idx = 0; vq_req_idx < adaptExt->num_queues; vq_req_idx++)
{
element = &adaptExt->processing_srbs[vq_req_idx];
LockContext = &adaptExt->dpc[vq_req_idx];
StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
StorPortAcquireSpinLock(DeviceExtension, DpcLock, &adaptExt->dpc[vq_req_idx], &LockHandle);
if (!IsListEmpty(&element->srb_list))
{
PLIST_ENTRY entry = element->srb_list.Flink;
Expand Down Expand Up @@ -1414,14 +1412,15 @@ VOID FORCEINLINE DispatchQueue(IN PVOID DeviceExtension, IN ULONG MessageId)
&adaptExt->dpc[MessageId - QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0)],
ULongToPtr(MessageId),
ULongToPtr(MessageId));
EXIT_FN();
return;
}
ProcessBuffer(DeviceExtension, MessageId, InterruptLock);
else
{
ProcessBuffer(DeviceExtension, MessageId, Skip_Locking);
}
EXIT_FN();
}

VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN STOR_SPINLOCK LockMode)
VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN CUSTOM_STOR_SPINLOCK LockMode)
{
PVirtIOSCSICmd cmd;
unsigned int len;
Expand All @@ -1433,7 +1432,6 @@ VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN STOR_SPINLOC
PSRB_EXTENSION srbExt = NULL;
PREQUEST_LIST element;
ULONG vq_req_idx;
PVOID LockContext = NULL; // sanity check for LockMode = InterruptLock or StartIoLock

ENTER_FN();

Expand All @@ -1450,9 +1448,8 @@ VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN STOR_SPINLOC

if (LockMode == DpcLock)
{
LockContext = &adaptExt->dpc[vq_req_idx];
StorPortAcquireSpinLock(DeviceExtension, LockMode, &adaptExt->dpc[vq_req_idx], &LockHandle);
}
StorPortAcquireSpinLock(DeviceExtension, LockMode, LockContext, &LockHandle);

do
{
Expand Down Expand Up @@ -1488,7 +1485,10 @@ VOID ProcessBuffer(IN PVOID DeviceExtension, IN ULONG MessageId, IN STOR_SPINLOC
}
} while (!virtqueue_enable_cb(vq));

StorPortReleaseSpinLock(DeviceExtension, &LockHandle);
if (LockMode == DpcLock)
{
StorPortReleaseSpinLock(DeviceExtension, &LockHandle);
}

EXIT_FN();
}
Expand All @@ -1499,7 +1499,7 @@ VOID VioScsiCompleteDpcRoutine(IN PSTOR_DPC Dpc, IN PVOID Context, IN PVOID Syst

ENTER_FN();
MessageId = PtrToUlong(SystemArgument1);
ProcessBuffer(Context, MessageId, DpcLock);
ProcessBuffer(Context, MessageId, Dpc_Lock);
EXIT_FN();
}

Expand All @@ -1510,7 +1510,6 @@ VOID CompletePendingRequestsOnReset(IN PVOID DeviceExtension)
ULONG vq_req_idx;
PREQUEST_LIST element;
STOR_LOCK_HANDLE LockHandle = {0};
PVOID LockContext = NULL; // sanity check for LockMode = InterruptLock or StartIoLock

adaptExt = (PADAPTER_EXTENSION)DeviceExtension;

Expand All @@ -1524,8 +1523,7 @@ VOID CompletePendingRequestsOnReset(IN PVOID DeviceExtension)
{
element = &adaptExt->processing_srbs[vq_req_idx];
RhelDbgPrint(TRACE_LEVEL_FATAL, " queue %d cnt %d\n", vq_req_idx, element->srb_cnt);
LockContext = &adaptExt->dpc[vq_req_idx];
StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
StorPortAcquireSpinLock(DeviceExtension, DpcLock, &adaptExt->dpc[vq_req_idx], &LockHandle);
while (!IsListEmpty(&element->srb_list))
{
PLIST_ENTRY entry = RemoveHeadList(&element->srb_list);
Expand Down
16 changes: 16 additions & 0 deletions vioscsi/vioscsi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,22 @@ typedef enum ACTION_ON_RESET
VioscsiResetBugCheck = 0xDEADDEAD,
} ACTION_ON_RESET;

typedef enum _CUSTOM_STOR_SPINLOCK
{
Skip_Locking = 0,
No_Lock = 0,
#if defined(NTDDI_WIN11_GE) && (NTDDI_VERSION >= NTDDI_WIN11_GE)
Invalid_Lock = InvalidLock,
#else
Invalid_Lock = 0,
#endif
Dpc_Lock = DpcLock,
StartIo_Lock = StartIoLock,
Interrupt_Lock = InterruptLock,
ThreadedDpc_Lock = ThreadedDpcLock,
DpcLevel_Lock = DpcLevelLock
} CUSTOM_STOR_SPINLOCK;

typedef struct _ADAPTER_EXTENSION
{
VirtIODevice vdev;
Expand Down

0 comments on commit 09adb8d

Please sign in to comment.