[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [RFC PATCH] hw/registerfields: Deposit field
|
From: |
Philippe Mathieu-Daudé |
|
Subject: |
Re: [Qemu-arm] [Qemu-devel] [RFC PATCH] hw/registerfields: Deposit fields "in place" |
|
Date: |
Mon, 4 Jun 2018 16:43:19 -0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 06/04/2018 03:01 PM, Alistair Francis wrote:
> On Sat, Jun 2, 2018 at 3:26 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
>> On 06/02/2018 05:55 PM, Philippe Mathieu-Daudé wrote:
>>> These macros are always used to deposit a field in place.
>>> Update them to take the pointer argument.
>>>
>>> As these macros are constructed using compound statements,
>>> it is easy to not use the return value, and the compiler
>>> doesn't warn. Thus this change makes these macros safer.
>>
>> "and the compiler doesn't warn [if the return value is ignored]."
>>
>> Adding __attribute__ ((warn_unused_result)) to depositXX() doesn't help
>> since the retval is used inside the compound statement.
>
> Doesn't this then limit someone from writing an if statement around a
> value in a register?
It does indeed, but I'm not sure this would be a good code practice.
Also as you can see in this patch, there is no such use in the codebase.
I'd rather use 2 explicit steps, calling FIELD_EX64() first:
if (FIELD_EX64(storage, reg, field) == val1) {
FIELD_DP64(&storage, reg, field, val2);
}
Maybe there is a way to write using __attribute__
((warn_unused_result)), eventually in 2 steps, 1 #define and 1 inlined func.
>
> Alistair
>
>>
>>>
>>> This also helps having a simpler/shorter code to review.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>> hw/arm/smmuv3-internal.h | 2 +-
>>> include/hw/registerfields.h | 14 +++++-----
I never noticed how git orderfile orders include...
>>> hw/arm/smmuv3.c | 28 +++++++++----------
>>> hw/dma/xlnx-zdma.c | 6 ++--
>>> hw/sd/sd.c | 4 +--
>>> hw/sd/sdhci.c | 56 ++++++++++++++++++-------------------
>>> 6 files changed, 53 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>> index a9d714b56e..2f020e2e90 100644
>>> --- a/hw/arm/smmuv3-internal.h
>>> +++ b/hw/arm/smmuv3-internal.h
>>> @@ -203,7 +203,7 @@ static inline bool smmuv3_eventq_enabled(SMMUv3State *s)
>>>
>>> static inline void smmu_write_cmdq_err(SMMUv3State *s, uint32_t err_type)
>>> {
>>> - s->cmdq.cons = FIELD_DP32(s->cmdq.cons, CMDQ_CONS, ERR, err_type);
>>> + FIELD_DP32(&s->cmdq.cons, CMDQ_CONS, ERR, err_type);
>>> }
>>>
>>> /* Commands */
>>> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
>>> index 2659a58737..14a12f6a48 100644
>>> --- a/include/hw/registerfields.h
>>> +++ b/include/hw/registerfields.h
>>> @@ -45,7 +45,7 @@
>>> #define ARRAY_FIELD_EX32(regs, reg, field) \
>>> FIELD_EX32((regs)[R_ ## reg], reg, field)
>>>
>>> -/* Deposit a register field.
>>> +/* Deposit a register field (in place).
>>> * Assigning values larger then the target field will result in
>>> * compilation warnings.
>>> */
>>> @@ -54,20 +54,20 @@
>>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
>>> } v = { .v = val }; \
>>> uint32_t d; \
>>> - d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> - R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>>> - d; })
>>> + d = deposit32(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> + R_ ## reg ## _ ## field ## _LENGTH, v.v);
>>> \
>>
>> I forgot to remove an extra space here.
>>
>>> + *(storage) = d; })
>>> #define FIELD_DP64(storage, reg, field, val) ({ \
>>> struct { \
>>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
>>> } v = { .v = val }; \
>>> uint64_t d; \
>>> - d = deposit64((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> + d = deposit64(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>>> - d; })
>>> + *(storage) = d; })
>>>
>>> /* Deposit a field to array of registers. */
>>> #define ARRAY_FIELD_DP32(regs, reg, field, val) \
>>> - (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
>>> + FIELD_DP32(&(regs)[R_ ## reg], reg, field, val);
>>>
>>> #endif
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 42dc521c13..6406b69d68 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -238,25 +238,25 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>>> * multi-level stream table
>>> */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported
>>> */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTENDIAN, 2); /* little endian
>>> */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
>>> + FIELD_DP32(&s->idr[0], IDR0, S1P, 1); /* stage 1 supported */
>>> + FIELD_DP32(&s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
>>> + FIELD_DP32(&s->idr[0], IDR0, COHACC, 1); /* IO coherent */
>>> + FIELD_DP32(&s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
>>> + FIELD_DP32(&s->idr[0], IDR0, TTENDIAN, 2); /* little endian */
>>> + FIELD_DP32(&s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
>>> /* terminated transaction will always be aborted/error returned */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TERM_MODEL, 1);
>>> + FIELD_DP32(&s->idr[0], IDR0, TERM_MODEL, 1);
>>> /* 2-level stream table supported */
>>> - s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, 1);
>>> + FIELD_DP32(&s->idr[0], IDR0, STLEVEL, 1);
>>>
>>> - s->idr[1] = FIELD_DP32(s->idr[1], IDR1, SIDSIZE, SMMU_IDR1_SIDSIZE);
>>> - s->idr[1] = FIELD_DP32(s->idr[1], IDR1, EVENTQS, SMMU_EVENTQS);
>>> - s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS, SMMU_CMDQS);
>>> + FIELD_DP32(&s->idr[1], IDR1, SIDSIZE, SMMU_IDR1_SIDSIZE);
>>> + FIELD_DP32(&s->idr[1], IDR1, EVENTQS, SMMU_EVENTQS);
>>> + FIELD_DP32(&s->idr[1], IDR1, CMDQS, SMMU_CMDQS);
>>>
>>> /* 4K and 64K granule support */
>>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
>>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44
>>> bits */
>>> + FIELD_DP32(&s->idr[5], IDR5, GRAN4K, 1);
>>> + FIELD_DP32(&s->idr[5], IDR5, GRAN64K, 1);
>>> + FIELD_DP32(&s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
>>>
>>> s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
>>> s->cmdq.prod = 0;
>>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
>>> index 8eea757aff..82fa3ea672 100644
>>> --- a/hw/dma/xlnx-zdma.c
>>> +++ b/hw/dma/xlnx-zdma.c
>>> @@ -426,10 +426,8 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf,
>>> uint32_t len)
>>> }
>>>
>>> /* Write back to buffered descriptor. */
>>> - s->dsc_dst.words[2] = FIELD_DP32(s->dsc_dst.words[2],
>>> - ZDMA_CH_DST_DSCR_WORD2,
>>> - SIZE,
>>> - dst_size);
>>> + FIELD_DP32(&s->dsc_dst.words[2],
>>> + ZDMA_CH_DST_DSCR_WORD2, SIZE, dst_size);
>>> }
>>> }
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 7af19fa06c..0f41028e91 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -301,10 +301,10 @@ static void sd_ocr_powerup(void *opaque)
>>> assert(!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP));
>>>
>>> /* card power-up OK */
>>> - sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
>>> + FIELD_DP32(&sd->ocr, OCR, CARD_POWER_UP, 1);
>>>
>>> if (sd->size > 1 * G_BYTE) {
>>> - sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_CAPACITY, 1);
>>> + FIELD_DP32(&sd->ocr, OCR, CARD_CAPACITY, 1);
>>> }
>>> }
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 63c44a4ee8..d42492f60a 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -94,21 +94,21 @@ static void sdhci_check_capareg(SDHCIState *s, Error
>>> **errp)
>>> case 4:
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS64BIT_V4);
>>> trace_sdhci_capareg("64-bit system bus (v4)", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, BUS64BIT_V4, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, BUS64BIT_V4, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, UHS_II);
>>> trace_sdhci_capareg("UHS-II", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, UHS_II, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, UHS_II, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA3);
>>> trace_sdhci_capareg("ADMA3", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA3, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, ADMA3, 0);
>>>
>>> /* fallthrough */
>>> case 3:
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, ASYNC_INT);
>>> trace_sdhci_capareg("async interrupt", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, ASYNC_INT, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, ASYNC_INT, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, SLOT_TYPE);
>>> if (val) {
>>> @@ -116,70 +116,70 @@ static void sdhci_check_capareg(SDHCIState *s, Error
>>> **errp)
>>> return;
>>> }
>>> trace_sdhci_capareg("slot type", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, SLOT_TYPE, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, SLOT_TYPE, 0);
>>>
>>> if (val != 2) {
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, EMBEDDED_8BIT);
>>> trace_sdhci_capareg("8-bit bus", val);
>>> }
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, EMBEDDED_8BIT, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, EMBEDDED_8BIT, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS_SPEED);
>>> trace_sdhci_capareg("bus speed mask", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, BUS_SPEED, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, BUS_SPEED, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, DRIVER_STRENGTH);
>>> trace_sdhci_capareg("driver strength mask", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, DRIVER_STRENGTH, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, DRIVER_STRENGTH, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, TIMER_RETUNING);
>>> trace_sdhci_capareg("timer re-tuning", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, TIMER_RETUNING, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, TIMER_RETUNING, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, SDR50_TUNING);
>>> trace_sdhci_capareg("use SDR50 tuning", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, SDR50_TUNING, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, SDR50_TUNING, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, RETUNING_MODE);
>>> trace_sdhci_capareg("re-tuning mode", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, RETUNING_MODE, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, RETUNING_MODE, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, CLOCK_MULT);
>>> trace_sdhci_capareg("clock multiplier", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, CLOCK_MULT, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, CLOCK_MULT, 0);
>>>
>>> /* fallthrough */
>>> case 2: /* default version */
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA2);
>>> trace_sdhci_capareg("ADMA2", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA2, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, ADMA2, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA1);
>>> trace_sdhci_capareg("ADMA1", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA1, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, ADMA1, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS64BIT);
>>> trace_sdhci_capareg("64-bit system bus (v3)", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, BUS64BIT, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, BUS64BIT, 0);
>>>
>>> /* fallthrough */
>>> case 1:
>>> y = FIELD_EX64(s->capareg, SDHC_CAPAB, TOUNIT);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, TOUNIT, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, TOUNIT, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, TOCLKFREQ);
>>> trace_sdhci_capareg(y ? "timeout (MHz)" : "Timeout (KHz)", val);
>>> if (sdhci_check_capab_freq_range(s, "timeout", val, errp)) {
>>> return;
>>> }
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, TOCLKFREQ, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, TOCLKFREQ, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, BASECLKFREQ);
>>> trace_sdhci_capareg(y ? "base (MHz)" : "Base (KHz)", val);
>>> if (sdhci_check_capab_freq_range(s, "base", val, errp)) {
>>> return;
>>> }
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, BASECLKFREQ, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, BASECLKFREQ, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH);
>>> if (val >= 3) {
>>> @@ -187,31 +187,31 @@ static void sdhci_check_capareg(SDHCIState *s, Error
>>> **errp)
>>> return;
>>> }
>>> trace_sdhci_capareg("max block length", sdhci_get_fifolen(s));
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, MAXBLOCKLENGTH, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, MAXBLOCKLENGTH, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, HIGHSPEED);
>>> trace_sdhci_capareg("high speed", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, HIGHSPEED, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, HIGHSPEED, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, SDMA);
>>> trace_sdhci_capareg("SDMA", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, SDMA, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, SDMA, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, SUSPRESUME);
>>> trace_sdhci_capareg("suspend/resume", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, SUSPRESUME, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, SUSPRESUME, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, V33);
>>> trace_sdhci_capareg("3.3v", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, V33, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, V33, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, V30);
>>> trace_sdhci_capareg("3.0v", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, V30, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, V30, 0);
>>>
>>> val = FIELD_EX64(s->capareg, SDHC_CAPAB, V18);
>>> trace_sdhci_capareg("1.8v", val);
>>> - msk = FIELD_DP64(msk, SDHC_CAPAB, V18, 0);
>>> + FIELD_DP64(&msk, SDHC_CAPAB, V18, 0);
>>> break;
>>>
>>> default:
>>> @@ -1017,10 +1017,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr
>>> offset, unsigned size)
>>> break;
>>> case SDHC_PRNSTS:
>>> ret = s->prnsts;
>>> - ret = FIELD_DP32(ret, SDHC_PRNSTS, DAT_LVL,
>>> - sdbus_get_dat_lines(&s->sdbus));
>>> - ret = FIELD_DP32(ret, SDHC_PRNSTS, CMD_LVL,
>>> - sdbus_get_cmd_line(&s->sdbus));
>>> + FIELD_DP32(&ret, SDHC_PRNSTS, DAT_LVL,
>>> sdbus_get_dat_lines(&s->sdbus));
>>> + FIELD_DP32(&ret, SDHC_PRNSTS, CMD_LVL,
>>> sdbus_get_cmd_line(&s->sdbus));
>>> break;
>>> case SDHC_HOSTCTL:
>>> ret = s->hostctl1 | (s->pwrcon << 8) | (s->blkgap << 16) |
>>>
>>
>