From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53403) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCuMv-0006OB-5y for qemu-devel@nongnu.org; Thu, 31 Jul 2014 13:42:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCuMo-0002pN-Pr for qemu-devel@nongnu.org; Thu, 31 Jul 2014 13:42:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36594) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCuMo-0002nC-I1 for qemu-devel@nongnu.org; Thu, 31 Jul 2014 13:42:14 -0400 Message-ID: <53DA7FEA.2060409@redhat.com> Date: Thu, 31 Jul 2014 13:42:02 -0400 From: John Snow MIME-Version: 1.0 References: <1404757089-4836-1-git-send-email-jsnow@redhat.com> <1404757089-4836-25-git-send-email-jsnow@redhat.com> <20140731131902.GA25929@stefanha-thinkpad.redhat.com> In-Reply-To: <20140731131902.GA25929@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 24/28] ahci: Add test_pci_spec to ahci-test. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com 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.