| Message ID | 1477285201-10244-12-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 10/24/2016 01:00 AM, David Gibson 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 <david@gibson.dropbear.id.au> > --- > 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; > > /** > Reviewed-by: John Snow <jsnow@redhat.com>
On Mon, 24 Oct 2016 16:00:00 +1100 David Gibson <david@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 <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@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; > > /**
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; /**
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 <david@gibson.dropbear.id.au> --- tests/ahci-test.c | 13 +++++++------ tests/libqos/ahci.c | 1 + tests/libqos/ahci.h | 1 + 3 files changed, 9 insertions(+), 6 deletions(-)