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

[John Snow <jsnow@redhat.com>]

On 01/19/2015 12:09 PM, Paolo Bonzini wrote:
>
>
> 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
>

No, whoops. Was thinking of the struct syntax. I am surprised it doesn't 
complain. I'll just remove it.

>> +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
>>
>