From: Eric Auger <eauger@redhat.com>
To: Shashi Mallela <shashi.mallela@linaro.org>,
peter.maydell@linaro.org, leif@nuviainc.com, rad@semihalf.com
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Date: Sun, 13 Jun 2021 16:13:52 +0200 [thread overview]
Message-ID: <27cf428d-f22f-c336-2b6c-a2feca1103d7@redhat.com> (raw)
In-Reply-To: <20210602180042.111347-4-shashi.mallela@linaro.org>
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
next prev parent reply other threads:[~2021-06-13 14:14 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-02 18:00 [PATCH v4 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
2021-06-02 18:00 ` [PATCH v4 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
2021-06-08 10:02 ` Peter Maydell
2021-06-11 16:21 ` Eric Auger
2021-06-11 17:23 ` Shashi Mallela
2021-07-06 7:38 ` Eric Auger
2021-07-06 13:24 ` shashi.mallela
2021-07-06 14:04 ` Eric Auger
2021-07-06 14:18 ` shashi.mallela
2021-06-12 6:52 ` Eric Auger
2021-07-06 7:29 ` Eric Auger
2021-06-02 18:00 ` [PATCH v4 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
2021-06-08 10:31 ` Peter Maydell
2021-06-12 6:08 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-21 9:51 ` Eric Auger
2021-06-28 21:51 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
2021-06-08 10:38 ` Peter Maydell
2021-06-13 14:13 ` Eric Auger [this message]
2021-06-16 21:02 ` shashi.mallela
2021-06-21 10:03 ` Eric Auger
2021-06-28 21:58 ` shashi.mallela
2021-06-13 14:39 ` Eric Auger
2021-06-28 15:55 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
2021-06-08 10:45 ` Peter Maydell
2021-06-13 15:55 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-21 10:13 ` Eric Auger
2021-06-28 22:04 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
2021-06-08 10:57 ` Peter Maydell
2021-06-02 18:00 ` [PATCH v4 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
2021-06-08 13:57 ` Peter Maydell
2021-06-10 23:39 ` Shashi Mallela
2021-06-11 8:30 ` Peter Maydell
2021-06-15 2:23 ` Shashi Mallela
2021-06-13 16:26 ` Eric Auger
2021-06-16 21:02 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
2021-06-03 11:42 ` Leif Lindholm
2021-06-03 15:31 ` shashi.mallela
2021-06-04 10:42 ` Leif Lindholm
2021-06-04 15:36 ` shashi.mallela
2021-07-08 19:40 ` Leif Lindholm
2021-07-08 20:05 ` Peter Maydell
2021-07-08 22:05 ` Leif Lindholm
2021-08-05 20:10 ` shashi.mallela
2021-06-02 18:00 ` [PATCH v4 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
2021-06-08 11:00 ` Peter Maydell
2021-06-08 10:00 ` [PATCH v4 0/8] GICv3 LPI and ITS feature implementation Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27cf428d-f22f-c336-2b6c-a2feca1103d7@redhat.com \
--to=eauger@redhat.com \
--cc=leif@nuviainc.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=rad@semihalf.com \
--cc=shashi.mallela@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).