Changeset 25148 in vbox

Timestamp:

Dec 2, 2009 2:20:41 PM (15 years ago)

Author:

vboxsync

Message:

#3987: Virtio: Fixed the race for TX queue notification state.

Location:

trunk/src/VBox/Devices

Files:

• Network/DevVirtioNet.cpp (modified) (15 diffs)
• VirtIO/Virtio.cpp (modified) (6 diffs)

trunk/src/VBox/Devices/Network/DevVirtioNet.cpp

@@ -114 +114 @@
     VPCISTATE               VPCI;
 
+    PDMCRITSECT             csRx;                           /**< Protects RX queue. */
+
     PDMINETWORKPORT         INetworkPort;
     PDMINETWORKCONFIG       INetworkConfig;
@@ -250 +252 @@
 }
 
+DECLINLINE(int) vnetCsRxEnter(PVNETSTATE pState, int rcBusy)
+{
+    return PDMCritSectEnter(&pState->csRx, rcBusy);
+}
+
+DECLINLINE(void) vnetCsRxLeave(PVNETSTATE pState)
+{
+    PDMCritSectLeave(&pState->csRx);
+}
+
 
 PDMBOTHCBDECL(uint32_t) vnetGetHostFeatures(void *pvState)
@@ -313 +325 @@
     VNETSTATE *pState = (VNETSTATE*)pvState;
     Log(("%s Reset triggered\n", INSTANCE(pState)));
+
+    int rc = vnetCsRxEnter(pState, VERR_SEM_BUSY);
+    if (RT_UNLIKELY(rc != VINF_SUCCESS))
+    {
+        LogRel(("vnetReset failed to enter RX critical section!\n"));
+        return;
+    }
     vpciReset(&pState->VPCI);
+    vnetCsRxLeave(pState);
+
     // TODO: Implement reset
     if (pState->fCableConnected)
@@ -358 +379 @@
     VNETSTATE *pState = (VNETSTATE *)pvUser;
 
+    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    if (RT_UNLIKELY(rc != VINF_SUCCESS))
+        return;
     STATUS |= VNET_S_LINK_UP;
     vpciRaiseInterrupt(&pState->VPCI, VERR_SEM_BUSY, VPCI_ISR_CONFIG);
     vnetWakeupReceive(pDevIns);
+    vnetCsLeave(pState);
 }
 
@@ -457 +482 @@
 static int vnetCanReceive(VNETSTATE *pState)
 {
-    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    int rc = vnetCsRxEnter(pState, VERR_SEM_BUSY);
+    AssertRCReturn(rc, rc);
+
     LogFlow(("%s vnetCanReceive\n", INSTANCE(pState)));
     if (!(pState->VPCI.uStatus & VPCI_STATUS_DRV_OK))
@@ -475 +502 @@
 
     LogFlow(("%s vnetCanReceive -> %Vrc\n", INSTANCE(pState), rc));
-    vnetCsLeave(pState);
+    vnetCsRxLeave(pState);
     return rc;
 }
@@ -675 +702 @@
 {
     VNETSTATE *pState = IFACE_TO_STATE(pInterface, INetworkPort);
-    int        rc = VINF_SUCCESS;
 
     Log2(("%s vnetReceive: pvBuf=%p cb=%u\n", INSTANCE(pState), pvBuf, cb));
-    rc = vnetCanReceive(pState);
+    int rc = vnetCanReceive(pState);
     if (RT_FAILURE(rc))
         return rc;
@@ -693 +719 @@
     if (vnetAddressFilter(pState, pvBuf, cb))
     {
-        rc = vnetHandleRxPacket(pState, pvBuf, cb);
-        STAM_REL_COUNTER_ADD(&pState->StatReceiveBytes, cb);
+        rc = vnetCsRxEnter(pState, VERR_SEM_BUSY);
+        if (RT_SUCCESS(rc))
+        {
+            rc = vnetHandleRxPacket(pState, pvBuf, cb);
+            STAM_REL_COUNTER_ADD(&pState->StatReceiveBytes, cb);
+            vnetCsRxLeave(pState);
+        }
     }
     vpciSetReadLed(&pState->VPCI, false);
     STAM_PROFILE_ADV_STOP(&pState->StatReceive, a);
-
     return rc;
 }
@@ -856 +886 @@
     VNETSTATE *pState = (VNETSTATE*)pvUser;
 
+    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    if (RT_UNLIKELY(rc != VINF_SUCCESS))
+        return;
     vringSetNotification(&pState->VPCI, &pState->pTxQueue->VRing, true);
     Log3(("%s vnetTxTimer: Expired, %d packets pending\n", INSTANCE(pState), 
           vringReadAvailIndex(&pState->VPCI, &pState->pTxQueue->VRing) - pState->pTxQueue->uNextAvailIndex));
     vnetTransmitPendingPackets(pState, pState->pTxQueue);
+    vnetCsLeave(pState);
 }
 
@@ -1114 +1148 @@
     VNETSTATE* pState = PDMINS_2_DATA(pDevIns, VNETSTATE*);
 
-    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    int rc = vnetCsRxEnter(pState, VERR_SEM_BUSY);
     if (RT_UNLIKELY(rc != VINF_SUCCESS))
         return rc;
-    vnetCsLeave(pState);
+    vnetCsRxLeave(pState);
     return VINF_SUCCESS;
 }
@@ -1168 +1202 @@
     VNETSTATE* pState = PDMINS_2_DATA(pDevIns, VNETSTATE*);
 
-    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    int rc = vnetCsRxEnter(pState, VERR_SEM_BUSY);
     if (RT_UNLIKELY(rc != VINF_SUCCESS))
         return rc;
-    vnetCsLeave(pState);
+    vnetCsRxLeave(pState);
     return VINF_SUCCESS;
 }
@@ -1395 +1429 @@
                     ("Cannot allocate TX buffer for virtio-net device\n"), VERR_NO_MEMORY);
 
+    /* Initialize critical section. */
+    char szTmp[sizeof(pState->VPCI.szInstance) + 2];
+    RTStrPrintf(szTmp, sizeof(szTmp), "%sRX", pState->VPCI.szInstance);
+    rc = PDMDevHlpCritSectInit(pDevIns, &pState->csRx, szTmp);
+    if (RT_FAILURE(rc))
+        return rc;
+
     /* Map our ports to IO space. */
     rc = PDMDevHlpPCIIORegionRegister(pDevIns, 0,
@@ -1505 +1546 @@
         pState->pTxBuf = NULL;
     }
+    if (PDMCritSectIsInitialized(&pState->csRx))
+        PDMR3CritSectDelete(&pState->csRx);
 
     return vpciDestruct(&pState->VPCI);
@@ -1563 +1606 @@
     AssertLogRelReturnVoid(iLUN == 0);
 
-    vnetCsEnter(pState, VERR_SEM_BUSY);
+    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    if (RT_FAILURE(rc))
+    {
+        LogRel(("vnetDetach failed to enter critical section!\n"));
+        return;
+    }
 
     /*
@@ -1594 +1642 @@
     AssertLogRelReturn(iLUN == 0, VERR_PDM_NO_SUCH_LUN);
 
-    vnetCsEnter(pState, VERR_SEM_BUSY);
+    int rc = vnetCsEnter(pState, VERR_SEM_BUSY);
+    if (RT_FAILURE(rc))
+    {
+        LogRel(("vnetAttach failed to enter critical section!\n"));
+        return rc;
+    }
 
     /*
      * Attach the driver.
      */
-    int rc = PDMDevHlpDriverAttach(pDevIns, 0, &pState->VPCI.IBase, &pState->pDrvBase, "Network Port");
+    rc = PDMDevHlpDriverAttach(pDevIns, 0, &pState->VPCI.IBase, &pState->pDrvBase, "Network Port");
     if (RT_SUCCESS(rc))
     {

trunk/src/VBox/Devices/VirtIO/Virtio.cpp

@@ -258 +258 @@
 int vpciRaiseInterrupt(VPCISTATE *pState, int rcBusy, uint8_t u8IntCause)
 {
-    int rc = vpciCsEnter(pState, rcBusy);
-    if (RT_UNLIKELY(rc != VINF_SUCCESS))
-        return rc;
+    // int rc = vpciCsEnter(pState, rcBusy);
+    // if (RT_UNLIKELY(rc != VINF_SUCCESS))
+    //     return rc;
 
     STAM_COUNTER_INC(&pState->StatIntsRaised);
@@ -268 +268 @@
     pState->uISR |= u8IntCause;
     PDMDevHlpPCISetIrq(pState->CTX_SUFF(pDevIns), 0, 1);
-    vpciCsLeave(pState);
+    // vpciCsLeave(pState);
     return VINF_SUCCESS;
 }
@@ -314 +314 @@
     const char *szInst = INSTANCE(pState);
     STAM_PROFILE_ADV_START(&pState->CTXSUFF(StatIORead), a);
+
+    /*
+     * We probably do not need to enter critical section when reading registers
+     * as the most of them are either constant or being changed during
+     * initialization only, the exception being ISR which can be raced by all
+     * threads but I see no big harm in it. It also happens to be the most read
+     * register as it gets read in interrupt handler. By dropping cs protection
+     * here we gain the ability to deliver RX packets to the guest while TX is
+     * holding cs transmitting queued packets.
+     *
+    rc = vpciCsEnter(pState, VINF_IOM_HC_IOPORT_READ);
+    if (RT_UNLIKELY(rc != VINF_SUCCESS))
+    {
+        STAM_PROFILE_ADV_STOP(&pState->CTXSUFF(StatIORead), a);
+        return rc;
+        }*/
 
     port -= pState->addrIOPort;
@@ -371 +387 @@
           szInst, port, cb*2, *pu32));
     STAM_PROFILE_ADV_STOP(&pState->CTXSUFF(StatIORead), a);
+    //vpciCsLeave(pState);
     return rc;
 }
@@ -405 +422 @@
     bool        fHasBecomeReady;
     STAM_PROFILE_ADV_START(&pState->CTXSUFF(StatIOWrite), a);
+
+    rc = vpciCsEnter(pState, VINF_IOM_HC_IOPORT_WRITE);
+    if (RT_UNLIKELY(rc != VINF_SUCCESS))
+    {
+        STAM_PROFILE_ADV_STOP(&pState->CTXSUFF(StatIOWrite), a);
+        return rc;
+    }
+
 
     port -= pState->addrIOPort;
@@ -494 +519 @@
 
     STAM_PROFILE_ADV_STOP(&pState->CTXSUFF(StatIOWrite), a);
+    vpciCsLeave(pState);
     return rc;
 }