From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCqX1-0000Ne-JJ for qemu-devel@nongnu.org; Thu, 31 Jul 2014 09:36:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCqWp-0001uc-Vt for qemu-devel@nongnu.org; Thu, 31 Jul 2014 09:36:31 -0400 Received: from mail-we0-x235.google.com ([2a00:1450:400c:c03::235]:48026) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCqWp-0001uY-Nr for qemu-devel@nongnu.org; Thu, 31 Jul 2014 09:36:19 -0400 Received: by mail-we0-f181.google.com with SMTP id k48so2828661wev.12 for ; Thu, 31 Jul 2014 06:36:18 -0700 (PDT) Date: Thu, 31 Jul 2014 14:36:13 +0100 From: Stefan Hajnoczi Message-ID: <20140731133613.GB25929@stefanha-thinkpad.redhat.com> References: <1404757089-4836-1-git-send-email-jsnow@redhat.com> <1404757089-4836-26-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="fQa200R4EO7jAQ6Z" Content-Disposition: inline In-Reply-To: <1404757089-4836-26-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH 25/28] ahci: add test_pci_enable to ahci-test. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com --fQa200R4EO7jAQ6Z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. --fQa200R4EO7jAQ6Z Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT2kZNAAoJEJykq7OBq3PINIQH/iNH04+fFLj6pKKudOVH07lB kh52xPYhgHx6Rxe4JjdK3HRy5cDcaUKVrLtZHC5mgliM3IPVr6tUJBFp+dyovPWw cdb1hUJ45l/vg7SkJ3TfY1hmcaosPpoH46zb8ZFb+mGT5IROsKuw2QsezW6yZE9r E4m793HtnReTbUKuPmvsROJ754q6FXbFyFywt7ZSD1F8dyGoDTc4om++yYtXz6i6 wygX5BGEe7g5g2zotDKxM2rpgr/XXOc4jwbAzA+d24Ls3N3jsXIF7T5aE78ln09O uABTgnYQcrihWoA7aq1L3tcB6D8mfgQR9rCdhRxVHq4jqHNV/4tK3UGWWyMbvVQ= =egGf -----END PGP SIGNATURE----- --fQa200R4EO7jAQ6Z--