[v4] qtest/libqos/pci: pci and msix fixes

[PATCH v4 2/7] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

Posted by Nicholas Piggin 7 months, 1 week ago

ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
unmapping it in these cases, to keep iomaps balanced.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/ahci-test.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 7cae6b58e0c..02c9d54f898 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -85,6 +85,8 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
     uint64_t hba_base;
     AHCICommandHeader cmd;
 
+    g_assert_cmphex(ahci->hba_bar.addr, ==, hba_old);
+
     ahci_fingerprint = qpci_config_readl(ahci->dev, PCI_VENDOR_ID);
     g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
 
@@ -193,18 +195,28 @@ static AHCIQState *ahci_boot(const char *cli, ...)
 
 /**
  * Clean up the PCI device, then terminate the QEMU instance.
+ * Should be called if ahci_pci_enable (or ahci_boot_and_enable)
+ * was not used, or device/pci was disabled later.
  */
-static void ahci_shutdown(AHCIQState *ahci)
+static void ahci_shutdown_pci_disabled(AHCIQState *ahci)
 {
     QOSState *qs = ahci->parent;
 
-    ahci_pci_disable(ahci);
     ahci_clean_mem(ahci);
     free_ahci_device(ahci->dev);
     g_free(ahci);
     qtest_shutdown(qs);
 }
 
+/**
+ * Clean up the PCI device, then terminate the QEMU instance.
+ */
+static void ahci_shutdown(AHCIQState *ahci)
+{
+    ahci_pci_disable(ahci);
+    ahci_shutdown_pci_disabled(ahci);
+}
+
 /**
  * Boot and fully enable the HBA device.
  * @see ahci_boot, ahci_pci_enable and ahci_hba_enable.
@@ -945,7 +957,7 @@ static void test_sanity(void)
 {
     AHCIQState *ahci;
     ahci = ahci_boot(NULL);
-    ahci_shutdown(ahci);
+    ahci_shutdown_pci_disabled(ahci);
 }
 
 /**
@@ -957,7 +969,7 @@ static void test_pci_spec(void)
     AHCIQState *ahci;
     ahci = ahci_boot(NULL);
     ahci_test_pci_spec(ahci);
-    ahci_shutdown(ahci);
+    ahci_shutdown_pci_disabled(ahci);
 }
 
 /**
@@ -1143,8 +1155,8 @@ static void test_migrate_sanity(void)
 
     ahci_migrate(src, dst, uri);
 
-    ahci_shutdown(src);
-    ahci_shutdown(dst);
+    ahci_shutdown_pci_disabled(src);
+    ahci_shutdown_pci_disabled(dst);
     g_free(uri);
 }
 
@@ -1182,7 +1194,7 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
     /* Verify pattern */
     g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
 
-    ahci_shutdown(src);
+    ahci_shutdown_pci_disabled(src);
     ahci_shutdown(dst);
     g_free(rx);
     g_free(tx);
@@ -1324,7 +1336,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
     g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
 
     /* Cleanup and go home. */
-    ahci_shutdown(src);
+    ahci_shutdown_pci_disabled(src);
     ahci_shutdown(dst);
     g_free(rx);
     g_free(tx);
@@ -1388,8 +1400,8 @@ static void test_flush_migrate(void)
     ahci_command_verify(dst, cmd);
 
     ahci_command_free(cmd);
-    ahci_shutdown(src);
-    ahci_shutdown(dst);
+    ahci_shutdown_pci_disabled(src);
+    ahci_shutdown_pci_disabled(dst);
     g_free(uri);
 }
 
@@ -1421,6 +1433,7 @@ static void test_reset(void)
         ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
         stop_ahci_device(ahci);
         ahci_clean_mem(ahci);
+        start_ahci_device(ahci);
     }
 
     ahci_shutdown(ahci);
@@ -1508,7 +1521,7 @@ static void test_reset_pending_callback(void)
     stop_ahci_device(ahci);
     ahci_clean_mem(ahci);
 
-    ahci_shutdown(ahci);
+    ahci_shutdown_pci_disabled(ahci);
 }
 
 static void test_ncq_simple(void)
-- 
2.47.1

Re: [PATCH v4 2/7] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

Posted by Akihiko Odaki 7 months ago

On 2025/04/11 13:41, Nicholas Piggin wrote:
> ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
> unmapping it in these cases, to keep iomaps balanced.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>

My address is duplicated.

> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/ahci-test.c | 35 ++++++++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 7cae6b58e0c..02c9d54f898 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -85,6 +85,8 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
>       uint64_t hba_base;
>       AHCICommandHeader cmd;
>   
> +    g_assert_cmphex(ahci->hba_bar.addr, ==, hba_old);
> +
>       ahci_fingerprint = qpci_config_readl(ahci->dev, PCI_VENDOR_ID);
>       g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
>   
> @@ -193,18 +195,28 @@ static AHCIQState *ahci_boot(const char *cli, ...)
>   
>   /**
>    * Clean up the PCI device, then terminate the QEMU instance.
> + * Should be called if ahci_pci_enable (or ahci_boot_and_enable)
> + * was not used, or device/pci was disabled later.
>    */
> -static void ahci_shutdown(AHCIQState *ahci)
> +static void ahci_shutdown_pci_disabled(AHCIQState *ahci)
>   {
>       QOSState *qs = ahci->parent;
>   
> -    ahci_pci_disable(ahci);
>       ahci_clean_mem(ahci);
>       free_ahci_device(ahci->dev);
>       g_free(ahci);
>       qtest_shutdown(qs);
>   }
>   
> +/**
> + * Clean up the PCI device, then terminate the QEMU instance.
> + */
> +static void ahci_shutdown(AHCIQState *ahci)
> +{
> +    ahci_pci_disable(ahci);
> +    ahci_shutdown_pci_disabled(ahci);
> +}
> +

I rather want to keep one unified function that performs all cleanup 
operations since it's error-prone to choose an appropriate cleanup 
function. ahci_shutdown() also calls ahci_clean_mem(), which checks if 
its cleanup operation is necessary before actually performing it so we 
can do the same for the BAR unmapping for consistency.



>   /**
>    * Boot and fully enable the HBA device.
>    * @see ahci_boot, ahci_pci_enable and ahci_hba_enable.
> @@ -945,7 +957,7 @@ static void test_sanity(void)
>   {
>       AHCIQState *ahci;
>       ahci = ahci_boot(NULL);
> -    ahci_shutdown(ahci);
> +    ahci_shutdown_pci_disabled(ahci);
>   }
>   
>   /**
> @@ -957,7 +969,7 @@ static void test_pci_spec(void)
>       AHCIQState *ahci;
>       ahci = ahci_boot(NULL);
>       ahci_test_pci_spec(ahci);
> -    ahci_shutdown(ahci);
> +    ahci_shutdown_pci_disabled(ahci);
>   }
>   
>   /**
> @@ -1143,8 +1155,8 @@ static void test_migrate_sanity(void)
>   
>       ahci_migrate(src, dst, uri);
>   
> -    ahci_shutdown(src);
> -    ahci_shutdown(dst);
> +    ahci_shutdown_pci_disabled(src);
> +    ahci_shutdown_pci_disabled(dst);
>       g_free(uri);
>   }
>   
> @@ -1182,7 +1194,7 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
>       /* Verify pattern */
>       g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
>   
> -    ahci_shutdown(src);
> +    ahci_shutdown_pci_disabled(src);
>       ahci_shutdown(dst);
>       g_free(rx);
>       g_free(tx);
> @@ -1324,7 +1336,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
>       g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
>   
>       /* Cleanup and go home. */
> -    ahci_shutdown(src);
> +    ahci_shutdown_pci_disabled(src);
>       ahci_shutdown(dst);
>       g_free(rx);
>       g_free(tx);
> @@ -1388,8 +1400,8 @@ static void test_flush_migrate(void)
>       ahci_command_verify(dst, cmd);
>   
>       ahci_command_free(cmd);
> -    ahci_shutdown(src);
> -    ahci_shutdown(dst);
> +    ahci_shutdown_pci_disabled(src);
> +    ahci_shutdown_pci_disabled(dst);
>       g_free(uri);
>   }
>   
> @@ -1421,6 +1433,7 @@ static void test_reset(void)
>           ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
>           stop_ahci_device(ahci);
>           ahci_clean_mem(ahci);
> +        start_ahci_device(ahci);
>       }
>   
>       ahci_shutdown(ahci);
> @@ -1508,7 +1521,7 @@ static void test_reset_pending_callback(void)
>       stop_ahci_device(ahci);
>       ahci_clean_mem(ahci);
>   
> -    ahci_shutdown(ahci);
> +    ahci_shutdown_pci_disabled(ahci);
>   }
>   
>   static void test_ncq_simple(void)

[PATCH v4 1/7] tests/qtest/ahci: unmap pci bar before reusing device
[PATCH v4 2/7] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped
[PATCH v4 3/7] tests/qtest/libquos/virtio: unmap pci bar when disabling device
[PATCH v4 4/7] tests/qtest/libquos/pci: Add migration fixup helper for pci devices
[PATCH v4 5/7] qtest/libqos/pci: Enforce balanced iomap/unmap
[PATCH v4 6/7] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
[PATCH v4 7/7] qtest/libqos/pci: Factor msix entry helpers into pci common code