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

[John Snow]

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.