On Mon, 24 Oct 2016 16:00:00 +1100
David Gibson <da...@gibson.dropbear.id.au> wrote:
> In a couple of places ahci-test makes assumptions about how the tokens
> returned from qpci_iomap() are formatted in ways it probably shouldn't.
>
> First in verify_state() it uses a non-NULL token to indicate that the AHCI
> device has been enabled (part of enabling is to iomap()). This changes it
> to use an explicit 'enabled' flag instead.
>
> Second, it uses the fact that the token contains a PCI address, stored when
> the BAR is mapped during initialization to check that the BAR has the same
> value after a migration. This changes it to explicitly read the BAR
> register before and after the migration and compare.
>
> Together, these changes will make the test more robust against changes to
> the internals of the libqos PCI layer.
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
Reviewed-by: Greg Kurz <gr...@kaod.org>
> tests/ahci-test.c | 13 +++++++------
> tests/libqos/ahci.c | 1 +
> tests/libqos/ahci.h | 1 +
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index 9c0adce..70bcafa 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -78,25 +78,23 @@ static void string_bswap16(uint16_t *s, size_t bytes)
> /**
> * Verify that the transfer did not corrupt our state at all.
> */
> -static void verify_state(AHCIQState *ahci)
> +static void verify_state(AHCIQState *ahci, uint64_t hba_old)
> {
> int i, j;
> uint32_t ahci_fingerprint;
> uint64_t hba_base;
> - uint64_t hba_stored;
> AHCICommandHeader cmd;
>
> ahci_fingerprint = qpci_config_readl(ahci->dev, PCI_VENDOR_ID);
> g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
>
> /* If we haven't initialized, this is as much as can be validated. */
> - if (!ahci->hba_base) {
> + if (!ahci->enabled) {
> return;
> }
>
> hba_base = (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
> - hba_stored = (uint64_t)(uintptr_t)ahci->hba_base;
> - g_assert_cmphex(hba_base, ==, hba_stored);
> + g_assert_cmphex(hba_base, ==, hba_old);
>
> g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
> g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP2), ==, ahci->cap2);
> @@ -119,12 +117,15 @@ static void ahci_migrate(AHCIQState *from, AHCIQState
> *to, const char *uri)
> QOSState *tmp = to->parent;
> QPCIDevice *dev = to->dev;
> char *uri_local = NULL;
> + uint64_t hba_old;
>
> if (uri == NULL) {
> uri_local = g_strdup_printf("%s%s", "unix:", mig_socket);
> uri = uri_local;
> }
>
> + hba_old = (uint64_t)qpci_config_readl(from->dev, PCI_BASE_ADDRESS_5);
> +
> /* context will be 'to' after completion. */
> migrate(from->parent, to->parent, uri);
>
> @@ -141,7 +142,7 @@ static void ahci_migrate(AHCIQState *from, AHCIQState
> *to, const char *uri)
> from->parent = tmp;
> from->dev = dev;
>
> - verify_state(to);
> + verify_state(to, hba_old);
> g_free(uri_local);
> }
>
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index 716ab79..8e789d8 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -351,6 +351,7 @@ void ahci_hba_enable(AHCIQState *ahci)
> reg = ahci_rreg(ahci, AHCI_GHC);
> ASSERT_BIT_SET(reg, AHCI_GHC_IE);
>
> + ahci->enabled = true;
> /* TODO: The device should now be idling and waiting for commands.
> * In the future, a small test-case to inspect the Register D2H FIS
> * and clear the initial interrupts might be good. */
> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
> index c69fb5a..9b0c1d7 100644
> --- a/tests/libqos/ahci.h
> +++ b/tests/libqos/ahci.h
> @@ -327,6 +327,7 @@ typedef struct AHCIQState {
> uint32_t cap;
> uint32_t cap2;
> AHCIPortQState port[32];
> + bool enabled;
> } AHCIQState;
>
> /**
