Re: [Qemu-devel] [PATCH 24/28] ahci: Add test_pci_spec to ahci-test.

[Stefan Hajnoczi]

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?

pgpmmHmWTOBXb.pgp
Description: PGP signature