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.mall...@linaro.org>
> > Reviewed-by: Peter Maydell <peter.mayd...@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
