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

[John Snow]

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.