From: Akihiko Odaki <akihiko.odaki@daynix.com>
To: Nicholas Piggin <npiggin@gmail.com>, qemu-devel@nongnu.org
Cc: "Fabiano Rosas" <farosas@suse.de>, "John Snow" <jsnow@redhat.com>,
"Laurent Vivier" <lvivier@redhat.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
qemu-block@nongnu.org
Subject: Re: [PATCH v4 2/7] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped
Date: Sat, 19 Apr 2025 17:37:29 +0900 [thread overview]
Message-ID: <e1b6adb7-c95c-4946-90d5-4fb9d25f7e4e@daynix.com> (raw)
In-Reply-To: <20250411044130.201724-3-npiggin@gmail.com>
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)
next prev parent reply other threads:[~2025-04-19 8:38 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-11 4:41 [PATCH v4 0/7] qtest/libqos/pci: pci and msix fixes Nicholas Piggin
2025-04-11 4:41 ` [PATCH v4 1/7] tests/qtest/ahci: unmap pci bar before reusing device Nicholas Piggin
2025-04-11 4:41 ` [PATCH v4 2/7] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped Nicholas Piggin
2025-04-19 8:37 ` Akihiko Odaki [this message]
2025-04-11 4:41 ` [PATCH v4 3/7] tests/qtest/libquos/virtio: unmap pci bar when disabling device Nicholas Piggin
2025-04-11 4:41 ` [PATCH v4 4/7] tests/qtest/libquos/pci: Add migration fixup helper for pci devices Nicholas Piggin
2025-04-19 8:40 ` Akihiko Odaki
2025-04-11 4:41 ` [PATCH v4 5/7] qtest/libqos/pci: Enforce balanced iomap/unmap Nicholas Piggin
2025-04-11 4:41 ` [PATCH v4 6/7] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0 Nicholas Piggin
2025-04-11 4:41 ` [PATCH v4 7/7] qtest/libqos/pci: Factor msix entry helpers into pci common code Nicholas Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e1b6adb7-c95c-4946-90d5-4fb9d25f7e4e@daynix.com \
--to=akihiko.odaki@daynix.com \
--cc=farosas@suse.de \
--cc=jsnow@redhat.com \
--cc=lvivier@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=npiggin@gmail.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).