On Wed, 30 Jun 2021 at 16:32, Shashi Mallela <shashi.mall...@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.mall...@linaro.org>
> ---
> hw/intc/arm_gicv3.c | 14 +++
> hw/intc/arm_gicv3_common.c | 1 +
> hw/intc/arm_gicv3_cpuif.c | 7 +-
> hw/intc/arm_gicv3_its.c | 24 ++++-
> hw/intc/arm_gicv3_redist.c | 142 +++++++++++++++++++++++++++++
> hw/intc/gicv3_internal.h | 8 ++
> include/hw/intc/arm_gicv3_common.h | 7 ++
> 7 files changed, 197 insertions(+), 6 deletions(-)
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index adaee72c1f..5adb55a01a 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -225,6 +225,7 @@ static MemTxResult process_its_cmd(GICv3ITSState *s,
> uint64_t value,
> bool ite_valid = false;
> uint64_t cte = 0;
> bool cte_valid = false;
> + uint64_t rdbase;
> IteEntry ite;
>
> if (cmd == NONE) {
> @@ -282,10 +283,18 @@ static MemTxResult process_its_cmd(GICv3ITSState *s,
> uint64_t value,
> * command in the queue
> */
> } else {
> - /*
> - * Current implementation only supports rdbase == procnum
> - * Hence rdbase physical address is ignored
> - */
This change doesn't make us start handling the "is a physical address"
case, so why delete the comment?
> + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;
> +
> + if (rdbase > s->gicv3->num_cpu) {
> + return res;
> + }
> +
> + 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) {
> memset(&ite, 0 , sizeof(ite));
> /* remove mapping from interrupt translation table */
> @@ -672,6 +681,13 @@ static void process_cmdq(GICv3ITSState *s)
> break;
> case GITS_CMD_INV:
> case GITS_CMD_INVALL:
> + /*
> + * Current implementation doesn't cache any ITS tables,
> + * but the calculated lpi priority information.We only
Missing space after "."
> + * need to trigger lpi priority re-calculation to be in
> + * sync with LPI config table or pending table changes.
> + */
> + gicv3_redist_update_lpi(s->gicv3->cpu);
I think we need to recalculate for all redistributors, not just the first one.
> break;
> default:
> break;
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index fc3d95dcc6..97553e6ada 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -254,6 +254,9 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr
> offset,
> if (cs->gicr_typer & GICR_TYPER_PLPIS) {
> if (value & GICR_CTLR_ENABLE_LPIS) {
> cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
> + /* Check for any pending interr in pending table */
> + gicv3_redist_update_lpi(cs);
> + gicv3_redist_update(cs);
> } else {
> cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
> }
> @@ -532,6 +535,145 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr
> offset, uint64_t data,
> return r;
> }
>
> +static void gicv3_redist_check_lpi_priority(GICv3CPUState *cs, int irq)
> +{
> + AddressSpace *as = &cs->gic->dma_as;
> + uint64_t lpict_baddr;
> + uint8_t lpite;
> + uint8_t prio;
> +
> + lpict_baddr = cs->gicr_propbaser & R_GICR_PROPBASER_PHYADDR_MASK;
> +
> + address_space_read(as, lpict_baddr + ((irq - GICV3_LPI_INTID_START) *
> + sizeof(lpite)), MEMTXATTRS_UNSPECIFIED, &lpite,
> + sizeof(lpite));
> +
> + if (!(lpite & LPI_CTE_ENABLED)) {
> + return;
> + }
> +
> + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) {
> + prio = lpite & LPI_PRIORITY_MASK;
> + } else {
> + prio = ((lpite & LPI_PRIORITY_MASK) >> 1) & 0x80;
Need to OR with 0x80, not AND.
> + }
> +
> + if ((prio < cs->hpplpi.prio) ||
> + ((prio == cs->hpplpi.prio) && (irq <= cs->hpplpi.irq))) {
> + cs->hpplpi.irq = irq;
> + cs->hpplpi.prio = prio;
> + /* LPIs are always non-secure Grp1 interrupts */
> + cs->hpplpi.grp = GICV3_G1NS;
> + }
> +}
> +
> +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 current LPI
> + * priority is lower than the last computed high priority lpi interrupt.
> + * If yes, replace current LPI as the new high priority lpi interrupt.
> + */
> + AddressSpace *as = &cs->gic->dma_as;
> + uint64_t lpipt_baddr;
> + uint32_t pendt_size = 0;
> + uint8_t 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;
> + }
> +
> + cs->hpplpi.prio = 0xff;
> +
> + 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)) {
> + continue;
> + }
> +
> + gicv3_redist_check_lpi_priority(cs, GICV3_LPI_INTID_START + i);
> + }
> +}
> +
> +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)
> +{
> + /*
> + * This function updates the pending bit in lpi pending table for
> + * the irq being activated or deactivated.
> + */
> + 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));
> +
> + /*
> + * check if this LPI is better than the current hpplpi, if yes
> + * just set hpplpi.prio and .irq without doing a full rescan
> + */
> + if (level) {
> + gicv3_redist_check_lpi_priority(cs, irq);
> + }
> +
> + ispend = extract32(pend, irq % 8, 1);
> +
> + /* no change in the value of pending bit,return */
Missing space after comma.
> + if (ispend == level) {
> + return;
> + }
I would put the "is the level same as it already was" check before
we go off and do the priority-check, not after. Then you can do all
the handling of "the level changed" at the end, with
if (level) {
gicv3_redist_check_lpi_priority(...);
} else {
gicv3_redist_update_lpi(...);
}
Also, if this is a level == 0, you don't need to do the full rescan
of the LPI table unless the LPI being deactivated is the current
highest priority LPI.
> + pend = deposit32(pend, irq % 8, 1, level ? 1 : 0);
> +
> + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> +
> + if (!level) {
> + gicv3_redist_update_lpi(cs);
> + }
> +}
> +
> +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
> +{
> + 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;
> + }
> +
> + /* 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 43ce4a8a95..c0ec1d9a66 100644
> --- a/hw/intc/gicv3_internal.h
> +++ b/hw/intc/gicv3_internal.h
> @@ -308,6 +308,11 @@ FIELD(GITS_TYPER, CIL, 36, 1)
>
> #define L1TABLE_ENTRY_SIZE 8
>
> +#define LPI_CTE_ENABLED TABLE_ENTRY_VALID_MASK
> +#define LPI_CTE_PRIORITY_OFFSET 2
> +#define LPI_CTE_PRIORITY_MASK ((1U << 6) - 1)
These aren't using the standard naming convention (_SHIFT for the
bitshift, and the _MASK should be in-place, not in the ls bits).
But you don't use the defines anyway, so you could just not bother
defining them.
thanks
-- PMM
