| Message ID | 1404757089-4836-25-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote: > +/*** Macro Utilities ***/ > +#define bitany(data, mask) (((data) & (mask)) != 0) > +#define bitset(data, mask) (((data) & (mask)) == (mask)) > +#define bitclr(data, mask) (((data) & (mask)) == 0) Unused, please remove. > +#ifdef AHCI_13_STRICT Please drop the #ifdef. #ifdefs mean dead code that is not being compiled or tested. Just decide which case we should take and keep that one. > + /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */ > + assert_bit_clear(datal, 0xFF); > +#else > + if (datal & 0xFF) { > + g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF); > + } > +#endif Tests should not produce output. Either this is a failure that needs to be fixed in QEMU, or it is not a failure and we shouldn't produce output. The rationale is that producing "warning" output means we now need to diff the make check output to see when it changes. In practice no one will pay attention and warnings will build up. > + > + /* BCC *must* equal 0x01. */ > + g_assert(PCI_BCC(datal) == 0x01); g_assert_cmphex() would make the error message more useful by including the incorrect value. The same applies elsewhere in this patch. > +#ifdef AHCI_13_STRICT > + /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */ > + g_assert((data & 0xFF) == PCI_CAP_ID_PM); > +#elif defined Q35 > + /* Intel ICH9 Family Datasheet 14.1.19 p.550 */ > + if ((data & 0xFF) != PCI_CAP_ID_MSI) { > + g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)", > + data & 0xFF, PCI_CAP_ID_MSI); > + } > + /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/ > +#else > + if ((data & 0xFF) != PCI_CAP_ID_PM) { > + g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)", > + data & 0xFF, PCI_CAP_ID_PM); > + } > +#endif Since the test hardcodes the q35 machine type when calling qtest_start(), I guess the Q35 case always applies. Please remove the #ifdef and warning (either it's a failure that needs to be fixed, or it's not a failure). > +static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header, > + uint8_t offset) > +{ > + uint8_t cid = header & 0xFF; > + uint8_t next = header >> 8; > + > + g_test_message("CID: %02x; next: %02x", cid, next); Debugging output that should be removed? > + > + switch (cid) { > + case PCI_CAP_ID_PM: > + ahci_test_pmcap(ahci, offset); > + break; > + case PCI_CAP_ID_MSI: > + ahci_test_msicap(ahci, offset); > + break; > + case PCI_CAP_ID_SATA: > + ahci_test_satacap(ahci, offset); > + break; > + > + default: > + g_test_message("Unknown CAP 0x%02x", cid); This should be a failure. If a new capability is added, the test should be extended for that capability. > +static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset) > +{ > + uint16_t dataw; > + uint32_t datal; > + > + g_test_message("Verifying SATACAP"); Debug message that should be removed? > +static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset) > +{ > + uint16_t dataw; > + uint32_t datal; > + > + g_test_message("Verifying MSICAP"); Debug message that should be removed? > + if (dataw & PCI_MSI_FLAGS_64BIT) { > + g_test_message("MSICAP is 64bit"); Debug message that should be removed? > + g_assert(qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_HI) == 0x00); > + g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_64) == 0x00); > + } else { > + g_test_message("MSICAP is 32bit"); Debug message that should be removed? > +static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset) > +{ > + uint16_t dataw; > + > + g_test_message("Verifying PMCAP"); Debug message that should be removed?
On 07/31/2014 09:19 AM, Stefan Hajnoczi wrote: > On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote: >> +#ifdef AHCI_13_STRICT > Please drop the #ifdef. #ifdefs mean dead code that is not being > compiled or tested. Just decide which case we should take and keep that > one. OK. It might be nice to have some feedback on which is preferred, then. Intel Q35, perhaps? It might be nice to leave some comments in at least to help document where the specifications diverge. > >> + /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */ >> + assert_bit_clear(datal, 0xFF); >> +#else >> + if (datal & 0xFF) { >> + g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF); >> + } >> +#endif > Tests should not produce output. Either this is a failure that needs to > be fixed in QEMU, or it is not a failure and we shouldn't produce > output. OK. In this case, I think the specification might be "wrong" in that the RID should in fact be implementation-dependent, and it is perhaps a typo or an oversight that it is set to 0x00. It would make sense to just delete this check and leave a comment explaining why it's not checked. Let me know if I am off-base here. > > The rationale is that producing "warning" output means we now need to > diff the make check output to see when it changes. In practice no one > will pay attention and warnings will build up. > >> + >> + /* BCC *must* equal 0x01. */ >> + g_assert(PCI_BCC(datal) == 0x01); > g_assert_cmphex() would make the error message more useful by including > the incorrect value. The same applies elsewhere in this patch. > >> +#ifdef AHCI_13_STRICT >> + /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */ >> + g_assert((data & 0xFF) == PCI_CAP_ID_PM); >> +#elif defined Q35 >> + /* Intel ICH9 Family Datasheet 14.1.19 p.550 */ >> + if ((data & 0xFF) != PCI_CAP_ID_MSI) { >> + g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)", >> + data & 0xFF, PCI_CAP_ID_MSI); >> + } >> + /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/ >> +#else >> + if ((data & 0xFF) != PCI_CAP_ID_PM) { >> + g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)", >> + data & 0xFF, PCI_CAP_ID_PM); >> + } >> +#endif > Since the test hardcodes the q35 machine type when calling > qtest_start(), I guess the Q35 case always applies. > > Please remove the #ifdef and warning (either it's a failure that needs > to be fixed, or it's not a failure). This is where I was unable to really make a judgment call and some more input would be nice. AHCI 1.3 and Intel Q35/ICH9 specifications are actually at odds with one another -- The ability to test either/or might not be terrible. The warnings here are somewhat minor, pedantic nitpicks ... though in a future patch I did fix the ordering, so I can just make these hard errors. > >> +static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header, >> + uint8_t offset) >> +{ >> + uint8_t cid = header & 0xFF; >> + uint8_t next = header >> 8; >> + >> + g_test_message("CID: %02x; next: %02x", cid, next); > Debugging output that should be removed? The output here is only visible via --verbose. If even this is undesirable, I can remove it completely ... but I figured it would be nice to leave in some really basic debugging messages. > >> + >> + switch (cid) { >> + case PCI_CAP_ID_PM: >> + ahci_test_pmcap(ahci, offset); >> + break; >> + case PCI_CAP_ID_MSI: >> + ahci_test_msicap(ahci, offset); >> + break; >> + case PCI_CAP_ID_SATA: >> + ahci_test_satacap(ahci, offset); >> + break; >> + >> + default: >> + g_test_message("Unknown CAP 0x%02x", cid); > This should be a failure. If a new capability is added, the test should > be extended for that capability. The specification does allow for any number of arbitrary capabilities, in which the specification has no say about order or compliance. Any further investigation really delves into the PCI specification, which was not my aim here. I think mentioning the inability to verify a capability is fine via --verbose for the purposes of basic AHCI sanity tests. At any rate, I don't think this is a failure -- at least not in THIS test, which I tried to keep as a "spec-adherent, QEMU indifferent" test. If we want to test QEMU-specifics, I would like to add those into a separate test case -- at which point failures for unrecognized capabilities would be appropriate. In the future, we might even abstract out these pieces that apply to PCI devices as a whole to be generic PCI spec compliance tests, with an AHCI and then a QEMU layer on top, in order of increasing specificity.
On Thu, Jul 31, 2014 at 01:42:02PM -0400, John Snow wrote: > > On 07/31/2014 09:19 AM, Stefan Hajnoczi wrote: > >On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote: > >>+static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header, > >>+ uint8_t offset) > >>+{ > >>+ uint8_t cid = header & 0xFF; > >>+ uint8_t next = header >> 8; > >>+ > >>+ g_test_message("CID: %02x; next: %02x", cid, next); > >Debugging output that should be removed? > The output here is only visible via --verbose. If even this is undesirable, > I can remove it completely ... but I figured it would be nice to leave in > some really basic debugging messages. When I come across verbose output during review I'm not sure whether you have it there because you weren't sure about something that still needs to be discussed before merging the patch. I'm trying to identify loose ends by asking these questions. > >>+ > >>+ switch (cid) { > >>+ case PCI_CAP_ID_PM: > >>+ ahci_test_pmcap(ahci, offset); > >>+ break; > >>+ case PCI_CAP_ID_MSI: > >>+ ahci_test_msicap(ahci, offset); > >>+ break; > >>+ case PCI_CAP_ID_SATA: > >>+ ahci_test_satacap(ahci, offset); > >>+ break; > >>+ > >>+ default: > >>+ g_test_message("Unknown CAP 0x%02x", cid); > >This should be a failure. If a new capability is added, the test should > >be extended for that capability. > The specification does allow for any number of arbitrary capabilities, in > which the specification has no say about order or compliance. Any further > investigation really delves into the PCI specification, which was not my aim > here. I think mentioning the inability to verify a capability is fine via > --verbose for the purposes of basic AHCI sanity tests. At any rate, I don't > think this is a failure -- at least not in THIS test, which I tried to keep > as a "spec-adherent, QEMU indifferent" test. If we want to test > QEMU-specifics, I would like to add those into a separate test case -- at > which point failures for unrecognized capabilities would be appropriate. > > In the future, we might even abstract out these pieces that apply to PCI > devices as a whole to be generic PCI spec compliance tests, with an AHCI and > then a QEMU layer on top, in order of increasing specificity. Okay, just keep in mind that only someone actively developing the test case will notice verbose output. By skipping unknown capabilities we won't have a reminder that this test case needs to be updated when a new one is added. Stefan
diff --git a/tests/ahci-test.c b/tests/ahci-test.c index b3ed85f..e0a8f34 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -43,6 +43,11 @@ /*** Supplementary PCI Config Space IDs & Masks ***/ #define PCI_DEVICE_ID_INTEL_Q35_AHCI (0x2922) +#define PCI_MSI_FLAGS_RESERVED (0xFF00) +#define PCI_PM_CTRL_RESERVED (0xFC) +#define PCI_BCC(REG32) ((REG32) >> 24) +#define PCI_PI(REG32) (((REG32) >> 8) & 0xFF) +#define PCI_SCC(REG32) (((REG32) >> 16) & 0xFF) /* To help make it clear that the HBA is not a pointer to local memory. */ typedef void HBA; @@ -52,9 +57,22 @@ static QGuestAllocator *guest_malloc; static QPCIBus *pcibus; static char tmp_path[] = "/tmp/qtest.XXXXXX"; +/*** Macro Utilities ***/ +#define bitany(data, mask) (((data) & (mask)) != 0) +#define bitset(data, mask) (((data) & (mask)) == (mask)) +#define bitclr(data, mask) (((data) & (mask)) == 0) +#define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, (mask)) +#define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0) + /*** Function Declarations ***/ static QPCIDevice *get_ahci_device(void); static void free_ahci_device(QPCIDevice *dev); +static void ahci_test_pci_spec(QPCIDevice *ahci); +static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header, + uint8_t offset); +static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset); +static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset); +static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset); /*** Utilities ***/ @@ -156,6 +174,246 @@ static void ahci_shutdown(QPCIDevice *ahci) qtest_shutdown(); } +/*** Specification Adherence Tests ***/ + +/** + * Implementation for test_pci_spec. Ensures PCI configuration space is sane. + */ +static void ahci_test_pci_spec(QPCIDevice *ahci) +{ + uint8_t datab; + uint16_t data; + uint32_t datal; + + /* Most of these bits should start cleared until we turn them on. */ + data = qpci_config_readw(ahci, PCI_COMMAND); + assert_bit_clear(data, PCI_COMMAND_MEMORY); + assert_bit_clear(data, PCI_COMMAND_MASTER); + assert_bit_clear(data, PCI_COMMAND_SPECIAL); /* Reserved */ + assert_bit_clear(data, PCI_COMMAND_VGA_PALETTE); /* Reserved */ + assert_bit_clear(data, PCI_COMMAND_PARITY); + assert_bit_clear(data, PCI_COMMAND_WAIT); /* Reserved */ + assert_bit_clear(data, PCI_COMMAND_SERR); + assert_bit_clear(data, PCI_COMMAND_FAST_BACK); + assert_bit_clear(data, PCI_COMMAND_INTX_DISABLE); + assert_bit_clear(data, 0xF800); /* Reserved */ + + data = qpci_config_readw(ahci, PCI_STATUS); + assert_bit_clear(data, 0x01 | 0x02 | 0x04); /* Reserved */ + assert_bit_clear(data, PCI_STATUS_INTERRUPT); + assert_bit_set(data, PCI_STATUS_CAP_LIST); /* must be set */ + assert_bit_clear(data, PCI_STATUS_UDF); /* Reserved */ + assert_bit_clear(data, PCI_STATUS_PARITY); + assert_bit_clear(data, PCI_STATUS_SIG_TARGET_ABORT); + assert_bit_clear(data, PCI_STATUS_REC_TARGET_ABORT); + assert_bit_clear(data, PCI_STATUS_REC_MASTER_ABORT); + assert_bit_clear(data, PCI_STATUS_SIG_SYSTEM_ERROR); + assert_bit_clear(data, PCI_STATUS_DETECTED_PARITY); + + /* RID occupies the low byte, CCs occupy the high three. */ + datal = qpci_config_readl(ahci, PCI_CLASS_REVISION); +#ifdef AHCI_13_STRICT + /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */ + assert_bit_clear(datal, 0xFF); +#else + if (datal & 0xFF) { + g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF); + } +#endif + + /* BCC *must* equal 0x01. */ + g_assert(PCI_BCC(datal) == 0x01); + if (PCI_SCC(datal) == 0x01) { + /* IDE */ + assert_bit_set(0x80000000, datal); + assert_bit_clear(0x60000000, datal); + } else if (PCI_SCC(datal) == 0x04) { + /* RAID */ + g_assert(PCI_PI(datal) == 0); + } else if (PCI_SCC(datal) == 0x06) { + /* AHCI */ + g_assert(PCI_PI(datal) == 0x01); + } else { + g_assert_not_reached(); + } + + datab = qpci_config_readb(ahci, PCI_CACHE_LINE_SIZE); + g_assert(datab == 0); + + datab = qpci_config_readb(ahci, PCI_LATENCY_TIMER); + g_assert(datab == 0); + + /* Only the bottom 7 bits must be off. */ + datab = qpci_config_readb(ahci, PCI_HEADER_TYPE); + assert_bit_clear(datab, 0x7F); + + /* BIST is optional, but the low 7 bits must always start off regardless. */ + datab = qpci_config_readb(ahci, PCI_BIST); + assert_bit_clear(datab, 0x7F); + + /* BARS 0-4 do not have a boot spec, but ABAR/BAR5 must be clean. */ + datal = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5); + g_assert(datal == 0); + + qpci_config_writel(ahci, PCI_BASE_ADDRESS_5, 0xFFFFFFFF); + datal = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5); + /* ABAR must be 32-bit, memory mapped, non-prefetchable and + * must be >= 512 bytes. To that end, bits 0-8 must be off. */ + assert_bit_clear(datal, 0xFF); + + /* Capability list MUST be present, */ + datal = qpci_config_readl(ahci, PCI_CAPABILITY_LIST); + /* But these bits are reserved. */ + assert_bit_clear(datal, ~0xFF); + g_assert_cmphex(datal, !=, 0); + + /* Check specification adherence for capability extenstions. */ + data = qpci_config_readw(ahci, datal); + +#ifdef AHCI_13_STRICT + /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */ + g_assert((data & 0xFF) == PCI_CAP_ID_PM); +#elif defined Q35 + /* Intel ICH9 Family Datasheet 14.1.19 p.550 */ + if ((data & 0xFF) != PCI_CAP_ID_MSI) { + g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)", + data & 0xFF, PCI_CAP_ID_MSI); + } + /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/ +#else + if ((data & 0xFF) != PCI_CAP_ID_PM) { + g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)", + data & 0xFF, PCI_CAP_ID_PM); + } +#endif + + ahci_test_pci_caps(ahci, data, (uint8_t)datal); + + /* Reserved. */ + datal = qpci_config_readl(ahci, PCI_CAPABILITY_LIST + 4); + g_assert(datal == 0); + + /* IPIN might vary, but ILINE must be off. */ + datab = qpci_config_readb(ahci, PCI_INTERRUPT_LINE); + g_assert(datab == 0); +} + +/** + * Test PCI capabilities for AHCI specification adherence. + */ +static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header, + uint8_t offset) +{ + uint8_t cid = header & 0xFF; + uint8_t next = header >> 8; + + g_test_message("CID: %02x; next: %02x", cid, next); + + switch (cid) { + case PCI_CAP_ID_PM: + ahci_test_pmcap(ahci, offset); + break; + case PCI_CAP_ID_MSI: + ahci_test_msicap(ahci, offset); + break; + case PCI_CAP_ID_SATA: + ahci_test_satacap(ahci, offset); + break; + + default: + g_test_message("Unknown CAP 0x%02x", cid); + } + + if (next) { + ahci_test_pci_caps(ahci, qpci_config_readw(ahci, next), next); + } +} + +/** + * Test SATA PCI capabilitity for AHCI specification adherence. + */ +static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset) +{ + uint16_t dataw; + uint32_t datal; + + g_test_message("Verifying SATACAP"); + + /* Assert that the SATACAP version is 1.0, And reserved bits are empty. */ + dataw = qpci_config_readw(ahci, offset + 2); + g_assert(dataw == 0x10); + + /* Grab the SATACR1 register. */ + datal = qpci_config_readw(ahci, offset + 4); + + switch (datal & 0x0F) { + case 0x04: /* BAR0 */ + case 0x05: /* BAR1 */ + case 0x06: + case 0x07: + case 0x08: + case 0x09: /* BAR5 */ + case 0x0F: /* Immediately following SATACR1 in PCI config space. */ + break; + default: + /* Invalid BARLOC for the Index Data Pair. */ + g_assert_not_reached(); + } + + /* Reserved. */ + g_assert((datal >> 24) == 0x00); +} + +/** + * Test MSI PCI capability for AHCI specification adherence. + */ +static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset) +{ + uint16_t dataw; + uint32_t datal; + + g_test_message("Verifying MSICAP"); + + dataw = qpci_config_readw(ahci, offset + PCI_MSI_FLAGS); + assert_bit_clear(dataw, PCI_MSI_FLAGS_ENABLE); + assert_bit_clear(dataw, PCI_MSI_FLAGS_QSIZE); + assert_bit_clear(dataw, PCI_MSI_FLAGS_RESERVED); + + datal = qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_LO); + g_assert(datal == 0x00); + + if (dataw & PCI_MSI_FLAGS_64BIT) { + g_test_message("MSICAP is 64bit"); + g_assert(qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_HI) == 0x00); + g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_64) == 0x00); + } else { + g_test_message("MSICAP is 32bit"); + g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_32) == 0x00); + } +} + +/** + * Test Power Management PCI capability for AHCI specification adherence. + */ +static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset) +{ + uint16_t dataw; + + g_test_message("Verifying PMCAP"); + + dataw = qpci_config_readw(ahci, offset + PCI_PM_PMC); + assert_bit_clear(dataw, PCI_PM_CAP_PME_CLOCK); + assert_bit_clear(dataw, PCI_PM_CAP_RESERVED); + assert_bit_clear(dataw, PCI_PM_CAP_D1); + assert_bit_clear(dataw, PCI_PM_CAP_D2); + + dataw = qpci_config_readw(ahci, offset + PCI_PM_CTRL); + assert_bit_clear(dataw, PCI_PM_CTRL_STATE_MASK); + assert_bit_clear(dataw, PCI_PM_CTRL_RESERVED); + assert_bit_clear(dataw, PCI_PM_CTRL_DATA_SEL_MASK); + assert_bit_clear(dataw, PCI_PM_CTRL_DATA_SCALE_MASK); +} + /******************************************************************************/ /* Test Interfaces */ /******************************************************************************/ @@ -170,6 +428,18 @@ static void test_sanity(void) ahci_shutdown(ahci); } +/** + * Ensure that the PCI configuration space for the AHCI device is in-line with + * the AHCI 1.3 specification for initial values. + */ +static void test_pci_spec(void) +{ + QPCIDevice *ahci; + ahci_boot(&ahci); + ahci_test_pci_spec(ahci); + ahci_shutdown(ahci); +} + /******************************************************************************/ int main(int argc, char **argv) @@ -197,6 +467,7 @@ int main(int argc, char **argv) /* Run the tests */ qtest_add_func("/ahci/sanity", test_sanity); + qtest_add_func("/ahci/pci_spec", test_pci_spec); ret = g_test_run();
Adds a specification adherence test for AHCI where the boot-up values for the PCI configuration space are compared against the AHCI 1.3 specification. This test does not itself attempt to engage the device. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/ahci-test.c | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 271 insertions(+)