From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44163) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDMFA-0001zt-9S for qemu-devel@nongnu.org; Fri, 01 Aug 2014 19:28:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XDMF5-0008Rm-Eu for qemu-devel@nongnu.org; Fri, 01 Aug 2014 19:28:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24352) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDMF5-0008RX-5n for qemu-devel@nongnu.org; Fri, 01 Aug 2014 19:28:07 -0400 Message-ID: <53DC227D.60205@redhat.com> Date: Fri, 01 Aug 2014 19:27:57 -0400 From: John Snow MIME-Version: 1.0 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> In-Reply-To: <20140731140111.GC25929@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Stefan Hajnoczi Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com 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) | (mask)) >> +#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)) | (mask)); >> +#define px_clr(port, reg, mask) px_wreg((port), (reg), \ >> + px_rreg((port), (reg)) & ~(mask)); > 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. one last question -- is there a reasonable way to have assertions that allow the test to shamble forward, still fail at the end, and still run subsequent 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. 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. 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.