qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.
Date: Thu, 31 Jul 2014 15:01:11 +0100
User-agent: Mutt/1.5.23 (2014-03-12)

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.

Attachment: pgpsIdLwkdm3h.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]