Re: [Qemu-ppc] [PATCHv5 11/12] tests: Don't assume structure of PCI IO base in ahci-test

[Greg Kurz]

On Mon, 24 Oct 2016 16:00:00 +1100
David Gibson <address@hidden> 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 <address@hidden>
> ---

Reviewed-by: Greg Kurz <address@hidden>

>  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;
>  
>  /**