[PATCH 00/14] arm_gicv3_its: Implement MOVI and MOVALL commands

[PATCH 00/14] arm_gicv3_its: Implement MOVI and MOVALL commands

[Peter Maydell]

I noticed while working on the new-in-GICv4 ITS commands
that we'd forgotten to implement the GICv3 MOVI and MOVALL
commands. These move interrupts from one redistributor to
another; Linux doesn't use MOVALL but does use MOVI if
it wants to power down a CPU (when it needs to migrate
interrupts formerly targeting that CPU to a different one).

This patchseries starts with a couple of patches that I
found useful to improve the debug logging when I was
writing this. It then has a collection of fixes for various
minor bugs that I noticed while I was in the code (including
yet another fix to the table lookup bounds checking...).
Finally the last two patches implement the two new commands.

thanks
-- PMM

Peter Maydell (14):
  target/arm: Log CPU index in 'Taking exception' log
  hw/intc/arm_gicv3_its: Add tracepoints
  hw/intc/arm_gicv3: Initialise dma_as in GIC, not ITS
  hw/intc/arm_gicv3_its: Don't clear GITS_CREADR when GITS_CTLR.ENABLED
    is set
  hw/intc/arm_gicv3_its: Don't clear GITS_CWRITER on writes to
    GITS_CBASER
  hw/intc/arm_gicv3: Honour GICD_CTLR.EnableGrp1NS for LPIs
  hw/intc/arm_gicv3_its: Sort ITS command list into numeric order
  hw/intc/arm_gicv3_redist: Remove unnecessary zero checks
  hw/intc/arm_gicv3: Set GICR_CTLR.CES if LPIs are supported
  hw/intc/arm_gicv3_its: Provide read accessor for translation_ops
  hw/intc/arm_gicv3_its: Make GITS_BASER<n> RAZ/WI for unimplemented
    registers
  hw/intc/arm_gicv3_its: Check table bounds against correct limit
  hw/intc/arm_gicv3_its: Implement MOVALL
  hw/intc/arm_gicv3_its: Implement MOVI

 hw/intc/gicv3_internal.h               |  43 ++++-
 include/hw/intc/arm_gicv3_its_common.h |   1 -
 target/arm/internals.h                 |   2 +-
 hw/intc/arm_gicv3.c                    |   1 +
 hw/intc/arm_gicv3_common.c             |   9 +
 hw/intc/arm_gicv3_its.c                | 258 +++++++++++++++++++++++--
 hw/intc/arm_gicv3_redist.c             | 115 ++++++++++-
 target/arm/helper.c                    |   9 +-
 target/arm/m_helper.c                  |   2 +-
 hw/intc/trace-events                   |   8 +
 10 files changed, 416 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH 01/14] target/arm: Log CPU index in 'Taking exception' log

[Peter Maydell]

In an SMP system it can be unclear which CPU is taking an exception;
add the CPU index (which is the same value used in the TCG 'Trace
%d:' logging) to the "Taking exception" log line to clarify it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 2 +-
 target/arm/helper.c    | 9 ++++++---
 target/arm/m_helper.c  | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 89f7610ebc5..3f05748ea47 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1130,7 +1130,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
                    ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs)
     __attribute__((nonnull));
 
-void arm_log_exception(int idx);
+void arm_log_exception(CPUState *cs);
 
 #endif /* !CONFIG_USER_ONLY */
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cfca0f5ba6d..4df12394021 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9317,8 +9317,10 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
     return target_el;
 }
 
-void arm_log_exception(int idx)
+void arm_log_exception(CPUState *cs)
 {
+    int idx = cs->exception_index;
+
     if (qemu_loglevel_mask(CPU_LOG_INT)) {
         const char *exc = NULL;
         static const char * const excnames[] = {
@@ -9352,7 +9354,8 @@ void arm_log_exception(int idx)
         if (!exc) {
             exc = "unknown";
         }
-        qemu_log_mask(CPU_LOG_INT, "Taking exception %d [%s]\n", idx, exc);
+        qemu_log_mask(CPU_LOG_INT, "Taking exception %d [%s] on CPU %d\n",
+                      idx, exc, cs->cpu_index);
     }
 }
 
@@ -10185,7 +10188,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     assert(!arm_feature(env, ARM_FEATURE_M));
 
-    arm_log_exception(cs->exception_index);
+    arm_log_exception(cs);
     qemu_log_mask(CPU_LOG_INT, "...from EL%d to EL%d\n", arm_current_el(env),
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 2c9922dc292..b11e927df1d 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2206,7 +2206,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     uint32_t lr;
     bool ignore_stackfaults;
 
-    arm_log_exception(cs->exception_index);
+    arm_log_exception(cs);
 
     /*
      * For exceptions we just mark as pending on the NVIC, and let that
-- 
2.25.1

Re: [PATCH 01/14] target/arm: Log CPU index in 'Taking exception' log

[Philippe Mathieu-Daudé via]

On 22/1/22 19:24, Peter Maydell wrote:
> In an SMP system it can be unclear which CPU is taking an exception;
> add the CPU index (which is the same value used in the TCG 'Trace
> %d:' logging) to the "Taking exception" log line to clarify it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/internals.h | 2 +-
>   target/arm/helper.c    | 9 ++++++---
>   target/arm/m_helper.c  | 2 +-
>   3 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 01/14] target/arm: Log CPU index in 'Taking exception' log

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> In an SMP system it can be unclear which CPU is taking an exception;
> add the CPU index (which is the same value used in the TCG 'Trace
> %d:' logging) to the "Taking exception" log line to clarify it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/internals.h | 2 +-
>   target/arm/helper.c    | 9 ++++++---
>   target/arm/m_helper.c  | 2 +-
>   3 files changed, 8 insertions(+), 5 deletions(-)

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

r~

[PATCH 02/14] hw/intc/arm_gicv3_its: Add tracepoints

[Peter Maydell]

The ITS currently has no tracepoints; add a minimal set
that allows basic monitoring of guest register accesses and
reading of commands from the command queue.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 11 +++++++++++
 hw/intc/trace-events    |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index b2f6a8c7f00..6d2549e64b1 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "trace.h"
 #include "hw/qdev-properties.h"
 #include "hw/intc/arm_gicv3_its_common.h"
 #include "gicv3_internal.h"
@@ -634,6 +635,8 @@ static void process_cmdq(GICv3ITSState *s)
 
         cmd = (data & CMD_MASK);
 
+        trace_gicv3_its_process_command(rd_offset, cmd);
+
         switch (cmd) {
         case GITS_CMD_INT:
             result = process_its_cmd(s, data, cq_offset, INTERRUPT);
@@ -818,6 +821,8 @@ static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
     bool result = true;
     uint32_t devid = 0;
 
+    trace_gicv3_its_translation_write(offset, data, size, attrs.requester_id);
+
     switch (offset) {
     case GITS_TRANSLATER:
         if (s->ctlr & R_GITS_CTLR_ENABLED_MASK) {
@@ -1107,6 +1112,7 @@ static MemTxResult gicv3_its_read(void *opaque, hwaddr offset, uint64_t *data,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest read at offset " TARGET_FMT_plx
                       "size %u\n", __func__, offset, size);
+        trace_gicv3_its_badread(offset, size);
         /*
          * The spec requires that reserved registers are RAZ/WI;
          * so use false returns from leaf functions as a way to
@@ -1114,6 +1120,8 @@ static MemTxResult gicv3_its_read(void *opaque, hwaddr offset, uint64_t *data,
          * the caller, or we'll cause a spurious guest data abort.
          */
         *data = 0;
+    } else {
+        trace_gicv3_its_read(offset, *data, size);
     }
     return MEMTX_OK;
 }
@@ -1140,12 +1148,15 @@ static MemTxResult gicv3_its_write(void *opaque, hwaddr offset, uint64_t data,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid guest write at offset " TARGET_FMT_plx
                       "size %u\n", __func__, offset, size);
+        trace_gicv3_its_badwrite(offset, data, size);
         /*
          * The spec requires that reserved registers are RAZ/WI;
          * so use false returns from leaf functions as a way to
          * trigger the guest-error logging but don't return it to
          * the caller, or we'll cause a spurious guest data abort.
          */
+    } else {
+        trace_gicv3_its_write(offset, data, size);
     }
     return MEMTX_OK;
 }
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 9aba7e3a7a4..b28cda4e08e 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -169,6 +169,14 @@ gicv3_redist_badwrite(uint32_t cpu, uint64_t offset, uint64_t data, unsigned siz
 gicv3_redist_set_irq(uint32_t cpu, int irq, int level) "GICv3 redistributor 0x%x interrupt %d level changed to %d"
 gicv3_redist_send_sgi(uint32_t cpu, int irq) "GICv3 redistributor 0x%x pending SGI %d"
 
+# arm_gicv3_its.c
+gicv3_its_read(uint64_t offset, uint64_t data, unsigned size) "GICv3 ITS read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
+gicv3_its_badread(uint64_t offset, unsigned size) "GICv3 ITS read: offset 0x%" PRIx64 " size %u: error"
+gicv3_its_write(uint64_t offset, uint64_t data, unsigned size) "GICv3 ITS write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
+gicv3_its_badwrite(uint64_t offset, uint64_t data, unsigned size) "GICv3 ITS write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u: error"
+gicv3_its_translation_write(uint64_t offset, uint64_t data, unsigned size, uint32_t requester_id) "GICv3 ITS TRANSLATER write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u requester_id 0x%x"
+gicv3_its_process_command(uint32_t rd_offset, uint8_t cmd) "GICv3 ITS: processing command at offset 0x%x: 0x%x"
+
 # armv7m_nvic.c
 nvic_recompute_state(int vectpending, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d vectpending_prio %d exception_prio %d"
 nvic_recompute_state_secure(int vectpending, bool vectpending_is_s_banked, int vectpending_prio, int exception_prio) "NVIC state recomputed: vectpending %d is_s_banked %d vectpending_prio %d exception_prio %d"
-- 
2.25.1

Re: [PATCH 02/14] hw/intc/arm_gicv3_its: Add tracepoints

[Philippe Mathieu-Daudé via]

On 22/1/22 19:24, Peter Maydell wrote:
> The ITS currently has no tracepoints; add a minimal set
> that allows basic monitoring of guest register accesses and
> reading of commands from the command queue.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 11 +++++++++++
>   hw/intc/trace-events    |  8 ++++++++
>   2 files changed, 19 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 02/14] hw/intc/arm_gicv3_its: Add tracepoints

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The ITS currently has no tracepoints; add a minimal set
> that allows basic monitoring of guest register accesses and
> reading of commands from the command queue.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 11 +++++++++++
>   hw/intc/trace-events    |  8 ++++++++
>   2 files changed, 19 insertions(+)

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

r~

[PATCH 03/14] hw/intc/arm_gicv3: Initialise dma_as in GIC, not ITS

[Peter Maydell]

In our implementation, all ITSes connected to a GIC share a single
AddressSpace, which we keep in the GICv3State::dma_as field and
initialized based on the GIC's 'sysmem' property. The right place
to set it up by calling address_space_init() is therefore in the
GIC's realize method, not the ITS's realize.

This fixes a theoretical bug where QEMU hangs on startup if the board
model creates two ITSes connected to the same GIC -- we would call
address_space_init() twice on the same AddressSpace*, which creates
an infinite loop in the QTAILQ that softmmu/memory.c uses to store
its list of AddressSpaces and causes any subsequent attempt to
iterate through that list to loop forever.  There aren't any board
models like that in the tree at the moment, though.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_common.c | 5 +++++
 hw/intc/arm_gicv3_its.c    | 3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 9884d2e39b9..579aa0cb9ed 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -357,6 +357,11 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (s->lpi_enable) {
+        address_space_init(&s->dma_as, s->dma,
+                           "gicv3-its-sysmem");
+    }
+
     s->cpu = g_new0(GICv3CPUState, s->num_cpu);
 
     for (i = 0; i < s->num_cpu; i++) {
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 6d2549e64b1..67f12d98af3 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -1194,9 +1194,6 @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
 
     gicv3_its_init_mmio(s, &gicv3_its_control_ops, &gicv3_its_translation_ops);
 
-    address_space_init(&s->gicv3->dma_as, s->gicv3->dma,
-                       "gicv3-its-sysmem");
-
     /* set the ITS default features supported */
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL, 1);
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, ITT_ENTRY_SIZE,
-- 
2.25.1

Re: [PATCH 03/14] hw/intc/arm_gicv3: Initialise dma_as in GIC, not ITS

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> In our implementation, all ITSes connected to a GIC share a single
> AddressSpace, which we keep in the GICv3State::dma_as field and
> initialized based on the GIC's 'sysmem' property. The right place
> to set it up by calling address_space_init() is therefore in the
> GIC's realize method, not the ITS's realize.
> 
> This fixes a theoretical bug where QEMU hangs on startup if the board
> model creates two ITSes connected to the same GIC -- we would call
> address_space_init() twice on the same AddressSpace*, which creates
> an infinite loop in the QTAILQ that softmmu/memory.c uses to store
> its list of AddressSpaces and causes any subsequent attempt to
> iterate through that list to loop forever.  There aren't any board
> models like that in the tree at the moment, though.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_common.c | 5 +++++
>   hw/intc/arm_gicv3_its.c    | 3 ---
>   2 files changed, 5 insertions(+), 3 deletions(-)

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

r~

[PATCH 04/14] hw/intc/arm_gicv3_its: Don't clear GITS_CREADR when GITS_CTLR.ENABLED is set

[Peter Maydell]

The current ITS code clears GITS_CREADR when GITS_CTLR.ENABLED is set.
This is not correct -- guest code can validly clear ENABLED and then
set it again and expect the ITS to continue processing where it left
off. Remove the erroneous assignment.

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

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 67f12d98af3..1763ba4a671 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -853,7 +853,6 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
             s->ctlr |= R_GITS_CTLR_ENABLED_MASK;
             extract_table_params(s);
             extract_cmdq_params(s);
-            s->creadr = 0;
             process_cmdq(s);
         } else {
             s->ctlr &= ~R_GITS_CTLR_ENABLED_MASK;
-- 
2.25.1

Re: [PATCH 04/14] hw/intc/arm_gicv3_its: Don't clear GITS_CREADR when GITS_CTLR.ENABLED is set

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The current ITS code clears GITS_CREADR when GITS_CTLR.ENABLED is set.
> This is not correct -- guest code can validly clear ENABLED and then
> set it again and expect the ITS to continue processing where it left
> off. Remove the erroneous assignment.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 1 -
>   1 file changed, 1 deletion(-)

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

r~

[PATCH 05/14] hw/intc/arm_gicv3_its: Don't clear GITS_CWRITER on writes to GITS_CBASER

[Peter Maydell]

The ITS specification says that when the guest writes to GITS_CBASER
this causes GITS_CREADR to be cleared.  However it does not have an
equivalent clause for GITS_CWRITER.  (This is because GITS_CREADR is
read-only, but GITS_CWRITER is writable and the guest can initialize
it.) Remove the code that clears GITS_CWRITER on GITS_CBASER writes.

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

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 1763ba4a671..d9ff7b88492 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -866,7 +866,6 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
         if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {
             s->cbaser = deposit64(s->cbaser, 0, 32, value);
             s->creadr = 0;
-            s->cwriter = s->creadr;
         }
         break;
     case GITS_CBASER + 4:
@@ -877,7 +876,6 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
         if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {
             s->cbaser = deposit64(s->cbaser, 32, 32, value);
             s->creadr = 0;
-            s->cwriter = s->creadr;
         }
         break;
     case GITS_CWRITER:
@@ -1027,7 +1025,6 @@ static bool its_writell(GICv3ITSState *s, hwaddr offset,
         if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {
             s->cbaser = value;
             s->creadr = 0;
-            s->cwriter = s->creadr;
         }
         break;
     case GITS_CWRITER:
-- 
2.25.1

Re: [PATCH 05/14] hw/intc/arm_gicv3_its: Don't clear GITS_CWRITER on writes to GITS_CBASER

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The ITS specification says that when the guest writes to GITS_CBASER
> this causes GITS_CREADR to be cleared.  However it does not have an
> equivalent clause for GITS_CWRITER.  (This is because GITS_CREADR is
> read-only, but GITS_CWRITER is writable and the guest can initialize
> it.) Remove the code that clears GITS_CWRITER on GITS_CBASER writes.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 3 ---
>   1 file changed, 3 deletions(-)

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

r~

[PATCH 06/14] hw/intc/arm_gicv3: Honour GICD_CTLR.EnableGrp1NS for LPIs

[Peter Maydell]

The GICD_CTLR distributor register has enable bits which control
whether the different interrupt groups (Group 0, Non-secure Group 1
and Secure Group 1) are forwarded to the CPU.  We get this right for
traditional interrupts, but forgot to account for it when adding
LPIs.  LPIs are always Group 1 NS and if the EnableGrp1NS bit is not
set we must not forward them to the CPU.

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

diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 715df5421dd..6d3c8ee231c 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -166,6 +166,7 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
     }
 
     if ((cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) && cs->gic->lpi_enable &&
+        (cs->gic->gicd_ctlr & GICD_CTLR_EN_GRP1NS) &&
         (cs->hpplpi.prio != 0xff)) {
         if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) {
             cs->hppi.irq = cs->hpplpi.irq;
-- 
2.25.1

Re: [PATCH 06/14] hw/intc/arm_gicv3: Honour GICD_CTLR.EnableGrp1NS for LPIs

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The GICD_CTLR distributor register has enable bits which control
> whether the different interrupt groups (Group 0, Non-secure Group 1
> and Secure Group 1) are forwarded to the CPU.  We get this right for
> traditional interrupts, but forgot to account for it when adding
> LPIs.  LPIs are always Group 1 NS and if the EnableGrp1NS bit is not
> set we must not forward them to the CPU.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

[PATCH 07/14] hw/intc/arm_gicv3_its: Sort ITS command list into numeric order

[Peter Maydell]

The list of #defines for the ITS command packet numbers is neither
in alphabetical nor numeric order. Sort it into numeric order.

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

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 1eeb99035da..5394266aaf4 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -314,16 +314,16 @@ FIELD(GITS_TYPER, CIL, 36, 1)
 #define CMD_MASK                  0xff
 
 /* ITS Commands */
-#define GITS_CMD_CLEAR            0x04
-#define GITS_CMD_DISCARD          0x0F
 #define GITS_CMD_INT              0x03
-#define GITS_CMD_MAPC             0x09
+#define GITS_CMD_CLEAR            0x04
+#define GITS_CMD_SYNC             0x05
 #define GITS_CMD_MAPD             0x08
-#define GITS_CMD_MAPI             0x0B
+#define GITS_CMD_MAPC             0x09
 #define GITS_CMD_MAPTI            0x0A
+#define GITS_CMD_MAPI             0x0B
 #define GITS_CMD_INV              0x0C
 #define GITS_CMD_INVALL           0x0D
-#define GITS_CMD_SYNC             0x05
+#define GITS_CMD_DISCARD          0x0F
 
 /* MAPC command fields */
 #define ICID_LENGTH                  16
-- 
2.25.1

Re: [PATCH 07/14] hw/intc/arm_gicv3_its: Sort ITS command list into numeric order

[Philippe Mathieu-Daudé via]

On 22/1/22 19:24, Peter Maydell wrote:
> The list of #defines for the ITS command packet numbers is neither
> in alphabetical nor numeric order. Sort it into numeric order.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/gicv3_internal.h | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 07/14] hw/intc/arm_gicv3_its: Sort ITS command list into numeric order

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The list of #defines for the ITS command packet numbers is neither
> in alphabetical nor numeric order. Sort it into numeric order.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/gicv3_internal.h | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

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

r~

[PATCH 08/14] hw/intc/arm_gicv3_redist: Remove unnecessary zero checks

[Peter Maydell]

The ITS-related parts of the redistributor code make some checks for
whether registers like GICR_PROPBASER and GICR_PENDBASER are zero.
There is no requirement in the specification for treating zeroes in
these address registers specially -- they contain guest physical
addresses and it is entirely valid (if unusual) for the guest to
choose to put the tables they address at guest physical address zero.
We use these values only to calculate guest addresses, and attempts
by the guest to use a bad address will be handled by the
address_space_* functions which we use to do the loads and stores.

Remove the unnecessary checks.

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

diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 99b11ca5eee..d81d8e5f076 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -591,8 +591,7 @@ void gicv3_redist_update_lpi_only(GICv3CPUState *cs)
     idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS),
                  GICD_TYPER_IDBITS);
 
-    if (!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
-        !cs->gicr_pendbaser) {
+    if (!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)) {
         return;
     }
 
@@ -673,9 +672,8 @@ void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
     idbits = MIN(FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, IDBITS),
                  GICD_TYPER_IDBITS);
 
-    if (!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
-         !cs->gicr_pendbaser || (irq > (1ULL << (idbits + 1)) - 1) ||
-         irq < GICV3_LPI_INTID_START) {
+    if (!(cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) ||
+        (irq > (1ULL << (idbits + 1)) - 1) || irq < GICV3_LPI_INTID_START) {
         return;
     }
 
-- 
2.25.1

Re: [PATCH 08/14] hw/intc/arm_gicv3_redist: Remove unnecessary zero checks

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The ITS-related parts of the redistributor code make some checks for
> whether registers like GICR_PROPBASER and GICR_PENDBASER are zero.
> There is no requirement in the specification for treating zeroes in
> these address registers specially -- they contain guest physical
> addresses and it is entirely valid (if unusual) for the guest to
> choose to put the tables they address at guest physical address zero.
> We use these values only to calculate guest addresses, and attempts
> by the guest to use a bad address will be handled by the
> address_space_* functions which we use to do the loads and stores.
> 
> Remove the unnecessary checks.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_redist.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)

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

r~

[PATCH 09/14] hw/intc/arm_gicv3: Set GICR_CTLR.CES if LPIs are supported

[Peter Maydell]

The GICR_CTLR.CES bit is a read-only bit which is set to 1 to indicate
that the GICR_CTLR.EnableLPIs bit can be written to 0 to disable
LPIs (as opposed to allowing LPIs to be enabled but not subsequently
disabled). Our implementation permits this, so advertise it
by setting CES to 1.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/gicv3_internal.h   | 1 +
 hw/intc/arm_gicv3_common.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 5394266aaf4..a316f6c58a5 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -110,6 +110,7 @@
 #define GICR_NSACR            (GICR_SGI_OFFSET + 0x0E00)
 
 #define GICR_CTLR_ENABLE_LPIS        (1U << 0)
+#define GICR_CTLR_CES                (1U << 1)
 #define GICR_CTLR_RWP                (1U << 3)
 #define GICR_CTLR_DPG0               (1U << 24)
 #define GICR_CTLR_DPG1NS             (1U << 25)
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 579aa0cb9ed..4ca5ae9bc56 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -429,6 +429,10 @@ static void arm_gicv3_common_reset(DeviceState *dev)
 
         cs->level = 0;
         cs->gicr_ctlr = 0;
+        if (s->lpi_enable) {
+            /* Our implementation supports clearing GICR_CTLR.EnableLPIs */
+            cs->gicr_ctlr |= GICR_CTLR_CES;
+        }
         cs->gicr_statusr[GICV3_S] = 0;
         cs->gicr_statusr[GICV3_NS] = 0;
         cs->gicr_waker = GICR_WAKER_ProcessorSleep | GICR_WAKER_ChildrenAsleep;
-- 
2.25.1

Re: [PATCH 09/14] hw/intc/arm_gicv3: Set GICR_CTLR.CES if LPIs are supported

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The GICR_CTLR.CES bit is a read-only bit which is set to 1 to indicate
> that the GICR_CTLR.EnableLPIs bit can be written to 0 to disable
> LPIs (as opposed to allowing LPIs to be enabled but not subsequently
> disabled). Our implementation permits this, so advertise it
> by setting CES to 1.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/gicv3_internal.h   | 1 +
>   hw/intc/arm_gicv3_common.c | 4 ++++
>   2 files changed, 5 insertions(+)

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

r~

[PATCH 10/14] hw/intc/arm_gicv3_its: Provide read accessor for translation_ops

[Peter Maydell]

The MemoryRegionOps gicv3_its_translation_ops currently provides only
a .write_with_attrs function, because the only register in this
region is the write-only GITS_TRANSLATER.  However, if you don't
provide a read function and the guest tries reading from this memory
region, QEMU will crash because
memory_region_read_with_attrs_accessor() calls a NULL pointer.

Add a read function which always returns 0, to cover both bogus
attempts to read GITS_TRANSLATER and also reads from the rest of the
region, which is documented to be reserved, RES0.

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

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index d9ff7b88492..b17f2631269 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -813,6 +813,18 @@ static void extract_cmdq_params(GICv3ITSState *s)
     }
 }
 
+static MemTxResult gicv3_its_translation_read(void *opaque, hwaddr offset,
+                                              uint64_t *data, unsigned size,
+                                              MemTxAttrs attrs)
+{
+    /*
+     * GITS_TRANSLATER is write-only, and all other addresses
+     * in the interrupt translation space frame are RES0.
+     */
+    *data = 0;
+    return MEMTX_OK;
+}
+
 static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
                                                uint64_t data, unsigned size,
                                                MemTxAttrs attrs)
@@ -1168,6 +1180,7 @@ static const MemoryRegionOps gicv3_its_control_ops = {
 };
 
 static const MemoryRegionOps gicv3_its_translation_ops = {
+    .read_with_attrs = gicv3_its_translation_read,
     .write_with_attrs = gicv3_its_translation_write,
     .valid.min_access_size = 2,
     .valid.max_access_size = 4,
-- 
2.25.1

Re: [PATCH 10/14] hw/intc/arm_gicv3_its: Provide read accessor for translation_ops

[Philippe Mathieu-Daudé via]

On 22/1/22 19:24, Peter Maydell wrote:
> The MemoryRegionOps gicv3_its_translation_ops currently provides only
> a .write_with_attrs function, because the only register in this
> region is the write-only GITS_TRANSLATER.  However, if you don't
> provide a read function and the guest tries reading from this memory
> region, QEMU will crash because
> memory_region_read_with_attrs_accessor() calls a NULL pointer.
> 
> Add a read function which always returns 0, to cover both bogus
> attempts to read GITS_TRANSLATER and also reads from the rest of the
> region, which is documented to be reserved, RES0.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)

> +static MemTxResult gicv3_its_translation_read(void *opaque, hwaddr offset,
> +                                              uint64_t *data, unsigned size,
> +                                              MemTxAttrs attrs)
> +{
> +    /*
> +     * GITS_TRANSLATER is write-only, and all other addresses
> +     * in the interrupt translation space frame are RES0.
> +     */
> +    *data = 0;

Maybe log GUEST_ERROR?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    return MEMTX_OK;
> +}

Re: [PATCH 10/14] hw/intc/arm_gicv3_its: Provide read accessor for translation_ops

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The MemoryRegionOps gicv3_its_translation_ops currently provides only
> a .write_with_attrs function, because the only register in this
> region is the write-only GITS_TRANSLATER.  However, if you don't
> provide a read function and the guest tries reading from this memory
> region, QEMU will crash because
> memory_region_read_with_attrs_accessor() calls a NULL pointer.
> 
> Add a read function which always returns 0, to cover both bogus
> attempts to read GITS_TRANSLATER and also reads from the rest of the
> region, which is documented to be reserved, RES0.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)

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

r~

[PATCH 11/14] hw/intc/arm_gicv3_its: Make GITS_BASER<n> RAZ/WI for unimplemented registers

[Peter Maydell]

The ITS has a bank of 8 GITS_BASER<n> registers, which allow the
guest to specify the base address of various data tables.  Each
register has a read-only type field indicating which table it is for
and a read-write field where the guest can write in the base address
(among other things).  We currently allow the guest to write the
writeable fields for all eight registers, even if the type field is 0
indicating "Unimplemented".  This means the guest can provoke QEMU
into asserting by writing an address into one of these unimplemented
base registers, which bypasses the "if (!value) continue" check in
extract_table_params() and lets us hit the assertion that the type
field is one of the permitted table types.

Prevent the assertion by not allowing the guest to write to the
unimplemented base registers. This means their value will remain 0
and extract_table_params() will ignore them.


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

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index b17f2631269..237198845d7 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -929,6 +929,10 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
         if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {
             index = (offset - GITS_BASER) / 8;
 
+            if (s->baser[index] == 0) {
+                /* Unimplemented GITS_BASERn: RAZ/WI */
+                break;
+            }
             if (offset & 7) {
                 value <<= 32;
                 value &= ~GITS_BASER_RO_MASK;
@@ -1025,6 +1029,10 @@ static bool its_writell(GICv3ITSState *s, hwaddr offset,
          */
         if (!(s->ctlr & R_GITS_CTLR_ENABLED_MASK)) {
             index = (offset - GITS_BASER) / 8;
+            if (s->baser[index] == 0) {
+                /* Unimplemented GITS_BASERn: RAZ/WI */
+                break;
+            }
             s->baser[index] &= GITS_BASER_RO_MASK;
             s->baser[index] |= (value & ~GITS_BASER_RO_MASK);
         }
-- 
2.25.1

Re: [PATCH 11/14] hw/intc/arm_gicv3_its: Make GITS_BASER<n> RAZ/WI for unimplemented registers

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> The ITS has a bank of 8 GITS_BASER<n> registers, which allow the
> guest to specify the base address of various data tables.  Each
> register has a read-only type field indicating which table it is for
> and a read-write field where the guest can write in the base address
> (among other things).  We currently allow the guest to write the
> writeable fields for all eight registers, even if the type field is 0
> indicating "Unimplemented".  This means the guest can provoke QEMU
> into asserting by writing an address into one of these unimplemented
> base registers, which bypasses the "if (!value) continue" check in
> extract_table_params() and lets us hit the assertion that the type
> field is one of the permitted table types.
> 
> Prevent the assertion by not allowing the guest to write to the
> unimplemented base registers. This means their value will remain 0
> and extract_table_params() will ignore them.
> 
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

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

r~

[PATCH 12/14] hw/intc/arm_gicv3_its: Check table bounds against correct limit

[Peter Maydell]

Currently when we fill in a TableDesc based on the value the guest
has written to the GITS_BASER<n> register, we calculate both:
 * num_entries : the number of entries in the table, constrained
   by the amount of memory the guest has given it
 * num_ids : the number of IDs we support for this table,
   constrained by the implementation choices and the architecture
   (eg DeviceIDs are 16 bits, so num_ids is 1 << 16)

When validating ITS commands, however, we check only num_ids,
thus allowing a broken guest to specify table entries that
index off the end of it. This will only corrupt guest memory,
but the ITS is supposed to reject such commands as invalid.

Instead of calculating both num_entries and num_ids, set
num_entries to the minimum of the two limits, and check that.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/intc/arm_gicv3_its_common.h |  1 -
 hw/intc/arm_gicv3_its.c                | 18 +++++++++---------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index b32c697207f..3e2ad2dff60 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -47,7 +47,6 @@ typedef struct {
     uint16_t entry_sz;
     uint32_t page_sz;
     uint32_t num_entries;
-    uint32_t num_ids;
     uint64_t base_addr;
 } TableDesc;
 
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 237198845d7..3f2ead45369 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -256,10 +256,10 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
 
     eventid = (value & EVENTID_MASK);
 
-    if (devid >= s->dt.num_ids) {
+    if (devid >= s->dt.num_entries) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: devid %d>=%d",
-                      __func__, devid, s->dt.num_ids);
+                      __func__, devid, s->dt.num_entries);
         return CMD_CONTINUE;
     }
 
@@ -300,7 +300,7 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         return CMD_CONTINUE;
     }
 
-    if (icid >= s->ct.num_ids) {
+    if (icid >= s->ct.num_entries) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
                       __func__, icid);
@@ -384,10 +384,10 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value,
 
     icid = value & ICID_MASK;
 
-    if (devid >= s->dt.num_ids) {
+    if (devid >= s->dt.num_entries) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid command attributes: devid %d>=%d",
-                      __func__, devid, s->dt.num_ids);
+                      __func__, devid, s->dt.num_entries);
         return CMD_CONTINUE;
     }
 
@@ -400,7 +400,7 @@ 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 ((icid >= s->ct.num_ids)
+    if ((icid >= s->ct.num_entries)
             || !dte_valid || (eventid >= num_eventids) ||
             (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) &&
              (pIntid != INTID_SPURIOUS))) {
@@ -485,7 +485,7 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset)
 
     valid = (value & CMD_FIELD_VALID_MASK);
 
-    if ((icid >= s->ct.num_ids) || (rdbase >= s->gicv3->num_cpu)) {
+    if ((icid >= s->ct.num_entries) || (rdbase >= s->gicv3->num_cpu)) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "ITS MAPC: invalid collection table attributes "
                       "icid %d rdbase %" PRIu64 "\n",  icid, rdbase);
@@ -566,7 +566,7 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
 
     valid = (value & CMD_FIELD_VALID_MASK);
 
-    if ((devid >= s->dt.num_ids) ||
+    if ((devid >= s->dt.num_entries) ||
         (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "ITS MAPD: invalid device table attributes "
@@ -791,7 +791,7 @@ static void extract_table_params(GICv3ITSState *s)
                                   L1TABLE_ENTRY_SIZE) *
                                  (page_sz / td->entry_sz));
         }
-        td->num_ids = 1ULL << idbits;
+        td->num_entries = MIN(td->num_entries, 1ULL << idbits);
     }
 }
 
-- 
2.25.1

Re: [PATCH 12/14] hw/intc/arm_gicv3_its: Check table bounds against correct limit

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> Currently when we fill in a TableDesc based on the value the guest
> has written to the GITS_BASER<n> register, we calculate both:
>   * num_entries : the number of entries in the table, constrained
>     by the amount of memory the guest has given it
>   * num_ids : the number of IDs we support for this table,
>     constrained by the implementation choices and the architecture
>     (eg DeviceIDs are 16 bits, so num_ids is 1 << 16)
> 
> When validating ITS commands, however, we check only num_ids,
> thus allowing a broken guest to specify table entries that
> index off the end of it. This will only corrupt guest memory,
> but the ITS is supposed to reject such commands as invalid.
> 
> Instead of calculating both num_entries and num_ids, set
> num_entries to the minimum of the two limits, and check that.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/intc/arm_gicv3_its_common.h |  1 -
>   hw/intc/arm_gicv3_its.c                | 18 +++++++++---------
>   2 files changed, 9 insertions(+), 10 deletions(-)

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

r~

[PATCH 13/14] hw/intc/arm_gicv3_its: Implement MOVALL

[Peter Maydell]

Implement the ITS MOVALL command, which takes all the pending
interrupts on a source redistributor and makes the not-pending on
that source redistributor and pending on a destination redistributor.

This is a GICv3 ITS command which we forgot to implement. (It is
not used by Linux guests.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/gicv3_internal.h   | 16 +++++++++++
 hw/intc/arm_gicv3_its.c    | 55 ++++++++++++++++++++++++++++++++++++++
 hw/intc/arm_gicv3_redist.c | 54 +++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index a316f6c58a5..da45975d92b 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -324,6 +324,7 @@ FIELD(GITS_TYPER, CIL, 36, 1)
 #define GITS_CMD_MAPI             0x0B
 #define GITS_CMD_INV              0x0C
 #define GITS_CMD_INVALL           0x0D
+#define GITS_CMD_MOVALL           0x0E
 #define GITS_CMD_DISCARD          0x0F
 
 /* MAPC command fields */
@@ -355,6 +356,10 @@ FIELD(MAPC, RDBASE, 16, 32)
 #define L2_TABLE_VALID_MASK       CMD_FIELD_VALID_MASK
 #define TABLE_ENTRY_VALID_MASK    (1ULL << 0)
 
+/* MOVALL command fields */
+FIELD(MOVALL_2, RDBASE1, 16, 36)
+FIELD(MOVALL_3, RDBASE2, 16, 36)
+
 /*
  * 12 bytes Interrupt translation Table Entry size
  * as per Table 5.3 in GICv3 spec
@@ -497,6 +502,17 @@ void gicv3_redist_update_lpi(GICv3CPUState *cs);
  * an incoming migration has loaded new state.
  */
 void gicv3_redist_update_lpi_only(GICv3CPUState *cs);
+/**
+ * gicv3_redist_movall_lpis:
+ * @src: source redistributor
+ * @dest: destination redistributor
+ *
+ * Scan the LPI pending table for @src, and for each pending LPI there
+ * mark it as not-pending for @src and pending for @dest, as required
+ * by the ITS MOVALL command.
+ */
+void gicv3_redist_movall_lpis(GICv3CPUState *src, GICv3CPUState *dest);
+
 void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns);
 void gicv3_init_cpuif(GICv3State *s);
 
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 3f2ead45369..ebc0403b3c7 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -582,6 +582,58 @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value,
     return update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL;
 }
 
+static ItsCmdResult process_movall(GICv3ITSState *s, uint64_t value,
+                                   uint32_t offset)
+{
+    AddressSpace *as = &s->gicv3->dma_as;
+    MemTxResult res = MEMTX_OK;
+    uint64_t rd1, rd2;
+
+    /* No fields in dwords 0 or 1 */
+    offset += NUM_BYTES_IN_DW;
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+
+    rd1 = FIELD_EX64(value, MOVALL_2, RDBASE1);
+    if (rd1 >= s->gicv3->num_cpu) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: RDBASE1 %" PRId64
+                      " out of range (must be less than %d)\n",
+                      __func__, rd1, s->gicv3->num_cpu);
+        return CMD_CONTINUE;
+    }
+
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+
+    rd2 = FIELD_EX64(value, MOVALL_3, RDBASE2);
+    if (rd2 >= s->gicv3->num_cpu) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: RDBASE2 %" PRId64
+                      " out of range (must be less than %d)\n",
+                      __func__, rd2, s->gicv3->num_cpu);
+        return CMD_CONTINUE;
+    }
+
+    if (rd1 == rd2) {
+        /* Move to same target must succeed as a no-op */
+        return CMD_CONTINUE;
+    }
+
+    /* Move all pending LPIs from redistributor 1 to redistributor 2 */
+    gicv3_redist_movall_lpis(&s->gicv3->cpu[rd1], &s->gicv3->cpu[rd2]);
+
+    return CMD_CONTINUE;
+}
+
 /*
  * Current implementation blocks until all
  * commands are processed
@@ -679,6 +731,9 @@ static void process_cmdq(GICv3ITSState *s)
                 gicv3_redist_update_lpi(&s->gicv3->cpu[i]);
             }
             break;
+        case GITS_CMD_MOVALL:
+            result = process_movall(s, data, cq_offset);
+            break;
         default:
             break;
         }
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index d81d8e5f076..d1645ba22c6 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -681,6 +681,60 @@ void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
     gicv3_redist_lpi_pending(cs, irq, level);
 }
 
+void gicv3_redist_movall_lpis(GICv3CPUState *src, GICv3CPUState *dest)
+{
+    /*
+     * We must move all pending LPIs from the source redistributor
+     * to the destination. That is, for every pending LPI X on
+     * src, we must set it not-pending on src and pending on dest.
+     * LPIs that are already pending on dest are not cleared.
+     *
+     * If LPIs are disabled on dest this is CONSTRAINED UNPREDICTABLE:
+     * we choose to NOP. If LPIs are disabled on source there's nothing
+     * to be transferred anyway.
+     */
+    AddressSpace *as = &src->gic->dma_as;
+    uint64_t idbits;
+    uint32_t pendt_size;
+    uint64_t src_baddr, dest_baddr;
+    int i;
+
+    if (!(src->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) ||
+        !(dest->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)) {
+        return;
+    }
+
+    idbits = MIN(FIELD_EX64(src->gicr_propbaser, GICR_PROPBASER, IDBITS),
+                 GICD_TYPER_IDBITS);
+    idbits = MIN(FIELD_EX64(dest->gicr_propbaser, GICR_PROPBASER, IDBITS),
+                 idbits);
+
+    pendt_size = 1ULL << (idbits + 1);
+    src_baddr = src->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;
+    dest_baddr = dest->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;
+
+    for (i = GICV3_LPI_INTID_START / 8; i < pendt_size / 8; i++) {
+        uint8_t src_pend, dest_pend;
+
+        address_space_read(as, src_baddr + i, MEMTXATTRS_UNSPECIFIED,
+                           &src_pend, sizeof(src_pend));
+        if (!src_pend) {
+            continue;
+        }
+        address_space_read(as, dest_baddr + i, MEMTXATTRS_UNSPECIFIED,
+                           &dest_pend, sizeof(dest_pend));
+        dest_pend |= src_pend;
+        src_pend = 0;
+        address_space_write(as, src_baddr + i, MEMTXATTRS_UNSPECIFIED,
+                            &src_pend, sizeof(src_pend));
+        address_space_write(as, dest_baddr + i, MEMTXATTRS_UNSPECIFIED,
+                            &dest_pend, sizeof(dest_pend));
+    }
+
+    gicv3_redist_update_lpi(src);
+    gicv3_redist_update_lpi(dest);
+}
+
 void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)
 {
     /* Update redistributor state for a change in an external PPI input line */
-- 
2.25.1

Re: [PATCH 13/14] hw/intc/arm_gicv3_its: Implement MOVALL

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> Implement the ITS MOVALL command, which takes all the pending
> interrupts on a source redistributor and makes the not-pending on
> that source redistributor and pending on a destination redistributor.
> 
> This is a GICv3 ITS command which we forgot to implement. (It is
> not used by Linux guests.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/gicv3_internal.h   | 16 +++++++++++
>   hw/intc/arm_gicv3_its.c    | 55 ++++++++++++++++++++++++++++++++++++++
>   hw/intc/arm_gicv3_redist.c | 54 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 125 insertions(+)

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

r~

[PATCH 14/14] hw/intc/arm_gicv3_its: Implement MOVI

[Peter Maydell]

Implement the ITS MOVI command. This command specifies a (physical) LPI
by DeviceID and EventID and provides a new ICID for it. The ITS must
find the interrupt translation table entry for the LPI, which will
tell it the old ICID. It then moves the pending state of the LPI from
the old redistributor to the new one and updates the ICID field in
the translation table entry.

This is another GICv3 ITS command that we forgot to implement.  Linux
does use this one, but only if the guest powers off one of its CPUs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
All the "validate guest command argument, log and return if bad"
logic feels very clunky here...
---
 hw/intc/gicv3_internal.h   |  16 ++++
 hw/intc/arm_gicv3_its.c    | 146 +++++++++++++++++++++++++++++++++++++
 hw/intc/arm_gicv3_redist.c |  53 ++++++++++++++
 3 files changed, 215 insertions(+)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index da45975d92b..b1af26df9f4 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -315,6 +315,7 @@ FIELD(GITS_TYPER, CIL, 36, 1)
 #define CMD_MASK                  0xff
 
 /* ITS Commands */
+#define GITS_CMD_MOVI             0x01
 #define GITS_CMD_INT              0x03
 #define GITS_CMD_CLEAR            0x04
 #define GITS_CMD_SYNC             0x05
@@ -360,6 +361,11 @@ FIELD(MAPC, RDBASE, 16, 32)
 FIELD(MOVALL_2, RDBASE1, 16, 36)
 FIELD(MOVALL_3, RDBASE2, 16, 36)
 
+/* MOVI command fields */
+FIELD(MOVI_0, DEVICEID, 32, 32)
+FIELD(MOVI_1, EVENTID, 0, 32)
+FIELD(MOVI_2, ICID, 0, 16)
+
 /*
  * 12 bytes Interrupt translation Table Entry size
  * as per Table 5.3 in GICv3 spec
@@ -502,6 +508,16 @@ void gicv3_redist_update_lpi(GICv3CPUState *cs);
  * an incoming migration has loaded new state.
  */
 void gicv3_redist_update_lpi_only(GICv3CPUState *cs);
+/**
+ * gicv3_redist_mov_lpi:
+ * @src: source redistributor
+ * @dest: destination redistributor
+ * @irq: LPI to update
+ *
+ * Move the pending state of the specified LPI from @src to @dest,
+ * as required by the ITS MOVI command.
+ */
+void gicv3_redist_mov_lpi(GICv3CPUState *src, GICv3CPUState *dest, int irq);
 /**
  * gicv3_redist_movall_lpis:
  * @src: source redistributor
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index ebc0403b3c7..51d9be4ae6f 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -634,6 +634,149 @@ static ItsCmdResult process_movall(GICv3ITSState *s, uint64_t value,
     return CMD_CONTINUE;
 }
 
+static ItsCmdResult process_movi(GICv3ITSState *s, uint64_t value,
+                                 uint32_t offset)
+{
+    AddressSpace *as = &s->gicv3->dma_as;
+    MemTxResult res = MEMTX_OK;
+    uint32_t devid, eventid, intid;
+    uint16_t old_icid, new_icid;
+    uint64_t old_cte, new_cte;
+    uint64_t old_rdbase, new_rdbase;
+    uint64_t dte;
+    bool dte_valid, ite_valid, cte_valid;
+    uint64_t num_eventids;
+    IteEntry ite = {};
+
+    devid = FIELD_EX64(value, MOVI_0, DEVICEID);
+
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+    eventid = FIELD_EX64(value, MOVI_1, EVENTID);
+
+    offset += NUM_BYTES_IN_DW;
+    value = address_space_ldq_le(as, s->cq.base_addr + offset,
+                                 MEMTXATTRS_UNSPECIFIED, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+    new_icid = FIELD_EX64(value, MOVI_2, ICID);
+
+    if (devid >= s->dt.num_entries) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: devid %d>=%d",
+                      __func__, devid, s->dt.num_entries);
+        return CMD_CONTINUE;
+    }
+    dte = get_dte(s, devid, &res);
+    if (res != MEMTX_OK) {
+        return CMD_STALL;
+    }
+
+    dte_valid = FIELD_EX64(dte, DTE, VALID);
+    if (!dte_valid) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: "
+                      "invalid dte: %"PRIx64" for %d\n",
+                      __func__, dte, devid);
+        return CMD_CONTINUE;
+    }
+
+    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, &old_icid, &intid, &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;
+    }
+
+    if (old_icid >= s->ct.num_entries) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
+                      __func__, old_icid);
+        return CMD_CONTINUE;
+    }
+
+    if (new_icid >= s->ct.num_entries) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid command attributes: ICID 0x%x\n",
+                      __func__, new_icid);
+        return CMD_CONTINUE;
+    }
+
+    cte_valid = get_cte(s, old_icid, &old_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__, old_cte);
+        return CMD_CONTINUE;
+    }
+
+    cte_valid = get_cte(s, new_icid, &new_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__, new_cte);
+        return CMD_CONTINUE;
+    }
+
+    old_rdbase = FIELD_EX64(old_cte, CTE, RDBASE);
+    if (old_rdbase >= s->gicv3->num_cpu) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: CTE has invalid rdbase 0x%"PRIx64"\n",
+                      __func__, old_rdbase);
+        return CMD_CONTINUE;
+    }
+
+    new_rdbase = FIELD_EX64(new_cte, CTE, RDBASE);
+    if (new_rdbase >= s->gicv3->num_cpu) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: CTE has invalid rdbase 0x%"PRIx64"\n",
+                      __func__, new_rdbase);
+        return CMD_CONTINUE;
+    }
+
+    if (old_rdbase != new_rdbase) {
+        /* Move the LPI from the old redistributor to the new one */
+        gicv3_redist_mov_lpi(&s->gicv3->cpu[old_rdbase],
+                             &s->gicv3->cpu[new_rdbase],
+                             intid);
+    }
+
+    /* Update the ICID field in the interrupt translation table entry */
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, 1);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, intid);
+    ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS);
+    ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, new_icid);
+    return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL;
+}
+
 /*
  * Current implementation blocks until all
  * commands are processed
@@ -731,6 +874,9 @@ static void process_cmdq(GICv3ITSState *s)
                 gicv3_redist_update_lpi(&s->gicv3->cpu[i]);
             }
             break;
+        case GITS_CMD_MOVI:
+            result = process_movi(s, data, cq_offset);
+            break;
         case GITS_CMD_MOVALL:
             result = process_movall(s, data, cq_offset);
             break;
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index d1645ba22c6..412a04f59cf 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -681,6 +681,59 @@ void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
     gicv3_redist_lpi_pending(cs, irq, level);
 }
 
+void gicv3_redist_mov_lpi(GICv3CPUState *src, GICv3CPUState *dest, int irq)
+{
+    /*
+     * Move the specified LPI's pending state from the source redistributor
+     * to the destination.
+     *
+     * If LPIs are disabled on dest this is CONSTRAINED UNPREDICTABLE:
+     * we choose to NOP. If LPIs are disabled on source there's nothing
+     * to be transferred anyway.
+     */
+    AddressSpace *as = &src->gic->dma_as;
+    uint64_t idbits;
+    uint32_t pendt_size;
+    uint64_t src_baddr;
+    uint8_t src_pend;
+
+    if (!(src->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) ||
+        !(dest->gicr_ctlr & GICR_CTLR_ENABLE_LPIS)) {
+        return;
+    }
+
+    idbits = MIN(FIELD_EX64(src->gicr_propbaser, GICR_PROPBASER, IDBITS),
+                 GICD_TYPER_IDBITS);
+    idbits = MIN(FIELD_EX64(dest->gicr_propbaser, GICR_PROPBASER, IDBITS),
+                 idbits);
+
+    pendt_size = 1ULL << (idbits + 1);
+    if ((irq / 8) >= pendt_size) {
+        return;
+    }
+
+    src_baddr = src->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;
+
+    address_space_read(as, src_baddr + (irq / 8),
+                       MEMTXATTRS_UNSPECIFIED, &src_pend, sizeof(src_pend));
+    if (!extract32(src_pend, irq % 8, 1)) {
+        /* Not pending on source, nothing to do */
+        return;
+    }
+    src_pend &= ~(1 << (irq % 8));
+    address_space_write(as, src_baddr + (irq / 8),
+                        MEMTXATTRS_UNSPECIFIED, &src_pend, sizeof(src_pend));
+    if (irq == src->hpplpi.irq) {
+        /*
+         * We just made this LPI not-pending so only need to update
+         * if it was previously the highest priority pending LPI
+         */
+        gicv3_redist_update_lpi(src);
+    }
+    /* Mark it pending on the destination */
+    gicv3_redist_lpi_pending(dest, irq, 1);
+}
+
 void gicv3_redist_movall_lpis(GICv3CPUState *src, GICv3CPUState *dest)
 {
     /*
-- 
2.25.1

Re: [PATCH 14/14] hw/intc/arm_gicv3_its: Implement MOVI

[Richard Henderson]

On 1/23/22 05:24, Peter Maydell wrote:
> Implement the ITS MOVI command. This command specifies a (physical) LPI
> by DeviceID and EventID and provides a new ICID for it. The ITS must
> find the interrupt translation table entry for the LPI, which will
> tell it the old ICID. It then moves the pending state of the LPI from
> the old redistributor to the new one and updates the ICID field in
> the translation table entry.
> 
> This is another GICv3 ITS command that we forgot to implement.  Linux
> does use this one, but only if the guest powers off one of its CPUs.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> All the "validate guest command argument, log and return if bad"
> logic feels very clunky here...
> ---
>   hw/intc/gicv3_internal.h   |  16 ++++
>   hw/intc/arm_gicv3_its.c    | 146 +++++++++++++++++++++++++++++++++++++
>   hw/intc/arm_gicv3_redist.c |  53 ++++++++++++++
>   3 files changed, 215 insertions(+)

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

r~