From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56775) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEEvj-00021A-TA for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:51:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XEEvf-0002UM-1j for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:51:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEEve-0002UC-RQ for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:51:42 -0400 Date: Mon, 4 Aug 2014 10:51:38 +0100 From: Stefan Hajnoczi Message-ID: <20140804095138.GA5784@stefanha-thinkpad.redhat.com> References: <1404757089-4836-1-git-send-email-jsnow@redhat.com> <1404757089-4836-27-git-send-email-jsnow@redhat.com> <20140731140111.GC25929@stefanha-thinkpad.redhat.com> <53DC227D.60205@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="huq684BweRXVnRxX" Content-Disposition: inline In-Reply-To: <53DC227D.60205@redhat.com> Subject: Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Stefan Hajnoczi , mst@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com --huq684BweRXVnRxX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 01, 2014 at 07:27:57PM -0400, John Snow wrote: >=20 > On 07/31/2014 10:01 AM, Stefan Hajnoczi wrote: > >On Mon, Jul 07, 2014 at 02:18:07PM -0400, John Snow wrote: > >>+/*** IO macros for the AHCI memory registers. ***/ > >>+#define void_incr(vptr, OFST) ((void *)((char *)(vptr) + (OFST))) > >I'm pretty sure QEMU takes advantage of GCC's void pointer arithmetic > >extension: > >https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Pointer-Arith.html#Pointer-= Arith > > > >In other words, vptr + OFST works and you don't need a macro. > > > >>+#define ahci_set(regno, mask) ahci_wreg((regno), ahci_rreg(regno) | (m= ask)) > >>+#define ahci_clr(regno, mask) ahci_wreg((regno), ahci_rreg(regno) & ~(= mask)) > >Unused. Please move to the patch that actually uses them. > > > >>+#define px_set(port, reg, mask) px_wreg((port), (reg), = \ > >>+ px_rreg((port), (reg)) | (mas= k)); > >>+#define px_clr(port, reg, mask) px_wreg((port), (reg), = \ > >>+ px_rreg((port), (reg)) & ~(ma= sk)); > >Unused. Please move to the patch that actually uses them. > > > >>+ /* We need to know the size of the region, > >>+ * but qpci_iomap doesn't save it. Recalculate it. */ > >It seems like many tests will want to check the BAR size. Please add an > >argument to qpci_iomap() so the caller gets the size. > > > >>+ if (bitset(cap, AHCI_CAP_SAM)) { > >>+ g_test_message("Supports AHCI-Only Mode: GHC_AE is Read-Only."= ); > >>+ assert_bit_set(reg, AHCI_GHC_AE); > >>+ } else { > >>+ g_test_message("Supports AHCI/Legacy mix."); > >>+ assert_bit_clear(reg, AHCI_GHC_AE); > >>+ } > >Let's just assert what QEMU implements. > > > >>+ /* 12 -- 23: Reserved */ > >>+ g_test_message("Verifying HBA reserved area is empty."); > >Debugging message that can be removed? > > > >More elsewhere in this patch. >=20 > one last question -- is there a reasonable way to have assertions that al= low > the test to shamble forward, still fail at the end, and still run subsequ= ent > test cases? > A lot of the warnings I threw in here are actually errors, but it'd still= be > useful to see the rest of the test run to completion as best as it can. >=20 > You can call g_test_set_nonfatal_assertions(), but it actually appears as= if > (on my machine at least) that this is a nop, which is not super helpful. >=20 > It'd be nice if the checked in version of the test showed you the myriad > failings, instead of just one at a time until you hack in/out certain > assertions manually if you are interested in other areas. I'm not aware of a glib API to do that. You can tell gtester which test cases to invoke if you need to skip or only run certain cases. That might be useful during development. Stefan --huq684BweRXVnRxX Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT31eqAAoJEJykq7OBq3PIvAUH/RejTkY9LcnJ30/uNCpmJ/qC h73sNHsOZXKdycqy+4W9s5+ElDHah67T3DS6nq+XRHqRWZJqjCL4sV7gop9Pzh/Y ZDqyMo5sA5ltt/d6+hGJVtdqqC/kLjVg8T/OkCa4d2shG9fWEmSgkX79JVkqEsCf RxrSQT+vA9vHw/KRq9VBpHPo4kaKevew3Ld9xcTZDoyHW3zyHRL+nEnM2XpXMcDm 09rqtXnhLEpAE+vQ0kY+NFPDpGLskoqpiuF/WpOAnB1edtsrJC7kd03gUkZTGe/b maDziS5edYMudZtxECf41ku0WLS+OJDXOSmFXDWIBFAUYKpayrIBE8QVlfn0tzM= =krHG -----END PGP SIGNATURE----- --huq684BweRXVnRxX--