fix FLR reset callback (#729)

A reset callback is allowed to call functions disallowed in quiescent
state. However, the FLR reset path neglected to account for this
properly, causing an incorrect assert to be triggered if, for example,
vfu_sgl_put() is called. To fix this, make sure all reset paths go
through call_reset_cb().

Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

6 files changed, 64 insertions, 33 deletions

diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index be4be95..5a6269a 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -887,19 +887,32 @@ out:
     return ret;
 }
 
+int
+call_reset_cb(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
+{
+    int ret;
+
+    if (vfu_ctx->reset == NULL) {
+        return 0;
+    }
+
+    vfu_ctx->in_cb = CB_RESET;
+    ret = vfu_ctx->reset(vfu_ctx, reason);
+    vfu_ctx->in_cb = CB_NONE;
+
+    return ret;
+}
+
 static int
-do_device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
+device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
 {
     int ret;
 
-    if (vfu_ctx->reset != NULL) {
-        vfu_ctx->in_cb = CB_RESET;
-        ret = vfu_ctx->reset(vfu_ctx, reason);
-        vfu_ctx->in_cb = CB_NONE;
-        if (ret < 0) {
-            return ret;
-        }
+    ret = call_reset_cb(vfu_ctx, reason);
+    if (ret < 0) {
+        return ret;
     }
+
     if (vfu_ctx->migration != NULL) {
         return handle_device_state(vfu_ctx, vfu_ctx->migration,
                                    VFIO_DEVICE_STATE_V1_RUNNING, false);
@@ -907,12 +920,6 @@ do_device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
     return 0;
 }
 
-int
-handle_device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
-{
-    return do_device_reset(vfu_ctx, reason);
-}
-
 static int
 handle_dirty_pages_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
 {
@@ -1189,7 +1196,7 @@ handle_request(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
 
     case VFIO_USER_DEVICE_RESET:
         vfu_log(vfu_ctx, LOG_INFO, "device reset by client");
-        ret = handle_device_reset(vfu_ctx, VFU_RESET_DEVICE);
+        ret = device_reset(vfu_ctx, VFU_RESET_DEVICE);
         break;
 
     case VFIO_USER_DIRTY_PAGES:
@@ -1495,6 +1502,7 @@ vfu_run_ctx(vfu_ctx_t *vfu_ctx)
              * be called at all.
              */
             if (vfu_ctx->quiesced) {
+		    // FIXME?
                 vfu_log(vfu_ctx, LOG_DEBUG, "device unquiesced");
                 vfu_ctx->quiesced = false;
             }
@@ -1614,7 +1622,7 @@ vfu_reset_ctx_quiesced(vfu_ctx_t *vfu_ctx)
     }
 
     /* FIXME what happens if the device reset callback fails? */
-    do_device_reset(vfu_ctx, VFU_RESET_LOST_CONN);
+    device_reset(vfu_ctx, VFU_RESET_LOST_CONN);
 
     if (vfu_ctx->irqs != NULL) {
         irqs_reset(vfu_ctx);
diff --git a/lib/pci_caps.c b/lib/pci_caps.c
index 50eb29b..73e2645 100644
--- a/lib/pci_caps.c
+++ b/lib/pci_caps.c
@@ -270,7 +270,7 @@ handle_px_pxdc_write(vfu_ctx_t *vfu_ctx, struct pxcap *px,
         }
         if (vfu_ctx->reset != NULL) {
             vfu_log(vfu_ctx, LOG_DEBUG, "initiate function level reset");
-            return vfu_ctx->reset(vfu_ctx, VFU_RESET_PCI_FLR);
+	    return call_reset_cb(vfu_ctx, VFU_RESET_PCI_FLR);
         } else {
             vfu_log(vfu_ctx, LOG_ERR, "FLR callback is not implemented");
         }
diff --git a/lib/private.h b/lib/private.h
index 346cfed..fdd804f 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -221,10 +221,10 @@ handle_dma_unmap(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
                  struct vfio_user_dma_unmap *dma_unmap);
 
 int
-handle_device_get_region_info(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg);
+call_reset_cb(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason);
 
 int
-handle_device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason);
+handle_device_get_region_info(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg);
 
 MOCK_DECLARE(bool, cmd_allowed_when_stopped_and_copying, uint16_t cmd);
 
diff --git a/test/py/libvfio_user.py b/test/py/libvfio_user.py
index 5f0872a..73fad5a 100644
--- a/test/py/libvfio_user.py
+++ b/test/py/libvfio_user.py
@@ -869,6 +869,16 @@ def __dma_unregister(ctx, info):
     dma_unregister(ctx, copy.copy(info.contents))
 
 
+def setup_flrc(ctx):
+    # flrc
+    cap = struct.pack("ccHHcc52c", to_byte(PCI_CAP_ID_EXP), b'\0', 0, 0, b'\0',
+                      b'\x10', *[b'\0' for _ in range(52)])
+    # FIXME adding capability after we've realized the device only works
+    # because of bug #618.
+    pos = vfu_pci_add_capability(ctx, pos=0, flags=0, data=cap)
+    assert pos == PCI_STD_HEADER_SIZEOF
+
+
 def quiesce_cb(ctx):
     return 0
 
diff --git a/test/py/test_pci_caps.py b/test/py/test_pci_caps.py
index 24e992f..edd1683 100644
--- a/test/py/test_pci_caps.py
+++ b/test/py/test_pci_caps.py
@@ -314,16 +314,6 @@ def __test_pci_cap_write_pmcs(sock):
                  count=len(data), data=data, expect=errno.ENOTSUP)
 
 
-def _setup_flrc(ctx):
-    # flrc
-    cap = struct.pack("ccHHcc52c", to_byte(PCI_CAP_ID_EXP), b'\0', 0, 0, b'\0',
-                      b'\x10', *[b'\0' for _ in range(52)])
-    # FIXME adding capability after we've realized the device only works
-    # because of bug #618.
-    pos = vfu_pci_add_capability(ctx, pos=0, flags=0, data=cap)
-    assert pos == PCI_STD_HEADER_SIZEOF
-
-
 @patch("libvfio_user.reset_cb", return_value=0)
 @patch('libvfio_user.quiesce_cb')
 def test_pci_cap_write_px(mock_quiesce, mock_reset):
@@ -333,7 +323,7 @@ def test_pci_cap_write_px(mock_quiesce, mock_reset):
     setup_pci_dev(realize=True)
     sock = connect_client(ctx)
 
-    _setup_flrc(ctx)
+    setup_flrc(ctx)
 
     # iflr
     offset = PCI_STD_HEADER_SIZEOF + 8
@@ -421,7 +411,7 @@ def test_pci_cap_write_pxdc2():
     setup_pci_dev(realize=True)
     sock = connect_client(ctx)
 
-    _setup_flrc(ctx)
+    setup_flrc(ctx)
 
     offset = (vfu_pci_find_capability(ctx, False, PCI_CAP_ID_EXP) +
               PCI_EXP_DEVCTL2)
@@ -436,9 +426,10 @@ def test_pci_cap_write_pxdc2():
 def test_pci_cap_write_pxlc2():
 
     setup_pci_dev(realize=True)
-    _setup_flrc(ctx)
-
     sock = connect_client(ctx)
+
+    setup_flrc(ctx)
+
     offset = (vfu_pci_find_capability(ctx, False, PCI_CAP_ID_EXP) +
               PCI_EXP_LNKCTL2)
     data = b'\xbe\xef'
diff --git a/test/py/test_quiesce.py b/test/py/test_quiesce.py
index 0e2a980..b1fb2fd 100644
--- a/test/py/test_quiesce.py
+++ b/test/py/test_quiesce.py
@@ -247,4 +247,26 @@ def test_allowed_funcs_in_quiesced_reset_busy(mock_quiesce, mock_reset):
     mock_reset.assert_called_once_with(ctx, VFU_RESET_DEVICE)
 
 
+@patch('libvfio_user.reset_cb', side_effect=_side_effect)
+@patch('libvfio_user.quiesce_cb')
+def test_flr(mock_quiesce, mock_reset):
+    """Test that an FLR reset callback is still able to call functions not
+    allowed in quiescent state."""
+
+    global ctx, sock
+
+    _map_dma_region(ctx, sock)
+
+    setup_flrc(ctx)
+
+    # iflr
+    offset = PCI_STD_HEADER_SIZEOF + 8
+    data = b'\x00\x80'
+    write_region(ctx, sock, VFU_PCI_DEV_CFG_REGION_IDX, offset=offset,
+                 count=len(data), data=data)
+
+    mock_quiesce.assert_called_with(ctx)
+    mock_reset.assert_called_once_with(ctx, VFU_RESET_PCI_FLR)
+
+
 # ex: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab: #