diff mbox series

[v2,3/8] hw/intc: GICv3 ITS command queue framework

Message ID 20210401024152.203896-4-shashi.mallela@linaro.org
State New
Headers show
Series GICv3 LPI and ITS feature implementation | expand

Commit Message

Shashi Mallela April 1, 2021, 2:41 a.m. UTC
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  | 347 +++++++++++++++++++++++++++++++++++++++
 hw/intc/gicv3_internal.h |  41 +++++
 2 files changed, 388 insertions(+)

-- 
2.27.0

Comments

Peter Maydell April 19, 2021, 10:30 a.m. UTC | #1
On Thu, 1 Apr 2021 at 03:41, 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  | 347 +++++++++++++++++++++++++++++++++++++++

>  hw/intc/gicv3_internal.h |  41 +++++

>  2 files changed, 388 insertions(+)

>

> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

> index 81373f049d..fcac33c836 100644

> --- a/hw/intc/arm_gicv3_its.c

> +++ b/hw/intc/arm_gicv3_its.c

> @@ -28,6 +28,347 @@ struct GICv3ITSClass {

>      void (*parent_reset)(DeviceState *dev);

>  };

>

> +static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)

> +{

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    uint64_t rdbase;

> +    uint64_t value;

> +    bool pta = false;

> +    MemTxResult res = MEMTX_OK;

> +

> +    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 the ITS fails to read or write DMA, it does not report that
by provoking a bus error for the CPU's write to GITS_CWRITER,
which is what will happen if you just propagate up the MemTxResult
from this DMA operation back through the register write function
to its caller. The spec doesn't seem to define a particular behaviour
for "I couldn't read the command out of the command queue", but
it would seem sensible to pick one of the options from the "Command errors"
section: ignore the command, or stall the command queue ("treat as
valid" is the other option there but doesn't seem relevant here).

"Stall" is probably best as otherwise we might loop forever through
a region of unreadable addresses.

> +    if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {

> +        /*

> +         * only bits[47:16] are considered instead of bits [51:16]

> +         * since with a physical address the target address must be

> +         * 64KB aligned

> +         */

> +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;

> +        pta = true;

> +    } else {

> +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;

> +    }

> +

> +    if (!pta && (rdbase < (s->gicv3->num_cpu))) {

> +        /*

> +         * 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.

> +         */

> +    }

> +

> +    offset += NUM_BYTES_IN_DW;

> +    return res;

> +}


TODO: review process_mapd and process_mapc

> +

> +static void update_cte(GICv3ITSState *s, uint16_t icid, uint64_t cte)

> +{

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    uint64_t value;

> +    uint8_t  page_sz_type;

> +    uint64_t l2t_addr;

> +    bool valid_l2t;

> +    uint32_t l2t_id;

> +    uint32_t page_sz = 0;

> +    uint32_t max_l2_entries;


I think it's worth having a comment here:

/*
 * 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.
 */

Q: why have you chosen to make the level-2 and flatmap entries
64 bits here ? They only need to contain the valid bit plus a
16 bit processor number. (We know the processor number fits in 16
bits because the GICR_TYPER.Processor_Number field in the redistributor
is that large.) Is there something I'm missing in the spec that makes
64-bit entries a good choice anyway ?

> +

> +    if (s->ct.indirect) {

> +        /* 2 level table */

> +        page_sz_type = FIELD_EX64(s->baser[1], GITS_BASER, PAGESIZE);

> +

> +        if (page_sz_type == 0) {

> +            page_sz = GITS_ITT_PAGE_SIZE_0;

> +        } else if (page_sz_type == 1) {

> +            page_sz = GITS_ITT_PAGE_SIZE_1;

> +        } else if (page_sz_type == 2) {

> +            page_sz = GITS_ITT_PAGE_SIZE_2;

> +        }


page_sz_type == 3 has a defined meaning: must be treated like 2.

If we're caching stuff in s->ct, maybe we should cache page_sz too ?

> +

> +        l2t_id = icid / (page_sz / L1TABLE_ENTRY_SIZE);

> +

> +        value = address_space_ldq_le(as,

> +                                     s->ct.base_addr +

> +                                     (l2t_id * L1TABLE_ENTRY_SIZE),

> +                                     MEMTXATTRS_UNSPECIFIED, NULL);

> +

> +        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;

> +

> +        if (valid_l2t) {

> +            max_l2_entries = page_sz / s->ct.entry_sz;

> +

> +            l2t_addr = (value >> page_sz_type) &

> +                        ((1ULL << (51 - page_sz_type)) - 1);


The spec text could be more clearly worded, but I don't think this is how
the physaddr is encoded in the L1 descriptor. I think that the intent is
that bits [51:0] are the physical address, with bits [N-1:0] of that being 0
because the L2 table must be page aligned. Your intepretation (that the
physaddr bit 0 is in bit N of the descriptor) wouldn't allow L2 tables
to be anywhere in the address space.

> +

> +            address_space_write(as, l2t_addr +

> +                                 ((icid % max_l2_entries) * GITS_CTE_SIZE),

> +                                 MEMTXATTRS_UNSPECIFIED,

> +                                 &cte, sizeof(cte));


Writing the CTE word using address_space_write() like this will
do the wrong thing on big-endian hosts. address_space_write() is the
"write a string of bytes to guest memory" function, so it will write
the uint64_t in whatever the host's byte order is. You want a fixed
byte order, so use address_space_ldq_le() or similar.

> +        }


> +    } else {

> +        /* Flat level table */

> +        address_space_write(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),

> +                            MEMTXATTRS_UNSPECIFIED, &cte,

> +                            sizeof(cte));

> +    }

> +}


I feel like there's a fair amount of code duplication between
get_cte(), get_dte(), update_cte() and update_dte() which could
perhaps be refactored out.

> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)

> +{

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    uint16_t icid;

> +    uint64_t rdbase;

> +    bool valid;

> +    bool pta = false;

> +    MemTxResult res = MEMTX_OK;

> +    uint64_t cte_entry;

> +    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);

> +

> +    icid = value & ICID_MASK;

> +

> +    if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {

> +        /*

> +         * only bits[47:16] are considered instead of bits [51:16]

> +         * since with a physical address the target address must be

> +         * 64KB aligned

> +         */

> +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;

> +        pta = true;

> +    } else {

> +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;

> +    }


GITS_TYPER.PTA is a read-only bit where we report to the guest how
our implementation behaves (ie whether these are processor numbers
or redistributor base addresses). We should implement one thing,
whichever is the easy one to implement for us, not both.

The specification is giving implementations a choice here because
there are different ways an ITS-to-redistributor interface might work:
 * the ITS might talk to the redistributor by literally writing to
   its registers via DMA (in which case setting GITS_TYPER.PTA makes
   its life easier)
 * the ITS might have its own backdoor or internal communications
   channel to the redistributors (in which case it wants to know a
   CPU number)

Looking forward, in patch 6 you add a comment:
  * Current implementation only supports rdbase == procnum
  * Hence rdbase physical address is ignored

So we're the second type of implementation. We should set
GITS_TYPER.PTA=0, and then this code can simply assume that
the data is always the "it's a CPU number" format.

> +

> +    valid = (value >> VALID_SHIFT) & VALID_MASK;

> +

> +    if (valid) {

> +        if ((icid > s->ct.max_collids) || (!pta &&

> +                (rdbase > s->gicv3->num_cpu))) {

> +            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {

> +                /* Generate System Error here if supported */

> +            }


Again, GITS_TYPER.SEIS is a bit which we use to tell the guest whether
we're going to generate system errors or not; it's not a bit which
we read in order to decide whether to generate them. For QEMU, the
CPU itself doesn't have system-error support properly yet, so for
us SEIS should be zero. In the ITS code we thus have two choices:
 (1) just don't put anything in the code
 (2) add a function call to an empty function called something
     like its_raise_serror() which just has a comment noting that
     it's a placeholder to mark where we would need to raise an SError
     if we wanted to support it

I think I would favour option 1.

> +            qemu_log_mask(LOG_GUEST_ERROR,

> +                "%s: invalid collection table attributes "

> +                "icid %d rdbase %lu\n", __func__, icid, rdbase);


This kind of LOG_GUEST_ERROR logging doesn't need to include the
__func__ string:
saying "ITS MAPC command" is more comprehensible to a reader of the
log who doesn't necessarily want to look into the QEMU source code
than saying "process_mapc".

> +            /*

> +             * in this implementation,in case of error

> +             * we ignore this command and move onto the next

> +             * command in the queue

> +             */

> +        } else {

> +            if (s->ct.valid) {

> +                /* add mapping entry to collection table */

> +                cte_entry = (valid & VALID_MASK) |

> +                            (pta ? ((rdbase & RDBASE_MASK) << 1ULL) :

> +                            ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL));

> +

> +                update_cte(s, icid, cte_entry);

> +            }


Put the s->ct.valid check inside update_cte(). Pass the
various parameters -- valid, pta, rdbase -- to update_cte()
and have it do the assembly into a word. That way all the code
dealing with the specific format of the table is inside that function,
rather than being partly here and partly there.

> +        }

> +    } else {

> +        if (s->ct.valid) {

> +            /* remove mapping entry from collection table */

> +            cte_entry = 0;

> +

> +            update_cte(s, icid, cte_entry);



> +        }

> +    }

> +

> +    offset += NUM_BYTES_IN_DW;

> +    offset += NUM_BYTES_IN_DW;

> +

> +    return res;

> +}

> +

> +static void update_dte(GICv3ITSState *s, uint32_t devid, uint64_t dte)

> +{

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    uint64_t value;

> +    uint8_t  page_sz_type;

> +    uint64_t l2t_addr;

> +    bool valid_l2t;

> +    uint32_t l2t_id;

> +    uint32_t page_sz = 0;

> +    uint32_t max_l2_entries;

> +

> +    if (s->dt.indirect) {

> +        /* 2 level table */

> +        page_sz_type = FIELD_EX64(s->baser[0], GITS_BASER, PAGESIZE);

> +

> +        if (page_sz_type == 0) {

> +            page_sz = GITS_ITT_PAGE_SIZE_0;

> +        } else if (page_sz_type == 1) {

> +            page_sz = GITS_ITT_PAGE_SIZE_1;

> +        } else if (page_sz_type == 2) {

> +            page_sz = GITS_ITT_PAGE_SIZE_2;

> +        }

> +

> +        l2t_id = devid / (page_sz / L1TABLE_ENTRY_SIZE);

> +

> +        value = address_space_ldq_le(as,

> +                                     s->dt.base_addr +

> +                                     (l2t_id * L1TABLE_ENTRY_SIZE),

> +                                     MEMTXATTRS_UNSPECIFIED, NULL);

> +

> +        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;

> +

> +        if (valid_l2t) {

> +            max_l2_entries = page_sz / s->dt.entry_sz;

> +

> +            l2t_addr = (value >> page_sz_type) &

> +                        ((1ULL << (51 - page_sz_type)) - 1);

> +

> +            address_space_write(as, l2t_addr +

> +                                 ((devid % max_l2_entries) * GITS_DTE_SIZE),

> +                                 MEMTXATTRS_UNSPECIFIED, &dte, sizeof(dte));

> +        }

> +    } else {

> +        /* Flat level table */

> +        address_space_write(as, s->dt.base_addr + (devid * GITS_DTE_SIZE),

> +                            MEMTXATTRS_UNSPECIFIED, &dte, sizeof(dte));

> +    }

> +}

> +

> +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,

> +                                 uint32_t offset)

> +{

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    uint32_t devid;

> +    uint8_t size;

> +    uint64_t itt_addr;

> +    bool valid;

> +    MemTxResult res = MEMTX_OK;

> +    uint64_t dte_entry = 0;

> +

> +    devid = (value >> DEVID_OFFSET) & DEVID_MASK;

> +

> +    offset += NUM_BYTES_IN_DW;

> +    value = address_space_ldq_le(as, s->cq.base_addr + offset,

> +                                 MEMTXATTRS_UNSPECIFIED, &res);

> +    size = (value & SIZE_MASK);

> +

> +    offset += NUM_BYTES_IN_DW;

> +    value = address_space_ldq_le(as, s->cq.base_addr + offset,

> +                                 MEMTXATTRS_UNSPECIFIED, &res);

> +    itt_addr = (value >> ITTADDR_OFFSET) & ITTADDR_MASK;

> +

> +    valid = (value >> VALID_SHIFT) & VALID_MASK;

> +

> +    if (valid) {

> +        if ((devid > s->dt.max_devids) ||

> +            (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {

> +            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {

> +                /* Generate System Error here if supported */

> +            }

> +            qemu_log_mask(LOG_GUEST_ERROR,

> +                "%s: invalid device table attributes "

> +                "devid %d or size %d\n", __func__, devid, size);

> +            /*

> +             * in this implementation,in case of error

> +             * we ignore this command and move onto the next

> +             * command in the queue

> +             */

> +        } else {

> +            if (s->dt.valid) {

> +                /* add mapping entry to device table */

> +                dte_entry = (valid & VALID_MASK) |

> +                            ((size & SIZE_MASK) << 1U) |

> +                            ((itt_addr & ITTADDR_MASK) << 6ULL);

> +

> +                update_dte(s, devid, dte_entry);

> +            }

> +        }

> +    } else {

> +        if (s->dt.valid) {

> +            /* remove mapping entry from device table */

> +            dte_entry = 0;

> +            update_dte(s, devid, dte_entry);

> +        }

> +    }

> +

> +    offset += NUM_BYTES_IN_DW;

> +    offset += NUM_BYTES_IN_DW;

> +

> +    return res;

> +}


Review comments for process_mapc() and update_cte() apply also to
process_mapd() and update_dte().

> +

> +/*

> + * Current implementation blocks until all

> + * commands are processed

> + */

> +static MemTxResult process_cmdq(GICv3ITSState *s)


This should return 'void' (see comments earlier).

> +{

> +    uint32_t wr_offset = 0;

> +    uint32_t rd_offset = 0;

> +    uint32_t cq_offset = 0;

> +    uint64_t data;

> +    AddressSpace *as = &s->gicv3->sysmem_as;

> +    MemTxResult res = MEMTX_OK;

> +    uint8_t cmd;

> +

> +    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);

> +        res = MEMTX_ERROR;

> +        return res;

> +    }

> +

> +    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);


I think I would just read all four quadwords of the command here
into a local uint64_t data[4], and pass that to the process_$cmd()
functions. That saves those functions doing the loads and handling
possible errors doing the loads themselves.

> +        cmd = (data & CMD_MASK);

> +

> +        switch (cmd) {

> +        case GITS_CMD_INT:

> +            break;

> +        case GITS_CMD_CLEAR:

> +            break;

> +        case GITS_CMD_SYNC:

> +            res = process_sync(s, cq_offset);

> +            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 {

> +            qemu_log_mask(LOG_GUEST_ERROR,

> +                "%s: %x cmd processing failed!!\n", __func__, cmd);

> +            break;

> +        }

> +    }

> +    return res;

> +}

> +

>  static bool extract_table_params(GICv3ITSState *s, int index)

>  {

>      uint16_t num_pages = 0;

> @@ -282,6 +623,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,

>          break;

>      case GITS_CWRITER:

>          s->cwriter = deposit64(s->cwriter, 0, 32, value);

> +        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s->creadr)) {

> +            result = process_cmdq(s);

> +        }

>          break;

>      case GITS_CWRITER + 4:

>          s->cwriter = deposit64(s->cwriter, 32, 32, value);

> @@ -416,6 +760,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,

>          break;

>      case GITS_CWRITER:

>          s->cwriter = value;

> +        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s->creadr)) {

> +            result = process_cmdq(s);

> +        }



I would move the check on CTLR.ENABLED into process_cmdq() itself
(which is already doing a writer != reader check).

>          break;

>      case GITS_TYPER:

>      case GITS_CREADR:

> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

> index b7701e8ca5..7e1ff426ef 100644

> --- a/hw/intc/gicv3_internal.h

> +++ b/hw/intc/gicv3_internal.h

> @@ -253,6 +253,12 @@ 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)

> +

>  FIELD(GITS_CTLR, ENABLED, 0, 1)

>  FIELD(GITS_CTLR, QUIESCENT, 31, 1)

>

> @@ -284,6 +290,41 @@ 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_LEN                  16

> +#define ICID_MASK                 ((1U << ICID_LEN) - 1)

> +#define RDBASE_LEN                32

> +#define RDBASE_OFFSET             16

> +#define RDBASE_MASK               ((1ULL << RDBASE_LEN) - 1)


If you want to define these by hand, you could at least use the same
suffixes that the FIELD() macro does: _SHIFT, _LENGTH and _MASK.

> +#define RDBASE_PROCNUM_LEN        16

> +#define RDBASE_PROCNUM_MASK       ((1ULL << RDBASE_PROCNUM_LEN) - 1)

> +

> +#define DEVID_OFFSET              32

> +#define DEVID_LEN                 32

> +#define DEVID_MASK                ((1ULL << DEVID_LEN) - 1)

> +

> +/* MAPD command fields */

> +#define ITTADDR_LEN               44

> +#define ITTADDR_OFFSET            8

> +#define ITTADDR_MASK              ((1ULL << ITTADDR_LEN) - 1)

> +#define SIZE_MASK                 0x1f

> +

> +#define VALID_SHIFT               63

> +#define VALID_MASK                0x1

>

>  /**

>   * Default features advertised by this version of ITS

> --

> 2.27


thanks
-- PMM
Shashi Mallela April 29, 2021, 11:38 p.m. UTC | #2
On Mon, 2021-04-19 at 11:30 +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 03:41, 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  | 347

> > +++++++++++++++++++++++++++++++++++++++

> >  hw/intc/gicv3_internal.h |  41 +++++

> >  2 files changed, 388 insertions(+)

> > 

> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c

> > index 81373f049d..fcac33c836 100644

> > --- a/hw/intc/arm_gicv3_its.c

> > +++ b/hw/intc/arm_gicv3_its.c

> > @@ -28,6 +28,347 @@ struct GICv3ITSClass {

> >      void (*parent_reset)(DeviceState *dev);

> >  };

> > 

> > +static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)

> > +{

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    uint64_t rdbase;

> > +    uint64_t value;

> > +    bool pta = false;

> > +    MemTxResult res = MEMTX_OK;

> > +

> > +    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 the ITS fails to read or write DMA, it does not report that

> by provoking a bus error for the CPU's write to GITS_CWRITER,

> which is what will happen if you just propagate up the MemTxResult

> from this DMA operation back through the register write function

> to its caller. The spec doesn't seem to define a particular behaviour

> for "I couldn't read the command out of the command queue", but

> it would seem sensible to pick one of the options from the "Command

> errors"

> section: ignore the command, or stall the command queue ("treat as

> valid" is the other option there but doesn't seem relevant here).

> 

> "Stall" is probably best as otherwise we might loop forever through

> a region of unreadable addresses.

>

> have implemented "stall" indication for all failing dma read/write

calls(based on the return value of each call) 
> 

> > +    if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {

> > +        /*

> > +         * only bits[47:16] are considered instead of bits [51:16]

> > +         * since with a physical address the target address must

> > be

> > +         * 64KB aligned

> > +         */

> > +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;

> > +        pta = true;

> > +    } else {

> > +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;

> > +    }

> > +

> > +    if (!pta && (rdbase < (s->gicv3->num_cpu))) {

> > +        /*

> > +         * 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.

> > +         */

> > +    }

> > +

> > +    offset += NUM_BYTES_IN_DW;

> > +    return res;

> > +}

> 

> TODO: review process_mapd and process_mapc

> 

> > +

> > +static void update_cte(GICv3ITSState *s, uint16_t icid, uint64_t

> > cte)

> > +{

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    uint64_t value;

> > +    uint8_t  page_sz_type;

> > +    uint64_t l2t_addr;

> > +    bool valid_l2t;

> > +    uint32_t l2t_id;

> > +    uint32_t page_sz = 0;

> > +    uint32_t max_l2_entries;

> 

> I think it's worth having a comment here:

> 

> /*

>  * 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.

>  */

> 

> Q: why have you chosen to make the level-2 and flatmap entries

> 64 bits here ? They only need to contain the valid bit plus a

> 16 bit processor number. (We know the processor number fits in 16

> bits because the GICR_TYPER.Processor_Number field in the

> redistributor

> is that large.) Is there something I'm missing in the spec that makes

> 64-bit entries a good choice anyway ?

> 

> I chose 64 bits to be able to have the data structure ready for both

the processor number and RD_base address possibilities, in case we add
RD_Base address support in the future
> > +

> > +    if (s->ct.indirect) {

> > +        /* 2 level table */

> > +        page_sz_type = FIELD_EX64(s->baser[1], GITS_BASER,

> > PAGESIZE);

> > +

> > +        if (page_sz_type == 0) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_0;

> > +        } else if (page_sz_type == 1) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_1;

> > +        } else if (page_sz_type == 2) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_2;

> > +        }

> 

> page_sz_type == 3 has a defined meaning: must be treated like 2.

> 

> If we're caching stuff in s->ct, maybe we should cache page_sz too ?

> 

> > +

> > +        l2t_id = icid / (page_sz / L1TABLE_ENTRY_SIZE);

> > +

> > +        value = address_space_ldq_le(as,

> > +                                     s->ct.base_addr +

> > +                                     (l2t_id *

> > L1TABLE_ENTRY_SIZE),

> > +                                     MEMTXATTRS_UNSPECIFIED,

> > NULL);

> > +

> > +        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;

> > +

> > +        if (valid_l2t) {

> > +            max_l2_entries = page_sz / s->ct.entry_sz;

> > +

> > +            l2t_addr = (value >> page_sz_type) &

> > +                        ((1ULL << (51 - page_sz_type)) - 1);

> 

> The spec text could be more clearly worded, but I don't think this is

> how

> the physaddr is encoded in the L1 descriptor. I think that the intent

> is

> that bits [51:0] are the physical address, with bits [N-1:0] of that

> being 0

> because the L2 table must be page aligned. Your intepretation (that

> the

> physaddr bit 0 is in bit N of the descriptor) wouldn't allow L2

> tables

> to be anywhere in the address space.

> 

> > +

> > +            address_space_write(as, l2t_addr +

> > +                                 ((icid % max_l2_entries) *

> > GITS_CTE_SIZE),

> > +                                 MEMTXATTRS_UNSPECIFIED,

> > +                                 &cte, sizeof(cte));

> 

> Writing the CTE word using address_space_write() like this will

> do the wrong thing on big-endian hosts. address_space_write() is the

> "write a string of bytes to guest memory" function, so it will write

> the uint64_t in whatever the host's byte order is. You want a fixed

> byte order, so use address_space_ldq_le() or similar.

> 

> > +        }

> > +    } else {

> > +        /* Flat level table */

> > +        address_space_write(as, s->ct.base_addr + (icid *

> > GITS_CTE_SIZE),

> > +                            MEMTXATTRS_UNSPECIFIED, &cte,

> > +                            sizeof(cte));

> > +    }

> > +}

> 

> I feel like there's a fair amount of code duplication between

> get_cte(), get_dte(), update_cte() and update_dte() which could

> perhaps be refactored out.

> 

> > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)

> > +{

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    uint16_t icid;

> > +    uint64_t rdbase;

> > +    bool valid;

> > +    bool pta = false;

> > +    MemTxResult res = MEMTX_OK;

> > +    uint64_t cte_entry;

> > +    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);

> > +

> > +    icid = value & ICID_MASK;

> > +

> > +    if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {

> > +        /*

> > +         * only bits[47:16] are considered instead of bits [51:16]

> > +         * since with a physical address the target address must

> > be

> > +         * 64KB aligned

> > +         */

> > +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;

> > +        pta = true;

> > +    } else {

> > +        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;

> > +    }

> 

> GITS_TYPER.PTA is a read-only bit where we report to the guest how

> our implementation behaves (ie whether these are processor numbers

> or redistributor base addresses). We should implement one thing,

> whichever is the easy one to implement for us, not both.

> 

> The specification is giving implementations a choice here because

> there are different ways an ITS-to-redistributor interface might

> work:

>  * the ITS might talk to the redistributor by literally writing to

>    its registers via DMA (in which case setting GITS_TYPER.PTA makes

>    its life easier)

>  * the ITS might have its own backdoor or internal communications

>    channel to the redistributors (in which case it wants to know a

>    CPU number)

> 

> Looking forward, in patch 6 you add a comment:

>   * Current implementation only supports rdbase == procnum

>   * Hence rdbase physical address is ignored

> 

> So we're the second type of implementation. We should set

> GITS_TYPER.PTA=0, and then this code can simply assume that

> the data is always the "it's a CPU number" format.

> 

> > +

> > +    valid = (value >> VALID_SHIFT) & VALID_MASK;

> > +

> > +    if (valid) {

> > +        if ((icid > s->ct.max_collids) || (!pta &&

> > +                (rdbase > s->gicv3->num_cpu))) {

> > +            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {

> > +                /* Generate System Error here if supported */

> > +            }

> 

> Again, GITS_TYPER.SEIS is a bit which we use to tell the guest

> whether

> we're going to generate system errors or not; it's not a bit which

> we read in order to decide whether to generate them. For QEMU, the

> CPU itself doesn't have system-error support properly yet, so for

> us SEIS should be zero. In the ITS code we thus have two choices:

>  (1) just don't put anything in the code

>  (2) add a function call to an empty function called something

>      like its_raise_serror() which just has a comment noting that

>      it's a placeholder to mark where we would need to raise an

> SError

>      if we wanted to support it

> 

> I think I would favour option 1.

> 

> > +            qemu_log_mask(LOG_GUEST_ERROR,

> > +                "%s: invalid collection table attributes "

> > +                "icid %d rdbase %lu\n", __func__, icid, rdbase);

> 

> This kind of LOG_GUEST_ERROR logging doesn't need to include the

> __func__ string:

> saying "ITS MAPC command" is more comprehensible to a reader of the

> log who doesn't necessarily want to look into the QEMU source code

> than saying "process_mapc".

> 

> > +            /*

> > +             * in this implementation,in case of error

> > +             * we ignore this command and move onto the next

> > +             * command in the queue

> > +             */

> > +        } else {

> > +            if (s->ct.valid) {

> > +                /* add mapping entry to collection table */

> > +                cte_entry = (valid & VALID_MASK) |

> > +                            (pta ? ((rdbase & RDBASE_MASK) <<

> > 1ULL) :

> > +                            ((rdbase & RDBASE_PROCNUM_MASK) <<

> > 1ULL));

> > +

> > +                update_cte(s, icid, cte_entry);

> > +            }

> 

> Put the s->ct.valid check inside update_cte(). Pass the

> various parameters -- valid, pta, rdbase -- to update_cte()

> and have it do the assembly into a word. That way all the code

> dealing with the specific format of the table is inside that

> function,

> rather than being partly here and partly there.

> 

> > +        }

> > +    } else {

> > +        if (s->ct.valid) {

> > +            /* remove mapping entry from collection table */

> > +            cte_entry = 0;

> > +

> > +            update_cte(s, icid, cte_entry);

> > +        }

> > +    }

> > +

> > +    offset += NUM_BYTES_IN_DW;

> > +    offset += NUM_BYTES_IN_DW;

> > +

> > +    return res;

> > +}

> > +

> > +static void update_dte(GICv3ITSState *s, uint32_t devid, uint64_t

> > dte)

> > +{

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    uint64_t value;

> > +    uint8_t  page_sz_type;

> > +    uint64_t l2t_addr;

> > +    bool valid_l2t;

> > +    uint32_t l2t_id;

> > +    uint32_t page_sz = 0;

> > +    uint32_t max_l2_entries;

> > +

> > +    if (s->dt.indirect) {

> > +        /* 2 level table */

> > +        page_sz_type = FIELD_EX64(s->baser[0], GITS_BASER,

> > PAGESIZE);

> > +

> > +        if (page_sz_type == 0) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_0;

> > +        } else if (page_sz_type == 1) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_1;

> > +        } else if (page_sz_type == 2) {

> > +            page_sz = GITS_ITT_PAGE_SIZE_2;

> > +        }

> > +

> > +        l2t_id = devid / (page_sz / L1TABLE_ENTRY_SIZE);

> > +

> > +        value = address_space_ldq_le(as,

> > +                                     s->dt.base_addr +

> > +                                     (l2t_id *

> > L1TABLE_ENTRY_SIZE),

> > +                                     MEMTXATTRS_UNSPECIFIED,

> > NULL);

> > +

> > +        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;

> > +

> > +        if (valid_l2t) {

> > +            max_l2_entries = page_sz / s->dt.entry_sz;

> > +

> > +            l2t_addr = (value >> page_sz_type) &

> > +                        ((1ULL << (51 - page_sz_type)) - 1);

> > +

> > +            address_space_write(as, l2t_addr +

> > +                                 ((devid % max_l2_entries) *

> > GITS_DTE_SIZE),

> > +                                 MEMTXATTRS_UNSPECIFIED, &dte,

> > sizeof(dte));

> > +        }

> > +    } else {

> > +        /* Flat level table */

> > +        address_space_write(as, s->dt.base_addr + (devid *

> > GITS_DTE_SIZE),

> > +                            MEMTXATTRS_UNSPECIFIED, &dte,

> > sizeof(dte));

> > +    }

> > +}

> > +

> > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,

> > +                                 uint32_t offset)

> > +{

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    uint32_t devid;

> > +    uint8_t size;

> > +    uint64_t itt_addr;

> > +    bool valid;

> > +    MemTxResult res = MEMTX_OK;

> > +    uint64_t dte_entry = 0;

> > +

> > +    devid = (value >> DEVID_OFFSET) & DEVID_MASK;

> > +

> > +    offset += NUM_BYTES_IN_DW;

> > +    value = address_space_ldq_le(as, s->cq.base_addr + offset,

> > +                                 MEMTXATTRS_UNSPECIFIED, &res);

> > +    size = (value & SIZE_MASK);

> > +

> > +    offset += NUM_BYTES_IN_DW;

> > +    value = address_space_ldq_le(as, s->cq.base_addr + offset,

> > +                                 MEMTXATTRS_UNSPECIFIED, &res);

> > +    itt_addr = (value >> ITTADDR_OFFSET) & ITTADDR_MASK;

> > +

> > +    valid = (value >> VALID_SHIFT) & VALID_MASK;

> > +

> > +    if (valid) {

> > +        if ((devid > s->dt.max_devids) ||

> > +            (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {

> > +            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {

> > +                /* Generate System Error here if supported */

> > +            }

> > +            qemu_log_mask(LOG_GUEST_ERROR,

> > +                "%s: invalid device table attributes "

> > +                "devid %d or size %d\n", __func__, devid, size);

> > +            /*

> > +             * in this implementation,in case of error

> > +             * we ignore this command and move onto the next

> > +             * command in the queue

> > +             */

> > +        } else {

> > +            if (s->dt.valid) {

> > +                /* add mapping entry to device table */

> > +                dte_entry = (valid & VALID_MASK) |

> > +                            ((size & SIZE_MASK) << 1U) |

> > +                            ((itt_addr & ITTADDR_MASK) << 6ULL);

> > +

> > +                update_dte(s, devid, dte_entry);

> > +            }

> > +        }

> > +    } else {

> > +        if (s->dt.valid) {

> > +            /* remove mapping entry from device table */

> > +            dte_entry = 0;

> > +            update_dte(s, devid, dte_entry);

> > +        }

> > +    }

> > +

> > +    offset += NUM_BYTES_IN_DW;

> > +    offset += NUM_BYTES_IN_DW;

> > +

> > +    return res;

> > +}

> 

> Review comments for process_mapc() and update_cte() apply also to

> process_mapd() and update_dte().

> 

> > +

> > +/*

> > + * Current implementation blocks until all

> > + * commands are processed

> > + */

> > +static MemTxResult process_cmdq(GICv3ITSState *s)

> 

> This should return 'void' (see comments earlier).

> 

> > +{

> > +    uint32_t wr_offset = 0;

> > +    uint32_t rd_offset = 0;

> > +    uint32_t cq_offset = 0;

> > +    uint64_t data;

> > +    AddressSpace *as = &s->gicv3->sysmem_as;

> > +    MemTxResult res = MEMTX_OK;

> > +    uint8_t cmd;

> > +

> > +    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);

> > +        res = MEMTX_ERROR;

> > +        return res;

> > +    }

> > +

> > +    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);

> 

> I think I would just read all four quadwords of the command here

> into a local uint64_t data[4], and pass that to the process_$cmd()

> functions. That saves those functions doing the loads and handling

> possible errors doing the loads themselves.

> 

> There are 2 reasons i didn't pick reading all 4 quadwords at once:-

> 1) not all commands have valid quadword data making the dma call

> for such RES0 quadwords insignificant

> 2) Since we are now stalling on any dma read/write,in case we fail

>    during RES0 quadword read ,stalling doesnt seem significant

>

> Have optimised most of the similar-looking boilerplate code though

> > +        cmd = (data & CMD_MASK);

> > +

> > +        switch (cmd) {

> > +        case GITS_CMD_INT:

> > +            break;

> > +        case GITS_CMD_CLEAR:

> > +            break;

> > +        case GITS_CMD_SYNC:

> > +            res = process_sync(s, cq_offset);

> > +            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 {

> > +            qemu_log_mask(LOG_GUEST_ERROR,

> > +                "%s: %x cmd processing failed!!\n", __func__,

> > cmd);

> > +            break;

> > +        }

> > +    }

> > +    return res;

> > +}

> > +

> >  static bool extract_table_params(GICv3ITSState *s, int index)

> >  {

> >      uint16_t num_pages = 0;

> > @@ -282,6 +623,9 @@ static MemTxResult its_writel(GICv3ITSState *s,

> > hwaddr offset,

> >          break;

> >      case GITS_CWRITER:

> >          s->cwriter = deposit64(s->cwriter, 0, 32, value);

> > +        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s-

> > >creadr)) {

> > +            result = process_cmdq(s);

> > +        }

> >          break;

> >      case GITS_CWRITER + 4:

> >          s->cwriter = deposit64(s->cwriter, 32, 32, value);

> > @@ -416,6 +760,9 @@ static MemTxResult its_writell(GICv3ITSState

> > *s, hwaddr offset,

> >          break;

> >      case GITS_CWRITER:

> >          s->cwriter = value;

> > +        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s-

> > >creadr)) {

> > +            result = process_cmdq(s);

> > +        }

> 

> I would move the check on CTLR.ENABLED into process_cmdq() itself

> (which is already doing a writer != reader check).

> 

> >          break;

> >      case GITS_TYPER:

> >      case GITS_CREADR:

> > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h

> > index b7701e8ca5..7e1ff426ef 100644

> > --- a/hw/intc/gicv3_internal.h

> > +++ b/hw/intc/gicv3_internal.h

> > @@ -253,6 +253,12 @@ 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)

> > +

> >  FIELD(GITS_CTLR, ENABLED, 0, 1)

> >  FIELD(GITS_CTLR, QUIESCENT, 31, 1)

> > 

> > @@ -284,6 +290,41 @@ 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_LEN                  16

> > +#define ICID_MASK                 ((1U << ICID_LEN) - 1)

> > +#define RDBASE_LEN                32

> > +#define RDBASE_OFFSET             16

> > +#define RDBASE_MASK               ((1ULL << RDBASE_LEN) - 1)

> 

> If you want to define these by hand, you could at least use the same

> suffixes that the FIELD() macro does: _SHIFT, _LENGTH and _MASK.

> 

> > +#define RDBASE_PROCNUM_LEN        16

> > +#define RDBASE_PROCNUM_MASK       ((1ULL << RDBASE_PROCNUM_LEN) -

> > 1)

> > +

> > +#define DEVID_OFFSET              32

> > +#define DEVID_LEN                 32

> > +#define DEVID_MASK                ((1ULL << DEVID_LEN) - 1)

> > +

> > +/* MAPD command fields */

> > +#define ITTADDR_LEN               44

> > +#define ITTADDR_OFFSET            8

> > +#define ITTADDR_MASK              ((1ULL << ITTADDR_LEN) - 1)

> > +#define SIZE_MASK                 0x1f

> > +

> > +#define VALID_SHIFT               63

> > +#define VALID_MASK                0x1

> > 

> >  /**

> >   * Default features advertised by this version of ITS

> > --

> > 2.27

> 

> thanks

> -- PMM
diff mbox series

Patch

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 81373f049d..fcac33c836 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -28,6 +28,347 @@  struct GICv3ITSClass {
     void (*parent_reset)(DeviceState *dev);
 };
 
+static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
+{
+    AddressSpace *as = &s->gicv3->sysmem_as;
+    uint64_t rdbase;
+    uint64_t value;
+    bool pta = false;
+    MemTxResult res = MEMTX_OK;
+
+    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 (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {
+        /*
+         * only bits[47:16] are considered instead of bits [51:16]
+         * since with a physical address the target address must be
+         * 64KB aligned
+         */
+        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;
+        pta = true;
+    } else {
+        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;
+    }
+
+    if (!pta && (rdbase < (s->gicv3->num_cpu))) {
+        /*
+         * 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.
+         */
+    }
+
+    offset += NUM_BYTES_IN_DW;
+    return res;
+}
+
+static void update_cte(GICv3ITSState *s, uint16_t icid, uint64_t cte)
+{
+    AddressSpace *as = &s->gicv3->sysmem_as;
+    uint64_t value;
+    uint8_t  page_sz_type;
+    uint64_t l2t_addr;
+    bool valid_l2t;
+    uint32_t l2t_id;
+    uint32_t page_sz = 0;
+    uint32_t max_l2_entries;
+
+    if (s->ct.indirect) {
+        /* 2 level table */
+        page_sz_type = FIELD_EX64(s->baser[1], GITS_BASER, PAGESIZE);
+
+        if (page_sz_type == 0) {
+            page_sz = GITS_ITT_PAGE_SIZE_0;
+        } else if (page_sz_type == 1) {
+            page_sz = GITS_ITT_PAGE_SIZE_1;
+        } else if (page_sz_type == 2) {
+            page_sz = GITS_ITT_PAGE_SIZE_2;
+        }
+
+        l2t_id = icid / (page_sz / L1TABLE_ENTRY_SIZE);
+
+        value = address_space_ldq_le(as,
+                                     s->ct.base_addr +
+                                     (l2t_id * L1TABLE_ENTRY_SIZE),
+                                     MEMTXATTRS_UNSPECIFIED, NULL);
+
+        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+        if (valid_l2t) {
+            max_l2_entries = page_sz / s->ct.entry_sz;
+
+            l2t_addr = (value >> page_sz_type) &
+                        ((1ULL << (51 - page_sz_type)) - 1);
+
+            address_space_write(as, l2t_addr +
+                                 ((icid % max_l2_entries) * GITS_CTE_SIZE),
+                                 MEMTXATTRS_UNSPECIFIED,
+                                 &cte, sizeof(cte));
+        }
+    } else {
+        /* Flat level table */
+        address_space_write(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
+                            MEMTXATTRS_UNSPECIFIED, &cte,
+                            sizeof(cte));
+    }
+}
+
+static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
+{
+    AddressSpace *as = &s->gicv3->sysmem_as;
+    uint16_t icid;
+    uint64_t rdbase;
+    bool valid;
+    bool pta = false;
+    MemTxResult res = MEMTX_OK;
+    uint64_t cte_entry;
+    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);
+
+    icid = value & ICID_MASK;
+
+    if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {
+        /*
+         * only bits[47:16] are considered instead of bits [51:16]
+         * since with a physical address the target address must be
+         * 64KB aligned
+         */
+        rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;
+        pta = true;
+    } else {
+        rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;
+    }
+
+    valid = (value >> VALID_SHIFT) & VALID_MASK;
+
+    if (valid) {
+        if ((icid > s->ct.max_collids) || (!pta &&
+                (rdbase > s->gicv3->num_cpu))) {
+            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {
+                /* Generate System Error here if supported */
+            }
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: invalid collection table attributes "
+                "icid %d rdbase %lu\n", __func__, icid, rdbase);
+            /*
+             * in this implementation,in case of error
+             * we ignore this command and move onto the next
+             * command in the queue
+             */
+        } else {
+            if (s->ct.valid) {
+                /* add mapping entry to collection table */
+                cte_entry = (valid & VALID_MASK) |
+                            (pta ? ((rdbase & RDBASE_MASK) << 1ULL) :
+                            ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL));
+
+                update_cte(s, icid, cte_entry);
+            }
+        }
+    } else {
+        if (s->ct.valid) {
+            /* remove mapping entry from collection table */
+            cte_entry = 0;
+
+            update_cte(s, icid, cte_entry);
+        }
+    }
+
+    offset += NUM_BYTES_IN_DW;
+    offset += NUM_BYTES_IN_DW;
+
+    return res;
+}
+
+static void update_dte(GICv3ITSState *s, uint32_t devid, uint64_t dte)
+{
+    AddressSpace *as = &s->gicv3->sysmem_as;
+    uint64_t value;
+    uint8_t  page_sz_type;
+    uint64_t l2t_addr;
+    bool valid_l2t;
+    uint32_t l2t_id;
+    uint32_t page_sz = 0;
+    uint32_t max_l2_entries;
+
+    if (s->dt.indirect) {
+        /* 2 level table */
+        page_sz_type = FIELD_EX64(s->baser[0], GITS_BASER, PAGESIZE);
+
+        if (page_sz_type == 0) {
+            page_sz = GITS_ITT_PAGE_SIZE_0;
+        } else if (page_sz_type == 1) {
+            page_sz = GITS_ITT_PAGE_SIZE_1;
+        } else if (page_sz_type == 2) {
+            page_sz = GITS_ITT_PAGE_SIZE_2;
+        }
+
+        l2t_id = devid / (page_sz / L1TABLE_ENTRY_SIZE);
+
+        value = address_space_ldq_le(as,
+                                     s->dt.base_addr +
+                                     (l2t_id * L1TABLE_ENTRY_SIZE),
+                                     MEMTXATTRS_UNSPECIFIED, NULL);
+
+        valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+        if (valid_l2t) {
+            max_l2_entries = page_sz / s->dt.entry_sz;
+
+            l2t_addr = (value >> page_sz_type) &
+                        ((1ULL << (51 - page_sz_type)) - 1);
+
+            address_space_write(as, l2t_addr +
+                                 ((devid % max_l2_entries) * GITS_DTE_SIZE),
+                                 MEMTXATTRS_UNSPECIFIED, &dte, sizeof(dte));
+        }
+    } else {
+        /* Flat level table */
+        address_space_write(as, s->dt.base_addr + (devid * GITS_DTE_SIZE),
+                            MEMTXATTRS_UNSPECIFIED, &dte, sizeof(dte));
+    }
+}
+
+static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,
+                                 uint32_t offset)
+{
+    AddressSpace *as = &s->gicv3->sysmem_as;
+    uint32_t devid;
+    uint8_t size;
+    uint64_t itt_addr;
+    bool valid;
+    MemTxResult res = MEMTX_OK;
+    uint64_t dte_entry = 0;
+
+    devid = (value >> DEVID_OFFSET) & DEVID_MASK;
+
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    size = (value & SIZE_MASK);
+
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    itt_addr = (value >> ITTADDR_OFFSET) & ITTADDR_MASK;
+
+    valid = (value >> VALID_SHIFT) & VALID_MASK;
+
+    if (valid) {
+        if ((devid > s->dt.max_devids) ||
+            (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
+            if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {
+                /* Generate System Error here if supported */
+            }
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: invalid device table attributes "
+                "devid %d or size %d\n", __func__, devid, size);
+            /*
+             * in this implementation,in case of error
+             * we ignore this command and move onto the next
+             * command in the queue
+             */
+        } else {
+            if (s->dt.valid) {
+                /* add mapping entry to device table */
+                dte_entry = (valid & VALID_MASK) |
+                            ((size & SIZE_MASK) << 1U) |
+                            ((itt_addr & ITTADDR_MASK) << 6ULL);
+
+                update_dte(s, devid, dte_entry);
+            }
+        }
+    } else {
+        if (s->dt.valid) {
+            /* remove mapping entry from device table */
+            dte_entry = 0;
+            update_dte(s, devid, dte_entry);
+        }
+    }
+
+    offset += NUM_BYTES_IN_DW;
+    offset += NUM_BYTES_IN_DW;
+
+    return res;
+}
+
+/*
+ * Current implementation blocks until all
+ * commands are processed
+ */
+static MemTxResult 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->sysmem_as;
+    MemTxResult res = MEMTX_OK;
+    uint8_t cmd;
+
+    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);
+        res = MEMTX_ERROR;
+        return res;
+    }
+
+    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:
+            res = process_sync(s, cq_offset);
+            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 {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: %x cmd processing failed!!\n", __func__, cmd);
+            break;
+        }
+    }
+    return res;
+}
+
 static bool extract_table_params(GICv3ITSState *s, int index)
 {
     uint16_t num_pages = 0;
@@ -282,6 +623,9 @@  static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
         break;
     case GITS_CWRITER:
         s->cwriter = deposit64(s->cwriter, 0, 32, value);
+        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s->creadr)) {
+            result = process_cmdq(s);
+        }
         break;
     case GITS_CWRITER + 4:
         s->cwriter = deposit64(s->cwriter, 32, 32, value);
@@ -416,6 +760,9 @@  static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
         break;
     case GITS_CWRITER:
         s->cwriter = value;
+        if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s->creadr)) {
+            result = process_cmdq(s);
+        }
         break;
     case GITS_TYPER:
     case GITS_CREADR:
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index b7701e8ca5..7e1ff426ef 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -253,6 +253,12 @@  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)
+
 FIELD(GITS_CTLR, ENABLED, 0, 1)
 FIELD(GITS_CTLR, QUIESCENT, 31, 1)
 
@@ -284,6 +290,41 @@  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_LEN                  16
+#define ICID_MASK                 ((1U << ICID_LEN) - 1)
+#define RDBASE_LEN                32
+#define RDBASE_OFFSET             16
+#define RDBASE_MASK               ((1ULL << RDBASE_LEN) - 1)
+#define RDBASE_PROCNUM_LEN        16
+#define RDBASE_PROCNUM_MASK       ((1ULL << RDBASE_PROCNUM_LEN) - 1)
+
+#define DEVID_OFFSET              32
+#define DEVID_LEN                 32
+#define DEVID_MASK                ((1ULL << DEVID_LEN) - 1)
+
+/* MAPD command fields */
+#define ITTADDR_LEN               44
+#define ITTADDR_OFFSET            8
+#define ITTADDR_MASK              ((1ULL << ITTADDR_LEN) - 1)
+#define SIZE_MASK                 0x1f
+
+#define VALID_SHIFT               63
+#define VALID_MASK                0x1
 
 /**
  * Default features advertised by this version of ITS