| Message ID | 20210602180042.111347-4-shashi.mallela@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | GICv3 LPI and ITS feature implementation | expand |
On Wed, 2 Jun 2021 at 19:00, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > Added functionality to trigger ITS command queue processing on > write to CWRITE register and process each command queue entry to > identify the command type and handle commands like MAPD,MAPC,SYNC. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 295 +++++++++++++++++++++++++++++++++++++++ > hw/intc/gicv3_internal.h | 37 +++++ > 2 files changed, 332 insertions(+) > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "ITS MAPC: invalid collection table attributes " > + "icid %d rdbase %lu\n", icid, rdbase); > + /* > + * in this implementation,in case of error Still missing space after comma. > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + res = update_cte(s, icid, valid, rdbase); > + } > + > + return res; > +} > + } else { > + /* > + * in this implementation,in case of dma read/write error > + * we stall the command processing > + */ Ditto. > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: %x cmd processing failed!!\n", __func__, cmd); The double-exclamation marks are unnecessary :-) > + break; > + } > + } > +} Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Hi Sashi, On 6/2/21 8:00 PM, Shashi Mallela wrote: > Added functionality to trigger ITS command queue processing on > write to CWRITE register and process each command queue entry to > identify the command type and handle commands like MAPD,MAPC,SYNC. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 295 +++++++++++++++++++++++++++++++++++++++ > hw/intc/gicv3_internal.h | 37 +++++ > 2 files changed, 332 insertions(+) > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index af60f19c98..6551c577b3 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) > return result; > } > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid, > + uint64_t rdbase) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t value; > + uint64_t l2t_addr; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + uint64_t cte = 0; > + MemTxResult res = MEMTX_OK; > + > + if (!s->ct.valid) { Isn't it a guest log error case. Also you return MEMTX_OK in that case. Is that what you want? > + return res; > + } > + > + if (valid) { > + /* add mapping entry to collection table */ > + cte = (valid & VALID_MASK) | > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); Do you really need to sanitize rdbase again? > + } > + > + /* > + * The specification defines the format of level 1 entries of a > + * 2-level table, but the format of level 2 entries and the format > + * of flat-mapped tables is IMPDEF. > + */ > + if (s->ct.indirect) { > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->ct.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > + > + if (valid_l2t) { > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + address_space_stq_le(as, l2t_addr + > + ((icid % max_l2_entries) * GITS_CTE_SIZE), > + cte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + } else { > + /* Flat level table */ > + address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE), > + cte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + return res; > +} > + > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint16_t icid; > + uint64_t rdbase; > + bool valid; > + MemTxResult res = MEMTX_OK; > + uint64_t value; > + > + offset += NUM_BYTES_IN_DW; > + offset += NUM_BYTES_IN_DW; May be relevant to add some trace points for debuggability. > + > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + icid = value & ICID_MASK; > + > + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; usually the mask is applied before the shift. > + > + valid = (value >> VALID_SHIFT) & VALID_MASK; use FIELD, see below > + > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) { you also need to check against ITS_CIDBITS limit? > + qemu_log_mask(LOG_GUEST_ERROR, > + "ITS MAPC: invalid collection table attributes " > + "icid %d rdbase %lu\n", icid, rdbase); > + /* > + * in this implementation,in case of error > + * we ignore this command and move onto the next > + * command in the queue spec says a command error occurs in that case. > + */ > + } else { > + res = update_cte(s, icid, valid, rdbase); > + } > + > + return res; > +} > + > +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid, > + uint8_t size, uint64_t itt_addr) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t value; > + uint64_t l2t_addr; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + uint64_t dte = 0; > + MemTxResult res = MEMTX_OK; > + > + if (s->dt.valid) { > + if (valid) { > + /* add mapping entry to device table */ > + dte = (valid & VALID_MASK) | > + ((size & SIZE_MASK) << 1U) | > + ((itt_addr & ITTADDR_MASK) << 6ULL); > + } > + } else { > + return res; > + } > + > + /* > + * The specification defines the format of level 1 entries of a > + * 2-level table, but the format of level 2 entries and the format > + * of flat-mapped tables is IMPDEF. > + */ > + if (s->dt.indirect) { > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->dt.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > + > + if (valid_l2t) { > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + address_space_stq_le(as, l2t_addr + > + ((devid % max_l2_entries) * GITS_DTE_SIZE), > + dte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + } else { > + /* Flat level table */ > + address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE), > + dte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + return res; > +} > + > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, you do not seem to use the input value, remove it? > + uint32_t offset) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint32_t devid; > + uint8_t size; > + uint64_t itt_addr; > + bool valid; > + MemTxResult res = MEMTX_OK; > + > + devid = (value >> DEVID_SHIFT) & DEVID_MASK; > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + size = (value & SIZE_MASK); > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; this looks weird to me, usually we apply the mask first and then shift. > + > + valid = (value >> VALID_SHIFT) & VALID_MASK; use FIELD_EX64()? > + > + if ((devid > s->dt.max_devids) || > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { ITS_IDBITS? > + qemu_log_mask(LOG_GUEST_ERROR, > + "ITS MAPD: invalid device table attributes " > + "devid %d or size %d\n", devid, size); > + /* > + * in this implementation, in case of error > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + res = update_dte(s, devid, valid, size, itt_addr); > + } > + > + return res; > +} > + > +/* > + * Current implementation blocks until all > + * commands are processed > + */ > +static void process_cmdq(GICv3ITSState *s) > +{> + uint32_t wr_offset = 0; > + uint32_t rd_offset = 0; > + uint32_t cq_offset = 0; > + uint64_t data; > + AddressSpace *as = &s->gicv3->dma_as; > + MemTxResult res = MEMTX_OK; > + uint8_t cmd; > + > + if (!(s->ctlr & ITS_CTLR_ENABLED)) { > + return; > + } > + > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); > + > + if (wr_offset > s->cq.max_entries) { Shouldn't this be checked on cwrite write instead? > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: invalid write offset " > + "%d\n", __func__, wr_offset); > + return; > + } > + > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); > + > + while (wr_offset != rd_offset) { > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); > + data = address_space_ldq_le(as, s->cq.base_addr + cq_offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + cmd = (data & CMD_MASK); > + > + switch (cmd) { > + case GITS_CMD_INT: > + break; > + case GITS_CMD_CLEAR: > + break; > + case GITS_CMD_SYNC: > + /* > + * Current implementation makes a blocking synchronous call > + * for every command issued earlier, hence the internal state > + * is already consistent by the time SYNC command is executed. > + * Hence no further processing is required for SYNC command. > + */ > + break; > + case GITS_CMD_MAPD: > + res = process_mapd(s, data, cq_offset); > + break; > + case GITS_CMD_MAPC: > + res = process_mapc(s, cq_offset); > + break; > + case GITS_CMD_MAPTI: > + break; > + case GITS_CMD_MAPI: > + break; > + case GITS_CMD_DISCARD: > + break; > + default: > + break; > + } > + if (res == MEMTX_OK) { > + rd_offset++; > + rd_offset %= s->cq.max_entries; > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset); > + } else { > + /* > + * in this implementation,in case of dma read/write error > + * we stall the command processing > + */ > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: %x cmd processing failed!!\n", __func__, cmd); > + break; > + } > + } > +} > + > static void extract_table_params(GICv3ITSState *s) > { > uint16_t num_pages = 0; > @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset, > case GITS_CWRITER: > s->cwriter = deposit64(s->cwriter, 0, 32, > (value & ~R_GITS_CWRITER_RETRY_MASK)); > + if (s->cwriter != s->creadr) { > + process_cmdq(s); I would expect process_cmdq() to be called as well on ITS enable > + } > break; > case GITS_CWRITER + 4: > s->cwriter = deposit64(s->cwriter, 32, 32, > @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset, > break; > case GITS_CWRITER: > s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; > + if (s->cwriter != s->creadr) { > + process_cmdq(s); > + } > break; > case GITS_CREADR: > case GITS_TYPER: > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > index d6aaa94e4c..0932a30560 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) > FIELD(GITS_CBASER, INNERCACHE, 59, 3) > FIELD(GITS_CBASER, VALID, 63, 1) > > +FIELD(GITS_CREADR, STALLED, 0, 1) > +FIELD(GITS_CREADR, OFFSET, 5, 15) > + > FIELD(GITS_CWRITER, RETRY, 0, 1) > FIELD(GITS_CWRITER, OFFSET, 5, 15) > > @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) > #define L1TABLE_ENTRY_SIZE 8 > > #define GITS_CMDQ_ENTRY_SIZE 32 > +#define NUM_BYTES_IN_DW 8 > + > +#define CMD_MASK 0xff > + > +/* ITS Commands */ > +#define GITS_CMD_CLEAR 0x04 > +#define GITS_CMD_DISCARD 0x0F > +#define GITS_CMD_INT 0x03 > +#define GITS_CMD_MAPC 0x09 > +#define GITS_CMD_MAPD 0x08 > +#define GITS_CMD_MAPI 0x0B > +#define GITS_CMD_MAPTI 0x0A > +#define GITS_CMD_SYNC 0x05 > + > +/* MAPC command fields */ > +#define ICID_LENGTH 16 > +#define ICID_MASK ((1U << ICID_LENGTH) - 1) can't you use FIELD') as well for the ICID? > +FIELD(MAPC, RDBASE, 16, 32) > + > +#define RDBASE_PROCNUM_LENGTH 16 > +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) - 1) why do we have both the RDBASE FIELD def and above defs? > + > +#define DEVID_SHIFT 32 > +#define DEVID_LENGTH 32 > +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) we don't have any DEVID field in MAPC, I guess it belongs to MAPD? > + > +/* MAPD command fields */ > +#define ITTADDR_LENGTH 44 > +#define ITTADDR_SHIFT 8 > +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) > +#define SIZE_MASK 0x1f Can't you homogenize the definition, use field() and/or prefix with the cmd name when not common to severals cmds? > + > +#define VALID_SHIFT 63 > +#define VALID_MASK 1ULL > > /** > * Default features advertised by this version of ITS > Thanks Eric
Hi, On 6/2/21 8:00 PM, Shashi Mallela wrote: > Added functionality to trigger ITS command queue processing on > write to CWRITE register and process each command queue entry to > identify the command type and handle commands like MAPD,MAPC,SYNC. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/intc/arm_gicv3_its.c | 295 +++++++++++++++++++++++++++++++++++++++ > hw/intc/gicv3_internal.h | 37 +++++ > 2 files changed, 332 insertions(+) > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index af60f19c98..6551c577b3 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) > return result; > } > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid, > + uint64_t rdbase) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t value; > + uint64_t l2t_addr; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + uint64_t cte = 0; > + MemTxResult res = MEMTX_OK; > + > + if (!s->ct.valid) { > + return res; > + } > + > + if (valid) { > + /* add mapping entry to collection table */ > + cte = (valid & VALID_MASK) | > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); > + } > + > + /* > + * The specification defines the format of level 1 entries of a > + * 2-level table, but the format of level 2 entries and the format > + * of flat-mapped tables is IMPDEF. > + */ > + if (s->ct.indirect) { > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->ct.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > + > + if (valid_l2t) { > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + address_space_stq_le(as, l2t_addr + > + ((icid % max_l2_entries) * GITS_CTE_SIZE), > + cte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + } else { > + /* Flat level table */ > + address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE), > + cte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + return res; > +} Looking at your DevTableDesc and CollTableDesc types again, they are basically the same except max_devids/max_collids. You may use a single one and it may be possible to define helpers to access an entry in the DT or CT. update_cte/update_dte looks quite similar if you compute the cte and dte externally and pass a pointer to the associated TableDesc? Thanks Eric > + > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint16_t icid; > + uint64_t rdbase; > + bool valid; > + MemTxResult res = MEMTX_OK; > + uint64_t value; > + > + offset += NUM_BYTES_IN_DW; > + offset += NUM_BYTES_IN_DW; > + > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + icid = value & ICID_MASK; > + > + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; > + > + valid = (value >> VALID_SHIFT) & VALID_MASK; > + > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "ITS MAPC: invalid collection table attributes " > + "icid %d rdbase %lu\n", icid, rdbase); > + /* > + * in this implementation,in case of error > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + res = update_cte(s, icid, valid, rdbase); > + } > + > + return res; > +} > + > +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid, > + uint8_t size, uint64_t itt_addr) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint64_t value; > + uint64_t l2t_addr; > + bool valid_l2t; > + uint32_t l2t_id; > + uint32_t max_l2_entries; > + uint64_t dte = 0; > + MemTxResult res = MEMTX_OK; > + > + if (s->dt.valid) { > + if (valid) { > + /* add mapping entry to device table */ > + dte = (valid & VALID_MASK) | > + ((size & SIZE_MASK) << 1U) | > + ((itt_addr & ITTADDR_MASK) << 6ULL); > + } > + } else { > + return res; > + } > + > + /* > + * The specification defines the format of level 1 entries of a > + * 2-level table, but the format of level 2 entries and the format > + * of flat-mapped tables is IMPDEF. > + */ > + if (s->dt.indirect) { > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > + > + value = address_space_ldq_le(as, > + s->dt.base_addr + > + (l2t_id * L1TABLE_ENTRY_SIZE), > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > + > + if (valid_l2t) { > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > + > + l2t_addr = value & ((1ULL << 51) - 1); > + > + address_space_stq_le(as, l2t_addr + > + ((devid % max_l2_entries) * GITS_DTE_SIZE), > + dte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + } else { > + /* Flat level table */ > + address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE), > + dte, MEMTXATTRS_UNSPECIFIED, &res); > + } > + return res; > +} > + > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, > + uint32_t offset) > +{ > + AddressSpace *as = &s->gicv3->dma_as; > + uint32_t devid; > + uint8_t size; > + uint64_t itt_addr; > + bool valid; > + MemTxResult res = MEMTX_OK; > + > + devid = (value >> DEVID_SHIFT) & DEVID_MASK; > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + size = (value & SIZE_MASK); > + > + offset += NUM_BYTES_IN_DW; > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + > + if (res != MEMTX_OK) { > + return res; > + } > + > + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; > + > + valid = (value >> VALID_SHIFT) & VALID_MASK; > + > + if ((devid > s->dt.max_devids) || > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "ITS MAPD: invalid device table attributes " > + "devid %d or size %d\n", devid, size); > + /* > + * in this implementation, in case of error > + * we ignore this command and move onto the next > + * command in the queue > + */ > + } else { > + res = update_dte(s, devid, valid, size, itt_addr); > + } > + > + return res; > +} > + > +/* > + * Current implementation blocks until all > + * commands are processed > + */ > +static void process_cmdq(GICv3ITSState *s) > +{ > + uint32_t wr_offset = 0; > + uint32_t rd_offset = 0; > + uint32_t cq_offset = 0; > + uint64_t data; > + AddressSpace *as = &s->gicv3->dma_as; > + MemTxResult res = MEMTX_OK; > + uint8_t cmd; > + > + if (!(s->ctlr & ITS_CTLR_ENABLED)) { > + return; > + } > + > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); > + > + if (wr_offset > s->cq.max_entries) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: invalid write offset " > + "%d\n", __func__, wr_offset); > + return; > + } > + > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); > + > + while (wr_offset != rd_offset) { > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); > + data = address_space_ldq_le(as, s->cq.base_addr + cq_offset, > + MEMTXATTRS_UNSPECIFIED, &res); > + cmd = (data & CMD_MASK); > + > + switch (cmd) { > + case GITS_CMD_INT: > + break; > + case GITS_CMD_CLEAR: > + break; > + case GITS_CMD_SYNC: > + /* > + * Current implementation makes a blocking synchronous call > + * for every command issued earlier, hence the internal state > + * is already consistent by the time SYNC command is executed. > + * Hence no further processing is required for SYNC command. > + */ > + break; > + case GITS_CMD_MAPD: > + res = process_mapd(s, data, cq_offset); > + break; > + case GITS_CMD_MAPC: > + res = process_mapc(s, cq_offset); > + break; > + case GITS_CMD_MAPTI: > + break; > + case GITS_CMD_MAPI: > + break; > + case GITS_CMD_DISCARD: > + break; > + default: > + break; > + } > + if (res == MEMTX_OK) { > + rd_offset++; > + rd_offset %= s->cq.max_entries; > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset); > + } else { > + /* > + * in this implementation,in case of dma read/write error > + * we stall the command processing > + */ > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: %x cmd processing failed!!\n", __func__, cmd); > + break; > + } > + } > +} > + > static void extract_table_params(GICv3ITSState *s) > { > uint16_t num_pages = 0; > @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset, > case GITS_CWRITER: > s->cwriter = deposit64(s->cwriter, 0, 32, > (value & ~R_GITS_CWRITER_RETRY_MASK)); > + if (s->cwriter != s->creadr) { > + process_cmdq(s); > + } > break; > case GITS_CWRITER + 4: > s->cwriter = deposit64(s->cwriter, 32, 32, > @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset, > break; > case GITS_CWRITER: > s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; > + if (s->cwriter != s->creadr) { > + process_cmdq(s); > + } > break; > case GITS_CREADR: > case GITS_TYPER: > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > index d6aaa94e4c..0932a30560 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) > FIELD(GITS_CBASER, INNERCACHE, 59, 3) > FIELD(GITS_CBASER, VALID, 63, 1) > > +FIELD(GITS_CREADR, STALLED, 0, 1) > +FIELD(GITS_CREADR, OFFSET, 5, 15) > + > FIELD(GITS_CWRITER, RETRY, 0, 1) > FIELD(GITS_CWRITER, OFFSET, 5, 15) > > @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) > #define L1TABLE_ENTRY_SIZE 8 > > #define GITS_CMDQ_ENTRY_SIZE 32 > +#define NUM_BYTES_IN_DW 8 > + > +#define CMD_MASK 0xff > + > +/* ITS Commands */ > +#define GITS_CMD_CLEAR 0x04 > +#define GITS_CMD_DISCARD 0x0F > +#define GITS_CMD_INT 0x03 > +#define GITS_CMD_MAPC 0x09 > +#define GITS_CMD_MAPD 0x08 > +#define GITS_CMD_MAPI 0x0B > +#define GITS_CMD_MAPTI 0x0A > +#define GITS_CMD_SYNC 0x05 > + > +/* MAPC command fields */ > +#define ICID_LENGTH 16 > +#define ICID_MASK ((1U << ICID_LENGTH) - 1) > +FIELD(MAPC, RDBASE, 16, 32) > + > +#define RDBASE_PROCNUM_LENGTH 16 > +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) - 1) > + > +#define DEVID_SHIFT 32 > +#define DEVID_LENGTH 32 > +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) > + > +/* MAPD command fields */ > +#define ITTADDR_LENGTH 44 > +#define ITTADDR_SHIFT 8 > +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) > +#define SIZE_MASK 0x1f > + > +#define VALID_SHIFT 63 > +#define VALID_MASK 1ULL > > /** > * Default features advertised by this version of ITS >
Hi Eric, Please find my responses inline (below):- On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote: > Hi Sashi, > > On 6/2/21 8:00 PM, Shashi Mallela wrote: > > Added functionality to trigger ITS command queue processing on > > write to CWRITE register and process each command queue entry to > > identify the command type and handle commands like MAPD,MAPC,SYNC. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/intc/arm_gicv3_its.c | 295 > > +++++++++++++++++++++++++++++++++++++++ > > hw/intc/gicv3_internal.h | 37 +++++ > > 2 files changed, 332 insertions(+) > > > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > > index af60f19c98..6551c577b3 100644 > > --- a/hw/intc/arm_gicv3_its.c > > +++ b/hw/intc/arm_gicv3_its.c > > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, > > uint32_t page_sz) > > return result; > > } > > > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, > > bool valid, > > + uint64_t rdbase) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint64_t value; > > + uint64_t l2t_addr; > > + bool valid_l2t; > > + uint32_t l2t_id; > > + uint32_t max_l2_entries; > > + uint64_t cte = 0; > > + MemTxResult res = MEMTX_OK; > > + > > + if (!s->ct.valid) { > Isn't it a guest log error case. Also you return MEMTX_OK in that > case. > Is that what you want? Yes,because the current implementation treats all command specific errors as "ignored" and moves onto next command in the queue.MEMTX return values are significant for dma read/write status and in case of error we stall the command processing > > + return res; > > + } > > + > > + if (valid) { > > + /* add mapping entry to collection table */ > > + cte = (valid & VALID_MASK) | > > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); > Do you really need to sanitize rdbase again? Not required,have rectified it. > > + } > > + > > + /* > > + * The specification defines the format of level 1 entries of > > a > > + * 2-level table, but the format of level 2 entries and the > > format > > + * of flat-mapped tables is IMPDEF. > > + */ > > + if (s->ct.indirect) { > > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > > + > > + value = address_space_ldq_le(as, > > + s->ct.base_addr + > > + (l2t_id * > > L1TABLE_ENTRY_SIZE), > > + MEMTXATTRS_UNSPECIFIED, > > &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if (valid_l2t) { > > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > > + > > + l2t_addr = value & ((1ULL << 51) - 1); > > + > > + address_space_stq_le(as, l2t_addr + > > + ((icid % max_l2_entries) * > > GITS_CTE_SIZE), > > + cte, MEMTXATTRS_UNSPECIFIED, > > &res); > > + } > > + } else { > > + /* Flat level table */ > > + address_space_stq_le(as, s->ct.base_addr + (icid * > > GITS_CTE_SIZE), > > + cte, MEMTXATTRS_UNSPECIFIED, &res); > > + } > > + return res; > > +} > > + > > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint16_t icid; > > + uint64_t rdbase; > > + bool valid; > > + MemTxResult res = MEMTX_OK; > > + uint64_t value; > > + > > + offset += NUM_BYTES_IN_DW; > > + offset += NUM_BYTES_IN_DW; > May be relevant to add some trace points for debuggability. Probably the trace functionality for ITS can be taken up as a seperate task/feature TODO. > > + > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + icid = value & ICID_MASK; > > + > > + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; > usually the mask is applied before the shift. Here we are extracting only 16 bit rdbase(processor number) value by masking with RDBASE_PROCNUM_MASK only after we have right shifted the rdbase offset from the 64 bit DW value. As an alternative,I could have used rdbase = (value & R_MAPC_RDBASE_MASK) to first extract the 32 bits rdbase value from DW and then later mask again with RDBASE_PROCNUM_MASK to narrow it down to 16 bit rdbase(processor number). > > + > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > use FIELD, see below > > + > > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3- > > >num_cpu)) { > you also need to check against ITS_CIDBITS limit? CIDBITS limits is being checked through the s->ct.max_collids member above > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "ITS MAPC: invalid collection table > > attributes " > > + "icid %d rdbase %lu\n", icid, rdbase); > > + /* > > + * in this implementation,in case of error > > + * we ignore this command and move onto the next > > + * command in the queue > spec says a command error occurs in that case. Yes,we chose to ignore the error'ed command and move onto the next one in the queue as per command error options in the spec > > + */ > > + } else { > > + res = update_cte(s, icid, valid, rdbase); > > + } > > + > > + return res; > > +} > > + > > +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, > > bool valid, > > + uint8_t size, uint64_t itt_addr) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint64_t value; > > + uint64_t l2t_addr; > > + bool valid_l2t; > > + uint32_t l2t_id; > > + uint32_t max_l2_entries; > > + uint64_t dte = 0; > > + MemTxResult res = MEMTX_OK; > > + > > + if (s->dt.valid) { > > + if (valid) { > > + /* add mapping entry to device table */ > > + dte = (valid & VALID_MASK) | > > + ((size & SIZE_MASK) << 1U) | > > + ((itt_addr & ITTADDR_MASK) << 6ULL); > > + } > > + } else { > > + return res; > > + } > > + > > + /* > > + * The specification defines the format of level 1 entries of > > a > > + * 2-level table, but the format of level 2 entries and the > > format > > + * of flat-mapped tables is IMPDEF. > > + */ > > + if (s->dt.indirect) { > > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > > + > > + value = address_space_ldq_le(as, > > + s->dt.base_addr + > > + (l2t_id * > > L1TABLE_ENTRY_SIZE), > > + MEMTXATTRS_UNSPECIFIED, > > &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if (valid_l2t) { > > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > > + > > + l2t_addr = value & ((1ULL << 51) - 1); > > + > > + address_space_stq_le(as, l2t_addr + > > + ((devid % max_l2_entries) * > > GITS_DTE_SIZE), > > + dte, MEMTXATTRS_UNSPECIFIED, > > &res); > > + } > > + } else { > > + /* Flat level table */ > > + address_space_stq_le(as, s->dt.base_addr + (devid * > > GITS_DTE_SIZE), > > + dte, MEMTXATTRS_UNSPECIFIED, &res); > > + } > > + return res; > > +} > > + > > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, > you do not seem to use the input value, remove it? yes we are using the input value,which is the 1st DW from the command to extract the deviceid (devid) field below > > + uint32_t offset) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint32_t devid; > > + uint8_t size; > > + uint64_t itt_addr; > > + bool valid; > > + MemTxResult res = MEMTX_OK; > > + > > + devid = (value >> DEVID_SHIFT) & DEVID_MASK; > > + > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + size = (value & SIZE_MASK); > > + > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; > this looks weird to me, usually we apply the mask first and then > shift. from the 64 bit DW,we right shift (by 8)to align the itt_addr at 0th position and extract 44 bits(0 to 43) using the mask > > + > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > use FIELD_EX64()? > > + > > + if ((devid > s->dt.max_devids) || > > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { > ITS_IDBITS? IDBITS is one of the fields in GITS_TYPER and the field naming is consistent with the spec definition > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "ITS MAPD: invalid device table attributes " > > + "devid %d or size %d\n", devid, size); > > + /* > > + * in this implementation, in case of error > > + * we ignore this command and move onto the next > > + * command in the queue > > + */ > > + } else { > > + res = update_dte(s, devid, valid, size, itt_addr); > > + } > > + > > + return res; > > +} > > + > > +/* > > + * Current implementation blocks until all > > + * commands are processed > > + */ > > +static void process_cmdq(GICv3ITSState *s) > > +{> + uint32_t wr_offset = 0; > > + uint32_t rd_offset = 0; > > + uint32_t cq_offset = 0; > > + uint64_t data; > > + AddressSpace *as = &s->gicv3->dma_as; > > + MemTxResult res = MEMTX_OK; > > + uint8_t cmd; > > + > > + if (!(s->ctlr & ITS_CTLR_ENABLED)) { > > + return; > > + } > > + > > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); > > + > > + if (wr_offset > s->cq.max_entries) { > Shouldn't this be checked on cwrite write instead? Yes we are checking within the cwriter write scope,just that the check is happening through this function (called during cwrite write) > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: invalid write offset " > > + "%d\n", __func__, wr_offset); > > + return; > > + } > > + > > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); > > + > > + while (wr_offset != rd_offset) { > > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); > > + data = address_space_ldq_le(as, s->cq.base_addr + > > cq_offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + cmd = (data & CMD_MASK); > > + > > + switch (cmd) { > > + case GITS_CMD_INT: > > + break; > > + case GITS_CMD_CLEAR: > > + break; > > + case GITS_CMD_SYNC: > > + /* > > + * Current implementation makes a blocking synchronous > > call > > + * for every command issued earlier, hence the > > internal state > > + * is already consistent by the time SYNC command is > > executed. > > + * Hence no further processing is required for SYNC > > command. > > + */ > > + break; > > + case GITS_CMD_MAPD: > > + res = process_mapd(s, data, cq_offset); > > + break; > > + case GITS_CMD_MAPC: > > + res = process_mapc(s, cq_offset); > > + break; > > + case GITS_CMD_MAPTI: > > + break; > > + case GITS_CMD_MAPI: > > + break; > > + case GITS_CMD_DISCARD: > > + break; > > + default: > > + break; > > + } > > + if (res == MEMTX_OK) { > > + rd_offset++; > > + rd_offset %= s->cq.max_entries; > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, > > rd_offset); > > + } else { > > + /* > > + * in this implementation,in case of dma read/write > > error > > + * we stall the command processing > > + */ > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, > > STALLED, 1); > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: %x cmd processing failed!!\n", > > __func__, cmd); > > + break; > > + } > > + } > > +} > > + > > static void extract_table_params(GICv3ITSState *s) > > { > > uint16_t num_pages = 0; > > @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, > > hwaddr offset, > > case GITS_CWRITER: > > s->cwriter = deposit64(s->cwriter, 0, 32, > > (value & > > ~R_GITS_CWRITER_RETRY_MASK)); > > + if (s->cwriter != s->creadr) { > > + process_cmdq(s); > I would expect process_cmdq() to be called as well on ITS enable Done > > + } > > break; > > case GITS_CWRITER + 4: > > s->cwriter = deposit64(s->cwriter, 32, 32, > > @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState > > *s, hwaddr offset, > > break; > > case GITS_CWRITER: > > s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; > > + if (s->cwriter != s->creadr) { > > + process_cmdq(s); > > + } > > break; > > case GITS_CREADR: > > case GITS_TYPER: > > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > > index d6aaa94e4c..0932a30560 100644 > > --- a/hw/intc/gicv3_internal.h > > +++ b/hw/intc/gicv3_internal.h > > @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) > > FIELD(GITS_CBASER, INNERCACHE, 59, 3) > > FIELD(GITS_CBASER, VALID, 63, 1) > > > > +FIELD(GITS_CREADR, STALLED, 0, 1) > > +FIELD(GITS_CREADR, OFFSET, 5, 15) > > + > > FIELD(GITS_CWRITER, RETRY, 0, 1) > > FIELD(GITS_CWRITER, OFFSET, 5, 15) > > > > @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) > > #define L1TABLE_ENTRY_SIZE 8 > > > > #define GITS_CMDQ_ENTRY_SIZE 32 > > +#define NUM_BYTES_IN_DW 8 > > + > > +#define CMD_MASK 0xff > > + > > +/* ITS Commands */ > > +#define GITS_CMD_CLEAR 0x04 > > +#define GITS_CMD_DISCARD 0x0F > > +#define GITS_CMD_INT 0x03 > > +#define GITS_CMD_MAPC 0x09 > > +#define GITS_CMD_MAPD 0x08 > > +#define GITS_CMD_MAPI 0x0B > > +#define GITS_CMD_MAPTI 0x0A > > +#define GITS_CMD_SYNC 0x05 > > + > > +/* MAPC command fields */ > > +#define ICID_LENGTH 16 > > +#define ICID_MASK ((1U << ICID_LENGTH) - 1) > can't you use FIELD') as well for the ICID? in addition to MAPC command ICID is a common field for MAPTI,MAPI commands as well,hence wanted to keep it common and seperate > > +FIELD(MAPC, RDBASE, 16, 32) > > + > > +#define RDBASE_PROCNUM_LENGTH 16 > > +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) > > - 1) > why do we have both the RDBASE FIELD def and above defs? RDBASE FIELD def points to the rdbase field within the MAPC command,while the RDBASE_PROCNUM_ defines are used to consider 16 bit PE number as the target destination instead of redistributor base address option. > > + > > +#define DEVID_SHIFT 32 > > +#define DEVID_LENGTH 32 > > +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) > we don't have any DEVID field in MAPC, I guess it belongs to MAPD? MAPC doesnt have a DEVID field ,but it is a common field in MAPD,INT,MAPI,MAPTI commands(at the same offset) > > + > > +/* MAPD command fields */ > > +#define ITTADDR_LENGTH 44 > > +#define ITTADDR_SHIFT 8 > > +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) > > +#define SIZE_MASK 0x1f > Can't you homogenize the definition, use field() and/or prefix with > the > cmd name when not common to severals cmds? Since ITTADDR_MASK is common to both MAPD command as well as device table entry field,didnt want to go with field() as the MAPD tag-name in device table entry would be insignificant > > > + > > +#define VALID_SHIFT 63 > > +#define VALID_MASK 1ULL > > > > /** > > * Default features advertised by this version of ITS > > > Thanks > > Eric >
On 6/16/21 11:02 PM, shashi.mallela@linaro.org wrote: > Hi Eric, > > Please find my responses inline (below):- > > On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote: >> Hi Sashi, >> >> On 6/2/21 8:00 PM, Shashi Mallela wrote: >>> Added functionality to trigger ITS command queue processing on >>> write to CWRITE register and process each command queue entry to >>> identify the command type and handle commands like MAPD,MAPC,SYNC. >>> >>> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> >>> --- >>> hw/intc/arm_gicv3_its.c | 295 >>> +++++++++++++++++++++++++++++++++++++++ >>> hw/intc/gicv3_internal.h | 37 +++++ >>> 2 files changed, 332 insertions(+) >>> >>> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c >>> index af60f19c98..6551c577b3 100644 >>> --- a/hw/intc/arm_gicv3_its.c >>> +++ b/hw/intc/arm_gicv3_its.c >>> @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, >>> uint32_t page_sz) >>> return result; >>> } >>> >>> +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, >>> bool valid, >>> + uint64_t rdbase) >>> +{ >>> + AddressSpace *as = &s->gicv3->dma_as; >>> + uint64_t value; >>> + uint64_t l2t_addr; >>> + bool valid_l2t; >>> + uint32_t l2t_id; >>> + uint32_t max_l2_entries; >>> + uint64_t cte = 0; >>> + MemTxResult res = MEMTX_OK; >>> + >>> + if (!s->ct.valid) { >> Isn't it a guest log error case. Also you return MEMTX_OK in that >> case. >> Is that what you want? > Yes,because the current implementation treats all command specific > errors as "ignored" and moves onto next command in the queue.MEMTX > return values are significant for dma read/write status and in case of > error we stall the command processing OK >>> + return res; >>> + } >>> + >>> + if (valid) { >>> + /* add mapping entry to collection table */ >>> + cte = (valid & VALID_MASK) | >>> + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); >> Do you really need to sanitize rdbase again? > Not required,have rectified it. >>> + } >>> + >>> + /* >>> + * The specification defines the format of level 1 entries of >>> a >>> + * 2-level table, but the format of level 2 entries and the >>> format >>> + * of flat-mapped tables is IMPDEF. >>> + */ >>> + if (s->ct.indirect) { >>> + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); >>> + >>> + value = address_space_ldq_le(as, >>> + s->ct.base_addr + >>> + (l2t_id * >>> L1TABLE_ENTRY_SIZE), >>> + MEMTXATTRS_UNSPECIFIED, >>> &res); >>> + >>> + if (res != MEMTX_OK) { >>> + return res; >>> + } >>> + >>> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; >>> + >>> + if (valid_l2t) { >>> + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; >>> + >>> + l2t_addr = value & ((1ULL << 51) - 1); >>> + >>> + address_space_stq_le(as, l2t_addr + >>> + ((icid % max_l2_entries) * >>> GITS_CTE_SIZE), >>> + cte, MEMTXATTRS_UNSPECIFIED, >>> &res); >>> + } >>> + } else { >>> + /* Flat level table */ >>> + address_space_stq_le(as, s->ct.base_addr + (icid * >>> GITS_CTE_SIZE), >>> + cte, MEMTXATTRS_UNSPECIFIED, &res); >>> + } >>> + return res; >>> +} >>> + >>> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) >>> +{ >>> + AddressSpace *as = &s->gicv3->dma_as; >>> + uint16_t icid; >>> + uint64_t rdbase; >>> + bool valid; >>> + MemTxResult res = MEMTX_OK; >>> + uint64_t value; >>> + >>> + offset += NUM_BYTES_IN_DW; >>> + offset += NUM_BYTES_IN_DW; >> May be relevant to add some trace points for debuggability. > Probably the trace functionality for ITS can be taken up as a seperate > task/feature TODO. Yes of course. It may just be useful for you as well to debug ;-) >>> + >>> + value = address_space_ldq_le(as, s->cq.base_addr + offset, >>> + MEMTXATTRS_UNSPECIFIED, &res); >>> + >>> + if (res != MEMTX_OK) { >>> + return res; >>> + } >>> + >>> + icid = value & ICID_MASK; >>> + >>> + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; >> usually the mask is applied before the shift. > Here we are extracting only 16 bit rdbase(processor number) value by > masking with RDBASE_PROCNUM_MASK only after we have right shifted the > rdbase offset from the 64 bit DW value. > As an alternative,I could have used rdbase = (value & > R_MAPC_RDBASE_MASK) to first extract the 32 bits rdbase value from DW > and then later mask again with RDBASE_PROCNUM_MASK to narrow it down to > 16 bit rdbase(processor number). My comment rather was about the fact that generally the mask applied to the shifted location and then you shift the masked field. I notived Peter also made this comment in 4/8 (FIELD macro). You tend to use the same pattern in different places in your series. >>> + >>> + valid = (value >> VALID_SHIFT) & VALID_MASK; >> use FIELD, see below >>> + >>> + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3- >>>> num_cpu)) { >> you also need to check against ITS_CIDBITS limit? > CIDBITS limits is being checked through the s->ct.max_collids member > above Ah OK >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "ITS MAPC: invalid collection table >>> attributes " >>> + "icid %d rdbase %lu\n", icid, rdbase); >>> + /* >>> + * in this implementation,in case of error >>> + * we ignore this command and move onto the next >>> + * command in the queue >> spec says a command error occurs in that case. > Yes,we chose to ignore the error'ed command and move onto the next one > in the queue as per command error options in the spec >>> + */ >>> + } else { >>> + res = update_cte(s, icid, valid, rdbase); >>> + } >>> + >>> + return res; >>> +} >>> + >>> +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, >>> bool valid, >>> + uint8_t size, uint64_t itt_addr) >>> +{ >>> + AddressSpace *as = &s->gicv3->dma_as; >>> + uint64_t value; >>> + uint64_t l2t_addr; >>> + bool valid_l2t; >>> + uint32_t l2t_id; >>> + uint32_t max_l2_entries; >>> + uint64_t dte = 0; >>> + MemTxResult res = MEMTX_OK; >>> + >>> + if (s->dt.valid) { >>> + if (valid) { >>> + /* add mapping entry to device table */ >>> + dte = (valid & VALID_MASK) | >>> + ((size & SIZE_MASK) << 1U) | >>> + ((itt_addr & ITTADDR_MASK) << 6ULL); >>> + } >>> + } else { >>> + return res; >>> + } >>> + >>> + /* >>> + * The specification defines the format of level 1 entries of >>> a >>> + * 2-level table, but the format of level 2 entries and the >>> format >>> + * of flat-mapped tables is IMPDEF. >>> + */ >>> + if (s->dt.indirect) { >>> + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); >>> + >>> + value = address_space_ldq_le(as, >>> + s->dt.base_addr + >>> + (l2t_id * >>> L1TABLE_ENTRY_SIZE), >>> + MEMTXATTRS_UNSPECIFIED, >>> &res); >>> + >>> + if (res != MEMTX_OK) { >>> + return res; >>> + } >>> + >>> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; >>> + >>> + if (valid_l2t) { >>> + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; >>> + >>> + l2t_addr = value & ((1ULL << 51) - 1); >>> + >>> + address_space_stq_le(as, l2t_addr + >>> + ((devid % max_l2_entries) * >>> GITS_DTE_SIZE), >>> + dte, MEMTXATTRS_UNSPECIFIED, >>> &res); >>> + } >>> + } else { >>> + /* Flat level table */ >>> + address_space_stq_le(as, s->dt.base_addr + (devid * >>> GITS_DTE_SIZE), >>> + dte, MEMTXATTRS_UNSPECIFIED, &res); >>> + } >>> + return res; >>> +} >>> + >>> +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, >> you do not seem to use the input value, remove it? > yes we are using the input value,which is the 1st DW from the command > to extract the deviceid (devid) field below Hum my mistake sorry. >>> + uint32_t offset) >>> +{ >>> + AddressSpace *as = &s->gicv3->dma_as; >>> + uint32_t devid; >>> + uint8_t size; >>> + uint64_t itt_addr; >>> + bool valid; >>> + MemTxResult res = MEMTX_OK; >>> + >>> + devid = (value >> DEVID_SHIFT) & DEVID_MASK; >>> + >>> + offset += NUM_BYTES_IN_DW; >>> + value = address_space_ldq_le(as, s->cq.base_addr + offset, >>> + MEMTXATTRS_UNSPECIFIED, &res); >>> + >>> + if (res != MEMTX_OK) { >>> + return res; >>> + } >>> + >>> + size = (value & SIZE_MASK); >>> + >>> + offset += NUM_BYTES_IN_DW; >>> + value = address_space_ldq_le(as, s->cq.base_addr + offset, >>> + MEMTXATTRS_UNSPECIFIED, &res); >>> + >>> + if (res != MEMTX_OK) { >>> + return res; >>> + } >>> + >>> + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; >> this looks weird to me, usually we apply the mask first and then >> shift. > from the 64 bit DW,we right shift (by 8)to align the itt_addr at 0th > position and extract 44 bits(0 to 43) using the mask ditto >>> + >>> + valid = (value >> VALID_SHIFT) & VALID_MASK; >> use FIELD_EX64()? >>> + >>> + if ((devid > s->dt.max_devids) || >>> + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { >> ITS_IDBITS? > IDBITS is one of the fields in GITS_TYPER and the field naming is > consistent with the spec definition >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "ITS MAPD: invalid device table attributes " >>> + "devid %d or size %d\n", devid, size); >>> + /* >>> + * in this implementation, in case of error >>> + * we ignore this command and move onto the next >>> + * command in the queue >>> + */ >>> + } else { >>> + res = update_dte(s, devid, valid, size, itt_addr); >>> + } >>> + >>> + return res; >>> +} >>> + >>> +/* >>> + * Current implementation blocks until all >>> + * commands are processed >>> + */ >>> +static void process_cmdq(GICv3ITSState *s) >>> +{> + uint32_t wr_offset = 0; >>> + uint32_t rd_offset = 0; >>> + uint32_t cq_offset = 0; >>> + uint64_t data; >>> + AddressSpace *as = &s->gicv3->dma_as; >>> + MemTxResult res = MEMTX_OK; >>> + uint8_t cmd; >>> + >>> + if (!(s->ctlr & ITS_CTLR_ENABLED)) { >>> + return; >>> + } >>> + >>> + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); >>> + >>> + if (wr_offset > s->cq.max_entries) { >> Shouldn't this be checked on cwrite write instead? > Yes we are checking within the cwriter write scope,just that the check > is happening through this function (called during cwrite write) >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: invalid write offset " >>> + "%d\n", __func__, wr_offset); >>> + return; >>> + } >>> + >>> + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); >>> + >>> + while (wr_offset != rd_offset) { >>> + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); >>> + data = address_space_ldq_le(as, s->cq.base_addr + >>> cq_offset, >>> + MEMTXATTRS_UNSPECIFIED, &res); >>> + cmd = (data & CMD_MASK); >>> + >>> + switch (cmd) { >>> + case GITS_CMD_INT: >>> + break; >>> + case GITS_CMD_CLEAR: >>> + break; >>> + case GITS_CMD_SYNC: >>> + /* >>> + * Current implementation makes a blocking synchronous >>> call >>> + * for every command issued earlier, hence the >>> internal state >>> + * is already consistent by the time SYNC command is >>> executed. >>> + * Hence no further processing is required for SYNC >>> command. >>> + */ >>> + break; >>> + case GITS_CMD_MAPD: >>> + res = process_mapd(s, data, cq_offset); >>> + break; >>> + case GITS_CMD_MAPC: >>> + res = process_mapc(s, cq_offset); >>> + break; >>> + case GITS_CMD_MAPTI: >>> + break; >>> + case GITS_CMD_MAPI: >>> + break; >>> + case GITS_CMD_DISCARD: >>> + break; >>> + default: >>> + break; >>> + } >>> + if (res == MEMTX_OK) { >>> + rd_offset++; >>> + rd_offset %= s->cq.max_entries; >>> + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, >>> rd_offset); >>> + } else { >>> + /* >>> + * in this implementation,in case of dma read/write >>> error >>> + * we stall the command processing >>> + */ >>> + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, >>> STALLED, 1); >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: %x cmd processing failed!!\n", >>> __func__, cmd); >>> + break; >>> + } >>> + } >>> +} >>> + >>> static void extract_table_params(GICv3ITSState *s) >>> { >>> uint16_t num_pages = 0; >>> @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, >>> hwaddr offset, >>> case GITS_CWRITER: >>> s->cwriter = deposit64(s->cwriter, 0, 32, >>> (value & >>> ~R_GITS_CWRITER_RETRY_MASK)); >>> + if (s->cwriter != s->creadr) { >>> + process_cmdq(s); >> I would expect process_cmdq() to be called as well on ITS enable > Done >>> + } >>> break; >>> case GITS_CWRITER + 4: >>> s->cwriter = deposit64(s->cwriter, 32, 32, >>> @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState >>> *s, hwaddr offset, >>> break; >>> case GITS_CWRITER: >>> s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; >>> + if (s->cwriter != s->creadr) { >>> + process_cmdq(s); >>> + } >>> break; >>> case GITS_CREADR: >>> case GITS_TYPER: >>> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h >>> index d6aaa94e4c..0932a30560 100644 >>> --- a/hw/intc/gicv3_internal.h >>> +++ b/hw/intc/gicv3_internal.h >>> @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) >>> FIELD(GITS_CBASER, INNERCACHE, 59, 3) >>> FIELD(GITS_CBASER, VALID, 63, 1) >>> >>> +FIELD(GITS_CREADR, STALLED, 0, 1) >>> +FIELD(GITS_CREADR, OFFSET, 5, 15) >>> + >>> FIELD(GITS_CWRITER, RETRY, 0, 1) >>> FIELD(GITS_CWRITER, OFFSET, 5, 15) >>> >>> @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) >>> #define L1TABLE_ENTRY_SIZE 8 >>> >>> #define GITS_CMDQ_ENTRY_SIZE 32 >>> +#define NUM_BYTES_IN_DW 8 >>> + >>> +#define CMD_MASK 0xff >>> + >>> +/* ITS Commands */ >>> +#define GITS_CMD_CLEAR 0x04 >>> +#define GITS_CMD_DISCARD 0x0F >>> +#define GITS_CMD_INT 0x03 >>> +#define GITS_CMD_MAPC 0x09 >>> +#define GITS_CMD_MAPD 0x08 >>> +#define GITS_CMD_MAPI 0x0B >>> +#define GITS_CMD_MAPTI 0x0A >>> +#define GITS_CMD_SYNC 0x05 >>> + >>> +/* MAPC command fields */ >>> +#define ICID_LENGTH 16 >>> +#define ICID_MASK ((1U << ICID_LENGTH) - 1) >> can't you use FIELD') as well for the ICID? > in addition to MAPC command ICID is a common field for MAPTI,MAPI > commands as well,hence wanted to keep it common and seperate >>> +FIELD(MAPC, RDBASE, 16, 32) >>> + >>> +#define RDBASE_PROCNUM_LENGTH 16 >>> +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) >>> - 1) >> why do we have both the RDBASE FIELD def and above defs? > RDBASE FIELD def points to the rdbase field within the MAPC > command,while the RDBASE_PROCNUM_ defines are used to consider 16 bit > PE number as the target destination instead of redistributor base > address option. >>> + >>> +#define DEVID_SHIFT 32 >>> +#define DEVID_LENGTH 32 >>> +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) >> we don't have any DEVID field in MAPC, I guess it belongs to MAPD? > MAPC doesnt have a DEVID field ,but it is a common field in > MAPD,INT,MAPI,MAPTI commands(at the same offset) Yes but above there is a command saying "MAPC command fields */ >>> + >>> +/* MAPD command fields */ >>> +#define ITTADDR_LENGTH 44 >>> +#define ITTADDR_SHIFT 8 >>> +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) >>> +#define SIZE_MASK 0x1f >> Can't you homogenize the definition, use field() and/or prefix with >> the >> cmd name when not common to severals cmds? > Since ITTADDR_MASK is common to both MAPD command as well as device > table entry field,didnt want to go with field() as the MAPD tag-name in > device table entry would be insignificant >> >>> + >>> +#define VALID_SHIFT 63 >>> +#define VALID_MASK 1ULL >>> >>> /** >>> * Default features advertised by this version of ITS >>> >> Thanks >> >> Eric >> > > > > Thanks Eric
Hi Eric, Had missed this comment earlier.Please find my response (inline)below:- On Sun, 2021-06-13 at 16:39 +0200, Eric Auger wrote: > Hi, > > On 6/2/21 8:00 PM, Shashi Mallela wrote: > > Added functionality to trigger ITS command queue processing on > > write to CWRITE register and process each command queue entry to > > identify the command type and handle commands like MAPD,MAPC,SYNC. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/intc/arm_gicv3_its.c | 295 > > +++++++++++++++++++++++++++++++++++++++ > > hw/intc/gicv3_internal.h | 37 +++++ > > 2 files changed, 332 insertions(+) > > > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > > index af60f19c98..6551c577b3 100644 > > --- a/hw/intc/arm_gicv3_its.c > > +++ b/hw/intc/arm_gicv3_its.c > > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, > > uint32_t page_sz) > > return result; > > } > > > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, > > bool valid, > > + uint64_t rdbase) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint64_t value; > > + uint64_t l2t_addr; > > + bool valid_l2t; > > + uint32_t l2t_id; > > + uint32_t max_l2_entries; > > + uint64_t cte = 0; > > + MemTxResult res = MEMTX_OK; > > + > > + if (!s->ct.valid) { > > + return res; > > + } > > + > > + if (valid) { > > + /* add mapping entry to collection table */ > > + cte = (valid & VALID_MASK) | > > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); > > + } > > + > > + /* > > + * The specification defines the format of level 1 entries of > > a > > + * 2-level table, but the format of level 2 entries and the > > format > > + * of flat-mapped tables is IMPDEF. > > + */ > > + if (s->ct.indirect) { > > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > > + > > + value = address_space_ldq_le(as, > > + s->ct.base_addr + > > + (l2t_id * > > L1TABLE_ENTRY_SIZE), > > + MEMTXATTRS_UNSPECIFIED, > > &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if (valid_l2t) { > > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > > + > > + l2t_addr = value & ((1ULL << 51) - 1); > > + > > + address_space_stq_le(as, l2t_addr + > > + ((icid % max_l2_entries) * > > GITS_CTE_SIZE), > > + cte, MEMTXATTRS_UNSPECIFIED, > > &res); > > + } > > + } else { > > + /* Flat level table */ > > + address_space_stq_le(as, s->ct.base_addr + (icid * > > GITS_CTE_SIZE), > > + cte, MEMTXATTRS_UNSPECIFIED, &res); > > + } > > + return res; > > +} > > Looking at your DevTableDesc and CollTableDesc types again, they are > basically the same except max_devids/max_collids. You may use a > single > one and it may be possible to define helpers to access an entry in > the > DT or CT. will replace DevTableDesc/CollTableDesc types with a common TableDesc type and introduce a new union member to hold one of max_devids/max_collids to be referenced by all relevant functions. > > update_cte/update_dte looks quite similar if you compute the cte and > dte > externally and pass a pointer to the associated TableDesc? update_cte/update_cte will (continue) to reference their respective tables via existing Gicv3ItsState type but through the newly defined common TableDesc type. Hope this helps. > > Thanks > > Eric > > > > + > > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint16_t icid; > > + uint64_t rdbase; > > + bool valid; > > + MemTxResult res = MEMTX_OK; > > + uint64_t value; > > + > > + offset += NUM_BYTES_IN_DW; > > + offset += NUM_BYTES_IN_DW; > > + > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + icid = value & ICID_MASK; > > + > > + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; > > + > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3- > > >num_cpu)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "ITS MAPC: invalid collection table > > attributes " > > + "icid %d rdbase %lu\n", icid, rdbase); > > + /* > > + * in this implementation,in case of error > > + * we ignore this command and move onto the next > > + * command in the queue > > + */ > > + } else { > > + res = update_cte(s, icid, valid, rdbase); > > + } > > + > > + return res; > > +} > > + > > +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, > > bool valid, > > + uint8_t size, uint64_t itt_addr) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint64_t value; > > + uint64_t l2t_addr; > > + bool valid_l2t; > > + uint32_t l2t_id; > > + uint32_t max_l2_entries; > > + uint64_t dte = 0; > > + MemTxResult res = MEMTX_OK; > > + > > + if (s->dt.valid) { > > + if (valid) { > > + /* add mapping entry to device table */ > > + dte = (valid & VALID_MASK) | > > + ((size & SIZE_MASK) << 1U) | > > + ((itt_addr & ITTADDR_MASK) << 6ULL); > > + } > > + } else { > > + return res; > > + } > > + > > + /* > > + * The specification defines the format of level 1 entries of > > a > > + * 2-level table, but the format of level 2 entries and the > > format > > + * of flat-mapped tables is IMPDEF. > > + */ > > + if (s->dt.indirect) { > > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > > + > > + value = address_space_ldq_le(as, > > + s->dt.base_addr + > > + (l2t_id * > > L1TABLE_ENTRY_SIZE), > > + MEMTXATTRS_UNSPECIFIED, > > &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if (valid_l2t) { > > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > > + > > + l2t_addr = value & ((1ULL << 51) - 1); > > + > > + address_space_stq_le(as, l2t_addr + > > + ((devid % max_l2_entries) * > > GITS_DTE_SIZE), > > + dte, MEMTXATTRS_UNSPECIFIED, > > &res); > > + } > > + } else { > > + /* Flat level table */ > > + address_space_stq_le(as, s->dt.base_addr + (devid * > > GITS_DTE_SIZE), > > + dte, MEMTXATTRS_UNSPECIFIED, &res); > > + } > > + return res; > > +} > > + > > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, > > + uint32_t offset) > > +{ > > + AddressSpace *as = &s->gicv3->dma_as; > > + uint32_t devid; > > + uint8_t size; > > + uint64_t itt_addr; > > + bool valid; > > + MemTxResult res = MEMTX_OK; > > + > > + devid = (value >> DEVID_SHIFT) & DEVID_MASK; > > + > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + size = (value & SIZE_MASK); > > + > > + offset += NUM_BYTES_IN_DW; > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + > > + if (res != MEMTX_OK) { > > + return res; > > + } > > + > > + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; > > + > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > > + > > + if ((devid > s->dt.max_devids) || > > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "ITS MAPD: invalid device table attributes " > > + "devid %d or size %d\n", devid, size); > > + /* > > + * in this implementation, in case of error > > + * we ignore this command and move onto the next > > + * command in the queue > > + */ > > + } else { > > + res = update_dte(s, devid, valid, size, itt_addr); > > + } > > + > > + return res; > > +} > > + > > +/* > > + * Current implementation blocks until all > > + * commands are processed > > + */ > > +static void process_cmdq(GICv3ITSState *s) > > +{ > > + uint32_t wr_offset = 0; > > + uint32_t rd_offset = 0; > > + uint32_t cq_offset = 0; > > + uint64_t data; > > + AddressSpace *as = &s->gicv3->dma_as; > > + MemTxResult res = MEMTX_OK; > > + uint8_t cmd; > > + > > + if (!(s->ctlr & ITS_CTLR_ENABLED)) { > > + return; > > + } > > + > > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); > > + > > + if (wr_offset > s->cq.max_entries) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: invalid write offset " > > + "%d\n", __func__, wr_offset); > > + return; > > + } > > + > > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); > > + > > + while (wr_offset != rd_offset) { > > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); > > + data = address_space_ldq_le(as, s->cq.base_addr + > > cq_offset, > > + MEMTXATTRS_UNSPECIFIED, &res); > > + cmd = (data & CMD_MASK); > > + > > + switch (cmd) { > > + case GITS_CMD_INT: > > + break; > > + case GITS_CMD_CLEAR: > > + break; > > + case GITS_CMD_SYNC: > > + /* > > + * Current implementation makes a blocking synchronous > > call > > + * for every command issued earlier, hence the > > internal state > > + * is already consistent by the time SYNC command is > > executed. > > + * Hence no further processing is required for SYNC > > command. > > + */ > > + break; > > + case GITS_CMD_MAPD: > > + res = process_mapd(s, data, cq_offset); > > + break; > > + case GITS_CMD_MAPC: > > + res = process_mapc(s, cq_offset); > > + break; > > + case GITS_CMD_MAPTI: > > + break; > > + case GITS_CMD_MAPI: > > + break; > > + case GITS_CMD_DISCARD: > > + break; > > + default: > > + break; > > + } > > + if (res == MEMTX_OK) { > > + rd_offset++; > > + rd_offset %= s->cq.max_entries; > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, > > rd_offset); > > + } else { > > + /* > > + * in this implementation,in case of dma read/write > > error > > + * we stall the command processing > > + */ > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, > > STALLED, 1); > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: %x cmd processing failed!!\n", > > __func__, cmd); > > + break; > > + } > > + } > > +} > > + > > static void extract_table_params(GICv3ITSState *s) > > { > > uint16_t num_pages = 0; > > @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, > > hwaddr offset, > > case GITS_CWRITER: > > s->cwriter = deposit64(s->cwriter, 0, 32, > > (value & > > ~R_GITS_CWRITER_RETRY_MASK)); > > + if (s->cwriter != s->creadr) { > > + process_cmdq(s); > > + } > > break; > > case GITS_CWRITER + 4: > > s->cwriter = deposit64(s->cwriter, 32, 32, > > @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState > > *s, hwaddr offset, > > break; > > case GITS_CWRITER: > > s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; > > + if (s->cwriter != s->creadr) { > > + process_cmdq(s); > > + } > > break; > > case GITS_CREADR: > > case GITS_TYPER: > > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > > index d6aaa94e4c..0932a30560 100644 > > --- a/hw/intc/gicv3_internal.h > > +++ b/hw/intc/gicv3_internal.h > > @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) > > FIELD(GITS_CBASER, INNERCACHE, 59, 3) > > FIELD(GITS_CBASER, VALID, 63, 1) > > > > +FIELD(GITS_CREADR, STALLED, 0, 1) > > +FIELD(GITS_CREADR, OFFSET, 5, 15) > > + > > FIELD(GITS_CWRITER, RETRY, 0, 1) > > FIELD(GITS_CWRITER, OFFSET, 5, 15) > > > > @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) > > #define L1TABLE_ENTRY_SIZE 8 > > > > #define GITS_CMDQ_ENTRY_SIZE 32 > > +#define NUM_BYTES_IN_DW 8 > > + > > +#define CMD_MASK 0xff > > + > > +/* ITS Commands */ > > +#define GITS_CMD_CLEAR 0x04 > > +#define GITS_CMD_DISCARD 0x0F > > +#define GITS_CMD_INT 0x03 > > +#define GITS_CMD_MAPC 0x09 > > +#define GITS_CMD_MAPD 0x08 > > +#define GITS_CMD_MAPI 0x0B > > +#define GITS_CMD_MAPTI 0x0A > > +#define GITS_CMD_SYNC 0x05 > > + > > +/* MAPC command fields */ > > +#define ICID_LENGTH 16 > > +#define ICID_MASK ((1U << ICID_LENGTH) - 1) > > +FIELD(MAPC, RDBASE, 16, 32) > > + > > +#define RDBASE_PROCNUM_LENGTH 16 > > +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) > > - 1) > > + > > +#define DEVID_SHIFT 32 > > +#define DEVID_LENGTH 32 > > +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) > > + > > +/* MAPD command fields */ > > +#define ITTADDR_LENGTH 44 > > +#define ITTADDR_SHIFT 8 > > +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) > > +#define SIZE_MASK 0x1f > > + > > +#define VALID_SHIFT 63 > > +#define VALID_MASK 1ULL > > > > /** > > * Default features advertised by this version of ITS > >
Hi Eric, Please find my responses to your latest comments (taken care of) inline:- On Mon, 2021-06-21 at 12:03 +0200, Eric Auger wrote: > > On 6/16/21 11:02 PM, shashi.mallela@linaro.org wrote: > > Hi Eric, > > > > Please find my responses inline (below):- > > > > On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote: > > > Hi Sashi, > > > > > > On 6/2/21 8:00 PM, Shashi Mallela wrote: > > > > Added functionality to trigger ITS command queue processing on > > > > write to CWRITE register and process each command queue entry > > > > to > > > > identify the command type and handle commands like > > > > MAPD,MAPC,SYNC. > > > > > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > > > --- > > > > hw/intc/arm_gicv3_its.c | 295 > > > > +++++++++++++++++++++++++++++++++++++++ > > > > hw/intc/gicv3_internal.h | 37 +++++ > > > > 2 files changed, 332 insertions(+) > > > > > > > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > > > > index af60f19c98..6551c577b3 100644 > > > > --- a/hw/intc/arm_gicv3_its.c > > > > +++ b/hw/intc/arm_gicv3_its.c > > > > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t > > > > value, > > > > uint32_t page_sz) > > > > return result; > > > > } > > > > > > > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, > > > > bool valid, > > > > + uint64_t rdbase) > > > > +{ > > > > + AddressSpace *as = &s->gicv3->dma_as; > > > > + uint64_t value; > > > > + uint64_t l2t_addr; > > > > + bool valid_l2t; > > > > + uint32_t l2t_id; > > > > + uint32_t max_l2_entries; > > > > + uint64_t cte = 0; > > > > + MemTxResult res = MEMTX_OK; > > > > + > > > > + if (!s->ct.valid) { > > > Isn't it a guest log error case. Also you return MEMTX_OK in that > > > case. > > > Is that what you want? > > Yes,because the current implementation treats all command specific > > errors as "ignored" and moves onto next command in the queue.MEMTX > > return values are significant for dma read/write status and in case > > of > > error we stall the command processing > OK > > > > + return res; > > > > + } > > > > + > > > > + if (valid) { > > > > + /* add mapping entry to collection table */ > > > > + cte = (valid & VALID_MASK) | > > > > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); > > > Do you really need to sanitize rdbase again? > > Not required,have rectified it. > > > > + } > > > > + > > > > + /* > > > > + * The specification defines the format of level 1 entries > > > > of > > > > a > > > > + * 2-level table, but the format of level 2 entries and > > > > the > > > > format > > > > + * of flat-mapped tables is IMPDEF. > > > > + */ > > > > + if (s->ct.indirect) { > > > > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); > > > > + > > > > + value = address_space_ldq_le(as, > > > > + s->ct.base_addr + > > > > + (l2t_id * > > > > L1TABLE_ENTRY_SIZE), > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + > > > > + if (res != MEMTX_OK) { > > > > + return res; > > > > + } > > > > + > > > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > > > + > > > > + if (valid_l2t) { > > > > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; > > > > + > > > > + l2t_addr = value & ((1ULL << 51) - 1); > > > > + > > > > + address_space_stq_le(as, l2t_addr + > > > > + ((icid % max_l2_entries) * > > > > GITS_CTE_SIZE), > > > > + cte, MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + } > > > > + } else { > > > > + /* Flat level table */ > > > > + address_space_stq_le(as, s->ct.base_addr + (icid * > > > > GITS_CTE_SIZE), > > > > + cte, MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + } > > > > + return res; > > > > +} > > > > + > > > > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t > > > > offset) > > > > +{ > > > > + AddressSpace *as = &s->gicv3->dma_as; > > > > + uint16_t icid; > > > > + uint64_t rdbase; > > > > + bool valid; > > > > + MemTxResult res = MEMTX_OK; > > > > + uint64_t value; > > > > + > > > > + offset += NUM_BYTES_IN_DW; > > > > + offset += NUM_BYTES_IN_DW; > > > May be relevant to add some trace points for debuggability. > > Probably the trace functionality for ITS can be taken up as a > > seperate > > task/feature TODO. > Yes of course. It may just be useful for you as well to debug ;-) > > > > + > > > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + > > > > + if (res != MEMTX_OK) { > > > > + return res; > > > > + } > > > > + > > > > + icid = value & ICID_MASK; > > > > + > > > > + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & > > > > RDBASE_PROCNUM_MASK; > > > usually the mask is applied before the shift. > > Here we are extracting only 16 bit rdbase(processor number) value > > by > > masking with RDBASE_PROCNUM_MASK only after we have right shifted > > the > > rdbase offset from the 64 bit DW value. > > As an alternative,I could have used rdbase = (value & > > R_MAPC_RDBASE_MASK) to first extract the 32 bits rdbase value from > > DW > > and then later mask again with RDBASE_PROCNUM_MASK to narrow it > > down to > > 16 bit rdbase(processor number). > My comment rather was about the fact that generally the mask applied > to > the shifted location and then you shift the masked field. I notived > Peter also made this comment in 4/8 (FIELD macro). You tend to use > the > same pattern in different places in your series. Accepted and have made changes across all relevant sections in all patch series > > > > + > > > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > > > use FIELD, see below > > > > + > > > > + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3- > > > > > num_cpu)) { > > > you also need to check against ITS_CIDBITS limit? > > CIDBITS limits is being checked through the s->ct.max_collids > > member > > above > Ah OK > > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > > + "ITS MAPC: invalid collection table > > > > attributes " > > > > + "icid %d rdbase %lu\n", icid, rdbase); > > > > + /* > > > > + * in this implementation,in case of error > > > > + * we ignore this command and move onto the next > > > > + * command in the queue > > > spec says a command error occurs in that case. > > Yes,we chose to ignore the error'ed command and move onto the next > > one > > in the queue as per command error options in the spec > > > > + */ > > > > + } else { > > > > + res = update_cte(s, icid, valid, rdbase); > > > > + } > > > > + > > > > + return res; > > > > +} > > > > + > > > > +static MemTxResult update_dte(GICv3ITSState *s, uint32_t > > > > devid, > > > > bool valid, > > > > + uint8_t size, uint64_t itt_addr) > > > > +{ > > > > + AddressSpace *as = &s->gicv3->dma_as; > > > > + uint64_t value; > > > > + uint64_t l2t_addr; > > > > + bool valid_l2t; > > > > + uint32_t l2t_id; > > > > + uint32_t max_l2_entries; > > > > + uint64_t dte = 0; > > > > + MemTxResult res = MEMTX_OK; > > > > + > > > > + if (s->dt.valid) { > > > > + if (valid) { > > > > + /* add mapping entry to device table */ > > > > + dte = (valid & VALID_MASK) | > > > > + ((size & SIZE_MASK) << 1U) | > > > > + ((itt_addr & ITTADDR_MASK) << 6ULL); > > > > + } > > > > + } else { > > > > + return res; > > > > + } > > > > + > > > > + /* > > > > + * The specification defines the format of level 1 entries > > > > of > > > > a > > > > + * 2-level table, but the format of level 2 entries and > > > > the > > > > format > > > > + * of flat-mapped tables is IMPDEF. > > > > + */ > > > > + if (s->dt.indirect) { > > > > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); > > > > + > > > > + value = address_space_ldq_le(as, > > > > + s->dt.base_addr + > > > > + (l2t_id * > > > > L1TABLE_ENTRY_SIZE), > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + > > > > + if (res != MEMTX_OK) { > > > > + return res; > > > > + } > > > > + > > > > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; > > > > + > > > > + if (valid_l2t) { > > > > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; > > > > + > > > > + l2t_addr = value & ((1ULL << 51) - 1); > > > > + > > > > + address_space_stq_le(as, l2t_addr + > > > > + ((devid % max_l2_entries) * > > > > GITS_DTE_SIZE), > > > > + dte, MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + } > > > > + } else { > > > > + /* Flat level table */ > > > > + address_space_stq_le(as, s->dt.base_addr + (devid * > > > > GITS_DTE_SIZE), > > > > + dte, MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + } > > > > + return res; > > > > +} > > > > + > > > > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t > > > > value, > > > you do not seem to use the input value, remove it? > > yes we are using the input value,which is the 1st DW from the > > command > > to extract the deviceid (devid) field below > Hum my mistake sorry. > > > > + uint32_t offset) > > > > +{ > > > > + AddressSpace *as = &s->gicv3->dma_as; > > > > + uint32_t devid; > > > > + uint8_t size; > > > > + uint64_t itt_addr; > > > > + bool valid; > > > > + MemTxResult res = MEMTX_OK; > > > > + > > > > + devid = (value >> DEVID_SHIFT) & DEVID_MASK; > > > > + > > > > + offset += NUM_BYTES_IN_DW; > > > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + > > > > + if (res != MEMTX_OK) { > > > > + return res; > > > > + } > > > > + > > > > + size = (value & SIZE_MASK); > > > > + > > > > + offset += NUM_BYTES_IN_DW; > > > > + value = address_space_ldq_le(as, s->cq.base_addr + offset, > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + > > > > + if (res != MEMTX_OK) { > > > > + return res; > > > > + } > > > > + > > > > + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; > > > this looks weird to me, usually we apply the mask first and then > > > shift. > > from the 64 bit DW,we right shift (by 8)to align the itt_addr at > > 0th > > position and extract 44 bits(0 to 43) using the mask > ditto Accepted and taken care of as indicated in the previous response > > > > + > > > > + valid = (value >> VALID_SHIFT) & VALID_MASK; > > > use FIELD_EX64()? > > > > + > > > > + if ((devid > s->dt.max_devids) || > > > > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { > > > ITS_IDBITS? > > IDBITS is one of the fields in GITS_TYPER and the field naming is > > consistent with the spec definition > > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > > + "ITS MAPD: invalid device table > > > > attributes " > > > > + "devid %d or size %d\n", devid, size); > > > > + /* > > > > + * in this implementation, in case of error > > > > + * we ignore this command and move onto the next > > > > + * command in the queue > > > > + */ > > > > + } else { > > > > + res = update_dte(s, devid, valid, size, itt_addr); > > > > + } > > > > + > > > > + return res; > > > > +} > > > > + > > > > +/* > > > > + * Current implementation blocks until all > > > > + * commands are processed > > > > + */ > > > > +static void process_cmdq(GICv3ITSState *s) > > > > +{> + uint32_t wr_offset = 0; > > > > + uint32_t rd_offset = 0; > > > > + uint32_t cq_offset = 0; > > > > + uint64_t data; > > > > + AddressSpace *as = &s->gicv3->dma_as; > > > > + MemTxResult res = MEMTX_OK; > > > > + uint8_t cmd; > > > > + > > > > + if (!(s->ctlr & ITS_CTLR_ENABLED)) { > > > > + return; > > > > + } > > > > + > > > > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); > > > > + > > > > + if (wr_offset > s->cq.max_entries) { > > > Shouldn't this be checked on cwrite write instead? > > Yes we are checking within the cwriter write scope,just that the > > check > > is happening through this function (called during cwrite write) > > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > > + "%s: invalid write offset " > > > > + "%d\n", __func__, wr_offset); > > > > + return; > > > > + } > > > > + > > > > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); > > > > + > > > > + while (wr_offset != rd_offset) { > > > > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); > > > > + data = address_space_ldq_le(as, s->cq.base_addr + > > > > cq_offset, > > > > + MEMTXATTRS_UNSPECIFIED, > > > > &res); > > > > + cmd = (data & CMD_MASK); > > > > + > > > > + switch (cmd) { > > > > + case GITS_CMD_INT: > > > > + break; > > > > + case GITS_CMD_CLEAR: > > > > + break; > > > > + case GITS_CMD_SYNC: > > > > + /* > > > > + * Current implementation makes a blocking > > > > synchronous > > > > call > > > > + * for every command issued earlier, hence the > > > > internal state > > > > + * is already consistent by the time SYNC command > > > > is > > > > executed. > > > > + * Hence no further processing is required for > > > > SYNC > > > > command. > > > > + */ > > > > + break; > > > > + case GITS_CMD_MAPD: > > > > + res = process_mapd(s, data, cq_offset); > > > > + break; > > > > + case GITS_CMD_MAPC: > > > > + res = process_mapc(s, cq_offset); > > > > + break; > > > > + case GITS_CMD_MAPTI: > > > > + break; > > > > + case GITS_CMD_MAPI: > > > > + break; > > > > + case GITS_CMD_DISCARD: > > > > + break; > > > > + default: > > > > + break; > > > > + } > > > > + if (res == MEMTX_OK) { > > > > + rd_offset++; > > > > + rd_offset %= s->cq.max_entries; > > > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, > > > > OFFSET, > > > > rd_offset); > > > > + } else { > > > > + /* > > > > + * in this implementation,in case of dma > > > > read/write > > > > error > > > > + * we stall the command processing > > > > + */ > > > > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, > > > > STALLED, 1); > > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > > + "%s: %x cmd processing failed!!\n", > > > > __func__, cmd); > > > > + break; > > > > + } > > > > + } > > > > +} > > > > + > > > > static void extract_table_params(GICv3ITSState *s) > > > > { > > > > uint16_t num_pages = 0; > > > > @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState > > > > *s, > > > > hwaddr offset, > > > > case GITS_CWRITER: > > > > s->cwriter = deposit64(s->cwriter, 0, 32, > > > > (value & > > > > ~R_GITS_CWRITER_RETRY_MASK)); > > > > + if (s->cwriter != s->creadr) { > > > > + process_cmdq(s); > > > I would expect process_cmdq() to be called as well on ITS enable > > Done > > > > + } > > > > break; > > > > case GITS_CWRITER + 4: > > > > s->cwriter = deposit64(s->cwriter, 32, 32, > > > > @@ -346,6 +638,9 @@ static MemTxResult > > > > its_writell(GICv3ITSState > > > > *s, hwaddr offset, > > > > break; > > > > case GITS_CWRITER: > > > > s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; > > > > + if (s->cwriter != s->creadr) { > > > > + process_cmdq(s); > > > > + } > > > > break; > > > > case GITS_CREADR: > > > > case GITS_TYPER: > > > > diff --git a/hw/intc/gicv3_internal.h > > > > b/hw/intc/gicv3_internal.h > > > > index d6aaa94e4c..0932a30560 100644 > > > > --- a/hw/intc/gicv3_internal.h > > > > +++ b/hw/intc/gicv3_internal.h > > > > @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) > > > > FIELD(GITS_CBASER, INNERCACHE, 59, 3) > > > > FIELD(GITS_CBASER, VALID, 63, 1) > > > > > > > > +FIELD(GITS_CREADR, STALLED, 0, 1) > > > > +FIELD(GITS_CREADR, OFFSET, 5, 15) > > > > + > > > > FIELD(GITS_CWRITER, RETRY, 0, 1) > > > > FIELD(GITS_CWRITER, OFFSET, 5, 15) > > > > > > > > @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) > > > > #define L1TABLE_ENTRY_SIZE 8 > > > > > > > > #define GITS_CMDQ_ENTRY_SIZE 32 > > > > +#define NUM_BYTES_IN_DW 8 > > > > + > > > > +#define CMD_MASK 0xff > > > > + > > > > +/* ITS Commands */ > > > > +#define GITS_CMD_CLEAR 0x04 > > > > +#define GITS_CMD_DISCARD 0x0F > > > > +#define GITS_CMD_INT 0x03 > > > > +#define GITS_CMD_MAPC 0x09 > > > > +#define GITS_CMD_MAPD 0x08 > > > > +#define GITS_CMD_MAPI 0x0B > > > > +#define GITS_CMD_MAPTI 0x0A > > > > +#define GITS_CMD_SYNC 0x05 > > > > + > > > > +/* MAPC command fields */ > > > > +#define ICID_LENGTH 16 > > > > +#define ICID_MASK ((1U << ICID_LENGTH) - 1) > > > can't you use FIELD') as well for the ICID? > > in addition to MAPC command ICID is a common field for MAPTI,MAPI > > commands as well,hence wanted to keep it common and seperate > > > > +FIELD(MAPC, RDBASE, 16, 32) > > > > + > > > > +#define RDBASE_PROCNUM_LENGTH 16 > > > > +#define RDBASE_PROCNUM_MASK ((1ULL << > > > > RDBASE_PROCNUM_LENGTH) > > > > - 1) > > > why do we have both the RDBASE FIELD def and above defs? > > RDBASE FIELD def points to the rdbase field within the MAPC > > command,while the RDBASE_PROCNUM_ defines are used to consider 16 > > bit > > PE number as the target destination instead of redistributor base > > address option. > > > > + > > > > +#define DEVID_SHIFT 32 > > > > +#define DEVID_LENGTH 32 > > > > +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) > > > we don't have any DEVID field in MAPC, I guess it belongs to > > > MAPD? > > MAPC doesnt have a DEVID field ,but it is a common field in > > MAPD,INT,MAPI,MAPTI commands(at the same offset) > Yes but above there is a command saying "MAPC command fields */ Have moved the DEVID defs to common section to avoid the confusion with MAPC related define > > > > + > > > > +/* MAPD command fields */ > > > > +#define ITTADDR_LENGTH 44 > > > > +#define ITTADDR_SHIFT 8 > > > > +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - > > > > 1) > > > > +#define SIZE_MASK 0x1f > > > Can't you homogenize the definition, use field() and/or prefix > > > with > > > the > > > cmd name when not common to severals cmds? > > Since ITTADDR_MASK is common to both MAPD command as well as device > > table entry field,didnt want to go with field() as the MAPD tag- > > name in > > device table entry would be insignificant > > > > + > > > > +#define VALID_SHIFT 63 > > > > +#define VALID_MASK 1ULL > > > > > > > > /** > > > > * Default features advertised by this version of ITS > > > > > > > Thanks > > > > > > Eric > > > > > > > > > > Thanks > > Eric >
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index af60f19c98..6551c577b3 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) return result; } +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid, + uint64_t rdbase) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t value; + uint64_t l2t_addr; + bool valid_l2t; + uint32_t l2t_id; + uint32_t max_l2_entries; + uint64_t cte = 0; + MemTxResult res = MEMTX_OK; + + if (!s->ct.valid) { + return res; + } + + if (valid) { + /* add mapping entry to collection table */ + cte = (valid & VALID_MASK) | + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL); + } + + /* + * The specification defines the format of level 1 entries of a + * 2-level table, but the format of level 2 entries and the format + * of flat-mapped tables is IMPDEF. + */ + if (s->ct.indirect) { + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); + + value = address_space_ldq_le(as, + s->ct.base_addr + + (l2t_id * L1TABLE_ENTRY_SIZE), + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; + + if (valid_l2t) { + max_l2_entries = s->ct.page_sz / s->ct.entry_sz; + + l2t_addr = value & ((1ULL << 51) - 1); + + address_space_stq_le(as, l2t_addr + + ((icid % max_l2_entries) * GITS_CTE_SIZE), + cte, MEMTXATTRS_UNSPECIFIED, &res); + } + } else { + /* Flat level table */ + address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE), + cte, MEMTXATTRS_UNSPECIFIED, &res); + } + return res; +} + +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint16_t icid; + uint64_t rdbase; + bool valid; + MemTxResult res = MEMTX_OK; + uint64_t value; + + offset += NUM_BYTES_IN_DW; + offset += NUM_BYTES_IN_DW; + + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + icid = value & ICID_MASK; + + rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK; + + valid = (value >> VALID_SHIFT) & VALID_MASK; + + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) { + qemu_log_mask(LOG_GUEST_ERROR, + "ITS MAPC: invalid collection table attributes " + "icid %d rdbase %lu\n", icid, rdbase); + /* + * in this implementation,in case of error + * we ignore this command and move onto the next + * command in the queue + */ + } else { + res = update_cte(s, icid, valid, rdbase); + } + + return res; +} + +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid, + uint8_t size, uint64_t itt_addr) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint64_t value; + uint64_t l2t_addr; + bool valid_l2t; + uint32_t l2t_id; + uint32_t max_l2_entries; + uint64_t dte = 0; + MemTxResult res = MEMTX_OK; + + if (s->dt.valid) { + if (valid) { + /* add mapping entry to device table */ + dte = (valid & VALID_MASK) | + ((size & SIZE_MASK) << 1U) | + ((itt_addr & ITTADDR_MASK) << 6ULL); + } + } else { + return res; + } + + /* + * The specification defines the format of level 1 entries of a + * 2-level table, but the format of level 2 entries and the format + * of flat-mapped tables is IMPDEF. + */ + if (s->dt.indirect) { + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); + + value = address_space_ldq_le(as, + s->dt.base_addr + + (l2t_id * L1TABLE_ENTRY_SIZE), + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK; + + if (valid_l2t) { + max_l2_entries = s->dt.page_sz / s->dt.entry_sz; + + l2t_addr = value & ((1ULL << 51) - 1); + + address_space_stq_le(as, l2t_addr + + ((devid % max_l2_entries) * GITS_DTE_SIZE), + dte, MEMTXATTRS_UNSPECIFIED, &res); + } + } else { + /* Flat level table */ + address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE), + dte, MEMTXATTRS_UNSPECIFIED, &res); + } + return res; +} + +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value, + uint32_t offset) +{ + AddressSpace *as = &s->gicv3->dma_as; + uint32_t devid; + uint8_t size; + uint64_t itt_addr; + bool valid; + MemTxResult res = MEMTX_OK; + + devid = (value >> DEVID_SHIFT) & DEVID_MASK; + + offset += NUM_BYTES_IN_DW; + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + size = (value & SIZE_MASK); + + offset += NUM_BYTES_IN_DW; + value = address_space_ldq_le(as, s->cq.base_addr + offset, + MEMTXATTRS_UNSPECIFIED, &res); + + if (res != MEMTX_OK) { + return res; + } + + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK; + + valid = (value >> VALID_SHIFT) & VALID_MASK; + + if ((devid > s->dt.max_devids) || + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) { + qemu_log_mask(LOG_GUEST_ERROR, + "ITS MAPD: invalid device table attributes " + "devid %d or size %d\n", devid, size); + /* + * in this implementation, in case of error + * we ignore this command and move onto the next + * command in the queue + */ + } else { + res = update_dte(s, devid, valid, size, itt_addr); + } + + return res; +} + +/* + * Current implementation blocks until all + * commands are processed + */ +static void process_cmdq(GICv3ITSState *s) +{ + uint32_t wr_offset = 0; + uint32_t rd_offset = 0; + uint32_t cq_offset = 0; + uint64_t data; + AddressSpace *as = &s->gicv3->dma_as; + MemTxResult res = MEMTX_OK; + uint8_t cmd; + + if (!(s->ctlr & ITS_CTLR_ENABLED)) { + return; + } + + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET); + + if (wr_offset > s->cq.max_entries) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid write offset " + "%d\n", __func__, wr_offset); + return; + } + + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET); + + while (wr_offset != rd_offset) { + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); + data = address_space_ldq_le(as, s->cq.base_addr + cq_offset, + MEMTXATTRS_UNSPECIFIED, &res); + cmd = (data & CMD_MASK); + + switch (cmd) { + case GITS_CMD_INT: + break; + case GITS_CMD_CLEAR: + break; + case GITS_CMD_SYNC: + /* + * Current implementation makes a blocking synchronous call + * for every command issued earlier, hence the internal state + * is already consistent by the time SYNC command is executed. + * Hence no further processing is required for SYNC command. + */ + break; + case GITS_CMD_MAPD: + res = process_mapd(s, data, cq_offset); + break; + case GITS_CMD_MAPC: + res = process_mapc(s, cq_offset); + break; + case GITS_CMD_MAPTI: + break; + case GITS_CMD_MAPI: + break; + case GITS_CMD_DISCARD: + break; + default: + break; + } + if (res == MEMTX_OK) { + rd_offset++; + rd_offset %= s->cq.max_entries; + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset); + } else { + /* + * in this implementation,in case of dma read/write error + * we stall the command processing + */ + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); + qemu_log_mask(LOG_GUEST_ERROR, + "%s: %x cmd processing failed!!\n", __func__, cmd); + break; + } + } +} + static void extract_table_params(GICv3ITSState *s) { uint16_t num_pages = 0; @@ -226,6 +515,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset, case GITS_CWRITER: s->cwriter = deposit64(s->cwriter, 0, 32, (value & ~R_GITS_CWRITER_RETRY_MASK)); + if (s->cwriter != s->creadr) { + process_cmdq(s); + } break; case GITS_CWRITER + 4: s->cwriter = deposit64(s->cwriter, 32, 32, @@ -346,6 +638,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset, break; case GITS_CWRITER: s->cwriter = value & ~R_GITS_CWRITER_RETRY_MASK; + if (s->cwriter != s->creadr) { + process_cmdq(s); + } break; case GITS_CREADR: case GITS_TYPER: diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index d6aaa94e4c..0932a30560 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -253,6 +253,9 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3) FIELD(GITS_CBASER, INNERCACHE, 59, 3) FIELD(GITS_CBASER, VALID, 63, 1) +FIELD(GITS_CREADR, STALLED, 0, 1) +FIELD(GITS_CREADR, OFFSET, 5, 15) + FIELD(GITS_CWRITER, RETRY, 0, 1) FIELD(GITS_CWRITER, OFFSET, 5, 15) @@ -289,6 +292,40 @@ FIELD(GITS_TYPER, CIL, 36, 1) #define L1TABLE_ENTRY_SIZE 8 #define GITS_CMDQ_ENTRY_SIZE 32 +#define NUM_BYTES_IN_DW 8 + +#define CMD_MASK 0xff + +/* ITS Commands */ +#define GITS_CMD_CLEAR 0x04 +#define GITS_CMD_DISCARD 0x0F +#define GITS_CMD_INT 0x03 +#define GITS_CMD_MAPC 0x09 +#define GITS_CMD_MAPD 0x08 +#define GITS_CMD_MAPI 0x0B +#define GITS_CMD_MAPTI 0x0A +#define GITS_CMD_SYNC 0x05 + +/* MAPC command fields */ +#define ICID_LENGTH 16 +#define ICID_MASK ((1U << ICID_LENGTH) - 1) +FIELD(MAPC, RDBASE, 16, 32) + +#define RDBASE_PROCNUM_LENGTH 16 +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) - 1) + +#define DEVID_SHIFT 32 +#define DEVID_LENGTH 32 +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1) + +/* MAPD command fields */ +#define ITTADDR_LENGTH 44 +#define ITTADDR_SHIFT 8 +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1) +#define SIZE_MASK 0x1f + +#define VALID_SHIFT 63 +#define VALID_MASK 1ULL /** * Default features advertised by this version of ITS
Added functionality to trigger ITS command queue processing on write to CWRITE register and process each command queue entry to identify the command type and handle commands like MAPD,MAPC,SYNC. Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/intc/arm_gicv3_its.c | 295 +++++++++++++++++++++++++++++++++++++++ hw/intc/gicv3_internal.h | 37 +++++ 2 files changed, 332 insertions(+) -- 2.27.0