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

[Peter Maydell]

On Mon, 13 Dec 2021 at 15:00, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> 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)?

-1 is only there to be a "definitely not a valid address" value,
and it's less typing than UINT64_MAX.

-- PMM