Re: [PATCH v2 6/8] hw/intc: GICv3 redistributor ITS processing

[shashi.mallela@linaro.org]

On Thu, 2021-04-29 at 18:16 -0400, shashi.mallela@linaro.org wrote:
On Mon, 2021-04-19 at 13:44 +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 03:41, Shashi Mallela <
> shashi.mallela@linaro.org> wrote:
> > Implemented lpi processing at redistributor to get lpi config info
> > from lpi configuration table,determine priority,set pending state
> > in
> > lpi pending table and forward the lpi to cpuif.Added logic to
> > invoke
> > redistributor lpi processing with translated LPI which set/clear
> > LPI
> > from ITS device as part of ITS INT,CLEAR,DISCARD command and
> > GITS_TRANSLATER processing.
> 
> Nit: commas should all have a space following.
> 
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > ---
> >  hw/intc/arm_gicv3.c        |   6 ++
> >  hw/intc/arm_gicv3_cpuif.c  |  15 +++--
> >  hw/intc/arm_gicv3_its.c    |   9 ++-
> >  hw/intc/arm_gicv3_redist.c | 124
> > +++++++++++++++++++++++++++++++++++++
> >  hw/intc/gicv3_internal.h   |   9 +++
> >  5 files changed, 158 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> > index 66eaa97198..618fa1af95 100644
> > --- a/hw/intc/arm_gicv3.c
> > +++ b/hw/intc/arm_gicv3.c
> > @@ -166,6 +166,12 @@ static void
> > gicv3_redist_update_noirqset(GICv3CPUState *cs)
> >          cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq);
> >      }
> > 
> > +    if (cs->gic->lpi_enable) {
> > +        if (gicv3_redist_update_lpi(cs)) {
> > +            seenbetter = true;
> > +        }
> > +    }
> > +
> >      /* If the best interrupt we just found would preempt whatever
> >       * was the previous best interrupt before this update, then
> >       * we know it's definitely the best one now.
> > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> > index 43ef1d7a84..c225b80f66 100644
> > --- a/hw/intc/arm_gicv3_cpuif.c
> > +++ b/hw/intc/arm_gicv3_cpuif.c
> > @@ -899,9 +899,14 @@ static void icc_activate_irq(GICv3CPUState
> > *cs, int irq)
> >          cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0);
> >          gicv3_redist_update(cs);
> >      } else {
> > -        gicv3_gicd_active_set(cs->gic, irq);
> > -        gicv3_gicd_pending_clear(cs->gic, irq);
> > -        gicv3_update(cs->gic, irq, 1);
> > +        if (irq >= GICV3_LPI_INTID_START) {
> > +            gicv3_redist_lpi_pending(cs, irq, 0);
> > +            gicv3_redist_update(cs);
> > +        } else {
> > +            gicv3_gicd_active_set(cs->gic, irq);
> > +            gicv3_gicd_pending_clear(cs->gic, irq);
> > +            gicv3_update(cs->gic, irq, 1);
> > +        }
> >      }
> >  }
> > 
> > @@ -1337,7 +1342,9 @@ static void icc_eoir_write(CPUARMState *env,
> > const ARMCPRegInfo *ri,
> >           * valid interrupt value read from the Interrupt
> > Acknowledge
> >           * register" and so this is UNPREDICTABLE. We choose to
> > ignore it.
> >           */
> > -        return;
> > +        if (!(cs->gic->lpi_enable && (irq >=
> > GICV3_LPI_INTID_START))) {
> > +            return;
> > +        }
> 
> This condition is in the wrong place. I think what you are trying to
> do is modify "what is the set of numbers we consider to be valid IRQ
> numbers", in which case you want to be changing the outer
> "if (irq >= cs->gic->num_irq)" condition, not adding an extra one
> inside it.
> 
> More importantly, the thing this condition is guarding is that
> the code below it assumes that IRQ numbers are in range for the
> GIC's own internal non-LPI interrupts. If you allow numbers
> > = cs->gic->num_irq through, then you will get array overruns
> when icc_deactivate_irq() tries to clear its Active bit.
> 
> >      }
> > 
> >      if (icc_highest_active_group(cs) != grp) {
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 0e3f176809..41e1e8b2a8 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -226,6 +226,7 @@ static MemTxResult process_int(GICv3ITSState
> > *s, uint64_t value,
> >      bool ite_valid = false;
> >      uint64_t cte = 0;
> >      bool cte_valid = false;
> > +    uint64_t rdbase;
> >      uint8_t buff[ITS_ITT_ENTRY_SIZE];
> >      uint64_t itt_addr;
> > 
> > @@ -278,12 +279,18 @@ static MemTxResult process_int(GICv3ITSState
> > *s, uint64_t value,
> >               * since with a physical address the target address
> > must be
> >               * 64KB aligned
> >               */
> > -
> > +            rdbase = (cte >> 1U) & RDBASE_MASK;
> 
> What's going on here? Shouldn't this be in a previous patch ?
> > 
> No,this part relates to the ITS processing wherein the rdbase is
extracted and used for subsequent redistributor trigger
> > >              /*
> >               * Current implementation only supports rdbase ==
> > procnum
> >               * Hence rdbase physical address is ignored
> >               */
> >          } else {
> > +            rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;
> > +            if ((cmd == CLEAR) || (cmd == DISCARD)) {
> > +                gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase],
> > pIntid, 0);
> > +            } else {
> > +                gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase],
> > pIntid, 1);
> > +            }
> > 
> >              if (cmd == DISCARD) {
> >                  /* remove mapping from interrupt translation table
> > */
> > diff --git a/hw/intc/arm_gicv3_redist.c
> > b/hw/intc/arm_gicv3_redist.c
> > index 325b974e70..71c648a616 100644
> > --- a/hw/intc/arm_gicv3_redist.c
> > +++ b/hw/intc/arm_gicv3_redist.c
> > @@ -254,6 +254,8 @@ static MemTxResult gicr_writel(GICv3CPUState
> > *cs, hwaddr offset,
> >          if (cs->gicr_typer & GICR_TYPER_PLPIS) {
> >              if (value & GICR_CTLR_ENABLE_LPIS) {
> >                  cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
> > +                /* Check for any pending interr in pending table
> > */
> > +                gicv3_redist_update(cs);
> >              } else {
> >                  cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
> >              }
> > @@ -548,6 +550,128 @@ MemTxResult gicv3_redist_write(void *opaque,
> > hwaddr offset, uint64_t data,
> >      return r;
> >  }
> > 
> > +bool gicv3_redist_update_lpi(GICv3CPUState *cs)
> 
> Could we have a comment explaining what this function does, please ?
> 
> > +{
> > +    AddressSpace *as = &cs->gic->sysmem_as;
> > +    uint64_t lpict_baddr, lpipt_baddr;
> > +    uint32_t pendt_size = 0;
> > +    uint8_t lpite;
> > +    uint8_t prio, pend;
> > +    int i;
> > +    bool seenbetter = false;
> > +
> > +    if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs-
> > >gicr_propbaser ||
> > +        !cs->gicr_pendbaser || cs->lpi_outofrange) {
> > +        return seenbetter;
> > +    }
> > +
> > +    lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> > PHYADDR);
> > +    lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> > +
> > +    lpipt_baddr =  FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER,
> > PHYADDR);
> > +    lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
> > +
> > +    /* Determine the highest priority pending interrupt among LPIs
> > */
> > +    pendt_size = (1UL << (FIELD_EX64(cs->gicr_propbaser,
> > GICR_PROPBASER,
> > +                          IDBITS) - 1));
> 
> This is where you should be handling "actually the table is empty"
> or "table size is capped at the implementation's maximum size".
> 
> > +
> > +    for (i = 0; i < pendt_size; i++) {
> > +        address_space_read(as, lpipt_baddr +
> > +                (((GICV3_LPI_INTID_START + i) / 8) *
> > sizeof(pend)),
> > +                MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> > +
> > +        if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) {
> > +            address_space_read(as, lpict_baddr + (i *
> > sizeof(lpite)),
> > +                      MEMTXATTRS_UNSPECIFIED, &lpite,
> > sizeof(lpite));
> > +
> > +            prio = ((lpite >> LPI_CTE_PRIORITY_OFFSET) &
> > +                     LPI_CTE_PRIORITY_MASK);
> > +            prio &= LPI_PRIORITY_MASK;
> > +
> > +            if (prio < cs->hppi.prio) {
> > +                cs->hppi.irq = GICV3_LPI_INTID_START + i;
> > +                cs->hppi.prio = prio;
> > +                /* LPIs are always non-secure Grp1 interrupts */
> > +                cs->hppi.grp = GICV3_G1NS;
> > +                seenbetter = true;
> > +            }
> > +        }
> > +    }
> 
> Oof. How big is an LPI configuration table, typically? I'm not sure
> we want
> to be scanning the entire LPI configuration table in guest memory for
> every
> time gicv3_redist_update() is called if we can avoid it.
> 
> A bit of benchmarking of how much this slows down emulation might
> be interesting.
> 
> The implementation scans the full LPI pending table,but only scans
> the LPI config table entry for which the pending bit is set on
every 
> gicv3_redist_update.
> With preliminary testing on linux guest os and kvm-unit-tests didnt
observe any noticeable delay during the ITS processing
 
> > +    return seenbetter;
> > +}
> > +
> > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int
> > level)
> > +{
> > +    AddressSpace *as = &cs->gic->sysmem_as;
> > +    uint64_t lpipt_baddr;
> > +    bool ispend = false;
> > +    uint8_t pend;
> > +
> > +    /*
> > +     * get the bit value corresponding to this irq in the
> > +     * lpi pending table
> > +     */
> > +    lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER,
> > PHYADDR);
> > +    lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
> > +
> > +    address_space_read(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > +                         MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > +    ispend = ((pend >> (irq % 8)) & 0x1);
> > +
> > +    if (ispend) {
> > +        if (!level) {
> > +            /*
> > +             * clear the pending bit and update the lpi pending
> > table
> > +             */
> > +            pend &= ~(1 << (irq % 8));
> > +
> > +            address_space_write(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > +                                 MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > +        }
> > +    } else {
> > +        if (level) {
> > +            /*
> > +             * if pending bit is not already set for this
> > irq,turn-on the
> > +             * pending bit and update the lpi pending table
> > +             */
> > +            pend |= (1 << (irq % 8));
> > +
> > +            address_space_write(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > +                                 MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > +        }
> > +    }
> > +}
> > 
> > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int
> > level)
> > +{
> > +    AddressSpace *as = &cs->gic->sysmem_as;
> > +    uint64_t lpict_baddr;
> > +    uint8_t lpite;
> > +
> > +    if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs-
> > >gicr_propbaser ||
> > +         !cs->gicr_pendbaser || cs->lpi_outofrange) {
> > +        return;
> > +    }
> > +
> > +    lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> > PHYADDR);
> > +    lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> > +
> > +    /* get the lpi config table entry corresponding to this irq */
> > +    address_space_read(as, lpict_baddr + ((irq -
> > GICV3_LPI_INTID_START) *
> > +                        sizeof(lpite)), MEMTXATTRS_UNSPECIFIED,
> > +                        &lpite, sizeof(lpite));
> > +
> > +    /* check if this irq is enabled before proceeding further */
> > +    if (!(lpite & LPI_CTE_ENABLED)) {
> > +        return;
> > +    }
> > +
> > +    /* set/clear the pending bit for this irq */
> > +    gicv3_redist_lpi_pending(cs, irq, level);
> > +
> > +    gicv3_redist_update(cs);
> > +}
> > +
> >  void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)
> >  {
> >      /* Update redistributor state for a change in an external PPI
> > input line */
> > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> > index a2718704d4..4c97c22850 100644
> > --- a/hw/intc/gicv3_internal.h
> > +++ b/hw/intc/gicv3_internal.h
> > @@ -306,6 +306,12 @@ FIELD(GITS_TYPER, CIL, 36, 1)
> > 
> >  #define L1TABLE_ENTRY_SIZE         8
> > 
> > +#define LPI_CTE_ENABLE_OFFSET      0
> > +#define LPI_CTE_ENABLED          VALID_MASK
> > +#define LPI_CTE_PRIORITY_OFFSET    2
> > +#define LPI_CTE_PRIORITY_MASK     ((1U << 6) - 1)
> > +#define LPI_PRIORITY_MASK         0xfc
> > +
> >  #define GITS_CMDQ_ENTRY_SIZE               32
> >  #define NUM_BYTES_IN_DW                     8
> > 
> > @@ -444,6 +450,9 @@ MemTxResult gicv3_redist_write(void *opaque,
> > hwaddr offset, uint64_t data,
> >                                 unsigned size, MemTxAttrs attrs);
> >  void gicv3_dist_set_irq(GICv3State *s, int irq, int level);
> >  void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level);
> > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int
> > level);
> > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int
> > level);
> > +bool gicv3_redist_update_lpi(GICv3CPUState *cs);
> >  void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq,
> > bool ns);
> >  void gicv3_init_cpuif(GICv3State *s);
> 
> thanks
> -- PMM