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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]