Hi Peter, On 08/03/18 19:28, Peter Maydell wrote: > On 17 February 2018 at 18:46, Eric Auger <eric.au...@redhat.com> 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 <eric.au...@redhat.com> >> >> --- >> >> 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 >