Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework

[Eric Auger]

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