Re: [PATCH v7 04/10] hw/intc: GICv3 ITS Command processing

[shashi.mallela@linaro.org]

Thanks for sharing the observations Neil.
Will take care of it in the next patch-set.

On Fri, 2021-08-06 at 10:58 +0200, Neil Armstrong wrote:
> Hi,
> 
> On 06/08/2021 00:29, Shashi Mallela wrote:
> > Added ITS command queue handling for MAPTI,MAPI commands,handled
> > ITS
> > translation which triggers an LPI via INT command as well as write
> > to GITS_TRANSLATER register,defined enum to differentiate between
> > ITS
> > command interrupt trigger and GITS_TRANSLATER based interrupt
> > trigger.
> > Each of these commands make use of other functionalities
> > implemented to
> > get device table entry,collection table entry or interrupt
> > translation
> > table entry required for their processing.
> > 
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/intc/arm_gicv3_its.c            | 348
> > +++++++++++++++++++++++++++++
> >  hw/intc/gicv3_internal.h           |  12 +
> >  include/hw/intc/arm_gicv3_common.h |   2 +
> >  3 files changed, 362 insertions(+)
> > 
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 8bdbebbeca..35308f1c32 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -29,6 +29,22 @@ struct GICv3ITSClass {
> >      void (*parent_reset)(DeviceState *dev);
> >  };
> >  
> > +/*
> > + * This is an internal enum used to distinguish between LPI
> > triggered
> > + * via command queue and LPI triggered via gits_translater write.
> > + */
> > +typedef enum ItsCmdType {
> > +    NONE = 0, /* internal indication for GITS_TRANSLATER write */
> > +    CLEAR = 1,
> > +    DISCARD = 2,
> > +    INT = 3,
> > +} ItsCmdType;
> > +
> > +typedef struct {
> > +    uint32_t iteh;
> > +    uint64_t itel;
> > +} IteEntry;
> > +
> >  static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz)
> >  {
> >      uint64_t result = 0;
> > @@ -50,6 +66,320 @@ static uint64_t baser_base_addr(uint64_t value,
> > uint32_t page_sz)
> >      return result;
> >  }
> >  
> > +static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t
> > *cte,
> > +                    MemTxResult *res)
> > +{
> > +    AddressSpace *as = &s->gicv3->dma_as;
> > +    uint64_t l2t_addr;
> > +    uint64_t value;
> > +    bool valid_l2t;
> > +    uint32_t l2t_id;
> > +    uint32_t max_l2_entries;
> > +
> > +    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) {
> > +            valid_l2t = (value & L2_TABLE_VALID_MASK) != 0;
> > +
> > +            if (valid_l2t) {
> > +                max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> > +
> > +                l2t_addr = value & ((1ULL << 51) - 1);
> > +
> > +                *cte =  address_space_ldq_le(as, l2t_addr +
> > +                                    ((icid % max_l2_entries) *
> > GITS_CTE_SIZE),
> > +                                    MEMTXATTRS_UNSPECIFIED, res);
> > +           }
> > +       }
> > +    } else {
> > +        /* Flat level table */
> > +        *cte =  address_space_ldq_le(as, s->ct.base_addr +
> > +                                     (icid * GITS_CTE_SIZE),
> > +                                      MEMTXATTRS_UNSPECIFIED,
> > res);
> > +    }
> > +
> > +    return (*cte & TABLE_ENTRY_VALID_MASK) != 0;
> > +}
> > +
> > +static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid,
> > uint64_t dte,
> > +                              IteEntry ite)
> > +{
> > +    AddressSpace *as = &s->gicv3->dma_as;
> > +    uint64_t itt_addr;
> > +    MemTxResult res = MEMTX_OK;
> > +
> > +    itt_addr = (dte & GITS_DTE_ITTADDR_MASK) >>
> > GITS_DTE_ITTADDR_SHIFT;
> > +    itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> > +
> > +    address_space_stq_le(as, itt_addr + (eventid *
> > sizeof(uint64_t)),
> > +                         ite.itel, MEMTXATTRS_UNSPECIFIED, &res);
> > +
> > +    if (res == MEMTX_OK) {
> > +        address_space_stl_le(as, itt_addr + ((eventid +
> > sizeof(uint64_t)) *
> > +                             sizeof(uint32_t)), ite.iteh,
> > +                             MEMTXATTRS_UNSPECIFIED, &res);
> 
> While adding support for ITS on Zephyr, I've spotted an issue with
> the ITE entry storage here.
> 
> From eventid 0 to 7, it goes well, but from 8 and all even eventids,
> the table gets trashed.
> 
> The actual storage is:
> 
> itel: itt_addr + (eventid * 8)
> iteh: itt_addr + ((eventid + 8) * 4)
> 
> For the first eventIDs, the offsets are:
> 
> EvenID  itel    iteh
> 0	0	32
> 1	8	36
> 2	16	40
> 3	24	44
> 4	32	48
> 5	40	52
> 6	48	56
> 7	56	60
> 8	64	64	<= the entry 8 simply disappears
> 9	72	68
> 10	80	72	<= trashed 9
> 11	88	76
> 12	96	80	<= trashes 10
> 
> and so on.
> 
> The correct storage would be:
> 
>     address_space_stq_le(as, itt_addr + (eventid * (sizeof(uint64_t)
> + sizeof(uint32_t))),
>                          itel, MEMTXATTRS_UNSPECIFIED, &res);
> 
>     if (res == MEMTX_OK) {
>         address_space_stl_le(as, itt_addr + (eventid *
> (sizeof(uint64_t) + sizeof(uint32_t))) +
> 			 sizeof(uint32_t), iteh,
> MEMTXATTRS_UNSPECIFIED,
>                              &res);
>     }
> 
> To store iteh right after itel.
> 
> > +    }
> > +   return res;
> > +}
> > +
> > +static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t
> > dte,
> > +                    uint16_t *icid, uint32_t *pIntid, MemTxResult
> > *res)
> > +{
> > +    AddressSpace *as = &s->gicv3->dma_as;
> > +    uint64_t itt_addr;
> > +    bool status = false;
> > +    IteEntry ite = {};
> > +
> > +    itt_addr = (dte & GITS_DTE_ITTADDR_MASK) >>
> > GITS_DTE_ITTADDR_SHIFT;
> > +    itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> > +
> > +    ite.itel = address_space_ldq_le(as, itt_addr +
> > +                                    (eventid * sizeof(uint64_t)),
> > +                                    MEMTXATTRS_UNSPECIFIED, res);
> > +
> > +    if (*res == MEMTX_OK) {
> > +        ite.iteh = address_space_ldl_le(as, itt_addr + ((eventid +
> > +                                    sizeof(uint64_t)) *
> > sizeof(uint32_t)),
> > +                                    MEMTXATTRS_UNSPECIFIED, res);
> 
> Same here:
>     itel = address_space_ldq_le(as, itt_addr + (eventid *
> (sizeof(uint64_t) + sizeof(uint32_t))),
>                                 MEMTXATTRS_UNSPECIFIED, res);
> 
>     if (*res == MEMTX_OK) {
>         iteh = address_space_ldl_le(as, itt_addr + (eventid *
> (sizeof(uint64_t) + sizeof(uint32_t))) + sizeof(uint32_t),
>                                     MEMTXATTRS_UNSPECIFIED, res);
> 
> > +
> > +        if (*res == MEMTX_OK) {
> > +            if (ite.itel & TABLE_ENTRY_VALID_MASK) {
> > +                if ((ite.itel >> ITE_ENTRY_INTTYPE_SHIFT) &
> > +                    GITS_TYPE_PHYSICAL) {
> > +                    *pIntid = (ite.itel & ITE_ENTRY_INTID_MASK) >>
> > +                               ITE_ENTRY_INTID_SHIFT;
> > +                    *icid = ite.iteh & ITE_ENTRY_ICID_MASK;
> > +                    status = true;
> > +                }
> > +            }
> > +        }
> > +    }
> > +    return status;
> > +}
> > +
> 
> This fixes the issue I see when mapping multiple eventIDs, then
> trying to exercise them with the INT command.
> 
> Thanks,
> Neil