Re: [Qemu-devel] [PATCH 11/14] libqos/ahci: Functional register helpers

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/01/2015 04:34, John Snow wrote:
> Introduce a set of "static inline" register helpers that are intended to
> replace the current set of macros with more functional versions that are
> better suited to inclusion in libqos than porcelain macros.
> 
> As a stopgap measure before eliminating the porcelain macros, define them
> to use the new functions defined in the ahci.h header.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ahci-test.c   | 25 ++++++++++-----------
>  tests/libqos/ahci.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index b84f942..ef751a4 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -47,22 +47,19 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
>  static bool ahci_pedantic;
>  
>  /*** IO macros for the AHCI memory registers. ***/
> -#define AHCI_READ(OFST) qpci_io_readl(ahci->dev, ahci->hba_base + (OFST))
> -#define AHCI_WRITE(OFST, VAL) qpci_io_writel(ahci->dev,                 \
> -                                             ahci->hba_base + (OFST), (VAL))
> -#define AHCI_RREG(regno)      AHCI_READ(4 * (regno))
> -#define AHCI_WREG(regno, val) AHCI_WRITE(4 * (regno), (val))
> -#define AHCI_SET(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) | (mask))
> -#define AHCI_CLR(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) & ~(mask))
> +#define AHCI_READ(OFST)       ahci_mread(ahci, (OFST))
> +#define AHCI_WRITE(OFST, VAL) ahci_mwrite(ahci, (OFST), (VAL))
> +#define AHCI_RREG(regno)      ahci_rreg(ahci, (regno))
> +#define AHCI_WREG(regno, val) ahci_wreg(ahci, (regno), (val))
> +#define AHCI_SET(regno, mask) ahci_set(ahci, (regno), (mask))
> +#define AHCI_CLR(regno, mask) ahci_clr(ahci, (regno), (mask))
>  
>  /*** IO macros for port-specific offsets inside of AHCI memory. ***/
> -#define PX_OFST(port, regno) (HBA_PORT_NUM_REG * (port) + AHCI_PORTS + (regno))
> -#define PX_RREG(port, regno)      AHCI_RREG(PX_OFST((port), (regno)))
> -#define PX_WREG(port, regno, val) AHCI_WREG(PX_OFST((port), (regno)), (val))
> -#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));
> +#define PX_OFST(port, regno)      ahci_px_ofst((port), (regno))
> +#define PX_RREG(port, regno)      ahci_px_rreg(ahci, (port), (regno))
> +#define PX_WREG(port, regno, val) ahci_px_wreg(ahci, (port), (regno), (val))
> +#define PX_SET(port, reg, mask)   ahci_px_set(ahci, (port), (reg), (mask))
> +#define PX_CLR(port, reg, mask)   ahci_px_clr(ahci, (port), (reg), (mask))
>  
>  /*** Function Declarations ***/
>  static QPCIDevice *get_ahci_device(uint32_t *fingerprint);
> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
> index 8e92385..56b1dfc 100644
> --- a/tests/libqos/ahci.h
> +++ b/tests/libqos/ahci.h
> @@ -354,4 +354,67 @@ typedef struct PRD {
>  /* For calculating how big the PRD table needs to be: */
>  #define CMD_TBL_SIZ(n) ((0x80 + ((n) * sizeof(PRD)) + 0x7F) & ~0x7F)
>  
> +/* Helpers for reading/writing AHCI HBA register values */
> +
> +static inline uint32_t ahci_mread(AHCIQState *ahci, size_t offset)
> +{
> +    return qpci_io_readl(ahci->dev, ahci->hba_base + offset);
> +}
> +
> +static inline void ahci_mwrite(AHCIQState *ahci, size_t offset, uint32_t value)
> +{
> +    qpci_io_writel(ahci->dev, ahci->hba_base + offset, value);
> +}
> +
> +static inline uint32_t ahci_rreg(AHCIQState *ahci, uint32_t reg_num)
> +{
> +    return ahci_mread(ahci, 4 * reg_num);
> +}
> +
> +static inline void ahci_wreg(AHCIQState *ahci, uint32_t reg_num, uint32_t value)
> +{
> +    ahci_mwrite(ahci, 4 * reg_num, value);
> +}
> +
> +static inline void ahci_set(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) | mask);
> +}
> +
> +static inline void ahci_clr(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) & ~mask);
> +}
> +
> +static inline size_t ahci_px_offset(uint8_t port, uint32_t reg_num)
> +{
> +    return AHCI_PORTS + (HBA_PORT_NUM_REG * port) + reg_num;
> +} __attribute__((__pure__))

This attribute applies to ahci_px_rreg, which I think is not what you want.

The compiler should figure out the "purity" anyway for a static inline
function, so my advice is to just drop it.

Paolo

> +static inline uint32_t ahci_px_rreg(AHCIQState *ahci, uint8_t port,
> +                                    uint32_t reg_num)
> +{
> +    return ahci_rreg(ahci, ahci_px_offset(port, reg_num));
> +}
> +
> +static inline void ahci_px_wreg(AHCIQState *ahci, uint8_t port,
> +                                uint32_t reg_num, uint32_t value)
> +{
> +    ahci_wreg(ahci, ahci_px_offset(port, reg_num), value);
> +}
> +
> +static inline void ahci_px_set(AHCIQState *ahci, uint8_t port,
> +                               uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_px_wreg(ahci, port, reg_num,
> +                 ahci_px_rreg(ahci, port, reg_num) | mask);
> +}
> +
> +static inline void ahci_px_clr(AHCIQState *ahci, uint8_t port,
> +                               uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_px_wreg(ahci, port, reg_num,
> +                 ahci_px_rreg(ahci, port, reg_num) & ~mask);
> +}
> +
>  #endif
>