Re: [PATCH 26/26] hw/intc/arm_gicv3_its: Factor out "find address of table entry" code

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The ITS has several tables which all share a similar format,
> described by the TableDesc struct: the guest may configure them
> to be a single-level table or a two-level table. Currently we
> open-code the process of finding the table entry in all the
> functions which read or write the device table or the collection
> table. Factor out the "get the address of the table entry"
> logic into a new function, so that the code which needs to
> read or write a table entry only needs to call table_entry_addr()
> and then perform a suitable load or store to that address.
>
> Note that the error handling is slightly complicated because
> we want to handle two cases differently:
>  * failure to read the L1 table entry should end up causing
>    a command stall, like other kinds of DMA error
>  * an L1 table entry that says there is no L2 table for this
>    index (ie whose valid bit is 0) must result in us treating
>    the table entry as not-valid on read, and discarding
>    writes (this is mandated by the spec)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is a worthwhile refactoring on its own, but still more
> so given that GICv4 adds another table in this format.
> ---
>  hw/intc/arm_gicv3_its.c | 212 +++++++++++++---------------------------
>  1 file changed, 70 insertions(+), 142 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 3bcc4c3db85..90a9fd3b3d4 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -83,44 +83,62 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t 
> page_sz)
>      return result;
>  }
>  
> +static uint64_t table_entry_addr(GICv3ITSState *s, TableDesc *td,
> +                                 uint32_t idx, MemTxResult *res)
> +{

It seems odd to have a uint64_t return type when....

> +    /*
> +     * Given a TableDesc describing one of the ITS in-guest-memory
> +     * tables and an index into it, return the guest address
> +     * corresponding to that table entry.
> +     * If there was a memory error reading the L1 table of an
> +     * indirect table, *res is set accordingly, and we return -1.
> +     * If the L1 table entry is marked not valid, we return -1 with
> +     * *res set to MEMTX_OK.
> +     *
> +     * The specification defines the format of level 1 entries of a
> +     * 2-level table, but the format of level 2 entries and the format
> +     * of flat-mapped tables is IMPDEF.
> +     */
> +    AddressSpace *as = &s->gicv3->dma_as;
> +    uint32_t l2idx;
> +    uint64_t l2;
> +    uint32_t num_l2_entries;
> +
> +    *res = MEMTX_OK;
> +
> +    if (!td->indirect) {
> +        /* Single level table */
> +        return td->base_addr + idx * td->entry_sz;
> +    }
> +
> +    /* Two level table */
> +    l2idx = idx / (td->page_sz / L1TABLE_ENTRY_SIZE);
> +
> +    l2 = address_space_ldq_le(as,
> +                              td->base_addr + (l2idx * L1TABLE_ENTRY_SIZE),
> +                              MEMTXATTRS_UNSPECIFIED, res);
> +    if (*res != MEMTX_OK) {
> +        return -1;
> +    }
> +    if (!(l2 & L2_TABLE_VALID_MASK)) {
> +        return -1;
> +    }

We can return signed results. I guess implicit conversion takes care of
it but I wonder if it would be cleaner to return an int (or maybe
compare against UNINT64_MAX == INVALID_TABLE_ENTRY)?

-- 
Alex Bennée