[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v9 06/14] hw/arm/smmuv3: Queue helpers
From: |
Auger Eric |
Subject: |
Re: [Qemu-arm] [PATCH v9 06/14] hw/arm/smmuv3: Queue helpers |
Date: |
Fri, 9 Mar 2018 17:43:10 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
Hi Peter,
On 08/03/18 19:28, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger <address@hidden> wrote:
>> We introduce helpers to read/write into the command and event
>> circular queues.
>>
>> smmuv3_write_eventq and smmuv3_cmq_consume will become static
>> in subsequent patches.
>>
>> Invalidation commands are not yet dealt with. We do not cache
>> data that need to be invalidated. This will change with vhost
>> integration.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v8 -> v9:
>> - fix CMD_SSID & CMD_ADDR + some renamings
>> - do cons increment after the execution of the command
>> - add Q_INCONSISTENT()
>>
>> v7 -> v8
>> - use address_space_rw
>> - helpers inspired from spec
>> ---
>> hw/arm/smmuv3-internal.h | 150 +++++++++++++++++++++++++++++++++++++++++++
>> hw/arm/smmuv3.c | 162
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> hw/arm/trace-events | 4 ++
>> 3 files changed, 316 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index 40b39a1..c0771ce 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -162,4 +162,154 @@ static inline uint64_t smmu_read64(uint64_t r,
>> unsigned offset,
>> void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask);
>> void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn);
>>
>> +/* Queue Handling */
>> +
>> +#define LOG2SIZE(q) extract64((q)->base, 0, 5)
>> +#define BASE(q) ((q)->base & SMMU_BASE_ADDR_MASK)
renamed Q_LOG2SIZE and Q_BASE respectively
>
> These are both very generic names for things in header files.
>
> Looking at the required behaviour of the *_BASE registers,
> if the LOG2SIZE field is written to a value larger than the maximum,
> it is supposed to read back as the value written but must behave
> as if it was set to the maximum. That suggests to me that your
> SMMUQueue struct should have a "log2size" field which is set when
> the guest writes to *_BASE (which is where you can cap it to the
> max value).
done
>
>> +#define WRAP_MASK(q) (1 << LOG2SIZE(q))
>> +#define INDEX_MASK(q) ((1 << LOG2SIZE(q)) - 1)
>> +#define WRAP_INDEX_MASK(q) ((1 << (LOG2SIZE(q) + 1)) - 1)
>
> WRAP_INDEX_MASK is unused (but see below for a possible use)
kept and adopted your suggestions below
>
>> +
>> +#define Q_CONS_ENTRY(q) (BASE(q) + \
>> + (q)->entry_size * ((q)->cons & INDEX_MASK(q)))
>> +#define Q_PROD_ENTRY(q) (BASE(q) + \
>> + (q)->entry_size * ((q)->prod & INDEX_MASK(q)))
>> +
>> +#define Q_CONS(q) ((q)->cons & INDEX_MASK(q))
>> +#define Q_PROD(q) ((q)->prod & INDEX_MASK(q))
>
> If you put these a bit earlier you can use them in the definitions
> of Q_CONS_ENTRY and Q_PROD_ENTRY.
done
>
>> +
>> +#define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> LOG2SIZE(q))
>> +#define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> LOG2SIZE(q))
>> +
>> +#define Q_FULL(q) \
>> + (((((q)->cons) & INDEX_MASK(q)) == \
>> + (((q)->prod) & INDEX_MASK(q))) && \
>> + ((((q)->cons) & WRAP_MASK(q)) != \
>> + (((q)->prod) & WRAP_MASK(q))))
>
> You could write this as
> ((cons ^ prod) & WRAP_INDEX_MASK) == WRAP_MASK
done
>
>> +
>> +#define Q_EMPTY(q) \
>> + (((((q)->cons) & INDEX_MASK(q)) == \
>> + (((q)->prod) & INDEX_MASK(q))) && \
>> + ((((q)->cons) & WRAP_MASK(q)) == \
>> + (((q)->prod) & WRAP_MASK(q))))
>
> and this as
> (cons & WRAP_INDEX_MASK) == (prod & WRAP_INDEX_MASK)
done
>
> (or as ((cons ^ prod) & WRAP_INDEX_MASK) == 0, but that's unnecessarily
> obscure I think.)
>
>
> This is all a bit macro-heavy. Do these really all need to be macros
> rather than functions?
>
>> +
>> +#define Q_INCONSISTENT(q) \
>> +((((((q)->prod) & INDEX_MASK(q)) > (((q)->cons) & INDEX_MASK(q))) && \
>> +((((q)->prod) & WRAP_MASK(q)) != (((q)->cons) & WRAP_MASK(q)))) || \
>> +(((((q)->prod) & INDEX_MASK(q)) < (((q)->cons) & INDEX_MASK(q))) && \
>> +((((q)->prod) & WRAP_MASK(q)) == (((q)->cons) & WRAP_MASK(q))))) \
>> +
>
> This never seems to be used. (Also it has a stray trailing '\',
> and isn't indented very clearly.
removed
>
>> +#define SMMUV3_CMDQ_ENABLED(s) \
>> + (FIELD_EX32(s->cr[0], CR0, CMDQEN))
>> +
>> +#define SMMUV3_EVENTQ_ENABLED(s) \
>> + (FIELD_EX32(s->cr[0], CR0, EVENTQEN))
Those were moved to static inline functions
>> +
>> +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);
>> +}
>> +
>> +void smmuv3_write_eventq(SMMUv3State *s, Evt *evt);
>> +
>> +/* Commands */
>> +
>> +enum {
>> + SMMU_CMD_PREFETCH_CONFIG = 0x01,
>> + SMMU_CMD_PREFETCH_ADDR,
>> + SMMU_CMD_CFGI_STE,
>> + SMMU_CMD_CFGI_STE_RANGE,
>> + SMMU_CMD_CFGI_CD,
>> + SMMU_CMD_CFGI_CD_ALL,
>> + SMMU_CMD_CFGI_ALL,
>> + SMMU_CMD_TLBI_NH_ALL = 0x10,
>> + SMMU_CMD_TLBI_NH_ASID,
>> + SMMU_CMD_TLBI_NH_VA,
>> + SMMU_CMD_TLBI_NH_VAA,
>> + SMMU_CMD_TLBI_EL3_ALL = 0x18,
>> + SMMU_CMD_TLBI_EL3_VA = 0x1a,
>> + SMMU_CMD_TLBI_EL2_ALL = 0x20,
>> + SMMU_CMD_TLBI_EL2_ASID,
>> + SMMU_CMD_TLBI_EL2_VA,
>> + SMMU_CMD_TLBI_EL2_VAA, /* 0x23 */
>> + SMMU_CMD_TLBI_S12_VMALL = 0x28,
>> + SMMU_CMD_TLBI_S2_IPA = 0x2a,
>> + SMMU_CMD_TLBI_NSNH_ALL = 0x30,
>> + SMMU_CMD_ATC_INV = 0x40,
>> + SMMU_CMD_PRI_RESP,
>> + SMMU_CMD_RESUME = 0x44,
>> + SMMU_CMD_STALL_TERM,
>> + SMMU_CMD_SYNC, /* 0x46 */
>> +};
>> +
>> +static const char *cmd_stringify[] = {
>> + [SMMU_CMD_PREFETCH_CONFIG] = "SMMU_CMD_PREFETCH_CONFIG",
>> + [SMMU_CMD_PREFETCH_ADDR] = "SMMU_CMD_PREFETCH_ADDR",
>> + [SMMU_CMD_CFGI_STE] = "SMMU_CMD_CFGI_STE",
>> + [SMMU_CMD_CFGI_STE_RANGE] = "SMMU_CMD_CFGI_STE_RANGE",
>> + [SMMU_CMD_CFGI_CD] = "SMMU_CMD_CFGI_CD",
>> + [SMMU_CMD_CFGI_CD_ALL] = "SMMU_CMD_CFGI_CD_ALL",
>> + [SMMU_CMD_CFGI_ALL] = "SMMU_CMD_CFGI_ALL",
>> + [SMMU_CMD_TLBI_NH_ALL] = "SMMU_CMD_TLBI_NH_ALL",
>> + [SMMU_CMD_TLBI_NH_ASID] = "SMMU_CMD_TLBI_NH_ASID",
>> + [SMMU_CMD_TLBI_NH_VA] = "SMMU_CMD_TLBI_NH_VA",
>> + [SMMU_CMD_TLBI_NH_VAA] = "SMMU_CMD_TLBI_NH_VAA",
>> + [SMMU_CMD_TLBI_EL3_ALL] = "SMMU_CMD_TLBI_EL3_ALL",
>> + [SMMU_CMD_TLBI_EL3_VA] = "SMMU_CMD_TLBI_EL3_VA",
>> + [SMMU_CMD_TLBI_EL2_ALL] = "SMMU_CMD_TLBI_EL2_ALL",
>> + [SMMU_CMD_TLBI_EL2_ASID] = "SMMU_CMD_TLBI_EL2_ASID",
>> + [SMMU_CMD_TLBI_EL2_VA] = "SMMU_CMD_TLBI_EL2_VA",
>> + [SMMU_CMD_TLBI_EL2_VAA] = "SMMU_CMD_TLBI_EL2_VAA",
>> + [SMMU_CMD_TLBI_S12_VMALL] = "SMMU_CMD_TLBI_S12_VMALL",
>> + [SMMU_CMD_TLBI_S2_IPA] = "SMMU_CMD_TLBI_S2_IPA",
>> + [SMMU_CMD_TLBI_NSNH_ALL] = "SMMU_CMD_TLBI_NSNH_ALL",
>> + [SMMU_CMD_ATC_INV] = "SMMU_CMD_ATC_INV",
>> + [SMMU_CMD_PRI_RESP] = "SMMU_CMD_PRI_RESP",
>> + [SMMU_CMD_RESUME] = "SMMU_CMD_RESUME",
>> + [SMMU_CMD_STALL_TERM] = "SMMU_CMD_STALL_TERM",
>> + [SMMU_CMD_SYNC] = "SMMU_CMD_SYNC",
>> +};
>> +
>> +#define SMMU_CMD_STRING(type) ( \
>> +(type < ARRAY_SIZE(cmd_stringify)) ? cmd_stringify[type] : "UNKNOWN" \
>> +)
>
> If this was a function you'd know what the type of 'type' is
> and so whether it needed to have a >= 0 check on it. Also it
> will hand you a NULL pointer for a value that's inside the
> array size but not initialized, like 0x24.
OK
>
>> +
>> +/* CMDQ fields */
>> +
>> +typedef enum {
>> + SMMU_CERROR_NONE = 0,
>> + SMMU_CERROR_ILL,
>> + SMMU_CERROR_ABT,
>> + SMMU_CERROR_ATC_INV_SYNC,
>> +} SMMUCmdError;
>> +
>> +enum { /* Command completion notification */
>> + CMD_SYNC_SIG_NONE,
>> + CMD_SYNC_SIG_IRQ,
>> + CMD_SYNC_SIG_SEV,
>> +};
>> +
>> +#define CMD_TYPE(x) extract32((x)->word[0], 0 , 8)
>> +#define CMD_SSEC(x) extract32((x)->word[0], 10, 1)
>> +#define CMD_SSV(x) extract32((x)->word[0], 11, 1)
>> +#define CMD_RESUME_AC(x) extract32((x)->word[0], 12, 1)
>> +#define CMD_RESUME_AB(x) extract32((x)->word[0], 13, 1)
>> +#define CMD_SYNC_CS(x) extract32((x)->word[0], 12, 2)
>> +#define CMD_SSID(x) extract32((x)->word[0], 12, 20)
>> +#define CMD_SID(x) ((x)->word[1])
>> +#define CMD_VMID(x) extract32((x)->word[1], 0 , 16)
>> +#define CMD_ASID(x) extract32((x)->word[1], 16, 16)
>> +#define CMD_RESUME_STAG(x) extract32((x)->word[2], 0 , 16)
>> +#define CMD_RESP(x) extract32((x)->word[2], 11, 2)
>> +#define CMD_LEAF(x) extract32((x)->word[2], 0 , 1)
>> +#define CMD_STE_RANGE(x) extract32((x)->word[2], 0 , 5)
>> +#define CMD_ADDR(x) ({ \
>> + uint64_t high = (uint64_t)(x)->word[3]; \
>> + uint64_t low = extract32((x)->word[2], 12, 20); \
>> + uint64_t addr = high << 32 | (low << 12); \
>> + addr; \
>> + })
>> +
>> +int smmuv3_cmdq_consume(SMMUv3State *s);
>> +
>> #endif
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 8779d3f..0b57215 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -94,6 +94,72 @@ void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t
>> new_gerrorn)
>> trace_smmuv3_write_gerrorn(acked, s->gerrorn);
>> }
>>
>> +static uint32_t queue_index_inc(uint32_t val,
>> + uint32_t qidx_mask, uint32_t qwrap_mask)
>> +{
>> + uint32_t i = (val + 1) & qidx_mask;
>> +
>> + if (i <= (val & qidx_mask)) {
>> + i = ((val & qwrap_mask) ^ qwrap_mask) | i;
>> + } else {
>> + i = (val & qwrap_mask) | i;
>> + }
>> + return i;
>
> This is unnecessarily complicated -- an index increment is just
> val = (val + 1) & INDEX_WRAP_MASK;
> which will automatically flip the wrap bit as required.
>
OK
>> +}
>> +
>> +static inline void queue_prod_incr(SMMUQueue *q)
>> +{
>> + q->prod = queue_index_inc(q->prod, INDEX_MASK(q), WRAP_MASK(q));
>
> Doesn't this trash the ERR code in bits [30:24], or are you
> keeping that somewhere else for efficiency?
in case there is an error we don't increment. But switching to deposit32
as it looks saner.
>
>> +}
>> +
>> +static inline void queue_cons_incr(SMMUQueue *q)
>> +{
>> + q->cons = queue_index_inc(q->cons, INDEX_MASK(q), WRAP_MASK(q));
>> +}
>> +
>> +static inline MemTxResult queue_read(SMMUQueue *q, void *data)
>> +{
>> + dma_addr_t addr = Q_CONS_ENTRY(q);
>> +
>> + return dma_memory_read(&address_space_memory, addr,
>> + (uint8_t *)data, q->entry_size);
>
> Does the compiler complain if you don't provide this cast?
no
>
>> +}
>> +
>> +static void queue_write(SMMUQueue *q, void *data)
>> +{
>> + dma_addr_t addr = Q_PROD_ENTRY(q);
>> + MemTxResult ret;
>> +
>> + ret = dma_memory_write(&address_space_memory, addr,
>> + (uint8_t *)data, q->entry_size);
>> + if (ret != MEMTX_OK) {
>> + return;
>
> Shouldn't we record or return this error to the caller,
> like queue_read() does, rather than throwing it away?
> I think that for the event queue (which is the only user
> here ) this should cause an EVENTQ_ABT_ERR.
done
>
>> + }
>> +
>> + queue_prod_incr(q);
>> +}
>> +
>> +void smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>> +{
>> + SMMUQueue *q = &s->eventq;
>> + bool q_empty = Q_EMPTY(q);
>> + bool q_full = Q_FULL(q);
>
> You only use these once each, and they're not very complicated
> expressions, so you might as well just have the uses be
> "if (Q_FULL(q)) { ..." &c.
done
>
>> +
>> + if (!SMMUV3_EVENTQ_ENABLED(s)) {
>> + return;
>> + }
>> +
>> + if (q_full) {
>> + return;
>> + }
>> +
>> + queue_write(q, evt);
>> +
>> + if (q_empty) {
>> + smmuv3_trigger_irq(s, SMMU_IRQ_EVTQ, 0);
>> + }
>> +}
>> +
>> static void smmuv3_init_regs(SMMUv3State *s)
>> {
>> /**
>> @@ -133,6 +199,102 @@ static void smmuv3_init_regs(SMMUv3State *s)
>> s->sid_split = 0;
>> }
>>
>> +int smmuv3_cmdq_consume(SMMUv3State *s)
>> +{
>> + SMMUCmdError cmd_error = SMMU_CERROR_NONE;
>> + SMMUQueue *q = &s->cmdq;
>> + uint32_t type = 0;
>> +
>> + if (!SMMUV3_CMDQ_ENABLED(s)) {
>> + return 0;
>> + }
>> + /*
>> + * some commands depend on register values, as above. In case those
>
> Where is "as above" referring to ?
I meant CMDQ enabled (CR0).
>
>> + * register values change while handling the command, spec says it
>> + * is UNPREDICTABLE whether the command is interpreted under the new
>> + * or old value.
>> + */
>> +
>> + while (!Q_EMPTY(q)) {
>> + uint32_t pending = s->gerror ^ s->gerrorn;
>> + Cmd cmd;
>> +
>> + trace_smmuv3_cmdq_consume(Q_PROD(q), Q_CONS(q),
>> + Q_PROD_WRAP(q), Q_CONS_WRAP(q));
>> +
>> + if (FIELD_EX32(pending, GERROR, CMDQ_ERR)) {
>> + break;
>> + }
>> +
>> + if (queue_read(q, &cmd) != MEMTX_OK) {
>> + cmd_error = SMMU_CERROR_ABT;
>> + break;
>> + }
>> +
>> + type = CMD_TYPE(&cmd);
>> +
>> + trace_smmuv3_cmdq_opcode(SMMU_CMD_STRING(type));
>> +
>> + switch (type) {
>> + case SMMU_CMD_SYNC:
>> + if (CMD_SYNC_CS(&cmd) & CMD_SYNC_SIG_IRQ) {
>> + smmuv3_trigger_irq(s, SMMU_IRQ_CMD_SYNC, 0);
>> + }
>> + break;
>> + case SMMU_CMD_PREFETCH_CONFIG:
>> + case SMMU_CMD_PREFETCH_ADDR:
>> + case SMMU_CMD_CFGI_STE:
>> + case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
>> + case SMMU_CMD_CFGI_CD:
>> + case SMMU_CMD_CFGI_CD_ALL:
>> + case SMMU_CMD_TLBI_NH_ALL:
>> + case SMMU_CMD_TLBI_NH_ASID:
>> + case SMMU_CMD_TLBI_NH_VA:
>> + case SMMU_CMD_TLBI_NH_VAA:
>> + case SMMU_CMD_TLBI_EL3_ALL:
>> + case SMMU_CMD_TLBI_EL3_VA:
>> + case SMMU_CMD_TLBI_EL2_ALL:
>> + case SMMU_CMD_TLBI_EL2_ASID:
>> + case SMMU_CMD_TLBI_EL2_VA:
>> + case SMMU_CMD_TLBI_EL2_VAA:
>> + case SMMU_CMD_TLBI_S12_VMALL:
>> + case SMMU_CMD_TLBI_S2_IPA:
>> + case SMMU_CMD_TLBI_NSNH_ALL:
>> + case SMMU_CMD_ATC_INV:
>> + case SMMU_CMD_PRI_RESP:
>> + case SMMU_CMD_RESUME:
>> + case SMMU_CMD_STALL_TERM:
>> + trace_smmuv3_unhandled_cmd(type);
>> + break;
>> + default:
>> + cmd_error = SMMU_CERROR_ILL;
>> + error_report("Illegal command type: %d", CMD_TYPE(&cmd));
>
> This isn't what error_report() is for. You can log it as a GUEST_ERROR.
OK
>
>> + break;
>> + }
>> + if (cmd_error) {
>> + break;
>> + }
>> + /*
>> + * We only increment the cons index after the completion of
>> + * the command. We do that because the SYNC returns immediatly
>
> "immediately"
>
>> + * and do not check the completion of previous commands
>
> "does not"
>
>> + */
>> + queue_cons_incr(q);
>> + }
>> +
>> + if (cmd_error) {
>> + error_report("Error on %s command execution: %d",
>> + SMMU_CMD_STRING(type), cmd_error);
>
> Again, not error_report(). Probably a good location for a trace_ point.
OK
>
>> + smmu_write_cmdq_err(s, cmd_error);
>> + smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_CMDQ_ERR_MASK);
>> + }
>> +
>> + trace_smmuv3_cmdq_consume_out(Q_PROD(q), Q_CONS(q),
>> + Q_PROD_WRAP(q), Q_CONS_WRAP(q));
>> +
>> + return 0;
>> +}
>> +
>> static void smmu_write_mmio(void *opaque, hwaddr addr,
>> uint64_t val, unsigned size)
>> {
>> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
>> index 2ddae40..1c5105d 100644
>> --- a/hw/arm/trace-events
>> +++ b/hw/arm/trace-events
>> @@ -18,3 +18,7 @@ smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size)
>> "addr: 0x%"PRIx64" va
>> smmuv3_trigger_irq(int irq) "irq=%d"
>> smmuv3_write_gerror(uint32_t toggled, uint32_t gerror) "toggled=0x%x, new
>> gerror=0x%x"
>> smmuv3_write_gerrorn(uint32_t acked, uint32_t gerrorn) "acked=0x%x, new
>> gerrorn=0x%x"
>> +smmuv3_unhandled_cmd(uint32_t type) "Unhandled command type=%d"
>> +smmuv3_cmdq_consume(uint32_t prod, uint32_t cons, uint8_t prod_wrap,
>> uint8_t cons_wrap) "prod=%d cons=%d prod.wrap=%d cons.wrap=%d"
>> +smmuv3_cmdq_opcode(const char *opcode) "<--- %s"
>> +smmuv3_cmdq_consume_out(uint32_t prod, uint32_t cons, uint8_t prod_wrap,
>> uint8_t cons_wrap) "prod:%d, cons:%d, prod_wrap:%d, cons_wrap:%d "
>> --
>> 2.5.5
>>
>
> thanks
> -- PMM
>