| Message ID | 20210602180042.111347-7-shashi.mallela@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | GICv3 LPI and ITS feature implementation | expand |
On Wed, 2 Jun 2021 at 19:00, 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. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/intc/arm_gicv3.c | 9 ++ > hw/intc/arm_gicv3_common.c | 1 + > hw/intc/arm_gicv3_cpuif.c | 7 +- > hw/intc/arm_gicv3_its.c | 14 ++- > hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++ > hw/intc/gicv3_internal.h | 10 ++ > include/hw/intc/arm_gicv3_common.h | 10 ++ > 7 files changed, 190 insertions(+), 6 deletions(-) The code for finding/updating the best pending LPI looks a lot better in this version -- thanks for working through that. An important thing which I hadn't realized previously: the hpplpi information counts as information cached from the LPI configuration tables (because it is based on the priority and enable-bit information from those tables). That means that when the guest sends the ITS INV or INVALL command we need to throw it away and recalculate by calling gicv3_redist_update_lpi(). (The idea here is that the guest can validly raise the priority of an interrupt by the sequence "write to table; INVALL; SYNC", and we need to correctly figure out that that might mean that that LPI is now the interrupt we should be taking.) > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index d63f8af604..4d19190b9c 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -165,6 +165,15 @@ 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 && cs->lpivalid) { You don't need a separate lpivalid flag -- you can use hpplpi.prio == 0xff as your "no pending LPI" indication. This is how the existing cs->hppi works. (irqbetter() will always return false if passed an 0xff priority, so you don't need to special case check anything here.) > + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { > + cs->hppi.irq = cs->hpplpi.irq; > + cs->hppi.prio = cs->hpplpi.prio; > + cs->hppi.grp = cs->hpplpi.grp; > + 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_common.c b/hw/intc/arm_gicv3_common.c > index 53dea2a775..223db16fec 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev) > memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr)); > > cs->hppi.prio = 0xff; > + cs->hpplpi.prio = 0xff; > > /* State in the CPU interface must *not* be reset here, because it > * is part of the CPU's reset domain, not the GIC device's. > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..5be3efaa3f 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > gicv3_redist_update(cs); > - } else { > + } else if (irq < GICV3_LPI_INTID_START) { > gicv3_gicd_active_set(cs->gic, irq); > gicv3_gicd_pending_clear(cs->gic, irq); > gicv3_update(cs->gic, irq, 1); > + } else { > + gicv3_redist_lpi_pending(cs, irq, 0); > } > } > > @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, > gicv3_redist_affid(cs), value); > > - if (irq >= cs->gic->num_irq) { > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && > + (irq >= GICV3_LPI_INTID_START)))) { Please put the line break after the first &&, not the second. That means that you avoid linebreaking in the middle of a () expression. Also you don't need the () on the outside of the !. > /* This handles two cases: > * 1. If software writes the ID of a spurious interrupt [ie 1020-1023] > * to the GICC_EOIR, the GIC ignores that write. > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 0a978cf55b..e0fbd4041f 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -211,6 +211,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; > uint64_t itel = 0; > uint32_t iteh = 0; > > @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, > * command in the queue > */ > } else { > - /* > - * Current implementation only supports rdbase == procnum > - * Hence rdbase physical address is ignored > - */ > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; > + assert(rdbase <= s->gicv3->num_cpu); We just read cte from guest memory. We mustn't allow guests to trigger assert()s in QEMU, so if the value is out of range then we need to handle it by treating the command as invalid, not by crashing. Also, your bounds-check is off by one; it should be "<", not "<=". > + > + 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 */ > res = update_ite(s, eventid, dte, itel, iteh); > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index fb9a4ee3cc..bfc6e4e9b9 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -255,6 +255,11 @@ 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 */ > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); > + gicv3_redist_update(cs); > } else { > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > } > @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > return r; > } > > +void gicv3_redist_update_lpi(GICv3CPUState *cs) > +{ > + /* > + * This function scans the LPI pending table and for each pending > + * LPI, reads the corresponding entry from LPI configuration table > + * to extract the priority info and determine if the LPI priority > + * is lower than the current high priority interrupt.If yes, update Missing space after ".". > + * high priority pending interrupt to that of LPI. > + */ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr, lpipt_baddr; > + uint32_t pendt_size = 0; > + uint8_t lpite; > + uint8_t prio, pend; > + int i; > + uint64_t idbits; You should set hpplpi.prio = 0xff; here, so you don't need to do it at every callsite. That is, what you're really doing in this function is "recalculate the hpplpi information from scratch". > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the set of missing brackets that clang complains about: it should be "!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)" because ! has higher priority than &. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + /* Determine the highest priority pending interrupt among LPIs */ > + pendt_size = (1ULL << (idbits + 1)); > + > + for (i = 0; i < pendt_size / 8; 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) { Better written as "if (the pend bit is not set) continue;" > + address_space_read(as, lpict_baddr + (i * sizeof(lpite)), > + MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite)); > + > + if (!(lpite & LPI_CTE_ENABLED)) { > + continue; > + } > + > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > + prio = lpite & LPI_PRIORITY_MASK; > + } else { > + prio = lpite & LPI_SPRIORITY_MASK; This isn't the right calculation. When reading a priority value when GICD_CTLR.DS is zero, you need to shift it right by one and set bit 7: prio = ((lpite & LPI_PRIORITY_MASK) >> 1) & 0x80; > + } > + > + if (prio <= cs->hpplpi.prio) { > + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; > + cs->hpplpi.prio = prio; > + /* LPIs are always non-secure Grp1 interrupts */ > + cs->hpplpi.grp = GICV3_G1NS; > + cs->lpivalid = true; > + } > + } > + } > +} > + > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_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 = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + 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)); > + } > + } You can simplify this code a bit: address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); ispend = extract32(pend, irq % 8, 1); if (ispend == level) { return; } pend = deposit32(pend, irq % 8, 1, level ? 1 : 0); address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); You can avoid doing a full update a lot of the time: * if this LPI is worse than the current value in hpplpi (where by "worse" I mean lower-priority by the same kind of comparison irqbetter() does) then we haven't changed the best-available pending LPI, so we don't need to do an update * if we set the pending bit to 1 and the LPI is enabled and the priority of this LPI is better than the current hpplpi, then we know this LPI is now the best, so we can just set hpplpi.prio and .irq without doing a full rescan * if we didn't actually change the value of the pending bit, we don't need to do an update (you get this for free if you take the simplification suggestion I make above, which does an early-return in the "no change" case) > +} > + > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr; > + uint8_t lpite; > + uint64_t idbits; > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the other set of missing brackets that clang complains about. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) || > + (irq > (1ULL << (idbits + 1)))) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + /* 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; > + } I don't think you need to make this check -- you can just set/clear the pending status of the LPI. If the LPI is not enabled then it will be ignored by gicv3_redist_update_lpi(). This is how non-LPI interrupts work and I think that LPIs behave the same way. (But it's a big spec, so I might have missed something -- if I'm wrong, please say so.) > + > + /* 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 91dbe01176..bcbccba573 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -308,6 +308,13 @@ 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 LPI_SPRIORITY_MASK 0x7e > + > #define GITS_CMDQ_ENTRY_SIZE 32 > #define NUM_BYTES_IN_DW 8 > > @@ -452,6 +459,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); > +void 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); > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index c1348cc60a..5d839da9c9 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -204,6 +204,16 @@ struct GICv3CPUState { > * real state above; it doesn't need to be migrated. > */ > PendingIrq hppi; > + > + /* > + * Current highest priority pending lpi for this CPU. > + * This is cached information that can be recalculated from the > + * real state above; it doesn't need to be migrated. This comment is true for hppi, but not for hpplpi. For hpplpi it is "cached information that can be recalculated from the LPI tables in guest memory". This means that we need either to: (1) call gicv3_redist_update_lpi() in an appropriate post-load function so that the field gets re-calculated on the destination end of a migration (2) migrate the hpplpi fields Option 1 is what we do for hppi: arm_gicv3_post_load() calls gicv3_full_update_noirqset(), which does a full recalculation of the GIC state. Calling gicv3_redist_update_lpi() in arm_gicv3_post_load() before it calls gicv3_full_update_noirqset() is probably the best thing. > + */ > + PendingIrq hpplpi; > + > + bool lpivalid; /* current highest priority lpi validity status */ > + > /* This is temporary working state, to avoid a malloc in gicv3_update() */ > bool seenbetter; > }; > -- > 2.27.0 > thanks -- PMM
Have addressed all comments except the ones with responses(inline) below:- On Jun 8 2021, at 9:57 am, Peter Maydell <peter.maydell@linaro.org> wrote: > On Wed, 2 Jun 2021 at 19:00, 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. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/intc/arm_gicv3.c | 9 ++ > > hw/intc/arm_gicv3_common.c | 1 + > > hw/intc/arm_gicv3_cpuif.c | 7 +- > > hw/intc/arm_gicv3_its.c | 14 ++- > > hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++ > > hw/intc/gicv3_internal.h | 10 ++ > > include/hw/intc/arm_gicv3_common.h | 10 ++ > > 7 files changed, 190 insertions(+), 6 deletions(-) > > The code for finding/updating the best pending LPI looks a lot > better in this version -- thanks for working through that. > > An important thing which I hadn't realized previously: > the hpplpi information counts as information cached from the > LPI configuration tables (because it is based on the priority > and enable-bit information from those tables). That means that when > the guest sends the ITS INV or INVALL command we need to throw it > away and recalculate by calling gicv3_redist_update_lpi(). > (The idea here is that the guest can validly raise the priority > of an interrupt by the sequence "write to table; INVALL; SYNC", > and we need to correctly figure out that that might mean that > that LPI is now the interrupt we should be taking.) > > Agreed,will be implementing the INV/INVALL command processing in addition to existing ITS commands > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index d63f8af604..4d19190b9c 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -165,6 +165,15 @@ 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 && cs->lpivalid) { You don't need a separate lpivalid flag -- you can use hpplpi.prio == 0xff as your "no pending LPI" indication. This is how the existing cs->hppi works. (irqbetter() will always return false if passed an 0xff priority, so you don't need to special case check anything here.) > + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { > + cs->hppi.irq = cs->hpplpi.irq; > + cs->hppi.prio = cs->hpplpi.prio; > + cs->hppi.grp = cs->hpplpi.grp; > + 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_common.c b/hw/intc/arm_gicv3_common.c > index 53dea2a775..223db16fec 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev) > memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr)); > > cs->hppi.prio = 0xff; > + cs->hpplpi.prio = 0xff; > > /* State in the CPU interface must *not* be reset here, because it > * is part of the CPU's reset domain, not the GIC device's. > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..5be3efaa3f 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > gicv3_redist_update(cs); > - } else { > + } else if (irq < GICV3_LPI_INTID_START) { > gicv3_gicd_active_set(cs->gic, irq); > gicv3_gicd_pending_clear(cs->gic, irq); > gicv3_update(cs->gic, irq, 1); > + } else { > + gicv3_redist_lpi_pending(cs, irq, 0); > } > } > > @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, > gicv3_redist_affid(cs), value); > > - if (irq >= cs->gic->num_irq) { > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && > + (irq >= GICV3_LPI_INTID_START)))) { Please put the line break after the first &&, not the second. That means that you avoid linebreaking in the middle of a () expression. Also you don't need the () on the outside of the !. > /* This handles two cases: > * 1. If software writes the ID of a spurious interrupt [ie 1020-1023] > * to the GICC_EOIR, the GIC ignores that write. > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 0a978cf55b..e0fbd4041f 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -211,6 +211,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; > uint64_t itel = 0; > uint32_t iteh = 0; > > @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, > * command in the queue > */ > } else { > - /* > - * Current implementation only supports rdbase == procnum > - * Hence rdbase physical address is ignored > - */ > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; > + assert(rdbase <= s->gicv3->num_cpu); We just read cte from guest memory. We mustn't allow guests to trigger assert()s in QEMU, so if the value is out of range then we need to handle it by treating the command as invalid, not by crashing. Also, your bounds-check is off by one; it should be "<", not "<=". > + > + 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 */ > res = update_ite(s, eventid, dte, itel, iteh); > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index fb9a4ee3cc..bfc6e4e9b9 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -255,6 +255,11 @@ 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 */ > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); > + gicv3_redist_update(cs); > } else { > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > } > @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > return r; > } > > +void gicv3_redist_update_lpi(GICv3CPUState *cs) > +{ > + /* > + * This function scans the LPI pending table and for each pending > + * LPI, reads the corresponding entry from LPI configuration table > + * to extract the priority info and determine if the LPI priority > + * is lower than the current high priority interrupt.If yes, update Missing space after ".". > + * high priority pending interrupt to that of LPI. > + */ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr, lpipt_baddr; > + uint32_t pendt_size = 0; > + uint8_t lpite; > + uint8_t prio, pend; > + int i; > + uint64_t idbits; You should set hpplpi.prio = 0xff; here, so you don't need to do it at every callsite. That is, what you're really doing in this function is "recalculate the hpplpi information from scratch". > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the set of missing brackets that clang complains about: it should be "!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)" because ! has higher priority than &. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + /* Determine the highest priority pending interrupt among LPIs */ > + pendt_size = (1ULL << (idbits + 1)); > + > + for (i = 0; i < pendt_size / 8; 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) { Better written as "if (the pend bit is not set) continue;" > + address_space_read(as, lpict_baddr + (i * sizeof(lpite)), > + MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite)); > + > + if (!(lpite & LPI_CTE_ENABLED)) { > + continue; > + } > + > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > + prio = lpite & LPI_PRIORITY_MASK; > + } else { > + prio = lpite & LPI_SPRIORITY_MASK; This isn't the right calculation. When reading a priority value when GICD_CTLR.DS is zero, you need to shift it right by one and set bit 7: prio = ((lpite & LPI_PRIORITY_MASK) >> 1) & 0x80; > + } > + > + if (prio <= cs->hpplpi.prio) { > + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; > + cs->hpplpi.prio = prio; > + /* LPIs are always non-secure Grp1 interrupts */ > + cs->hpplpi.grp = GICV3_G1NS; > + cs->lpivalid = true; > + } > + } > + } > +} > + > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_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 = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + 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)); > + } > + } You can simplify this code a bit: address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); ispend = extract32(pend, irq % 8, 1); if (ispend == level) { return; } pend = deposit32(pend, irq % 8, 1, level ? 1 : 0); address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)), MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend)); > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); You can avoid doing a full update a lot of the time: * if this LPI is worse than the current value in hpplpi (where by "worse" I mean lower-priority by the same kind of comparison irqbetter() does) then we haven't changed the best-available pending LPI, so we don't need to do an update * if we set the pending bit to 1 and the LPI is enabled and the priority of this LPI is better than the current hpplpi, then we know this LPI is now the best, so we can just set hpplpi.prio and .irq without doing a full rescan * if we didn't actually change the value of the pending bit, we don't need to do an update (you get this for free if you take the simplification suggestion I make above, which does an early-return in the "no change" case) > Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info. Have confirmed this with the kvm-unit-tests as well,wherein the LPIs are never processed and test cases fail. > +} > + > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr; > + uint8_t lpite; > + uint64_t idbits; > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || This is the other set of missing brackets that clang complains about. > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) || > + (irq > (1ULL << (idbits + 1)))) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + /* 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; > + } I don't think you need to make this check -- you can just set/clear the pending status of the LPI. If the LPI is not enabled then it will be ignored by gicv3_redist_update_lpi(). This is how non-LPI interrupts work and I think that LPIs behave the same way. (But it's a big spec, so I might have missed something -- if I'm wrong, please say so.) > + > + /* 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 91dbe01176..bcbccba573 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -308,6 +308,13 @@ 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 LPI_SPRIORITY_MASK 0x7e > + > #define GITS_CMDQ_ENTRY_SIZE 32 > #define NUM_BYTES_IN_DW 8 > > @@ -452,6 +459,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); > +void 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); > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index c1348cc60a..5d839da9c9 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -204,6 +204,16 @@ struct GICv3CPUState { > * real state above; it doesn't need to be migrated. > */ > PendingIrq hppi; > + > + /* > + * Current highest priority pending lpi for this CPU. > + * This is cached information that can be recalculated from the > + * real state above; it doesn't need to be migrated. This comment is true for hppi, but not for hpplpi. For hpplpi it is "cached information that can be recalculated from the LPI tables in guest memory". This means that we need either to: (1) call gicv3_redist_update_lpi() in an appropriate post-load function so that the field gets re-calculated on the destination end of a migration (2) migrate the hpplpi fields Option 1 is what we do for hppi: arm_gicv3_post_load() calls gicv3_full_update_noirqset(), which does a full recalculation of the GIC state. Calling gicv3_redist_update_lpi() in arm_gicv3_post_load() before it calls gicv3_full_update_noirqset() is probably the best thing. > + */ > + PendingIrq hpplpi; > + > + bool lpivalid; /* current highest priority lpi validity status */ > + > /* This is temporary working state, to avoid a malloc in gicv3_update() */ > bool seenbetter; > }; > -- > 2.27.0 > thanks -- PMM <div>Have addressed all comments except the ones with responses(inline) below<span data-emoji-typing="true">:-</span></div><br><div class="gmail_quote_attribution">On Jun 8 2021, at 9:57 am, Peter Maydell <peter.maydell@linaro.org> wrote:</div><blockquote><div><div>On Wed, 2 Jun 2021 at 19:00, Shashi Mallela <shashi.mallela@linaro.org> wrote:</div><div>></div><div>> Implemented lpi processing at redistributor to get lpi config info</div><div>> from lpi configuration table,determine priority,set pending state in</div><div>> lpi pending table and forward the lpi to cpuif.Added logic to invoke</div><div>> redistributor lpi processing with translated LPI which set/clear LPI</div><div>> from ITS device as part of ITS INT,CLEAR,DISCARD command and</div><div>> GITS_TRANSLATER processing.</div><div>></div><div>> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org></div><div>> ---</div><div>> hw/intc/arm_gicv3.c | 9 ++</div><div>> hw/intc/arm_gicv3_common.c | 1 +</div><div>> hw/intc/arm_gicv3_cpuif.c | 7 +-</div><div>> hw/intc/arm_gicv3_its.c | 14 ++-</div><div>> hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++</div><div>> hw/intc/gicv3_internal.h | 10 ++</div><div>> include/hw/intc/arm_gicv3_common.h | 10 ++</div><div>> 7 files changed, 190 insertions(+), 6 deletions(-)</div><br><div>The code for finding/updating the best pending LPI looks a lot</div><div>better in this version -- thanks for working through that.</div><br><div>An important thing which I hadn't realized previously:</div><div>the hpplpi information counts as information cached from the</div><div>LPI configuration tables (because it is based on the priority</div><div>and enable-bit information from those tables). That means that when</div><div>the guest sends the ITS INV or INVALL command we need to throw it</div><div>away and recalculate by calling gicv3_redist_update_lpi().</div><div>(The idea here is that the guest can validly raise the priority</div><div>of an interrupt by the sequence "write to table; INVALL; SYNC",</div><div>and we need to correctly figure out that that might mean that</div><div>that LPI is now the interrupt we should be taking.)</div></div></blockquote><div><div> > Agreed,will be implementing the INV/INVALL command processing in addition to existing ITS commands</div><br><div>> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c</div><div>> index d63f8af604..4d19190b9c 100644</div><div>> --- a/hw/intc/arm_gicv3.c</div><div>> +++ b/hw/intc/arm_gicv3.c</div><div>> @@ -165,6 +165,15 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)</div><div>> cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq);</div><div>> }</div><div>></div><div>> + if (cs->gic->lpi_enable && cs->lpivalid) {</div><br><div>You don't need a separate lpivalid flag -- you can use</div><div>hpplpi.prio == 0xff as your "no pending LPI" indication.</div><div>This is how the existing cs->hppi works.</div><div>(irqbetter() will always return false if passed an 0xff priority,</div><div>so you don't need to special case check anything here.)</div><br><div>> + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) {</div><div>> + cs->hppi.irq = cs->hpplpi.irq;</div><div>> + cs->hppi.prio = cs->hpplpi.prio;</div><div>> + cs->hppi.grp = cs->hpplpi.grp;</div><div>> + seenbetter = true;</div><div>> + }</div><div>> + }</div><div>> +</div><div>> /* If the best interrupt we just found would preempt whatever</div><div>> * was the previous best interrupt before this update, then</div><div>> * we know it's definitely the best one now.</div><div>> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c</div><div>> index 53dea2a775..223db16fec 100644</div><div>> --- a/hw/intc/arm_gicv3_common.c</div><div>> +++ b/hw/intc/arm_gicv3_common.c</div><div>> @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev)</div><div>> memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr));</div><div>></div><div>> cs->hppi.prio = 0xff;</div><div>> + cs->hpplpi.prio = 0xff;</div><div>></div><div>> /* State in the CPU interface must *not* be reset here, because it</div><div>> * is part of the CPU's reset domain, not the GIC device's.</div><div>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c</div><div>> index 81f94c7f4a..5be3efaa3f 100644</div><div>> --- a/hw/intc/arm_gicv3_cpuif.c</div><div>> +++ b/hw/intc/arm_gicv3_cpuif.c</div><div>> @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq)</div><div>> cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1);</div><div>> cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0);</div><div>> gicv3_redist_update(cs);</div><div>> - } else {</div><div>> + } else if (irq < GICV3_LPI_INTID_START) {</div><div>> gicv3_gicd_active_set(cs->gic, irq);</div><div>> gicv3_gicd_pending_clear(cs->gic, irq);</div><div>> gicv3_update(cs->gic, irq, 1);</div><div>> + } else {</div><div>> + gicv3_redist_lpi_pending(cs, irq, 0);</div><div>> }</div><div>> }</div><div>></div><div>> @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,</div><div>> trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1,</div><div>> gicv3_redist_affid(cs), value);</div><div>></div><div>> - if (irq >= cs->gic->num_irq) {</div><div>> + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable &&</div><div>> + (irq >= GICV3_LPI_INTID_START)))) {</div><br><div>Please put the line break after the first &&, not the second. That means</div><div>that you avoid linebreaking in the middle of a () expression.</div><div>Also you don't need the () on the outside of the !.</div><br><div>> /* This handles two cases:</div><div>> * 1. If software writes the ID of a spurious interrupt [ie 1020-1023]</div><div>> * to the GICC_EOIR, the GIC ignores that write.</div><div>> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c</div><div>> index 0a978cf55b..e0fbd4041f 100644</div><div>> --- a/hw/intc/arm_gicv3_its.c</div><div>> +++ b/hw/intc/arm_gicv3_its.c</div><div>> @@ -211,6 +211,7 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,</div><div>> bool ite_valid = false;</div><div>> uint64_t cte = 0;</div><div>> bool cte_valid = false;</div><div>> + uint64_t rdbase;</div><div>> uint64_t itel = 0;</div><div>> uint32_t iteh = 0;</div><div>></div><div>> @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,</div><div>> * command in the queue</div><div>> */</div><div>> } else {</div><div>> - /*</div><div>> - * Current implementation only supports rdbase == procnum</div><div>> - * Hence rdbase physical address is ignored</div><div>> - */</div><div>> + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;</div><div>> + assert(rdbase <= s->gicv3->num_cpu);</div><br><div>We just read cte from guest memory. We mustn't allow guests to</div><div>trigger assert()s in QEMU, so if the value is out of range then</div><div>we need to handle it by treating the command as invalid, not by crashing.</div><br><div>Also, your bounds-check is off by one; it should be "<", not "<=".</div><br><div>> +</div><div>> + if ((cmd == CLEAR) || (cmd == DISCARD)) {</div><div>> + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);</div><div>> + } else {</div><div>> + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);</div><div>> + }</div><div>> +</div><div>> if (cmd == DISCARD) {</div><div>> /* remove mapping from interrupt translation table */</div><div>> res = update_ite(s, eventid, dte, itel, iteh);</div><div>> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c</div><div>> index fb9a4ee3cc..bfc6e4e9b9 100644</div><div>> --- a/hw/intc/arm_gicv3_redist.c</div><div>> +++ b/hw/intc/arm_gicv3_redist.c</div><div>> @@ -255,6 +255,11 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,</div><div>> if (cs->gicr_typer & GICR_TYPER_PLPIS) {</div><div>> if (value & GICR_CTLR_ENABLE_LPIS) {</div><div>> cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;</div><div>> + /* Check for any pending interr in pending table */</div><div>> + cs->lpivalid = false;</div><div>> + cs->hpplpi.prio = 0xff;</div><div>> + gicv3_redist_update_lpi(cs);</div><div>> + gicv3_redist_update(cs);</div><div>> } else {</div><div>> cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;</div><div>> }</div><div>> @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,</div><div>> return r;</div><div>> }</div><div>></div><div>> +void gicv3_redist_update_lpi(GICv3CPUState *cs)</div><div>> +{</div><div>> + /*</div><div>> + * This function scans the LPI pending table and for each pending</div><div>> + * LPI, reads the corresponding entry from LPI configuration table</div><div>> + * to extract the priority info and determine if the LPI priority</div><div>> + * is lower than the current high priority interrupt.If yes, update</div><br><div>Missing space after ".".</div><br><div>> + * high priority pending interrupt to that of LPI.</div><div>> + */</div><div>> + AddressSpace *as = &cs->gic->dma_as;</div><div>> + uint64_t lpict_baddr, lpipt_baddr;</div><div>> + uint32_t pendt_size = 0;</div><div>> + uint8_t lpite;</div><div>> + uint8_t prio, pend;</div><div>> + int i;</div><div>> + uint64_t idbits;</div><br><div>You should set hpplpi.prio = 0xff; here, so you don't need to do</div><div>it at every callsite.</div><br><div>That is, what you're really doing in this function is "recalculate the</div><div>hpplpi information from scratch".</div><br><div>> +</div><div>> + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS),</div><div>> + GICD_TYPER_IDBITS);</div><div>> +</div><div>> + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||</div><br><div>This is the set of missing brackets that clang complains about: it should</div><div>be "!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)" because ! has higher priority</div><div>than &.</div><br><div>> + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) {</div><div>> + return;</div><div>> + }</div><div>> +</div><div>> + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK;</div><div>> +</div><div>> + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;</div><div>> +</div><div>> + /* Determine the highest priority pending interrupt among LPIs */</div><div>> + pendt_size = (1ULL << (idbits + 1));</div><div>> +</div><div>> + for (i = 0; i < pendt_size / 8; i++) {</div><div>> + address_space_read(as, lpipt_baddr +</div><div>> + (((GICV3_LPI_INTID_START + i) / 8) * sizeof(pend)),</div><div>> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><div>> +</div><div>> + if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) {</div><br><div>Better written as "if (the pend bit is not set) continue;"</div><br><div>> + address_space_read(as, lpict_baddr + (i * sizeof(lpite)),</div><div>> + MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite));</div><div>> +</div><div>> + if (!(lpite & LPI_CTE_ENABLED)) {</div><div>> + continue;</div><div>> + }</div><div>> +</div><div>> + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) {</div><div>> + prio = lpite & LPI_PRIORITY_MASK;</div><div>> + } else {</div><div>> + prio = lpite & LPI_SPRIORITY_MASK;</div><br><div>This isn't the right calculation. When reading a priority value</div><div>when GICD_CTLR.DS is zero, you need to shift it right by one</div><div>and set bit 7:</div><div>prio = ((lpite & LPI_PRIORITY_MASK) >> 1) & 0x80;</div><br><div>> + }</div><div>> +</div><div>> + if (prio <= cs->hpplpi.prio) {</div><div>> + cs->hpplpi.irq = GICV3_LPI_INTID_START + i;</div><div>> + cs->hpplpi.prio = prio;</div><div>> + /* LPIs are always non-secure Grp1 interrupts */</div><div>> + cs->hpplpi.grp = GICV3_G1NS;</div><div>> + cs->lpivalid = true;</div><div>> + }</div><div>> + }</div><div>> + }</div><div>> +}</div><div>> +</div><div>> +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)</div><div>> +{</div><div>> + AddressSpace *as = &cs->gic->dma_as;</div><div>> + uint64_t lpipt_baddr;</div><div>> + bool ispend = false;</div><div>> + uint8_t pend;</div><div>> +</div><div>> + /*</div><div>> + * get the bit value corresponding to this irq in the</div><div>> + * lpi pending table</div><div>> + */</div><div>> + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;</div><div>> +</div><div>> + address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),</div><div>> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><div>> + ispend = ((pend >> (irq % 8)) & 0x1);</div><div>> +</div><div>> + if (ispend) {</div><div>> + if (!level) {</div><div>> + /*</div><div>> + * clear the pending bit and update the lpi pending table</div><div>> + */</div><div>> + pend &= ~(1 << (irq % 8));</div><div>> +</div><div>> + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),</div><div>> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><div>> + }</div><div>> + } else {</div><div>> + if (level) {</div><div>> + /*</div><div>> + * if pending bit is not already set for this irq,turn-on the</div><div>> + * pending bit and update the lpi pending table</div><div>> + */</div><div>> + pend |= (1 << (irq % 8));</div><div>> +</div><div>> + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),</div><div>> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><div>> + }</div><div>> + }</div><br><div>You can simplify this code a bit:</div><br><div>address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),</div><div>MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><div>ispend = extract32(pend, irq % 8, 1);</div><div>if (ispend == level) {</div><div>return;</div><div>}</div><div>pend = deposit32(pend, irq % 8, 1, level ? 1 : 0);</div><div>address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),</div><div>MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));</div><br><br><div>> + cs->lpivalid = false;</div><div>> + cs->hpplpi.prio = 0xff;</div><div>> + gicv3_redist_update_lpi(cs);</div><br><div>You can avoid doing a full update a lot of the time:</div><div>* if this LPI is worse than the current value in hpplpi</div><div>(where by "worse" I mean lower-priority by the same kind of</div><div>comparison irqbetter() does) then we haven't changed the best-available</div><div>pending LPI, so we don't need to do an update</div><div>* if we set the pending bit to 1 and the LPI is enabled and the priority</div><div>of this LPI is better than the current hpplpi, then we know this LPI</div><div>is now the best, so we can just set hpplpi.prio and .irq without</div><div>doing a full rescan</div><div>* if we didn't actually change the value of the pending bit, we</div><div>don't need to do an update (you get this for free if you take the</div><div>simplification suggestion I make above, which does an early-return</div><div>in the "no change" case)</div><br><div>> Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because</div><div>this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info.</div><div>Have confirmed this with the kvm-unit-tests as well,wherein the LPIs are never processed and test cases fail.</div><br><div>> +}</div><div>> +</div><div>> +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)</div><div>> +{</div><div>> + AddressSpace *as = &cs->gic->dma_as;</div><div>> + uint64_t lpict_baddr;</div><div>> + uint8_t lpite;</div><div>> + uint64_t idbits;</div><div>> +</div><div>> + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS),</div><div>> + GICD_TYPER_IDBITS);</div><div>> +</div><div>> + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||</div><br><div>This is the other set of missing brackets that clang complains about.</div><br><div>> + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) ||</div><div>> + (irq > (1ULL << (idbits + 1)))) {</div><div>> + return;</div><div>> + }</div><div>> +</div><div>> + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK;</div><div>> +</div><div>> + /* get the lpi config table entry corresponding to this irq */</div><div>> + address_space_read(as, lpict_baddr + ((irq - GICV3_LPI_INTID_START) *</div><div>> + sizeof(lpite)), MEMTXATTRS_UNSPECIFIED,</div><div>> + &lpite, sizeof(lpite));</div><div>> +</div><div>> + /* check if this irq is enabled before proceeding further */</div><div>> + if (!(lpite & LPI_CTE_ENABLED)) {</div><div>> + return;</div><div>> + }</div><br><div>I don't think you need to make this check -- you can just set/clear</div><div>the pending status of the LPI. If the LPI is not enabled then it will</div><div>be ignored by gicv3_redist_update_lpi(). This is how non-LPI interrupts</div><div>work and I think that LPIs behave the same way. (But it's a big spec,</div><div>so I might have missed something -- if I'm wrong, please say so.)</div><br><div>> +</div><div>> + /* set/clear the pending bit for this irq */</div><div>> + gicv3_redist_lpi_pending(cs, irq, level);</div><div>> +</div><div>> + gicv3_redist_update(cs);</div><div>> +}</div><div>> +</div><div>> void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)</div><div>> {</div><div>> /* Update redistributor state for a change in an external PPI input line */</div><div>> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h</div><div>> index 91dbe01176..bcbccba573 100644</div><div>> --- a/hw/intc/gicv3_internal.h</div><div>> +++ b/hw/intc/gicv3_internal.h</div><div>> @@ -308,6 +308,13 @@ FIELD(GITS_TYPER, CIL, 36, 1)</div><div>></div><div>> #define L1TABLE_ENTRY_SIZE 8</div><div>></div><div>> +#define LPI_CTE_ENABLE_OFFSET 0</div><div>> +#define LPI_CTE_ENABLED VALID_MASK</div><div>> +#define LPI_CTE_PRIORITY_OFFSET 2</div><div>> +#define LPI_CTE_PRIORITY_MASK ((1U << 6) - 1)</div><div>> +#define LPI_PRIORITY_MASK 0xfc</div><div>> +#define LPI_SPRIORITY_MASK 0x7e</div><div>> +</div><div>> #define GITS_CMDQ_ENTRY_SIZE 32</div><div>> #define NUM_BYTES_IN_DW 8</div><div>></div><div>> @@ -452,6 +459,9 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,</div><div>> unsigned size, MemTxAttrs attrs);</div><div>> void gicv3_dist_set_irq(GICv3State *s, int irq, int level);</div><div>> void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level);</div><div>> +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level);</div><div>> +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level);</div><div>> +void gicv3_redist_update_lpi(GICv3CPUState *cs);</div><div>> void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns);</div><div>> void gicv3_init_cpuif(GICv3State *s);</div><div>></div><div>> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h</div><div>> index c1348cc60a..5d839da9c9 100644</div><div>> --- a/include/hw/intc/arm_gicv3_common.h</div><div>> +++ b/include/hw/intc/arm_gicv3_common.h</div><div>> @@ -204,6 +204,16 @@ struct GICv3CPUState {</div><div>> * real state above; it doesn't need to be migrated.</div><div>> */</div><div>> PendingIrq hppi;</div><div>> +</div><div>> + /*</div><div>> + * Current highest priority pending lpi for this CPU.</div><div>> + * This is cached information that can be recalculated from the</div><div>> + * real state above; it doesn't need to be migrated.</div><br><div>This comment is true for hppi, but not for hpplpi. For hpplpi</div><div>it is "cached information that can be recalculated from the LPI</div><div>tables in guest memory".</div><br><div>This means that we need either to:</div><div>(1) call gicv3_redist_update_lpi() in an appropriate post-load function</div><div>so that the field gets re-calculated on the destination end of a migration</div><div>(2) migrate the hpplpi fields</div><br><div>Option 1 is what we do for hppi: arm_gicv3_post_load() calls</div><div>gicv3_full_update_noirqset(), which does a full recalculation of the</div><div>GIC state. Calling gicv3_redist_update_lpi() in arm_gicv3_post_load()</div><div>before it calls gicv3_full_update_noirqset() is probably the best thing.</div><br><div>> + */</div><div>> + PendingIrq hpplpi;</div><div>> +</div><div>> + bool lpivalid; /* current highest priority lpi validity status */</div><div>> +</div><div>> /* This is temporary working state, to avoid a malloc in gicv3_update() */</div><div>> bool seenbetter;</div><div>> };</div><div>> --</div><div>> 2.27.0</div><div>></div><br><div>thanks</div><div>-- PMM</div></div><img class="mailspring-open" alt="Sent from Mailspring" width="0" height="0" style="border:0; width:0; height:0;" src="https://link.getmailspring.com/open/551DAA51-CB17-423D-896F-AF0443A5E7AE@getmailspring.com?me=2a4b90d6&recipient=cWVtdS1kZXZlbEBub25nbnUub3Jn">
On Fri, 11 Jun 2021 at 00:39, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > Have addressed all comments except the ones with responses(inline) below:- > > On Jun 8 2021, at 9:57 am, Peter Maydell <peter.maydell@linaro.org> wrote: > > > + cs->lpivalid = false; > > + cs->hpplpi.prio = 0xff; > > + gicv3_redist_update_lpi(cs); > > You can avoid doing a full update a lot of the time: > * if this LPI is worse than the current value in hpplpi > (where by "worse" I mean lower-priority by the same kind of > comparison irqbetter() does) then we haven't changed the best-available > pending LPI, so we don't need to do an update > * if we set the pending bit to 1 and the LPI is enabled and the priority > of this LPI is better than the current hpplpi, then we know this LPI > is now the best, so we can just set hpplpi.prio and .irq without > doing a full rescan > * if we didn't actually change the value of the pending bit, we > don't need to do an update (you get this for free if you take the > simplification suggestion I make above, which does an early-return > in the "no change" case) > > > Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because > this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info. If the LPI is lower priority (higher number) than the current hpplpi then it would not change the existing hpplpi info in a full-scan. If the LPI being activated is higher priority (lower number) than the current hpplpi then that is my point 2 above, and we set it as the hpplpi without needing the full-scan. And for the other cases (eg highest-priority LPI being deactivated) we should fall through to the default "call update_lpi" case. So I don't really understand why this wouldn't work. -- PMM
Hi Shashi, On 6/2/21 8:00 PM, Shashi Mallela wrote: > Implemented lpi processing at redistributor to get lpi config info s/Implemented/Implement here are elsewhere. > 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. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/intc/arm_gicv3.c | 9 ++ > hw/intc/arm_gicv3_common.c | 1 + > hw/intc/arm_gicv3_cpuif.c | 7 +- > hw/intc/arm_gicv3_its.c | 14 ++- > hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++ > hw/intc/gicv3_internal.h | 10 ++ > include/hw/intc/arm_gicv3_common.h | 10 ++ > 7 files changed, 190 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index d63f8af604..4d19190b9c 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -165,6 +165,15 @@ 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 && cs->lpivalid) { > + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { > + cs->hppi.irq = cs->hpplpi.irq; > + cs->hppi.prio = cs->hpplpi.prio; > + cs->hppi.grp = cs->hpplpi.grp; > + 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_common.c b/hw/intc/arm_gicv3_common.c > index 53dea2a775..223db16fec 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev) > memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr)); > > cs->hppi.prio = 0xff; > + cs->hpplpi.prio = 0xff; > > /* State in the CPU interface must *not* be reset here, because it > * is part of the CPU's reset domain, not the GIC device's. > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 81f94c7f4a..5be3efaa3f 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > gicv3_redist_update(cs); > - } else { > + } else if (irq < GICV3_LPI_INTID_START) { > gicv3_gicd_active_set(cs->gic, irq); > gicv3_gicd_pending_clear(cs->gic, irq); > gicv3_update(cs->gic, irq, 1); > + } else { > + gicv3_redist_lpi_pending(cs, irq, 0); > } > } > > @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, > trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, > gicv3_redist_affid(cs), value); > > - if (irq >= cs->gic->num_irq) { > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && > + (irq >= GICV3_LPI_INTID_START)))) { > /* This handles two cases: > * 1. If software writes the ID of a spurious interrupt [ie 1020-1023] > * to the GICC_EOIR, the GIC ignores that write. > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > index 0a978cf55b..e0fbd4041f 100644 > --- a/hw/intc/arm_gicv3_its.c > +++ b/hw/intc/arm_gicv3_its.c > @@ -211,6 +211,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; > uint64_t itel = 0; > uint32_t iteh = 0; > > @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, > * command in the queue > */ > } else { > - /* > - * Current implementation only supports rdbase == procnum > - * Hence rdbase physical address is ignored > - */ > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; > + assert(rdbase <= s->gicv3->num_cpu); > + > + 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 */ > res = update_ite(s, eventid, dte, itel, iteh); > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c > index fb9a4ee3cc..bfc6e4e9b9 100644 > --- a/hw/intc/arm_gicv3_redist.c > +++ b/hw/intc/arm_gicv3_redist.c > @@ -255,6 +255,11 @@ 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 */ > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); > + gicv3_redist_update(cs); > } else { > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > } > @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, > return r; > } > > +void gicv3_redist_update_lpi(GICv3CPUState *cs) > +{ > + /* > + * This function scans the LPI pending table and for each pending > + * LPI, reads the corresponding entry from LPI configuration table > + * to extract the priority info and determine if the LPI priority > + * is lower than the current high priority interrupt.If yes, update> + * high priority pending interrupt to that of LPI. "update high priority pending interrupt to that of LPI" may need some rewording > + */ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr, lpipt_baddr; > + uint32_t pendt_size = 0; > + uint8_t lpite; > + uint8_t prio, pend; > + int i; > + uint64_t idbits; > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + /* Determine the highest priority pending interrupt among LPIs */ > + pendt_size = (1ULL << (idbits + 1)); > + > + for (i = 0; i < pendt_size / 8; 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)); > + > + if (!(lpite & LPI_CTE_ENABLED)) { > + continue; > + } > + > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > + prio = lpite & LPI_PRIORITY_MASK; > + } else { > + prio = lpite & LPI_SPRIORITY_MASK; > + } > + > + if (prio <= cs->hpplpi.prio) { > + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; > + cs->hpplpi.prio = prio; > + /* LPIs are always non-secure Grp1 interrupts */ > + cs->hpplpi.grp = GICV3_G1NS; > + cs->lpivalid = true; > + } > + } > + } > +} > + nit: add a comment to explain what the function does, that's not straightforward gievn its name. > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_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 = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; > + > + 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)); > + } > + } > + cs->lpivalid = false; > + cs->hpplpi.prio = 0xff; > + gicv3_redist_update_lpi(cs); > +} > + > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) > +{ > + AddressSpace *as = &cs->gic->dma_as; > + uint64_t lpict_baddr; > + uint8_t lpite; > + uint64_t idbits; > + > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), > + GICD_TYPER_IDBITS); > + > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || > + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) || > + (irq > (1ULL << (idbits + 1)))) { > + return; > + } > + > + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; > + > + /* 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 91dbe01176..bcbccba573 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -308,6 +308,13 @@ 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 LPI_SPRIORITY_MASK 0x7e > + > #define GITS_CMDQ_ENTRY_SIZE 32 > #define NUM_BYTES_IN_DW 8 > > @@ -452,6 +459,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); > +void 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); > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index c1348cc60a..5d839da9c9 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -204,6 +204,16 @@ struct GICv3CPUState { > * real state above; it doesn't need to be migrated. > */ > PendingIrq hppi; > + > + /* > + * Current highest priority pending lpi for this CPU. > + * This is cached information that can be recalculated from the > + * real state above; it doesn't need to be migrated. > + */ > + PendingIrq hpplpi; > + > + bool lpivalid; /* current highest priority lpi validity status */ > + > /* This is temporary working state, to avoid a malloc in gicv3_update() */ > bool seenbetter; > };> Thanks Eric
On Jun 11 2021, at 4:30 am, Peter Maydell <peter.maydell@linaro.org> wrote: > On Fri, 11 Jun 2021 at 00:39, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > > > Have addressed all comments except the ones with responses(inline) below:- > > > > On Jun 8 2021, at 9:57 am, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > + cs->lpivalid = false; > > > + cs->hpplpi.prio = 0xff; > > > + gicv3_redist_update_lpi(cs); > > > > You can avoid doing a full update a lot of the time: > > * if this LPI is worse than the current value in hpplpi > > (where by "worse" I mean lower-priority by the same kind of > > comparison irqbetter() does) then we haven't changed the best-available > > pending LPI, so we don't need to do an update > > * if we set the pending bit to 1 and the LPI is enabled and the priority > > of this LPI is better than the current hpplpi, then we know this LPI > > is now the best, so we can just set hpplpi.prio and .irq without > > doing a full rescan > > * if we didn't actually change the value of the pending bit, we > > don't need to do an update (you get this for free if you take the > > simplification suggestion I make above, which does an early-return > > in the "no change" case) > > > > > Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because > > this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info. > > If the LPI is lower priority (higher number) than the current > hpplpi then it would not change the existing hpplpi info in > a full-scan. If the LPI being activated is higher priority > (lower number) than the current hpplpi then that is my point 2 above, > and we set it as the hpplpi without needing the full-scan. And for > the other cases (eg highest-priority LPI being deactivated) we > should fall through to the default "call update_lpi" case. > > So I don't really understand why this wouldn't work. > -- PMM Have got this working as per comments.Please ignore my last comment. <br><br><div class="gmail_quote_attribution">On Jun 11 2021, at 4:30 am, Peter Maydell <peter.maydell@linaro.org> wrote:</div><blockquote><div><div>On Fri, 11 Jun 2021 at 00:39, Shashi Mallela <shashi.mallela@linaro.org> wrote:</div><div>></div><div>> Have addressed all comments except the ones with responses(inline) below:-</div><div>></div><div>> On Jun 8 2021, at 9:57 am, Peter Maydell <peter.maydell@linaro.org> wrote:</div><div>></div><div>> > + cs->lpivalid = false;</div><div>> > + cs->hpplpi.prio = 0xff;</div><div>> > + gicv3_redist_update_lpi(cs);</div><div>></div><div>> You can avoid doing a full update a lot of the time:</div><div>> * if this LPI is worse than the current value in hpplpi</div><div>> (where by "worse" I mean lower-priority by the same kind of</div><div>> comparison irqbetter() does) then we haven't changed the best-available</div><div>> pending LPI, so we don't need to do an update</div><div>> * if we set the pending bit to 1 and the LPI is enabled and the priority</div><div>> of this LPI is better than the current hpplpi, then we know this LPI</div><div>> is now the best, so we can just set hpplpi.prio and .irq without</div><div>> doing a full rescan</div><div>> * if we didn't actually change the value of the pending bit, we</div><div>> don't need to do an update (you get this for free if you take the</div><div>> simplification suggestion I make above, which does an early-return</div><div>> in the "no change" case)</div><div>></div><div>> > Accepted the code simplification,but by not calling gicv3_redist_update_lpi(cs) here ,the scenario of an activated LPI fails because</div><div>> this LPI's priority (which could be lower than current hpplpi) is never checked/updated and gicv3_redist_update_noirqset() fails to present a valid active high priority LPI(if applicable) to the cpu,since it is always checking against a stale hpplpi info.</div><br><div>If the LPI is lower priority (higher number) than the current</div><div>hpplpi then it would not change the existing hpplpi info in</div><div>a full-scan. If the LPI being activated is higher priority</div><div>(lower number) than the current hpplpi then that is my point 2 above,</div><div>and we set it as the hpplpi without needing the full-scan. And for</div><div>the other cases (eg highest-priority LPI being deactivated) we</div><div>should fall through to the default "call update_lpi" case.</div><br><div>So I don't really understand why this wouldn't work.</div><br><div>-- PMM</div></div></blockquote><div><br><div>Have got this working as per comments.Please ignore my last comment.</div></div>
Hi Eric, Have accepted all comments with responses inline (below):- On Sun, 2021-06-13 at 18:26 +0200, Eric Auger wrote: > Hi Shashi, > > On 6/2/21 8:00 PM, Shashi Mallela wrote: > > Implemented lpi processing at redistributor to get lpi config info > s/Implemented/Implement here are elsewhere. > > 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. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/intc/arm_gicv3.c | 9 ++ > > hw/intc/arm_gicv3_common.c | 1 + > > hw/intc/arm_gicv3_cpuif.c | 7 +- > > hw/intc/arm_gicv3_its.c | 14 ++- > > hw/intc/arm_gicv3_redist.c | 145 > > +++++++++++++++++++++++++++++ > > hw/intc/gicv3_internal.h | 10 ++ > > include/hw/intc/arm_gicv3_common.h | 10 ++ > > 7 files changed, 190 insertions(+), 6 deletions(-) > > > > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > > index d63f8af604..4d19190b9c 100644 > > --- a/hw/intc/arm_gicv3.c > > +++ b/hw/intc/arm_gicv3.c > > @@ -165,6 +165,15 @@ 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 && cs->lpivalid) { > > + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { > > + cs->hppi.irq = cs->hpplpi.irq; > > + cs->hppi.prio = cs->hpplpi.prio; > > + cs->hppi.grp = cs->hpplpi.grp; > > + 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_common.c > > b/hw/intc/arm_gicv3_common.c > > index 53dea2a775..223db16fec 100644 > > --- a/hw/intc/arm_gicv3_common.c > > +++ b/hw/intc/arm_gicv3_common.c > > @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState > > *dev) > > memset(cs->gicr_ipriorityr, 0, sizeof(cs- > > >gicr_ipriorityr)); > > > > cs->hppi.prio = 0xff; > > + cs->hpplpi.prio = 0xff; > > > > /* State in the CPU interface must *not* be reset here, > > because it > > * is part of the CPU's reset domain, not the GIC > > device's. > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > > index 81f94c7f4a..5be3efaa3f 100644 > > --- a/hw/intc/arm_gicv3_cpuif.c > > +++ b/hw/intc/arm_gicv3_cpuif.c > > @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState > > *cs, int irq) > > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, > > 1); > > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > > gicv3_redist_update(cs); > > - } else { > > + } else if (irq < GICV3_LPI_INTID_START) { > > gicv3_gicd_active_set(cs->gic, irq); > > gicv3_gicd_pending_clear(cs->gic, irq); > > gicv3_update(cs->gic, irq, 1); > > + } else { > > + gicv3_redist_lpi_pending(cs, irq, 0); > > } > > } > > > > @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, > > const ARMCPRegInfo *ri, > > trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, > > gicv3_redist_affid(cs), value); > > > > - if (irq >= cs->gic->num_irq) { > > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && > > + (irq >= GICV3_LPI_INTID_START)))) { > > /* This handles two cases: > > * 1. If software writes the ID of a spurious interrupt > > [ie 1020-1023] > > * to the GICC_EOIR, the GIC ignores that write. > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c > > index 0a978cf55b..e0fbd4041f 100644 > > --- a/hw/intc/arm_gicv3_its.c > > +++ b/hw/intc/arm_gicv3_its.c > > @@ -211,6 +211,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; > > uint64_t itel = 0; > > uint32_t iteh = 0; > > > > @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState > > *s, uint64_t value, > > * command in the queue > > */ > > } else { > > - /* > > - * Current implementation only supports rdbase == procnum > > - * Hence rdbase physical address is ignored > > - */ > > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; > > + assert(rdbase <= s->gicv3->num_cpu); > > + > > + 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 */ > > res = update_ite(s, eventid, dte, itel, iteh); > > diff --git a/hw/intc/arm_gicv3_redist.c > > b/hw/intc/arm_gicv3_redist.c > > index fb9a4ee3cc..bfc6e4e9b9 100644 > > --- a/hw/intc/arm_gicv3_redist.c > > +++ b/hw/intc/arm_gicv3_redist.c > > @@ -255,6 +255,11 @@ 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 > > */ > > + cs->lpivalid = false; > > + cs->hpplpi.prio = 0xff; > > + gicv3_redist_update_lpi(cs); > > + gicv3_redist_update(cs); > > } else { > > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; > > } > > @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, > > hwaddr offset, uint64_t data, > > return r; > > } > > > > +void gicv3_redist_update_lpi(GICv3CPUState *cs) > > +{ > > + /* > > + * This function scans the LPI pending table and for each > > pending > > + * LPI, reads the corresponding entry from LPI configuration > > table > > + * to extract the priority info and determine if the LPI > > priority > > + * is lower than the current high priority interrupt.If yes, > > update> + * high priority pending interrupt to that of LPI. > > "update high priority pending interrupt to that of LPI" may need some > rewording Will do > > + */ > > + AddressSpace *as = &cs->gic->dma_as; > > + uint64_t lpict_baddr, lpipt_baddr; > > + uint32_t pendt_size = 0; > > + uint8_t lpite; > > + uint8_t prio, pend; > > + int i; > > + uint64_t idbits; > > + > > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, > > IDBITS), > > + GICD_TYPER_IDBITS); > > + > > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs- > > >gicr_propbaser || > > + !cs->gicr_pendbaser || (idbits < > > GICR_PROPBASER_IDBITS_THRESHOLD)) { > > + return; > > + } > > + > > + lpict_baddr = cs->gicr_propbaser & > > R_GICR_PROPBASER_PHYADDR_MASK; > > + > > + lpipt_baddr = cs->gicr_pendbaser & > > R_GICR_PENDBASER_PHYADDR_MASK; > > + > > + /* Determine the highest priority pending interrupt among LPIs > > */ > > + pendt_size = (1ULL << (idbits + 1)); > > + > > + for (i = 0; i < pendt_size / 8; 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)); > > + > > + if (!(lpite & LPI_CTE_ENABLED)) { > > + continue; > > + } > > + > > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > > + prio = lpite & LPI_PRIORITY_MASK; > > + } else { > > + prio = lpite & LPI_SPRIORITY_MASK; > > + } > > + > > + if (prio <= cs->hpplpi.prio) { > > + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; > > + cs->hpplpi.prio = prio; > > + /* LPIs are always non-secure Grp1 interrupts */ > > + cs->hpplpi.grp = GICV3_G1NS; > > + cs->lpivalid = true; > > + } > > + } > > + } > > +} > > + > nit: add a comment to explain what the function does, that's not > straightforward gievn its name. Will do > > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int > > level) > > +{ > > + AddressSpace *as = &cs->gic->dma_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 = cs->gicr_pendbaser & > > R_GICR_PENDBASER_PHYADDR_MASK; > > + > > + 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)); > > + } > > + } > > + cs->lpivalid = false; > > + cs->hpplpi.prio = 0xff; > > + gicv3_redist_update_lpi(cs); > > +} > > + > > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int > > level) > > +{ > > + AddressSpace *as = &cs->gic->dma_as; > > + uint64_t lpict_baddr; > > + uint8_t lpite; > > + uint64_t idbits; > > + > > + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, > > IDBITS), > > + GICD_TYPER_IDBITS); > > + > > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs- > > >gicr_propbaser || > > + !cs->gicr_pendbaser || (idbits < > > GICR_PROPBASER_IDBITS_THRESHOLD) || > > + (irq > (1ULL << (idbits + 1)))) { > > + return; > > + } > > + > > + lpict_baddr = cs->gicr_propbaser & > > R_GICR_PROPBASER_PHYADDR_MASK; > > + > > + /* 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 91dbe01176..bcbccba573 100644 > > --- a/hw/intc/gicv3_internal.h > > +++ b/hw/intc/gicv3_internal.h > > @@ -308,6 +308,13 @@ 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 LPI_SPRIORITY_MASK 0x7e > > + > > #define GITS_CMDQ_ENTRY_SIZE 32 > > #define NUM_BYTES_IN_DW 8 > > > > @@ -452,6 +459,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); > > +void 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); > > > > diff --git a/include/hw/intc/arm_gicv3_common.h > > b/include/hw/intc/arm_gicv3_common.h > > index c1348cc60a..5d839da9c9 100644 > > --- a/include/hw/intc/arm_gicv3_common.h > > +++ b/include/hw/intc/arm_gicv3_common.h > > @@ -204,6 +204,16 @@ struct GICv3CPUState { > > * real state above; it doesn't need to be migrated. > > */ > > PendingIrq hppi; > > + > > + /* > > + * Current highest priority pending lpi for this CPU. > > + * This is cached information that can be recalculated from > > the > > + * real state above; it doesn't need to be migrated. > > + */ > > + PendingIrq hpplpi; > > + > > + bool lpivalid; /* current highest priority lpi validity status > > */ > > + > > /* This is temporary working state, to avoid a malloc in > > gicv3_update() */ > > bool seenbetter; > > };> > > Thanks > > Eric >
diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c index d63f8af604..4d19190b9c 100644 --- a/hw/intc/arm_gicv3.c +++ b/hw/intc/arm_gicv3.c @@ -165,6 +165,15 @@ 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 && cs->lpivalid) { + if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) { + cs->hppi.irq = cs->hpplpi.irq; + cs->hppi.prio = cs->hpplpi.prio; + cs->hppi.grp = cs->hpplpi.grp; + 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_common.c b/hw/intc/arm_gicv3_common.c index 53dea2a775..223db16fec 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -435,6 +435,7 @@ static void arm_gicv3_common_reset(DeviceState *dev) memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr)); cs->hppi.prio = 0xff; + cs->hpplpi.prio = 0xff; /* State in the CPU interface must *not* be reset here, because it * is part of the CPU's reset domain, not the GIC device's. diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index 81f94c7f4a..5be3efaa3f 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -898,10 +898,12 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); gicv3_redist_update(cs); - } else { + } else if (irq < GICV3_LPI_INTID_START) { gicv3_gicd_active_set(cs->gic, irq); gicv3_gicd_pending_clear(cs->gic, irq); gicv3_update(cs->gic, irq, 1); + } else { + gicv3_redist_lpi_pending(cs, irq, 0); } } @@ -1317,7 +1319,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1, gicv3_redist_affid(cs), value); - if (irq >= cs->gic->num_irq) { + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable && + (irq >= GICV3_LPI_INTID_START)))) { /* This handles two cases: * 1. If software writes the ID of a spurious interrupt [ie 1020-1023] * to the GICC_EOIR, the GIC ignores that write. diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index 0a978cf55b..e0fbd4041f 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -211,6 +211,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; uint64_t itel = 0; uint32_t iteh = 0; @@ -267,10 +268,15 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value, * command in the queue */ } else { - /* - * Current implementation only supports rdbase == procnum - * Hence rdbase physical address is ignored - */ + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK; + assert(rdbase <= s->gicv3->num_cpu); + + 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 */ res = update_ite(s, eventid, dte, itel, iteh); diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c index fb9a4ee3cc..bfc6e4e9b9 100644 --- a/hw/intc/arm_gicv3_redist.c +++ b/hw/intc/arm_gicv3_redist.c @@ -255,6 +255,11 @@ 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 */ + cs->lpivalid = false; + cs->hpplpi.prio = 0xff; + gicv3_redist_update_lpi(cs); + gicv3_redist_update(cs); } else { cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; } @@ -534,6 +539,146 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, return r; } +void gicv3_redist_update_lpi(GICv3CPUState *cs) +{ + /* + * This function scans the LPI pending table and for each pending + * LPI, reads the corresponding entry from LPI configuration table + * to extract the priority info and determine if the LPI priority + * is lower than the current high priority interrupt.If yes, update + * high priority pending interrupt to that of LPI. + */ + AddressSpace *as = &cs->gic->dma_as; + uint64_t lpict_baddr, lpipt_baddr; + uint32_t pendt_size = 0; + uint8_t lpite; + uint8_t prio, pend; + int i; + uint64_t idbits; + + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), + GICD_TYPER_IDBITS); + + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD)) { + return; + } + + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; + + lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; + + /* Determine the highest priority pending interrupt among LPIs */ + pendt_size = (1ULL << (idbits + 1)); + + for (i = 0; i < pendt_size / 8; 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)); + + if (!(lpite & LPI_CTE_ENABLED)) { + continue; + } + + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { + prio = lpite & LPI_PRIORITY_MASK; + } else { + prio = lpite & LPI_SPRIORITY_MASK; + } + + if (prio <= cs->hpplpi.prio) { + cs->hpplpi.irq = GICV3_LPI_INTID_START + i; + cs->hpplpi.prio = prio; + /* LPIs are always non-secure Grp1 interrupts */ + cs->hpplpi.grp = GICV3_G1NS; + cs->lpivalid = true; + } + } + } +} + +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) +{ + AddressSpace *as = &cs->gic->dma_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 = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK; + + 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)); + } + } + cs->lpivalid = false; + cs->hpplpi.prio = 0xff; + gicv3_redist_update_lpi(cs); +} + +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) +{ + AddressSpace *as = &cs->gic->dma_as; + uint64_t lpict_baddr; + uint8_t lpite; + uint64_t idbits; + + idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS), + GICD_TYPER_IDBITS); + + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser || + !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) || + (irq > (1ULL << (idbits + 1)))) { + return; + } + + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK; + + /* 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 91dbe01176..bcbccba573 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -308,6 +308,13 @@ 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 LPI_SPRIORITY_MASK 0x7e + #define GITS_CMDQ_ENTRY_SIZE 32 #define NUM_BYTES_IN_DW 8 @@ -452,6 +459,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); +void 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); diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index c1348cc60a..5d839da9c9 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -204,6 +204,16 @@ struct GICv3CPUState { * real state above; it doesn't need to be migrated. */ PendingIrq hppi; + + /* + * Current highest priority pending lpi for this CPU. + * This is cached information that can be recalculated from the + * real state above; it doesn't need to be migrated. + */ + PendingIrq hpplpi; + + bool lpivalid; /* current highest priority lpi validity status */ + /* This is temporary working state, to avoid a malloc in gicv3_update() */ bool seenbetter; };
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. Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/intc/arm_gicv3.c | 9 ++ hw/intc/arm_gicv3_common.c | 1 + hw/intc/arm_gicv3_cpuif.c | 7 +- hw/intc/arm_gicv3_its.c | 14 ++- hw/intc/arm_gicv3_redist.c | 145 +++++++++++++++++++++++++++++ hw/intc/gicv3_internal.h | 10 ++ include/hw/intc/arm_gicv3_common.h | 10 ++ 7 files changed, 190 insertions(+), 6 deletions(-) -- 2.27.0