Index: /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp =================================================================== --- /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 80682) +++ /trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (revision 80683) @@ -515,10 +515,4 @@ /** True if in the process of resetting */ bool fResetting; - - /** True if in the process of suspending */ - bool fSuspending; - - /** True if in the process of powering off */ - bool fPoweringOff; /** True if in the process of quiescing I/O */ @@ -1673,30 +1667,22 @@ } -/** Let guest to use the queues */ -static void enableQueues(PVIRTIOSCSI pThis) -{ - for (int qIdx = 0; qIdx < VIRTIOSCSI_QUEUE_CNT; qIdx++) - virtioQueueEnable(pThis->hVirtio, qIdx, true); -} - -/** Tell guest to stop using the queues */ -static void disableQueues(PVIRTIOSCSI pThis) -{ - /** Tell guest to stop sending stuff */ - for (int qIdx = 0; qIdx < VIRTIOSCSI_QUEUE_CNT; qIdx++) - virtioQueueEnable(pThis->hVirtio, qIdx, false); -} - -/** - * Handle callback from PDM's async notification mechanism. - * In this case, the callback is triggered when I/O activity is quiesced. - * - * @returns true if we've quiesced, false if we're still working. - * @param pDevIns The device instance. +/** + * Is asynchronous handling of suspend or power off notification completed? + * + * This is called to check whether the device has quiesced. Don't deadlock. + * Avoid blocking. Do NOT wait for anything. + * + * @returns true if done, false if more work to be done. + * + * @param pDevIns The device instance. + * @remarks The caller will enter the device critical section. + * @thread EMT(0) */ static DECLCALLBACK(bool) virtioScsiDeviceQuiesced(PPDMDEVINS pDevIns) { - LogFunc(("\n")); + LogFunc(("Device I/O activity quiesced.\n")); PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); + + pThis->fQuiescing = false; if (pThis->fResetting) @@ -1707,17 +1693,10 @@ virtioResetAll(pThis->hVirtio); - /** Reset negotiable device-specific config parameters to VirtIO-specified default values */ + /** Reset locally-owned negotiable device-specific config parameters + * to VirtIO spec-mandated default values */ pThis->virtioScsiConfig.uSenseSize = VIRTIOSCSI_SENSE_SIZE_DEFAULT; pThis->virtioScsiConfig.uCdbSize = VIRTIOSCSI_CDB_SIZE_DEFAULT; - - pThis->fQuiescing = false; - - /** fQuiescing and fReset flags get cleared during [re-]initialization */ - } - else if (pThis->fSuspending || pThis->fPoweringOff) - { - /* Lower-level driver (DrvVD) has already suspended any I/O on wait queue */ - } - return false; + } + return true; } @@ -1726,6 +1705,6 @@ PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - pThis->fQuiescing = true; /* inhibit worker thread's de-queing */ - disableQueues(pThis); /* inhibit guest use of virtq's */ + /** Prevent worker threads from removing/processing elements from virtq's */ + pThis->fQuiescing = true; PDMDevHlpSetAsyncNotification(pDevIns, virtioScsiDeviceQuiesced); @@ -1763,12 +1742,43 @@ /** - * @interface_method_impl{PDMDEVREG,pfnResume} - */ -static DECLCALLBACK(void) virtioScsiResume(PPDMDEVINS pDevIns) + * @copydoc FNPDMDEVRESET + */ +static DECLCALLBACK(void) virtioScsiReset(PPDMDEVINS pDevIns) { LogFunc(("\n")); PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - pThis->fSuspending = pThis->fQuiescing = false; - enableQueues(pThis); + pThis->fResetting = true; + virtioScsiQuiesceDevice(pDevIns); +} + +/** + * @interface_method_impl{PDMDEVREG,pfnResume} + */ +static DECLCALLBACK(void) virtioScsiResume(PPDMDEVINS pDevIns) +{ + LogFunc(("\n")); + + PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); + + pThis->fQuiescing = false; + + /** Wake worker threads flagged to skip pulling queue entries during quiesce + * to ensure they re-check their queues. Active request queues may already + * be awake due to new reqs coming in. + */ + for (uint16_t qIdx = 0; qIdx < VIRTIOSCSI_REQ_QUEUE_CNT; qIdx++) + { + PWORKER pWorker = &pThis->aWorker[qIdx]; + + if (ASMAtomicReadBool(&pWorker->fSleeping)) + { + Log6Func(("waking %s worker.\n", QUEUENAME(qIdx))); + int rc = SUPSemEventSignal(pThis->pSupDrvSession, pWorker->hEvtProcess); + AssertRC(rc); + } + } + + /** Ensure guest is working the queues too. */ + virtioPropagateResumeNotification(pThis->hVirtio); } @@ -1776,14 +1786,18 @@ * @interface_method_impl{PDMDEVREG,pfnSuspend} */ -static DECLCALLBACK(void) virtioScsiSuspend(PPDMDEVINS pDevIns) +static DECLCALLBACK(void) virtioScsiSuspendOrPoweroff(PPDMDEVINS pDevIns) { LogFunc(("\n")); + virtioScsiQuiesceDevice(pDevIns); + PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - pThis->fSuspending = true; - - virtioScsiQuiesceDevice(pDevIns); - + /** VM is halted, thus no new I/O being dumped into queues by the guest. + * Workers have been flagged to stop pulling stuff already queued-up by the guest. + * Now tell lower-level to to suspend reqs (for example, DrvVD suspends all reqs + * on its wait queue, and we will get a callback as the state changes to + * suspended (and later, resumed) for each). + */ for (uint32_t i = 0; i < pThis->cTargets; i++) { @@ -1793,49 +1807,4 @@ pTarget->pDrvMediaEx->pfnNotifySuspend(pTarget->pDrvMediaEx); } -} - - /** - * Common worker for virtioScsiSuspend and virtioScsiPowerOff. - */ -static void virtioScsiPowerOn(PPDMDEVINS pDevIns) -{ - LogFunc(("\n")); - PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - pThis->fPoweringOff = pThis->fQuiescing = false; - enableQueues(pThis); -} - - /** - * Common worker for virtioScsiSuspend and virtioScsiPowerOff. - */ -static void virtioScsiPowerOff(PPDMDEVINS pDevIns) -{ - LogFunc(("\n")); - - PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - - pThis->fPoweringOff = true; - virtioScsiQuiesceDevice(pDevIns); - - for (uint32_t i = 0; i < pThis->cTargets; i++) - { - PVIRTIOSCSITARGET pTarget = &pThis->aTargetInstances[i]; - if (pTarget->pDrvBase) - if (pTarget->pDrvMediaEx) - pTarget->pDrvMediaEx->pfnNotifySuspend(pTarget->pDrvMediaEx); - } -} - -/** - * @copydoc FNPDMDEVRESET - */ -static DECLCALLBACK(void) virtioScsiReset(PPDMDEVINS pDevIns) -{ - LogFunc(("\n")); - - PVIRTIOSCSI pThis = PDMINS_2_DATA(pDevIns, PVIRTIOSCSI); - pThis->fResetting = true; - - virtioScsiQuiesceDevice(pDevIns); } @@ -2279,5 +2248,5 @@ if (RT_FAILURE(rc)) return PDMDevHlpVMSetError(pDevIns, rc, RT_SRC_POS, - N_("LsiLogic: Failed to create SUP event semaphore")); + N_("DevVirtioSCSI: Failed to create SUP event semaphore")); } } @@ -2421,7 +2390,7 @@ /* .pfnRelocate = */ virtioScsiRelocate, /* .pfnMemSetup = */ NULL, - /* .pfnPowerOn = */ virtioScsiPowerOn, + /* .pfnPowerOn = */ NULL, /* .pfnReset = */ virtioScsiReset, - /* .pfnSuspend = */ virtioScsiSuspend, + /* .pfnSuspend = */ virtioScsiSuspendOrPoweroff, /* .pfnResume = */ virtioScsiResume, /* .pfnAttach = */ virtioScsiAttach, @@ -2429,5 +2398,5 @@ /* .pfnQueryInterface = */ NULL, /* .pfnInitComplete = */ NULL, - /* .pfnPowerOff = */ virtioScsiPowerOff, + /* .pfnPowerOff = */ virtioScsiSuspendOrPoweroff, /* .pfnSoftReset = */ NULL, /* .pfnReserved0 = */ NULL, Index: /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp =================================================================== --- /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp (revision 80682) +++ /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp (revision 80683) @@ -88,4 +88,5 @@ return VINF_SUCCESS; } + /** @@ -294,5 +295,5 @@ virtioWriteUsedRingIdx(pVirtio, qIdx, pVirtqProxy->uUsedIdx); - virtioNotifyGuestDriver(pVirtio, qIdx); + virtioNotifyGuestDriver(pVirtio, qIdx, false); return VINF_SUCCESS; @@ -309,7 +310,14 @@ Log6Func(("%s\n", pVirtqProxy->szVirtqName)); - /** Inform client */ pVirtio->virtioCallbacks.pfnVirtioQueueNotified((VIRTIOHANDLE)pVirtio, pVirtio->pClientContext, qIdx); +} + +/** + * See API comments in header file for description + */ +void virtioPropagateResumeNotification(VIRTIOHANDLE hVirtio) +{ + virtioNotifyGuestDriver((PVIRTIOSTATE)hVirtio, (uint16_t)NULL /* qIdx */, true /* fForce */); } @@ -319,13 +327,20 @@ * and depending on negotiated and realtime constraints flagged by the guest driver. * See VirtIO 1.0 specification (section 2.4.7). - */ -static void virtioNotifyGuestDriver(PVIRTIOSTATE pVirtio, uint16_t qIdx) + * + * @param pVirtio - Instance state + * @param qIdx - Queue to check for guest interrupt handling preference + * @param fForce - Overrides qIdx, forcing notification, regardless of driver's + * notification preferences. This is a safeguard to prevent + * stalls upon resuming the VM. VirtIO 1.0 specification Section 4.1.5.5 + * indicates spurious interrupts are harmless to guest driver's state, + * as they only cause the guest driver to scan queues for work to do. + */ +static void virtioNotifyGuestDriver(PVIRTIOSTATE pVirtio, uint16_t qIdx, bool fForce) { PVIRTQ_PROXY_T pVirtqProxy = &pVirtio->virtqProxy[qIdx]; - AssertMsgReturnVoid(DRIVER_OK(pVirtio) && pVirtio->uQueueEnable[qIdx], - ("Guest driver not in ready state.\n")); - - if (pVirtio->uQueueMsixVector[qIdx] == VIRTIO_MSI_NO_VECTOR) + AssertMsgReturnVoid(DRIVER_OK(pVirtio), ("Guest driver not in ready state.\n")); + + if (pVirtio->uMsixConfig == VIRTIO_MSI_NO_VECTOR) { if (pVirtio->uDriverFeatures & VIRTIO_F_EVENT_IDX) @@ -333,5 +348,5 @@ if (pVirtqProxy->fEventThresholdReached) { - virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_VIRTQ_INTERRUPT); + virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_VIRTQ_INTERRUPT, fForce); pVirtqProxy->fEventThresholdReached = false; return; @@ -342,11 +357,10 @@ { /** If guest driver hasn't suppressed interrupts, interrupt */ - if (!(virtioReadUsedFlags(pVirtio, qIdx) & VIRTQ_AVAIL_F_NO_INTERRUPT)) + if (fForce || !(virtioReadUsedFlags(pVirtio, qIdx) & VIRTQ_AVAIL_F_NO_INTERRUPT)) { - virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_VIRTQ_INTERRUPT); + virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_VIRTQ_INTERRUPT, fForce); return; } Log6Func(("...skipping interrupt. Guest flagged VIRTQ_AVAIL_F_NO_INTERRUPT for queue\n")); - } } @@ -392,6 +406,10 @@ * @param uCause Interrupt cause bit mask to set in PCI ISR port. */ -static int virtioRaiseInterrupt(PVIRTIOSTATE pVirtio, uint8_t uCause) -{ +static int virtioRaiseInterrupt(PVIRTIOSTATE pVirtio, uint8_t uCause, bool fForce) +{ + + if (fForce) + Log6Func(("reason: resumed after suspend\n")); + else if (uCause == VIRTIO_ISR_VIRTQ_INTERRUPT) Log6Func(("reason: buffer added to 'used' ring.\n")); @@ -480,5 +498,5 @@ { pVirtio->fGenUpdatePending = true; - virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_DEVICE_CONFIG); + virtioRaiseInterrupt(pVirtio, VIRTIO_ISR_DEVICE_CONFIG, false /* fForce */); } } Index: /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h =================================================================== --- /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h (revision 80682) +++ /trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h (revision 80683) @@ -289,4 +289,12 @@ void virtioResetAll(VIRTIOHANDLE hVirtio); +/** + * This sends notification ('kicks') guest driver to check queues for any new + * elements in the used queue to process. It should be called after resuming + * in case anything was added to the queues during suspend/quiescing and a + * notification was missed, to prevent the guest from stalling after suspend. + */ +void virtioPropagateResumeNotification(VIRTIOHANDLE hVirtio); + Index: /trunk/src/VBox/Devices/VirtIO/Virtio_1_0_impl.h =================================================================== --- /trunk/src/VBox/Devices/VirtIO/Virtio_1_0_impl.h (revision 80682) +++ /trunk/src/VBox/Devices/VirtIO/Virtio_1_0_impl.h (revision 80683) @@ -535,6 +535,6 @@ static void virtioResetQueue (PVIRTIOSTATE pVirtio, uint16_t qIdx); -static void virtioNotifyGuestDriver (PVIRTIOSTATE pVirtio, uint16_t qIdx); -static int virtioRaiseInterrupt (PVIRTIOSTATE pVirtio, uint8_t uCause); +static void virtioNotifyGuestDriver (PVIRTIOSTATE pVirtio, uint16_t qIdx, bool fForce); +static int virtioRaiseInterrupt (PVIRTIOSTATE pVirtio, uint8_t uCause, bool fForce); static void virtioLowerInterrupt (PVIRTIOSTATE pVirtio); static void virtioQueueNotified (PVIRTIOSTATE pVirtio, uint16_t qidx, uint16_t uDescIdx);