Re: [PATCH v6 06/10] hw/intc: GICv3 redistributor ITS processing

[Peter Maydell]

On Wed, 7 Jul 2021 at 03:18, 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>

> +
> +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)) {

If you structure the loop below appropriately, you can avoid having to
explicitly check idbits against GICR_PROPBASER_IDBITS_THRESHOLD; see below.

> +        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;
> +        }

I don't think this code is right. The pending table size includes one
bit for every interrupt ID, including the low ones below GICV3_LPID_INTID_START
(ie there's a spare 1K for IMPDEF purposes at the start). Here the
loop is iterating through the whole pending table with 'i' a byte
offset into it -- but then when we calculate the address to read we
add GICV3_LPI_INTID_START to it. The loop body seems to be written
as if 'i' is an intid, but the for() is not doing that.

Also, you read a byte of pending information, which tells you about
the pending state of 8 interrupts, but you only call
gicv3_redist_check_lpi_priority() for one of them.

> +
> +        gicv3_redist_check_lpi_priority(cs, GICV3_LPI_INTID_START + i);
> +    }

I think you probably want to structure the loop something like this:

for (offset = GICV3_LPI_INTID_START / 8; offset < pendt_size / 8; offset++) {
    read the byte at lpipt_baddr + offset;
    while (byte) {
        bit = ctz32(byte);
        gicv3_redist_check_lpi_priority(cs, offset * 8 + bit);
        byte &= ~(1 << bit);
    }
}

(this is why you don't need to explicitly check GICR_PROPBASER_IDBITS_THRESHOLD:
if idbits is too low then the loop condition is just always false and we
don't check any pending bits.)


> +}
> +
> +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));
> +
> +    ispend = extract32(pend, irq % 8, 1);
> +
> +    /* no change in the value of pending bit, return */
> +    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));
> +
> +    /*
> +     * 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);
> +    } else {
> +        if (irq == cs->hpplpi.irq) {
> +            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);

You should ignore irq values less than GICV3_LPI_INTID_START here.
(We checked when we put the map into the ITE but this is an easy
way to get rid of the 1023 special case.)

> +
> +    if (!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
> +         !cs->gicr_pendbaser || (idbits < GICR_PROPBASER_IDBITS_THRESHOLD) ||

You don't need to explicitly look for "idbits <
GICR_PROPBASER_IDBITS_THRESHOLD":
if that is true then the irq will always be greater than the maximum imposed by
idbits and we'll throw it out that way.

> +         (irq > (1ULL << (idbits + 1)))) {

This is off by one -- 1 << (bitcount) is not the largest permitted intID,
it is one greater than that.

> +        return;
> +    }
> +
> +    /* set/clear the pending bit for this irq */
> +    gicv3_redist_lpi_pending(cs, irq, level);
> +
> +    gicv3_redist_update(cs);
> +}

thanks
-- PMM