[PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Peter Maydell]

I've been working on the ITS to add support for the GICv4 functionality.
In the course of that I found a handful of bugs in it and also some
places where the code benefited from refactoring to make it a better
base to put in the GICv4 parts. This patchset is just the bugfixes
and cleanups, because there are enough patches here that I figured it
made sense to send them out now rather than holding on to them.

Most of these patches were in v1 and have been reviewed already.

Changes from v1:
 * first half of the series is now upstream
 * patch 1 now has the '1ULL' and uint64_t fixes that were
   partly split across two patches in the old series and partly missing
 * new patches 12 and 13

NB: I left the returns of -1 in patch 11.

Patches still needing review: 1, 12, 13

thanks
-- PMM

Peter Maydell (13):
  hw/intc/arm_gicv3_its: Fix event ID bounds checks
  hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention
  hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value
  hw/intc/arm_gicv3_its: Don't use data if reading command failed
  hw/intc/arm_gicv3_its: Use enum for return value of process_*
    functions
  hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd()
  hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting
  hw/intc/arm_gicv3_its: Fix return codes in process_mapti()
  hw/intc/arm_gicv3_its: Fix return codes in process_mapc()
  hw/intc/arm_gicv3_its: Fix return codes in process_mapd()
  hw/intc/arm_gicv3_its: Factor out "find address of table entry" code
  hw/intc/arm_gicv3_its: Check indexes before use, not after
  hw/intc/arm_gicv3_its: Range-check ICID before indexing into
    collection table

 hw/intc/arm_gicv3_its.c | 492 ++++++++++++++++++----------------------
 1 file changed, 225 insertions(+), 267 deletions(-)

-- 
2.25.1

[PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks

[Peter Maydell]

In process_its_cmd() and process_mapti() we must check the
event ID against a limit defined by the size field in the DTE,
which specifies the number of ID bits minus one. Convert
this code to our num_foo convention:
 * change the variable names
 * use uint64_t and 1ULL when calculating the number
   of valid event IDs, because DTE.SIZE is 5 bits and
   so num_eventids may be up to 2^32
 * fix the off-by-one error in the comparison

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index fa3cdb57554..6d11fa02040 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -225,7 +225,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
     MemTxResult res = MEMTX_OK;
     bool dte_valid;
     uint64_t dte = 0;
-    uint32_t max_eventid;
+    uint64_t num_eventids;
     uint16_t icid = 0;
     uint32_t pIntid = 0;
     bool ite_valid = false;
@@ -258,7 +258,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
     dte_valid = FIELD_EX64(dte, DTE, VALID);
 
     if (dte_valid) {
-        max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
+        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
 
         ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
 
@@ -299,10 +299,11 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
                       dte_valid ? "valid" : "invalid",
                       ite_valid ? "valid" : "invalid",
                       cte_valid ? "valid" : "invalid");
-    } else if (eventid > max_eventid) {
+    } else if (eventid >= num_eventids) {
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid command attributes: eventid %d > %d\n",
-                      __func__, eventid, max_eventid);
+                      "%s: invalid command attributes: eventid %d >= %"
+                      PRId64 "\n",
+                      __func__, eventid, num_eventids);
     } else {
         /*
          * Current implementation only supports rdbase == procnum
@@ -336,7 +337,8 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
     AddressSpace *as = &s->gicv3->dma_as;
     uint32_t devid, eventid;
     uint32_t pIntid = 0;
-    uint32_t max_eventid, max_Intid;
+    uint64_t num_eventids;
+    uint32_t max_Intid;
     bool dte_valid;
     MemTxResult res = MEMTX_OK;
     uint16_t icid = 0;
@@ -376,11 +378,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
         return result;
     }
     dte_valid = FIELD_EX64(dte, DTE, VALID);
-    max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
+    num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
     max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;
 
     if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
-            || !dte_valid || (eventid > max_eventid) ||
+            || !dte_valid || (eventid >= num_eventids) ||
             (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) &&
              (pIntid != INTID_SPURIOUS))) {
         qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.25.1

Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks

[Alex Bennée]

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

> In process_its_cmd() and process_mapti() we must check the
> event ID against a limit defined by the size field in the DTE,
> which specifies the number of ID bits minus one. Convert
> this code to our num_foo convention:
>  * change the variable names
>  * use uint64_t and 1ULL when calculating the number
>    of valid event IDs, because DTE.SIZE is 5 bits and
>    so num_eventids may be up to 2^32
>  * fix the off-by-one error in the comparison
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_its.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index fa3cdb57554..6d11fa02040 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -225,7 +225,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      MemTxResult res = MEMTX_OK;
>      bool dte_valid;
>      uint64_t dte = 0;
> -    uint32_t max_eventid;
> +    uint64_t num_eventids;
>      uint16_t icid = 0;
>      uint32_t pIntid = 0;
>      bool ite_valid = false;
> @@ -258,7 +258,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      dte_valid = FIELD_EX64(dte, DTE, VALID);
>  
>      if (dte_valid) {
> -        max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
> +        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
>  
>          ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
>  
> @@ -299,10 +299,11 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>                        dte_valid ? "valid" : "invalid",
>                        ite_valid ? "valid" : "invalid",
>                        cte_valid ? "valid" : "invalid");
> -    } else if (eventid > max_eventid) {
> +    } else if (eventid >= num_eventids) {
>          qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: invalid command attributes: eventid %d > %d\n",
> -                      __func__, eventid, max_eventid);
> +                      "%s: invalid command attributes: eventid %d >= %"
> +                      PRId64 "\n",
> +                      __func__, eventid, num_eventids);
>      } else {
>          /*
>           * Current implementation only supports rdbase == procnum
> @@ -336,7 +337,8 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      AddressSpace *as = &s->gicv3->dma_as;
>      uint32_t devid, eventid;
>      uint32_t pIntid = 0;
> -    uint32_t max_eventid, max_Intid;
> +    uint64_t num_eventids;
> +    uint32_t max_Intid;
>      bool dte_valid;
>      MemTxResult res = MEMTX_OK;
>      uint16_t icid = 0;
> @@ -376,11 +378,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
>          return result;
>      }
>      dte_valid = FIELD_EX64(dte, DTE, VALID);
> -    max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
> +    num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
>      max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;
>  
>      if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
> -            || !dte_valid || (eventid > max_eventid) ||
> +            || !dte_valid || (eventid >= num_eventids) ||
>              (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) &&
>               (pIntid != INTID_SPURIOUS))) {

It seems process_mapti has the similar "catch all the errors" if leg
that I split apart in process_its_command to make it easier to see what
failed.

However:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks

[Richard Henderson]

On 1/12/22 04:10, Peter Maydell wrote:
> In process_its_cmd() and process_mapti() we must check the
> event ID against a limit defined by the size field in the DTE,
> which specifies the number of ID bits minus one. Convert
> this code to our num_foo convention:
>   * change the variable names
>   * use uint64_t and 1ULL when calculating the number
>     of valid event IDs, because DTE.SIZE is 5 bits and
>     so num_eventids may be up to 2^32
>   * fix the off-by-one error in the comparison
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)

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


> +        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);

Could be written 2 << N, instead of 1 << (N + 1).

r~

Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks

[Peter Maydell]

On Fri, 28 Jan 2022 at 01:33, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/12/22 04:10, Peter Maydell wrote:
> > In process_its_cmd() and process_mapti() we must check the
> > event ID against a limit defined by the size field in the DTE,
> > which specifies the number of ID bits minus one. Convert
> > this code to our num_foo convention:
> >   * change the variable names
> >   * use uint64_t and 1ULL when calculating the number
> >     of valid event IDs, because DTE.SIZE is 5 bits and
> >     so num_eventids may be up to 2^32
> >   * fix the off-by-one error in the comparison
> >
> > Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> > ---
> >   hw/intc/arm_gicv3_its.c | 18 ++++++++++--------
> >   1 file changed, 10 insertions(+), 8 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> > +        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
>
> Could be written 2 << N, instead of 1 << (N + 1).

It could, but the spec defines the field as containing
"number of supported bits, minus 1", so I think that
using an expression that matches that is clearer.

Aside: clang optimizes both of these expressions to
the same thing; gcc does not:

https://godbolt.org/z/nsz4Mdxhq

(not that it will make a perf difference we care about here).

-- PMM

[PATCH v2 02/13] hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention

[Peter Maydell]

The bounds check on the number of interrupt IDs is correct, but
doesn't match our convention; change the variable name, initialize it
to the 2^n value rather than (2^n)-1, and use >= instead of > in the
comparison.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 6d11fa02040..5919b1a3b7f 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -338,7 +338,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
     uint32_t devid, eventid;
     uint32_t pIntid = 0;
     uint64_t num_eventids;
-    uint32_t max_Intid;
+    uint32_t num_intids;
     bool dte_valid;
     MemTxResult res = MEMTX_OK;
     uint16_t icid = 0;
@@ -379,11 +379,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
     }
     dte_valid = FIELD_EX64(dte, DTE, VALID);
     num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
-    max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;
+    num_intids = 1ULL << (GICD_TYPER_IDBITS + 1);
 
     if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
             || !dte_valid || (eventid >= num_eventids) ||
-            (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) &&
+            (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) &&
              (pIntid != INTID_SPURIOUS))) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes "
-- 
2.25.1

Re: [PATCH v2 02/13] hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention

[Richard Henderson]

On 1/12/22 04:10, Peter Maydell wrote:
> The bounds check on the number of interrupt IDs is correct, but
> doesn't match our convention; change the variable name, initialize it
> to the 2^n value rather than (2^n)-1, and use >= instead of > in the
> comparison.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 6d11fa02040..5919b1a3b7f 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -338,7 +338,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
>       uint32_t devid, eventid;
>       uint32_t pIntid = 0;
>       uint64_t num_eventids;
> -    uint32_t max_Intid;
> +    uint32_t num_intids;

Does this now need to be uint64_t, like num_eventids?

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

r~

Re: [PATCH v2 02/13] hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention

[Peter Maydell]

On Fri, 28 Jan 2022 at 01:35, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/12/22 04:10, Peter Maydell wrote:
> > The bounds check on the number of interrupt IDs is correct, but
> > doesn't match our convention; change the variable name, initialize it
> > to the 2^n value rather than (2^n)-1, and use >= instead of > in the
> > comparison.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >   hw/intc/arm_gicv3_its.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 6d11fa02040..5919b1a3b7f 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -338,7 +338,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
> >       uint32_t devid, eventid;
> >       uint32_t pIntid = 0;
> >       uint64_t num_eventids;
> > -    uint32_t max_Intid;
> > +    uint32_t num_intids;
>
> Does this now need to be uint64_t, like num_eventids?

No, because GICD_TYPER_IDBITS is 0xf, so num_intids is 2^16
and still fits in a 32-bit integer.

thanks
-- PMM

[PATCH v2 03/13] hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value

[Peter Maydell]

process_its_cmd() returns a bool, like all the other process_ functions.
However we were putting its return value into 'res', not 'result',
which meant we would ignore it when deciding whether to continue
or stall the command queue. Fix the typo.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/intc/arm_gicv3_its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 5919b1a3b7f..a6c2299a091 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -678,10 +678,10 @@ static void process_cmdq(GICv3ITSState *s)
 
         switch (cmd) {
         case GITS_CMD_INT:
-            res = process_its_cmd(s, data, cq_offset, INTERRUPT);
+            result = process_its_cmd(s, data, cq_offset, INTERRUPT);
             break;
         case GITS_CMD_CLEAR:
-            res = process_its_cmd(s, data, cq_offset, CLEAR);
+            result = process_its_cmd(s, data, cq_offset, CLEAR);
             break;
         case GITS_CMD_SYNC:
             /*
-- 
2.25.1

[PATCH v2 04/13] hw/intc/arm_gicv3_its: Don't use data if reading command failed

[Peter Maydell]

In process_cmdq(), we read 64 bits of the command packet, which
contain the command identifier, which we then switch() on to dispatch
to an appropriate sub-function.  However, if address_space_ldq_le()
reports a memory transaction failure, we still read the command
identifier out of the data and switch() on it.  Restructure the code
so that we stop immediately (stalling the command queue) in this
case.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index a6c2299a091..c1f76682d04 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -672,8 +672,13 @@ static void process_cmdq(GICv3ITSState *s)
         data = address_space_ldq_le(as, s->cq.base_addr + cq_offset,
                                     MEMTXATTRS_UNSPECIFIED, &res);
         if (res != MEMTX_OK) {
-            result = false;
+            s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: could not read command at 0x%" PRIx64 "\n",
+                          __func__, s->cq.base_addr + cq_offset);
+            break;
         }
+
         cmd = (data & CMD_MASK);
 
         switch (cmd) {
-- 
2.25.1

[PATCH v2 05/13] hw/intc/arm_gicv3_its: Use enum for return value of process_* functions

[Peter Maydell]

When an ITS detects an error in a command, it has an
implementation-defined (CONSTRAINED UNPREDICTABLE) choice of whether
to ignore the command, proceeding to the next one in the queue, or to
stall the ITS command queue, processing nothing further.  The
behaviour required when the read of the command packet from memory
fails is less clearly documented, but the same set of choices as for
command errors seem reasonable.

The intention of the QEMU implementation, as documented in the
comments, is that if we encounter a memory error reading the command
packet or one of the various data tables then we should stall, but
for command parameter errors we should ignore the queue and continue.
However, we don't actually do this.  To get the desired behaviour,
the various process_* functions need to return true to cause
process_cmdq() to advance to the next command and keep processing,
and false to stall command processing.  What they mostly do is return
false for any kind of error.

To make the code clearer, replace the 'bool' return from the process_
functions with an enum which may be either CMD_STALL or CMD_CONTINUE.
In this commit no behaviour changes; in subsequent commits we will
adjust the error-return paths for the process_ functions one by one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/intc/arm_gicv3_its.c | 59 ++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index c1f76682d04..10901a5e709 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -45,6 +45,23 @@ typedef struct {
     uint64_t itel;
 } IteEntry;
 
+/*
+ * The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options
+ * if a command parameter is not correct. These include both "stall
+ * processing of the command queue" and "ignore this command, and
+ * keep processing the queue". In our implementation we choose that
+ * memory transaction errors reading the command packet provoke a
+ * stall, but errors in parameters cause us to ignore the command
+ * and continue processing.
+ * The process_* functions which handle individual ITS commands all
+ * return an ItsCmdResult which tells process_cmdq() whether it should
+ * stall or keep going.
+ */
+typedef enum ItsCmdResult {
+    CMD_STALL = 0,
+    CMD_CONTINUE = 1,
+} ItsCmdResult;
+
 static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz)
 {
     uint64_t result = 0;
@@ -217,8 +234,8 @@ static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
  * 3. handling of ITS CLEAR command
  * 4. handling of ITS DISCARD command
  */
-static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
-                            ItsCmdType cmd)
+static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
+                                    uint32_t offset, ItsCmdType cmd)
 {
     AddressSpace *as = &s->gicv3->dma_as;
     uint32_t devid, eventid;
@@ -231,7 +248,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
     bool ite_valid = false;
     uint64_t cte = 0;
     bool cte_valid = false;
-    bool result = false;
+    ItsCmdResult result = CMD_STALL;
     uint64_t rdbase;
 
     if (cmd == NONE) {
@@ -324,15 +341,15 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
         if (cmd == DISCARD) {
             IteEntry ite = {};
             /* remove mapping from interrupt translation table */
-            result = update_ite(s, eventid, dte, ite);
+            result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
         }
     }
 
     return result;
 }
 
-static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
-                          bool ignore_pInt)
+static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
+                                  uint32_t offset, bool ignore_pInt)
 {
     AddressSpace *as = &s->gicv3->dma_as;
     uint32_t devid, eventid;
@@ -343,7 +360,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
     MemTxResult res = MEMTX_OK;
     uint16_t icid = 0;
     uint64_t dte = 0;
-    bool result = false;
+    ItsCmdResult result = CMD_STALL;
 
     devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
     offset += NUM_BYTES_IN_DW;
@@ -404,7 +421,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
         ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
         ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
 
-        result = update_ite(s, eventid, dte, ite);
+        result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
     }
 
     return result;
@@ -472,14 +489,14 @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
     }
 }
 
-static bool process_mapc(GICv3ITSState *s, uint32_t offset)
+static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
 {
     AddressSpace *as = &s->gicv3->dma_as;
     uint16_t icid;
     uint64_t rdbase;
     bool valid;
     MemTxResult res = MEMTX_OK;
-    bool result = false;
+    ItsCmdResult result = CMD_STALL;
     uint64_t value;
 
     offset += NUM_BYTES_IN_DW;
@@ -509,7 +526,7 @@ static bool process_mapc(GICv3ITSState *s, uint32_t offset)
          * command in the queue
          */
     } else {
-        result = update_cte(s, icid, valid, rdbase);
+        result = update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL;
     }
 
     return result;
@@ -578,7 +595,8 @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
     }
 }
 
-static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset)
+static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
+                                 uint32_t offset)
 {
     AddressSpace *as = &s->gicv3->dma_as;
     uint32_t devid;
@@ -586,7 +604,7 @@ static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset)
     uint64_t itt_addr;
     bool valid;
     MemTxResult res = MEMTX_OK;
-    bool result = false;
+    ItsCmdResult result = CMD_STALL;
 
     devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
 
@@ -623,7 +641,7 @@ static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset)
          * command in the queue
          */
     } else {
-        result = update_dte(s, devid, valid, size, itt_addr);
+        result = update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
     }
 
     return result;
@@ -641,7 +659,6 @@ static void process_cmdq(GICv3ITSState *s)
     uint64_t data;
     AddressSpace *as = &s->gicv3->dma_as;
     MemTxResult res = MEMTX_OK;
-    bool result = true;
     uint8_t cmd;
     int i;
 
@@ -668,6 +685,8 @@ static void process_cmdq(GICv3ITSState *s)
     }
 
     while (wr_offset != rd_offset) {
+        ItsCmdResult result = CMD_CONTINUE;
+
         cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE);
         data = address_space_ldq_le(as, s->cq.base_addr + cq_offset,
                                     MEMTXATTRS_UNSPECIFIED, &res);
@@ -726,18 +745,16 @@ static void process_cmdq(GICv3ITSState *s)
         default:
             break;
         }
-        if (result) {
+        if (result == CMD_CONTINUE) {
             rd_offset++;
             rd_offset %= s->cq.num_entries;
             s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset);
         } else {
-            /*
-             * in this implementation, in case of dma read/write error
-             * we stall the command processing
-             */
+            /* CMD_STALL */
             s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
             qemu_log_mask(LOG_GUEST_ERROR,
-                          "%s: %x cmd processing failed\n", __func__, cmd);
+                          "%s: 0x%x cmd processing failed, stalling\n",
+                          __func__, cmd);
             break;
         }
     }
-- 
2.25.1

[PATCH v2 06/13] hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd()

[Peter Maydell]

Fix process_its_cmd() to consistently return CMD_STALL for
memory errors and CMD_CONTINUE for parameter errors, as
we claim in the comments that we do.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 10901a5e709..0929116c0fe 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -248,7 +248,6 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
     bool ite_valid = false;
     uint64_t cte = 0;
     bool cte_valid = false;
-    ItsCmdResult result = CMD_STALL;
     uint64_t rdbase;
 
     if (cmd == NONE) {
@@ -262,7 +261,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
     }
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     eventid = (value & EVENTID_MASK);
@@ -270,7 +269,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
     dte = get_dte(s, devid, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
     dte_valid = FIELD_EX64(dte, DTE, VALID);
 
@@ -280,7 +279,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
 
         if (res != MEMTX_OK) {
-            return result;
+            return CMD_STALL;
         }
 
         if (ite_valid) {
@@ -288,14 +287,14 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         }
 
         if (res != MEMTX_OK) {
-            return result;
+            return CMD_STALL;
         }
     } else {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: "
                       "invalid dte: %"PRIx64" for %d (MEM_TX: %d)\n",
                       __func__, dte, devid, res);
-        return result;
+        return CMD_CONTINUE;
     }
 
 
@@ -307,7 +306,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: devid %d>=%d",
                       __func__, devid, s->dt.num_ids);
-
+        return CMD_CONTINUE;
     } else if (!dte_valid || !ite_valid || !cte_valid) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: "
@@ -316,11 +315,13 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
                       dte_valid ? "valid" : "invalid",
                       ite_valid ? "valid" : "invalid",
                       cte_valid ? "valid" : "invalid");
+        return CMD_CONTINUE;
     } else if (eventid >= num_eventids) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: eventid %d >= %"
                       PRId64 "\n",
                       __func__, eventid, num_eventids);
+        return CMD_CONTINUE;
     } else {
         /*
          * Current implementation only supports rdbase == procnum
@@ -329,7 +330,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         rdbase = FIELD_EX64(cte, CTE, RDBASE);
 
         if (rdbase >= s->gicv3->num_cpu) {
-            return result;
+            return CMD_CONTINUE;
         }
 
         if ((cmd == CLEAR) || (cmd == DISCARD)) {
@@ -341,11 +342,10 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         if (cmd == DISCARD) {
             IteEntry ite = {};
             /* remove mapping from interrupt translation table */
-            result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+            return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
         }
+        return CMD_CONTINUE;
     }
-
-    return result;
 }
 
 static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
-- 
2.25.1

[PATCH v2 07/13] hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting

[Peter Maydell]

Refactor process_its_cmd() so that it consistently uses
the structure
  do thing;
  if (error condition) {
      return early;
  }
  do next thing;

rather than doing some of the work nested inside if (not error)
code blocks.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 103 +++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 53 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 0929116c0fe..5dc6846fe3f 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -273,79 +273,76 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
     }
     dte_valid = FIELD_EX64(dte, DTE, VALID);
 
-    if (dte_valid) {
-        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
-
-        ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
-
-        if (res != MEMTX_OK) {
-            return CMD_STALL;
-        }
-
-        if (ite_valid) {
-            cte_valid = get_cte(s, icid, &cte, &res);
-        }
-
-        if (res != MEMTX_OK) {
-            return CMD_STALL;
-        }
-    } else {
+    if (!dte_valid) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: "
-                      "invalid dte: %"PRIx64" for %d (MEM_TX: %d)\n",
-                      __func__, dte, devid, res);
+                      "invalid dte: %"PRIx64" for %d\n",
+                      __func__, dte, devid);
         return CMD_CONTINUE;
     }
 
+    num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
+
+    ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+
+    if (!ite_valid) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: invalid ITE\n",
+                      __func__);
+        return CMD_CONTINUE;
+    }
+
+    cte_valid = get_cte(s, icid, &cte, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+    if (!cte_valid) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: "
+                      "invalid cte: %"PRIx64"\n",
+                      __func__, cte);
+        return CMD_CONTINUE;
+    }
 
-    /*
-     * In this implementation, in case of guest errors we ignore the
-     * command and move onto the next command in the queue.
-     */
     if (devid >= s->dt.num_ids) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: devid %d>=%d",
                       __func__, devid, s->dt.num_ids);
         return CMD_CONTINUE;
-    } else if (!dte_valid || !ite_valid || !cte_valid) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid command attributes: "
-                      "dte: %s, ite: %s, cte: %s\n",
-                      __func__,
-                      dte_valid ? "valid" : "invalid",
-                      ite_valid ? "valid" : "invalid",
-                      cte_valid ? "valid" : "invalid");
-        return CMD_CONTINUE;
-    } else if (eventid >= num_eventids) {
+    }
+    if (eventid >= num_eventids) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: eventid %d >= %"
                       PRId64 "\n",
                       __func__, eventid, num_eventids);
         return CMD_CONTINUE;
-    } else {
-        /*
-         * Current implementation only supports rdbase == procnum
-         * Hence rdbase physical address is ignored
-         */
-        rdbase = FIELD_EX64(cte, CTE, RDBASE);
+    }
 
-        if (rdbase >= s->gicv3->num_cpu) {
-            return CMD_CONTINUE;
-        }
+    /*
+     * Current implementation only supports rdbase == procnum
+     * Hence rdbase physical address is ignored
+     */
+    rdbase = FIELD_EX64(cte, CTE, RDBASE);
 
-        if ((cmd == CLEAR) || (cmd == DISCARD)) {
-            gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);
-        } else {
-            gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);
-        }
-
-        if (cmd == DISCARD) {
-            IteEntry ite = {};
-            /* remove mapping from interrupt translation table */
-            return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
-        }
+    if (rdbase >= s->gicv3->num_cpu) {
         return CMD_CONTINUE;
     }
+
+    if ((cmd == CLEAR) || (cmd == DISCARD)) {
+        gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);
+    } else {
+        gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);
+    }
+
+    if (cmd == DISCARD) {
+        IteEntry ite = {};
+        /* remove mapping from interrupt translation table */
+        return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+    }
+    return CMD_CONTINUE;
 }
 
 static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
-- 
2.25.1

[PATCH v2 08/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapti()

[Peter Maydell]

Fix process_mapti() to consistently return CMD_STALL for memory
errors and CMD_CONTINUE for parameter errors, as we claim in the
comments that we do.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 5dc6846fe3f..010779a9fdc 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -357,7 +357,7 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
     MemTxResult res = MEMTX_OK;
     uint16_t icid = 0;
     uint64_t dte = 0;
-    ItsCmdResult result = CMD_STALL;
+    IteEntry ite = {};
 
     devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
     offset += NUM_BYTES_IN_DW;
@@ -365,7 +365,7 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
                                  MEMTXATTRS_UNSPECIFIED, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     eventid = (value & EVENTID_MASK);
@@ -381,7 +381,7 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
                                  MEMTXATTRS_UNSPECIFIED, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     icid = value & ICID_MASK;
@@ -389,7 +389,7 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
     dte = get_dte(s, devid, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
     dte_valid = FIELD_EX64(dte, DTE, VALID);
     num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
@@ -409,19 +409,17 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
          * we ignore this command and move onto the next
          * command in the queue
          */
-    } else {
-        /* add ite entry to interrupt translation table */
-        IteEntry ite = {};
-        ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid);
-        ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
-        ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);
-        ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
-        ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
-
-        result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+        return CMD_CONTINUE;
     }
 
-    return result;
+    /* add ite entry to interrupt translation table */
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
+    ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid);
+
+    return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
 }
 
 static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
-- 
2.25.1

[PATCH v2 09/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapc()

[Peter Maydell]

Fix process_mapc() to consistently return CMD_STALL for memory
errors and CMD_CONTINUE for parameter errors, as we claim in the
comments that we do.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 010779a9fdc..80ef4dbcadf 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -491,7 +491,6 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
     uint64_t rdbase;
     bool valid;
     MemTxResult res = MEMTX_OK;
-    ItsCmdResult result = CMD_STALL;
     uint64_t value;
 
     offset += NUM_BYTES_IN_DW;
@@ -501,7 +500,7 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
                                  MEMTXATTRS_UNSPECIFIED, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     icid = value & ICID_MASK;
@@ -520,11 +519,10 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
          * we ignore this command and move onto the next
          * command in the queue
          */
-    } else {
-        result = update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL;
+        return CMD_CONTINUE;
     }
 
-    return result;
+    return update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL;
 }
 
 static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
-- 
2.25.1

[PATCH v2 10/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapd()

[Peter Maydell]

Fix process_mapd() to consistently return CMD_STALL for memory
errors and CMD_CONTINUE for parameter errors, as we claim in the
comments that we do.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 80ef4dbcadf..917201c148f 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -597,7 +597,6 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
     uint64_t itt_addr;
     bool valid;
     MemTxResult res = MEMTX_OK;
-    ItsCmdResult result = CMD_STALL;
 
     devid = ((value & DEVID_MASK) >> DEVID_SHIFT);
 
@@ -606,7 +605,7 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
                                  MEMTXATTRS_UNSPECIFIED, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     size = (value & SIZE_MASK);
@@ -616,7 +615,7 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
                                  MEMTXATTRS_UNSPECIFIED, &res);
 
     if (res != MEMTX_OK) {
-        return result;
+        return CMD_STALL;
     }
 
     itt_addr = (value & ITTADDR_MASK) >> ITTADDR_SHIFT;
@@ -633,11 +632,10 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
          * we ignore this command and move onto the next
          * command in the queue
          */
-    } else {
-        result = update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
+        return CMD_CONTINUE;
     }
 
-    return result;
+    return update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
 }
 
 /*
-- 
2.25.1

[PATCH v2 11/13] hw/intc/arm_gicv3_its: Factor out "find address of table entry" code

[Peter Maydell]

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>
Reviewed-by: Richard Henderson <richard.henderson@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 917201c148f..985e316eda9 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);
 }
 
 /*
@@ -426,11 +415,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;
 
@@ -444,44 +429,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)
@@ -529,11 +488,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;
 
@@ -548,44 +503,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,
-- 
2.25.1

Re: [PATCH v2 11/13] 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 12/13] hw/intc/arm_gicv3_its: Check indexes before use, not after

[Peter Maydell]

In a few places in the ITS command handling functions, we were
doing the range-check of an event ID or device ID only after using
it as a table index; move the checks to before the uses.

This misordering wouldn't have very bad effects because the
tables are in guest memory anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 42 ++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 985e316eda9..ef6c0f55ff9 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -255,6 +255,13 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
 
     eventid = (value & EVENTID_MASK);
 
+    if (devid >= s->dt.num_ids) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: devid %d>=%d",
+                      __func__, devid, s->dt.num_ids);
+        return CMD_CONTINUE;
+    }
+
     dte = get_dte(s, devid, &res);
 
     if (res != MEMTX_OK) {
@@ -272,6 +279,14 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
 
     num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
 
+    if (eventid >= num_eventids) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: eventid %d >= %"
+                      PRId64 "\n",
+                      __func__, eventid, num_eventids);
+        return CMD_CONTINUE;
+    }
+
     ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
     if (res != MEMTX_OK) {
         return CMD_STALL;
@@ -296,20 +311,6 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         return CMD_CONTINUE;
     }
 
-    if (devid >= s->dt.num_ids) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid command attributes: devid %d>=%d",
-                      __func__, devid, s->dt.num_ids);
-        return CMD_CONTINUE;
-    }
-    if (eventid >= num_eventids) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid command attributes: eventid %d >= %"
-                      PRId64 "\n",
-                      __func__, eventid, num_eventids);
-        return CMD_CONTINUE;
-    }
-
     /*
      * Current implementation only supports rdbase == procnum
      * Hence rdbase physical address is ignored
@@ -375,6 +376,13 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
 
     icid = value & ICID_MASK;
 
+    if (devid >= s->dt.num_ids) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: devid %d>=%d",
+                      __func__, devid, s->dt.num_ids);
+        return CMD_CONTINUE;
+    }
+
     dte = get_dte(s, devid, &res);
 
     if (res != MEMTX_OK) {
@@ -384,14 +392,14 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
     num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
     num_intids = 1ULL << (GICD_TYPER_IDBITS + 1);
 
-    if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
+    if ((icid >= s->ct.num_ids)
             || !dte_valid || (eventid >= num_eventids) ||
             (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) &&
              (pIntid != INTID_SPURIOUS))) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes "
-                      "devid %d or icid %d or eventid %d or pIntid %d or"
-                      "unmapped dte %d\n", __func__, devid, icid, eventid,
+                      "icid %d or eventid %d or pIntid %d or"
+                      "unmapped dte %d\n", __func__, icid, eventid,
                       pIntid, dte_valid);
         /*
          * in this implementation, in case of error
-- 
2.25.1

Re: [PATCH v2 12/13] hw/intc/arm_gicv3_its: Check indexes before use, not after

[Alex Bennée]

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

> In a few places in the ITS command handling functions, we were
> doing the range-check of an event ID or device ID only after using
> it as a table index; move the checks to before the uses.
>
> This misordering wouldn't have very bad effects because the
> tables are in guest memory anyway.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 12/13] hw/intc/arm_gicv3_its: Check indexes before use, not after

[Richard Henderson]

On 1/12/22 04:10, Peter Maydell wrote:
> In a few places in the ITS command handling functions, we were
> doing the range-check of an event ID or device ID only after using
> it as a table index; move the checks to before the uses.
> 
> This misordering wouldn't have very bad effects because the
> tables are in guest memory anyway.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 42 ++++++++++++++++++++++++-----------------
>   1 file changed, 25 insertions(+), 17 deletions(-)

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

r~

[PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

[Peter Maydell]

In process_its_cmd(), we read an ICID out of the interrupt table
entry, and then use it as an index into the collection table.  Add a
check that it is within range for the collection table first.

This check is not strictly necessary, because:
 * we range check the ICID from the guest before writing it into
   the interrupt table entry, so the the only way to get an
   out of range ICID in process_its_cmd() is if a badly-behaved
   guest is writing directly to the interrupt table memory
 * the collection table is in guest memory, so QEMU won't fall
   over if we read off the end of it

However, it seems clearer to include the check.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index ef6c0f55ff9..b2f6a8c7f00 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -299,6 +299,13 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         return CMD_CONTINUE;
     }
 
+    if (icid >= s->ct.num_ids) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
+                      __func__, icid);
+        return CMD_CONTINUE;
+    }
+
     cte_valid = get_cte(s, icid, &cte, &res);
     if (res != MEMTX_OK) {
         return CMD_STALL;
-- 
2.25.1

Re: [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

[Alex Bennée]

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

> In process_its_cmd(), we read an ICID out of the interrupt table
> entry, and then use it as an index into the collection table.  Add a
> check that it is within range for the collection table first.
>
> This check is not strictly necessary, because:
>  * we range check the ICID from the guest before writing it into
>    the interrupt table entry, so the the only way to get an
>    out of range ICID in process_its_cmd() is if a badly-behaved
>    guest is writing directly to the interrupt table memory
>  * the collection table is in guest memory, so QEMU won't fall
>    over if we read off the end of it
>
> However, it seems clearer to include the check.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

[Richard Henderson]

On 1/12/22 04:10, Peter Maydell wrote:
> In process_its_cmd(), we read an ICID out of the interrupt table
> entry, and then use it as an index into the collection table.  Add a
> check that it is within range for the collection table first.
> 
> This check is not strictly necessary, because:
>   * we range check the ICID from the guest before writing it into
>     the interrupt table entry, so the the only way to get an
>     out of range ICID in process_its_cmd() is if a badly-behaved
>     guest is writing directly to the interrupt table memory
>   * the collection table is in guest memory, so QEMU won't fall
>     over if we read off the end of it
> 
> However, it seems clearer to include the check.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 7 +++++++
>   1 file changed, 7 insertions(+)

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

r~

Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Alex Bennée]

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

> I've been working on the ITS to add support for the GICv4 functionality.
> In the course of that I found a handful of bugs in it and also some
> places where the code benefited from refactoring to make it a better
> base to put in the GICv4 parts. This patchset is just the bugfixes
> and cleanups, because there are enough patches here that I figured it
> made sense to send them out now rather than holding on to them.
>
> Most of these patches were in v1 and have been reviewed already.

I've reviewed the patches and they look good to me. kvm-unit-tests is
still failing some tests but the ones it fails hasn't changed from
before this patch:

  ✗  env QEMU=$HOME/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 ./run_tests.sh -g gic
  PASS gicv2-ipi (3 tests)
  PASS gicv2-mmio (17 tests, 1 skipped)
  FAIL gicv2-mmio-up (17 tests, 2 unexpected failures)
  FAIL gicv2-mmio-3p (17 tests, 3 unexpected failures)
  PASS gicv3-ipi (3 tests)
  PASS gicv2-active (1 tests)
  PASS gicv3-active (1 tests)

That said running with -d unimp,guest_errors there are some things that
should probably be double checked, e.g.:

  /home/alex/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 -nodefaults -machine virt -accel tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=
  ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/gic.flat -machine gic-version=2 -smp 1 -append "mmio" -d unimp,guest_errors
  PASS: gicv2: mmio: all CPUs have interrupts
  gic_dist_readb: Bad offset 8           
  gic_dist_readb: Bad offset 9                                                          
  gic_dist_readb: Bad offset a                                                          
  gic_dist_readb: Bad offset b                                                          
  INFO: gicv2: mmio: IIDR: 0x00000000                                                   
  gic_dist_writeb: Bad offset 4                                                         
  gic_dist_writeb: Bad offset 5                                                         
  gic_dist_writeb: Bad offset 6 
  gic_dist_writeb: Bad offset 7 
  gic_dist_writeb: Bad offset 4 
  gic_dist_writeb: Bad offset 5 
  gic_dist_writeb: Bad offset 6  
  gic_dist_writeb: Bad offset 7  
  gic_dist_writeb: Bad offset 4  
  gic_dist_writeb: Bad offset 5  
  gic_dist_writeb: Bad offset 6 
  gic_dist_writeb: Bad offset 7 
  PASS: gicv2: mmio: GICD_TYPER is read-only
  gic_dist_readb: Bad offset 8  
  gic_dist_readb: Bad offset 9   
  gic_dist_readb: Bad offset a   
  gic_dist_readb: Bad offset b   
  gic_dist_writeb: Bad offset 8  
  gic_dist_writeb: Bad offset 9 
  gic_dist_writeb: Bad offset a 
  gic_dist_writeb: Bad offset b 
  gic_dist_readb: Bad offset 8  
  gic_dist_readb: Bad offset 9                                                          
  gic_dist_readb: Bad offset a                                                          
  gic_dist_readb: Bad offset b                                                          
  gic_dist_writeb: Bad offset 8                                                         
  gic_dist_writeb: Bad offset 9                                                         
  gic_dist_writeb: Bad offset a                                                         
  gic_dist_writeb: Bad offset b                                                         
  gic_dist_readb: Bad offset 8                                                          
  gic_dist_readb: Bad offset 9                                                          
  gic_dist_readb: Bad offset a                                                          
  gic_dist_readb: Bad offset b                      
  PASS: gicv2: mmio: GICD_IIDR is read-only                                             
  gic_dist_writeb: Bad offset fe8                                                       
  gic_dist_writeb: Bad offset fe9         
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  gic_dist_writeb: Bad offset fe8
  gic_dist_writeb: Bad offset fe9
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  gic_dist_writeb: Bad offset fe8
  gic_dist_writeb: Bad offset fe9
  gic_dist_writeb: Bad offset fea
  gic_dist_writeb: Bad offset feb
  PASS: gicv2: mmio: ICPIDR2 is read-only
  INFO: gicv2: mmio: value of ICPIDR2: 0x0000002b
  PASS: gicv2: mmio: IPRIORITYR: consistent priority masking
  INFO: gicv2: mmio: IPRIORITYR: priority mask is 0xffffffff
  PASS: gicv2: mmio: IPRIORITYR: implements at least 4 priority bits
  INFO: gicv2: mmio: IPRIORITYR: 8 priority bits implemented
  PASS: gicv2: mmio: IPRIORITYR: clearing priorities
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  gic_dist_writeb: Bad offset 520
  gic_dist_writeb: Bad offset 521
  gic_dist_writeb: Bad offset 522
  gic_dist_writeb: Bad offset 523
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  gic_dist_writeb: Bad offset 520
  gic_dist_writeb: Bad offset 521
  gic_dist_writeb: Bad offset 522
  gic_dist_writeb: Bad offset 523
  gic_dist_readb: Bad offset 520
  gic_dist_readb: Bad offset 521
  gic_dist_readb: Bad offset 522
  gic_dist_readb: Bad offset 523
  PASS: gicv2: mmio: IPRIORITYR: accesses beyond limit RAZ/WI
  PASS: gicv2: mmio: IPRIORITYR: accessing last SPIs
  PASS: gicv2: mmio: IPRIORITYR: priorities are preserved
  PASS: gicv2: mmio: IPRIORITYR: byte reads successful
  PASS: gicv2: mmio: IPRIORITYR: byte writes successful
  PASS: gicv2: mmio: ITARGETSR: bits for non-existent CPUs masked
  INFO: gicv2: mmio: ITARGETSR: 7 non-existent CPUs
  PASS: gicv2: mmio: ITARGETSR: accesses beyond limit RAZ/WI
  FAIL: gicv2: mmio: ITARGETSR: register content preserved
  INFO: gicv2: mmio: ITARGETSR: writing 01010001 reads back as 00000000
  PASS: gicv2: mmio: ITARGETSR: byte reads successful
  FAIL: gicv2: mmio: ITARGETSR: byte writes successful
  INFO: gicv2: mmio: ITARGETSR: writing 0x1f into bytes 2 => 0x00000000
  SUMMARY: 17 tests, 2 unexpected failures


>
> Changes from v1:
>  * first half of the series is now upstream
>  * patch 1 now has the '1ULL' and uint64_t fixes that were
>    partly split across two patches in the old series and partly missing
>  * new patches 12 and 13
>
> NB: I left the returns of -1 in patch 11.
>
> Patches still needing review: 1, 12, 13
>
> thanks
> -- PMM
>
> Peter Maydell (13):
>   hw/intc/arm_gicv3_its: Fix event ID bounds checks
>   hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention
>   hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value
>   hw/intc/arm_gicv3_its: Don't use data if reading command failed
>   hw/intc/arm_gicv3_its: Use enum for return value of process_*
>     functions
>   hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd()
>   hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapti()
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapc()
>   hw/intc/arm_gicv3_its: Fix return codes in process_mapd()
>   hw/intc/arm_gicv3_its: Factor out "find address of table entry" code
>   hw/intc/arm_gicv3_its: Check indexes before use, not after
>   hw/intc/arm_gicv3_its: Range-check ICID before indexing into
>     collection table
>
>  hw/intc/arm_gicv3_its.c | 492 ++++++++++++++++++----------------------
>  1 file changed, 225 insertions(+), 267 deletions(-)


-- 
Alex Bennée

Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Peter Maydell]

On Tue, 18 Jan 2022 at 17:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > I've been working on the ITS to add support for the GICv4 functionality.
> > In the course of that I found a handful of bugs in it and also some
> > places where the code benefited from refactoring to make it a better
> > base to put in the GICv4 parts. This patchset is just the bugfixes
> > and cleanups, because there are enough patches here that I figured it
> > made sense to send them out now rather than holding on to them.
> >
> > Most of these patches were in v1 and have been reviewed already.
>
> I've reviewed the patches and they look good to me. kvm-unit-tests is
> still failing some tests but the ones it fails hasn't changed from
> before this patch:
>
>   ✗  env QEMU=$HOME/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 ./run_tests.sh -g gic
>   PASS gicv2-ipi (3 tests)
>   PASS gicv2-mmio (17 tests, 1 skipped)
>   FAIL gicv2-mmio-up (17 tests, 2 unexpected failures)
>   FAIL gicv2-mmio-3p (17 tests, 3 unexpected failures)
>   PASS gicv3-ipi (3 tests)
>   PASS gicv2-active (1 tests)
>   PASS gicv3-active (1 tests)
>
> That said running with -d unimp,guest_errors there are some things that
> should probably be double checked, e.g.:

Almost all of the logging seems to be where the test code is
doing stuff that the GIC spec says isn't valid. Also, this
test is gicv2, which is unrelated to either the gicv3 code
or to the ITS...

>   /home/alex/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 -nodefaults -machine virt -accel tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=
>   ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/gic.flat -machine gic-version=2 -smp 1 -append "mmio" -d unimp,guest_errors
>   PASS: gicv2: mmio: all CPUs have interrupts
>   gic_dist_readb: Bad offset 8
>   gic_dist_readb: Bad offset 9
>   gic_dist_readb: Bad offset a
>   gic_dist_readb: Bad offset b

This is GICD_IIDR, which is a 32-bit register. The test looks like it's
trying to read it as 4 separate bytes, which is out of spec, and
is why our implementation is warning about it.

>   INFO: gicv2: mmio: IIDR: 0x00000000
>   gic_dist_writeb: Bad offset 4
>   gic_dist_writeb: Bad offset 5
>   gic_dist_writeb: Bad offset 6
>   gic_dist_writeb: Bad offset 7
>   gic_dist_writeb: Bad offset 4
>   gic_dist_writeb: Bad offset 5
>   gic_dist_writeb: Bad offset 6
>   gic_dist_writeb: Bad offset 7
>   gic_dist_writeb: Bad offset 4
>   gic_dist_writeb: Bad offset 5
>   gic_dist_writeb: Bad offset 6
>   gic_dist_writeb: Bad offset 7

These complaints are because the test is trying to write
to GICD_TYPER, which is not permitted.

>   PASS: gicv2: mmio: GICD_TYPER is read-only
>   gic_dist_readb: Bad offset 8
>   gic_dist_readb: Bad offset 9
>   gic_dist_readb: Bad offset a
>   gic_dist_readb: Bad offset b

More attempts to do byte accesses to a word-only register.

>   gic_dist_writeb: Bad offset 8
>   gic_dist_writeb: Bad offset 9
>   gic_dist_writeb: Bad offset a
>   gic_dist_writeb: Bad offset b
>   gic_dist_readb: Bad offset 8
>   gic_dist_readb: Bad offset 9
>   gic_dist_readb: Bad offset a
>   gic_dist_readb: Bad offset b
>   gic_dist_writeb: Bad offset 8
>   gic_dist_writeb: Bad offset 9
>   gic_dist_writeb: Bad offset a
>   gic_dist_writeb: Bad offset b
>   gic_dist_readb: Bad offset 8
>   gic_dist_readb: Bad offset 9
>   gic_dist_readb: Bad offset a
>   gic_dist_readb: Bad offset b
>   PASS: gicv2: mmio: GICD_IIDR is read-only
>   gic_dist_writeb: Bad offset fe8
>   gic_dist_writeb: Bad offset fe9
>   gic_dist_writeb: Bad offset fea
>   gic_dist_writeb: Bad offset feb
>   gic_dist_writeb: Bad offset fe8
>   gic_dist_writeb: Bad offset fe9
>   gic_dist_writeb: Bad offset fea
>   gic_dist_writeb: Bad offset feb
>   gic_dist_writeb: Bad offset fe8
>   gic_dist_writeb: Bad offset fe9
>   gic_dist_writeb: Bad offset fea
>   gic_dist_writeb: Bad offset feb

Writing bytes to a register that is both read-only and also 32-bit only.

>   PASS: gicv2: mmio: ICPIDR2 is read-only
>   INFO: gicv2: mmio: value of ICPIDR2: 0x0000002b
>   PASS: gicv2: mmio: IPRIORITYR: consistent priority masking
>   INFO: gicv2: mmio: IPRIORITYR: priority mask is 0xffffffff
>   PASS: gicv2: mmio: IPRIORITYR: implements at least 4 priority bits
>   INFO: gicv2: mmio: IPRIORITYR: 8 priority bits implemented
>   PASS: gicv2: mmio: IPRIORITYR: clearing priorities
>   gic_dist_readb: Bad offset 520
>   gic_dist_readb: Bad offset 521
>   gic_dist_readb: Bad offset 522
>   gic_dist_readb: Bad offset 523
>   gic_dist_writeb: Bad offset 520
>   gic_dist_writeb: Bad offset 521
>   gic_dist_writeb: Bad offset 522
>   gic_dist_writeb: Bad offset 523
>   gic_dist_readb: Bad offset 520
>   gic_dist_readb: Bad offset 521
>   gic_dist_readb: Bad offset 522
>   gic_dist_readb: Bad offset 523
>   gic_dist_writeb: Bad offset 520
>   gic_dist_writeb: Bad offset 521
>   gic_dist_writeb: Bad offset 522
>   gic_dist_writeb: Bad offset 523
>   gic_dist_readb: Bad offset 520
>   gic_dist_readb: Bad offset 521
>   gic_dist_readb: Bad offset 522
>   gic_dist_readb: Bad offset 523

I suspect from what the following test says that this is an
attempt to write beyond the end of the valid IPRIORITYR registers,
which isn't permitted.

>   PASS: gicv2: mmio: IPRIORITYR: accesses beyond limit RAZ/WI
>   PASS: gicv2: mmio: IPRIORITYR: accessing last SPIs
>   PASS: gicv2: mmio: IPRIORITYR: priorities are preserved
>   PASS: gicv2: mmio: IPRIORITYR: byte reads successful
>   PASS: gicv2: mmio: IPRIORITYR: byte writes successful
>   PASS: gicv2: mmio: ITARGETSR: bits for non-existent CPUs masked
>   INFO: gicv2: mmio: ITARGETSR: 7 non-existent CPUs
>   PASS: gicv2: mmio: ITARGETSR: accesses beyond limit RAZ/WI
>   FAIL: gicv2: mmio: ITARGETSR: register content preserved
>   INFO: gicv2: mmio: ITARGETSR: writing 01010001 reads back as 00000000
>   PASS: gicv2: mmio: ITARGETSR: byte reads successful
>   FAIL: gicv2: mmio: ITARGETSR: byte writes successful
>   INFO: gicv2: mmio: ITARGETSR: writing 0x1f into bytes 2 => 0x00000000
>   SUMMARY: 17 tests, 2 unexpected failures

These ITARGETSR failures are not correct (or you're not running the
test case the way it's supposed to be). Your command line runs
only one CPU, and for a uniprocessor GIC the ITARGETRSn registers
are supposed to be RAZ/WI, whereas the test seems to be expecting
something else.

thanks
-- PMM

Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Andre Przywara]

On Tue, 18 Jan 2022 19:41:56 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

Hi Peter, Alex,

thanks for the heads up!

> On Tue, 18 Jan 2022 at 17:42, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >  
> > > I've been working on the ITS to add support for the GICv4 functionality.
> > > In the course of that I found a handful of bugs in it and also some
> > > places where the code benefited from refactoring to make it a better
> > > base to put in the GICv4 parts. This patchset is just the bugfixes
> > > and cleanups, because there are enough patches here that I figured it
> > > made sense to send them out now rather than holding on to them.
> > >
> > > Most of these patches were in v1 and have been reviewed already.  
> >
> > I've reviewed the patches and they look good to me. kvm-unit-tests is
> > still failing some tests but the ones it fails hasn't changed from
> > before this patch:
> >
> >   ✗  env QEMU=$HOME/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 ./run_tests.sh -g gic
> >   PASS gicv2-ipi (3 tests)
> >   PASS gicv2-mmio (17 tests, 1 skipped)
> >   FAIL gicv2-mmio-up (17 tests, 2 unexpected failures)
> >   FAIL gicv2-mmio-3p (17 tests, 3 unexpected failures)
> >   PASS gicv3-ipi (3 tests)
> >   PASS gicv2-active (1 tests)
> >   PASS gicv3-active (1 tests)
> >
> > That said running with -d unimp,guest_errors there are some things that
> > should probably be double checked, e.g.:  
> 
> Almost all of the logging seems to be where the test code is
> doing stuff that the GIC spec says isn't valid.

That sounds like a plausible explanation for a unit test suite, but
does not seem to be actually true in this case, see below.

> Also, this
> test is gicv2, which is unrelated to either the gicv3 code
> or to the ITS...

This is true.

> 
> >   /home/alex/lsrc/qemu.git/builds/arm.all/qemu-system-aarch64 -nodefaults -machine virt -accel tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=
> >   ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/gic.flat -machine gic-version=2 -smp 1 -append "mmio" -d unimp,guest_errors
> >   PASS: gicv2: mmio: all CPUs have interrupts
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b  
> 
> This is GICD_IIDR, which is a 32-bit register. The test looks like it's
> trying to read it as 4 separate bytes, which is out of spec, and
> is why our implementation is warning about it.

Looking at k-u-t's arm/gic.c and QEMU's hw/intc/arm_gic.c I see two
problems here: QEMU implements word accesses as four successive calls to
gic_dist_readb() - which is probably fine if that helps code design,
but it won't allow it to actually spot access size issues. I just
remember that we spent some brain cells and CPP macros on getting the
access size right in KVM - hence those tests in kvm-unit-tests.
 
But more importantly it looks like GICD_IIDR is actually not
implemented: There is a dubious "if (offset < 0x08) return 0;" line,
but IIDR (offset 0x8) would actually fall through, and hit the bad_reg
label, which would return 0 (and cause the message, if enabled).
Also the name and how it's called suggests that this deals with bytes
only, but returns uint32_t, and for instance deals with bit 10 in
TYPER. I see how this eventually falls into place magically (the upper
three bytes return 0, and get ORed into the >8 bit result of offset 8),
but those messages are definitely false alarm then.

If that helps: from a GIC MMIO perspective 8-bit accesses are actually
the exception rather than the norm (ARM IHI 0048B.b 4.1.4 GIC register
access).

> >   INFO: gicv2: mmio: IIDR: 0x00000000
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7
> >   gic_dist_writeb: Bad offset 4
> >   gic_dist_writeb: Bad offset 5
> >   gic_dist_writeb: Bad offset 6
> >   gic_dist_writeb: Bad offset 7  
> 
> These complaints are because the test is trying to write
> to GICD_TYPER, which is not permitted.

Writes are not permitted, yes, but k-u-t emits a proper str, so there
should be only three lines, not twelve.

> >   PASS: gicv2: mmio: GICD_TYPER is read-only
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b  
> 
> More attempts to do byte accesses to a word-only register.

The messages come actually again because IIDR is not handled at all,
and there are only four of them because of the design of gic_dist_read().
k-u-t issues a proper ldr here.
I think we refrained from actually testing illegal access sizes,
because that could trigger external aborts/SErrors on real hardware.

> >   gic_dist_writeb: Bad offset 8
> >   gic_dist_writeb: Bad offset 9
> >   gic_dist_writeb: Bad offset a
> >   gic_dist_writeb: Bad offset b
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b
> >   gic_dist_writeb: Bad offset 8
> >   gic_dist_writeb: Bad offset 9
> >   gic_dist_writeb: Bad offset a
> >   gic_dist_writeb: Bad offset b
> >   gic_dist_readb: Bad offset 8
> >   gic_dist_readb: Bad offset 9
> >   gic_dist_readb: Bad offset a
> >   gic_dist_readb: Bad offset b
> >   PASS: gicv2: mmio: GICD_IIDR is read-only
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb
> >   gic_dist_writeb: Bad offset fe8
> >   gic_dist_writeb: Bad offset fe9
> >   gic_dist_writeb: Bad offset fea
> >   gic_dist_writeb: Bad offset feb  
> 
> Writing bytes to a register that is both read-only and also 32-bit only.

Yes, the read-only violation is expected, but the code only does ldr.

> >   PASS: gicv2: mmio: ICPIDR2 is read-only
> >   INFO: gicv2: mmio: value of ICPIDR2: 0x0000002b
> >   PASS: gicv2: mmio: IPRIORITYR: consistent priority masking
> >   INFO: gicv2: mmio: IPRIORITYR: priority mask is 0xffffffff
> >   PASS: gicv2: mmio: IPRIORITYR: implements at least 4 priority bits
> >   INFO: gicv2: mmio: IPRIORITYR: 8 priority bits implemented
> >   PASS: gicv2: mmio: IPRIORITYR: clearing priorities
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523
> >   gic_dist_writeb: Bad offset 520
> >   gic_dist_writeb: Bad offset 521
> >   gic_dist_writeb: Bad offset 522
> >   gic_dist_writeb: Bad offset 523
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523
> >   gic_dist_writeb: Bad offset 520
> >   gic_dist_writeb: Bad offset 521
> >   gic_dist_writeb: Bad offset 522
> >   gic_dist_writeb: Bad offset 523
> >   gic_dist_readb: Bad offset 520
> >   gic_dist_readb: Bad offset 521
> >   gic_dist_readb: Bad offset 522
> >   gic_dist_readb: Bad offset 523  
> 
> I suspect from what the following test says that this is an
> attempt to write beyond the end of the valid IPRIORITYR registers,
> which isn't permitted.

Trying to manipulate non-implemented SPIs is not really useful (and
indeed typically points to guest bugs), but it is permitted by the
GICv2 spec, which says: "A register field corresponding to an
unimplemented interrupt is RAZ/WI." - which is actually what bad_reg
implements - just minus the message.

> >   PASS: gicv2: mmio: IPRIORITYR: accesses beyond limit RAZ/WI
> >   PASS: gicv2: mmio: IPRIORITYR: accessing last SPIs
> >   PASS: gicv2: mmio: IPRIORITYR: priorities are preserved
> >   PASS: gicv2: mmio: IPRIORITYR: byte reads successful
> >   PASS: gicv2: mmio: IPRIORITYR: byte writes successful
> >   PASS: gicv2: mmio: ITARGETSR: bits for non-existent CPUs masked
> >   INFO: gicv2: mmio: ITARGETSR: 7 non-existent CPUs
> >   PASS: gicv2: mmio: ITARGETSR: accesses beyond limit RAZ/WI
> >   FAIL: gicv2: mmio: ITARGETSR: register content preserved
> >   INFO: gicv2: mmio: ITARGETSR: writing 01010001 reads back as 00000000
> >   PASS: gicv2: mmio: ITARGETSR: byte reads successful
> >   FAIL: gicv2: mmio: ITARGETSR: byte writes successful
> >   INFO: gicv2: mmio: ITARGETSR: writing 0x1f into bytes 2 => 0x00000000
> >   SUMMARY: 17 tests, 2 unexpected failures  
> 
> These ITARGETSR failures are not correct (or you're not running the
> test case the way it's supposed to be). Your command line runs
> only one CPU, and for a uniprocessor GIC the ITARGETRSn registers
> are supposed to be RAZ/WI, whereas the test seems to be expecting
> something else.

Interesting, indeed the *whole* of GICD_ITARGETSRs are RAZ/WI on a
uniprocessor implementation, where is also says that bits for
non-implemented CPU interfaces as RAZ/WI, which would suggest that at
least bit 0 is preserved (what this test checks).
I will double check the spec again on what uniprocessor means
precisely, and then send a kvm-unit-tests patch.

But running with -smp [2..8] still reports issues - but we know of these
for a while, I think, and they are not really critical.

Cheers,
Andre

Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Peter Maydell]

On Tue, 18 Jan 2022 at 23:30, Andre Przywara <andre.przywara@arm.com> wrote:
> Looking at k-u-t's arm/gic.c and QEMU's hw/intc/arm_gic.c I see two
> problems here: QEMU implements word accesses as four successive calls to
> gic_dist_readb() - which is probably fine if that helps code design,
> but it won't allow it to actually spot access size issues. I just
> remember that we spent some brain cells and CPP macros on getting the
> access size right in KVM - hence those tests in kvm-unit-tests.

Thanks for looking at this. I should have read the code rather
than dashing off a reply last thing in the evening based just
on the test case output! I think I was confusing how our GICv3
emulation handles register accesses (with separate functions for
byte/halfword/word/quad accesses) with the GICv2 emulation
(which as you say calls down into the byte emulation code
wherever it can).

> But more importantly it looks like GICD_IIDR is actually not
> implemented: There is a dubious "if (offset < 0x08) return 0;" line,
> but IIDR (offset 0x8) would actually fall through, and hit the bad_reg
> label, which would return 0 (and cause the message, if enabled).

Mmm. I actually have a patch in target-arm.next from Petr Pavlu
which implements GICC_IIDR, but we do indeed not implement the
distributor equivalent.

> If that helps: from a GIC MMIO perspective 8-bit accesses are actually
> the exception rather than the norm (ARM IHI 0048B.b 4.1.4 GIC register
> access).

Yes. We got this right in the GICv3 emulation design, where almost
all the logic is in the 32-bit accessor functions and the 8/16-bit
functions deal only with the very few registers that have to
permit non-word accesses. The GICv2 code is a lot older (and to
be fair to it, started out as 11MPcore interrupt controller
emulation, and I bet the docs of that were not very specific about
what registers could or could not be accessed byte at a time).
Unless we want to rewrite all that logic in the GICv2 emulation
(which I at least do not :-)) I think we'll have to live with
the warnings about bad-offsets reporting for each byte rather
than just once for the word access.

-- PMM

Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Andre Przywara]

On Wed, 19 Jan 2022 10:15:52 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

Hi Peter,

> On Tue, 18 Jan 2022 at 23:30, Andre Przywara <andre.przywara@arm.com> wrote:
> > Looking at k-u-t's arm/gic.c and QEMU's hw/intc/arm_gic.c I see two
> > problems here: QEMU implements word accesses as four successive calls to
> > gic_dist_readb() - which is probably fine if that helps code design,
> > but it won't allow it to actually spot access size issues. I just
> > remember that we spent some brain cells and CPP macros on getting the
> > access size right in KVM - hence those tests in kvm-unit-tests.  
> 
> Thanks for looking at this. I should have read the code rather
> than dashing off a reply last thing in the evening based just
> on the test case output! I think I was confusing how our GICv3
> emulation handles register accesses (with separate functions for
> byte/halfword/word/quad accesses) with the GICv2 emulation
> (which as you say calls down into the byte emulation code
> wherever it can).

No worries!

> > But more importantly it looks like GICD_IIDR is actually not
> > implemented: There is a dubious "if (offset < 0x08) return 0;" line,
> > but IIDR (offset 0x8) would actually fall through, and hit the bad_reg
> > label, which would return 0 (and cause the message, if enabled).  
> 
> Mmm. I actually have a patch in target-arm.next from Petr Pavlu
> which implements GICC_IIDR, but we do indeed not implement the
> distributor equivalent.

Well, returning 0 is actually not the worst idea. Using proper ID
values might not even be feasible for QEMU, or would create some hassle
with versioning. With 0 all a user can assume is spec compliance.

> > If that helps: from a GIC MMIO perspective 8-bit accesses are actually
> > the exception rather than the norm (ARM IHI 0048B.b 4.1.4 GIC register
> > access).  
> 
> Yes. We got this right in the GICv3 emulation design, where almost
> all the logic is in the 32-bit accessor functions and the 8/16-bit
> functions deal only with the very few registers that have to
> permit non-word accesses.

Indeed. I dusted off my old GICv3 MMIO patches for kvm-unit-tests, and
QEMU passed with flying colours. With the debug switch I see it
reporting exactly the violating accesses we except to see.
Will send those patches ASAP.

> The GICv2 code is a lot older (and to
> be fair to it, started out as 11MPcore interrupt controller
> emulation, and I bet the docs of that were not very specific about
> what registers could or could not be accessed byte at a time).
> Unless we want to rewrite all that logic in the GICv2 emulation
> (which I at least do not :-))

... and I can't ...

> I think we'll have to live with
> the warnings about bad-offsets reporting for each byte rather
> than just once for the word access.

Yeah, if those warnings appear only with that debug switch, there is
probably little reason to change that code just because of this. At
least it seemed to work quite well over the years.

Cheers,
Andre

P.S. I changed k-u-t to special case the UP case, so that TCG passes.
But now KVM fails, of course. So I will have to make a patch for the
kernel ...