Re: [Qemu-devel] [PATCH 25/28] ahci: add test_pci_enable to ahci-test.

[Stefan Hajnoczi]

On Mon, Jul 07, 2014 at 02:18:06PM -0400, John Snow wrote:
> +/* Test the Q35/ICH9 behavior in preference to AHCI 1.3 behavior */
> +#define Q35
> +
> +#ifndef Q35
> +#define AHCI_13_STRICT
> +#endif
> +

Please don't use an #ifdef.  If you really want to keep both code paths
around, make it a C variable (e.g. a bool).  That way the compiler will
parse the code at least.

But there's nothing wrong with keeping the code Q35-only for now,
especially if you comment which assertions are Q35 quirks.  I would
simply drop the non-Q35 code paths.

> +/**
> + * Map BAR5/ABAR, and engage the PCI device.
> + */
> +static QPCIDevice *start_ahci_device(QPCIDevice *ahci, HBA **hba_base)

Now I see how HBA is used.  The test case shouldn't do this, please drop
HBA.

Either decide how all of libqtest/libqos should type guest physical
memory addresses and refactor the code, or just use the types from
libqtest/libqos (void*).

If every test case author invents their own type we'll have a lot of
boilerplate and test cases will be inconsistent.

> +{
> +    uint16_t data;
> +
> +    /* Map AHCI's ABAR (BAR5) */
> +    *hba_base = qpci_iomap(ahci, 5);
> +
> +    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
> +    qpci_device_enable(ahci);
> +
> +#ifdef USE_MSI
> +    data |= PCI_COMMAND_INTX_DISABLE;
> +    qpci_config_writew(ahci, PCI_COMMAND, data);
> +    data = qpci_config_readw(ahci, PCI_COMMAND);
> +#endif

Same comment as other #ifdefs.  Please don't use them.  Either test both
MSI and non-MSI cases if the difference matters, or just keep one code
path without #ifdefs.

> +
> +    /* These bits should now test as on. */
> +    data = qpci_config_readw(ahci, PCI_COMMAND);
> +    assert_bit_set(data, PCI_COMMAND_IO);
> +    assert_bit_set(data, PCI_COMMAND_MEMORY);
> +    assert_bit_set(data, PCI_COMMAND_MASTER);

Please move this to qpci_device_enable() so that all tests benefit from
these assertions.

> +#ifdef USE_MSI
> +    assert_bit_set(data, PCI_COMMAND_INTX_DISABLE);
> +#endif

MSI stuff should probably be in pci.h/pci.c/pci-pc.c so other MSI users
can take advantage of it.

pgpbGHG4tKZ0O.pgp
Description: PGP signature