Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

On Fri, Aug 01, 2014 at 07:27:57PM -0400, John Snow wrote:
> 
> 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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]