Peter Maydell <peter.mayd...@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.mayd...@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
