diff mbox series

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

Message ID 20211211191135.1764649-27-peter.maydell@linaro.org
State Superseded
Headers show
Series arm gicv3 ITS: Various bug fixes and refactorings | expand

Commit Message

Peter Maydell Dec. 11, 2021, 7:11 p.m. UTC 
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(-)

Comments

Richard Henderson Dec. 12, 2021, 10:52 p.m. UTC | #1 
On 12/11/21 11:11 AM, Peter Maydell wrote:
> 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.
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Alex Bennée Dec. 13, 2021, 2:53 p.m. UTC | #2 
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)?
Peter Maydell Dec. 13, 2021, 3:48 p.m. UTC | #3 
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
diff mbox series

Patch

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)
+{
+    /*
+     * 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;
+    }
+
+    num_l2_entries = td->page_sz / td->entry_sz;
+    return (l2 & ((1ULL << 51) - 1)) + (idx % num_l2_entries) * td->entry_sz;
+}
+
 static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte,
                     MemTxResult *res)
 {
     AddressSpace *as = &s->gicv3->dma_as;
-    uint64_t l2t_addr;
-    uint64_t value;
-    bool valid_l2t;
-    uint32_t l2t_id;
-    uint32_t num_l2_entries;
+    uint64_t entry_addr = table_entry_addr(s, &s->ct, icid, res);
 
-    if (s->ct.indirect) {
-        l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
-
-        value = address_space_ldq_le(as,
-                                     s->ct.base_addr +
-                                     (l2t_id * L1TABLE_ENTRY_SIZE),
-                                     MEMTXATTRS_UNSPECIFIED, res);
-
-        if (*res == MEMTX_OK) {
-            valid_l2t = (value & L2_TABLE_VALID_MASK) != 0;
-
-            if (valid_l2t) {
-                num_l2_entries = s->ct.page_sz / s->ct.entry_sz;
-
-                l2t_addr = value & ((1ULL << 51) - 1);
-
-                *cte =  address_space_ldq_le(as, l2t_addr +
-                                    ((icid % num_l2_entries) * GITS_CTE_SIZE),
-                                    MEMTXATTRS_UNSPECIFIED, res);
-           }
-       }
-    } else {
-        /* Flat level table */
-        *cte =  address_space_ldq_le(as, s->ct.base_addr +
-                                     (icid * GITS_CTE_SIZE),
-                                      MEMTXATTRS_UNSPECIFIED, res);
+    if (entry_addr == -1) {
+        return false; /* not valid */
     }
 
+    *cte = address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res);
     return FIELD_EX64(*cte, CTE, VALID);
 }
 
@@ -189,41 +207,12 @@  static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
 static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
 {
     AddressSpace *as = &s->gicv3->dma_as;
-    uint64_t l2t_addr;
-    uint64_t value;
-    bool valid_l2t;
-    uint32_t l2t_id;
-    uint32_t num_l2_entries;
+    uint64_t entry_addr = table_entry_addr(s, &s->dt, devid, res);
 
-    if (s->dt.indirect) {
-        l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
-
-        value = address_space_ldq_le(as,
-                                     s->dt.base_addr +
-                                     (l2t_id * L1TABLE_ENTRY_SIZE),
-                                     MEMTXATTRS_UNSPECIFIED, res);
-
-        if (*res == MEMTX_OK) {
-            valid_l2t = (value & L2_TABLE_VALID_MASK) != 0;
-
-            if (valid_l2t) {
-                num_l2_entries = s->dt.page_sz / s->dt.entry_sz;
-
-                l2t_addr = value & ((1ULL << 51) - 1);
-
-                value =  address_space_ldq_le(as, l2t_addr +
-                                   ((devid % num_l2_entries) * GITS_DTE_SIZE),
-                                   MEMTXATTRS_UNSPECIFIED, res);
-            }
-        }
-    } else {
-        /* Flat level table */
-        value = address_space_ldq_le(as, s->dt.base_addr +
-                                     (devid * GITS_DTE_SIZE),
-                                     MEMTXATTRS_UNSPECIFIED, res);
+    if (entry_addr == -1) {
+        return 0; /* a DTE entry with the Valid bit clear */
     }
-
-    return value;
+    return address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res);
 }
 
 /*
@@ -424,11 +413,7 @@  static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
                        uint64_t rdbase)
 {
     AddressSpace *as = &s->gicv3->dma_as;
-    uint64_t value;
-    uint64_t l2t_addr;
-    bool valid_l2t;
-    uint32_t l2t_id;
-    uint32_t num_l2_entries;
+    uint64_t entry_addr;
     uint64_t cte = 0;
     MemTxResult res = MEMTX_OK;
 
@@ -442,44 +427,18 @@  static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
         cte = FIELD_DP64(cte, CTE, RDBASE, rdbase);
     }
 
-    /*
-     * 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.
-     */
-    if (s->ct.indirect) {
-        l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
-
-        value = address_space_ldq_le(as,
-                                     s->ct.base_addr +
-                                     (l2t_id * L1TABLE_ENTRY_SIZE),
-                                     MEMTXATTRS_UNSPECIFIED, &res);
-
-        if (res != MEMTX_OK) {
-            return false;
-        }
-
-        valid_l2t = (value & L2_TABLE_VALID_MASK) != 0;
-
-        if (valid_l2t) {
-            num_l2_entries = s->ct.page_sz / s->ct.entry_sz;
-
-            l2t_addr = value & ((1ULL << 51) - 1);
-
-            address_space_stq_le(as, l2t_addr +
-                                 ((icid % num_l2_entries) * GITS_CTE_SIZE),
-                                 cte, MEMTXATTRS_UNSPECIFIED, &res);
-        }
-    } else {
-        /* Flat level table */
-        address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
-                             cte, MEMTXATTRS_UNSPECIFIED, &res);
-    }
+    entry_addr = table_entry_addr(s, &s->ct, icid, &res);
     if (res != MEMTX_OK) {
+        /* memory access error: stall */
         return false;
-    } else {
+    }
+    if (entry_addr == -1) {
+        /* No L2 table for this index: discard write and continue */
         return true;
     }
+
+    address_space_stq_le(as, entry_addr, cte, MEMTXATTRS_UNSPECIFIED, &res);
+    return res == MEMTX_OK;
 }
 
 static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
@@ -527,11 +486,7 @@  static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
                        uint8_t size, uint64_t itt_addr)
 {
     AddressSpace *as = &s->gicv3->dma_as;
-    uint64_t value;
-    uint64_t l2t_addr;
-    bool valid_l2t;
-    uint32_t l2t_id;
-    uint32_t num_l2_entries;
+    uint64_t entry_addr;
     uint64_t dte = 0;
     MemTxResult res = MEMTX_OK;
 
@@ -546,44 +501,17 @@  static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
         return true;
     }
 
-    /*
-     * 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.
-     */
-    if (s->dt.indirect) {
-        l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
-
-        value = address_space_ldq_le(as,
-                                     s->dt.base_addr +
-                                     (l2t_id * L1TABLE_ENTRY_SIZE),
-                                     MEMTXATTRS_UNSPECIFIED, &res);
-
-        if (res != MEMTX_OK) {
-            return false;
-        }
-
-        valid_l2t = (value & L2_TABLE_VALID_MASK) != 0;
-
-        if (valid_l2t) {
-            num_l2_entries = s->dt.page_sz / s->dt.entry_sz;
-
-            l2t_addr = value & ((1ULL << 51) - 1);
-
-            address_space_stq_le(as, l2t_addr +
-                                 ((devid % num_l2_entries) * GITS_DTE_SIZE),
-                                 dte, MEMTXATTRS_UNSPECIFIED, &res);
-        }
-    } else {
-        /* Flat level table */
-        address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE),
-                             dte, MEMTXATTRS_UNSPECIFIED, &res);
-    }
+    entry_addr = table_entry_addr(s, &s->dt, devid, &res);
     if (res != MEMTX_OK) {
+        /* memory access error: stall */
         return false;
-    } else {
+    }
+    if (entry_addr == -1) {
+        /* No L2 table for this index: discard write and continue */
         return true;
     }
+    address_space_stq_le(as, entry_addr, dte, MEMTXATTRS_UNSPECIFIED, &res);
+    return res == MEMTX_OK;
 }
 
 static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,