arm/arm64: Add ITS tests

Allocate the LPI configuration and per re-distributor pending table. Set redistributor's PROPBASER and PENDBASER. The LPIs are enabled by default in the config table.

Also introduce a helper routine that allows to set the pending table bit for a given LPI.

Signed-off-by Eric Auger <[email protected]>

---

v4 -> v5:
- Moved some reformattings previously done in "arm/arm64: ITS: its_enable_defaults", in this patch
- added assert(!gicv3_redist_base()) in gicv3_lpi_alloc_tables()
- revert for_each_present_cpu() change

v2 -> v3:
- Move the helpers in lib/arm/gic-v3.c and prefix them with "gicv3_" and add _lpi prefix too

v1 -> v2:
- remove memory attributes

---
  lib/arm/asm/gic-v3.h | 15 +++++++++++++
  lib/arm/gic-v3.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 68 insertions(+)

lib/arm/asm/gic-v3.h +0 -0

index 47df051..064cc68 100644

lib/arm/asm/gic-v3.h +10 -0

+
+#define GICR_PENDBASER_PTZ		BIT_ULL(62)
+
+#define LPI_PROP_GROUP1			(1 << 1)
+#define LPI_PROP_ENABLED		(1 << 0)
+#define LPI_PROP_DEFAULT_PRIO		0xa0
+#define LPI_PROP_DEFAULT		(LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1 | LPI_PROP_ENABLED)
+
  #include <asm/arch_gicv3.h>
 
  #ifndef __ASSEMBLY__

@@ -66,6 +75,8 @@ struct gicv3_data {
  	void *dist_base;
  	void *redist_bases[GICV3_NR_REDISTS];
  	void *redist_base[NR_CPUS];
+	u8 *lpi_prop;
+	void *lpi_pend[NR_CPUS];
  	unsigned int irq_nr;
  };
  extern struct gicv3_data gicv3_data;

lib/arm/asm/gic-v3.h +2 -0

+extern void gicv3_lpi_set_clr_pending(int rdist, int n, bool set);
+extern void gicv3_lpi_alloc_tables(void);
 
  static inline void gicv3_do_wait_for_rwp(void *base)
  {

lib/arm/gic-v3.c +9 -0

index feecb5e..d752bd4 100644
@@ -5,6 +5,7 @@
   */
  #include <asm/gic.h>
  #include <asm/io.h>
+#include <alloc_page.h>
 
  void gicv3_set_redist_base(size_t stride)
  {

@@ -147,3 +148,55 @@ void gicv3_ipi_send_single(int irq, int cpu)
  	cpumask_set_cpu(cpu, &dest);
  	gicv3_ipi_send_mask(irq, &dest);
  }
+
+#if defined(__aarch64__)
+
+/*
+ * alloc_lpi_tables - Allocate LPI config and pending tables
+ * and set PROPBASER (shared by all rdistributors) and per
+ * redistributor PENDBASER.
+ *

lib/arm/gic-v3.c +34 -0

+
+	gicv3_data.lpi_prop = alloc_pages(order);
+
+	/* ID bits = 13, ie. up to 14b LPI INTID */
+	prop_val = (u64)(virt_to_phys(gicv3_data.lpi_prop)) | 13;
+
+	for_each_present_cpu(cpu) {
+		u64 pend_val;
+		void *ptr;
+
+		ptr = gicv3_data.redist_base[cpu];
+
+		writeq(prop_val, ptr + GICR_PROPBASER);
+
+		gicv3_data.lpi_pend[cpu] = alloc_pages(order);
+		pend_val = (u64)(virt_to_phys(gicv3_data.lpi_pend[cpu]));
+		writeq(pend_val, ptr + GICR_PENDBASER);
+	}
+}
+
+void gicv3_lpi_set_clr_pending(int rdist, int n, bool set)
+{
+	u8 *ptr = gicv3_data.lpi_pend[rdist];
+	u8 mask = 1 << (n % 8), byte;
+
+	ptr += (n / 8);
+	byte = *ptr;
+	if (set)
+		byte |=  mask;
+	else
+		byte &= ~mask;
+	*ptr = byte;
+}
+#endif /* __aarch64__ */

Hi Eric,

Quoted 28 lines · Eric Auger

Allocate the LPI configuration and per re-distributor pending table. Set redistributor's PROPBASER and PENDBASER. The LPIs are enabled by default in the config table.

Also introduce a helper routine that allows to set the pending table bit for a given LPI.

Signed-off-by Eric Auger <[email protected]>

---

v4 -> v5:
- Moved some reformattings previously done in "arm/arm64: ITS: its_enable_defaults", in this patch
- added assert(!gicv3_redist_base()) in gicv3_lpi_alloc_tables()
- revert for_each_present_cpu() change

v2 -> v3:
- Move the helpers in lib/arm/gic-v3.c and prefix them with "gicv3_" and add _lpi prefix too

v1 -> v2:
- remove memory attributes
---
  lib/arm/asm/gic-v3.h | 15 +++++++++++++
  lib/arm/gic-v3.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++

  2 files changed, 68 insertions(+)

Quoted 9 lines

lib/arm/asm/gic-v3.h +0 -0

index 47df051..064cc68 100644
@@ -50,6 +50,15 @@
   #define MPIDR_TO_SGI_AFFINITY(cluster_id, level) \

      (MPIDR_AFFINITY_LEVEL(cluster_id, level) <<
ICC_SGI1R_AFFINITY_## level ## _SHIFT)
  +#define GICR_PROPBASER_IDBITS_MASK    (0x1f)

Again this can be dropped, but not a problem.

Quoted 23 lines

lib/arm/asm/gic-v3.h +7 -0

+
+#define GICR_PENDBASER_PTZ        BIT_ULL(62)
+
+#define LPI_PROP_GROUP1            (1 << 1)
+#define LPI_PROP_ENABLED        (1 << 0)
+#define LPI_PROP_DEFAULT_PRIO        0xa0
+#define LPI_PROP_DEFAULT        (LPI_PROP_DEFAULT_PRIO |

LPI_PROP_GROUP1 | LPI_PROP_ENABLED)
+
  #include <asm/arch_gicv3.h>
    #ifndef __ASSEMBLY__

lib/arm/asm/gic-v3.h +2 -0

@@ -66,6 +75,8 @@ struct gicv3_data {
       void *dist_base;
       void *redist_bases[GICV3_NR_REDISTS];
       void *redist_base[NR_CPUS];
+    u8 *lpi_prop;
+    void *lpi_pend[NR_CPUS];
       unsigned int irq_nr;
   };
   extern struct gicv3_data gicv3_data;

@@ -82,6 +93,10 @@ extern void gicv3_write_eoir(u32 irqstat);
   extern void gicv3_ipi_send_single(int irq, int cpu);

lib/arm/asm/gic-v3.h extern void gicv3_write_eoir(u32 irqstat); · line 97

Quoted 4 lines

lib/arm/asm/gic-v3.h +2 -0

   extern void gicv3_ipi_send_mask(int irq, const cpumask_t *dest);
   extern void gicv3_set_redist_base(size_t stride);
+extern void gicv3_lpi_set_config(int n, u8 val);
+extern u8 gicv3_lpi_get_config(int n);

These two declarations can be dropped, and I think it's better to move their macro implementations here (they're now in patch #7). But also not a problem.

Quoted 32 lines

lib/arm/asm/gic-v3.h +2 -0

+extern void gicv3_lpi_set_clr_pending(int rdist, int n, bool set);
+extern void gicv3_lpi_alloc_tables(void);
     static inline void gicv3_do_wait_for_rwp(void *base)
   {

lib/arm/gic-v3.c +13 -0

index feecb5e..d752bd4 100644
@@ -5,6 +5,7 @@
    */
   #include <asm/gic.h>
   #include <asm/io.h>
+#include <alloc_page.h>
     void gicv3_set_redist_base(size_t stride)
   {

@@ -147,3 +148,55 @@ void gicv3_ipi_send_single(int irq, int cpu)
       cpumask_set_cpu(cpu, &dest);
       gicv3_ipi_send_mask(irq, &dest);
   }
+
+#if defined(__aarch64__)
+
+/*
+ * alloc_lpi_tables - Allocate LPI config and pending tables
+ * and set PROPBASER (shared by all rdistributors) and per
+ * redistributor PENDBASER.
+ *
+ * gicv3_set_redist_base() must be called before
+ */
+void gicv3_lpi_alloc_tables(void)
+{

lib/arm/gic-v3.c void gicv3_ipi_send_single(int irq, int cpu) · line 168

Quoted 6 lines

lib/arm/gic-v3.c +6 -0

+    unsigned long n = SZ_64K >> PAGE_SHIFT;
+    unsigned long order = fls(n);
+    u64 prop_val;
+    int cpu;
+
+    assert(!gicv3_redist_base());

I guess you wanted assert(gicv3_redist_base())? With this confirmed,

Reviewed-by Zenghui Yu <[email protected]>

Thanks

Quoted 35 lines

lib/arm/gic-v3.c +34 -0

+
+    gicv3_data.lpi_prop = alloc_pages(order);
+
+    /* ID bits = 13, ie. up to 14b LPI INTID */
+    prop_val = (u64)(virt_to_phys(gicv3_data.lpi_prop)) | 13;
+
+    for_each_present_cpu(cpu) {
+        u64 pend_val;
+        void *ptr;
+
+        ptr = gicv3_data.redist_base[cpu];
+
+        writeq(prop_val, ptr + GICR_PROPBASER);
+
+        gicv3_data.lpi_pend[cpu] = alloc_pages(order);
+        pend_val = (u64)(virt_to_phys(gicv3_data.lpi_pend[cpu]));
+        writeq(pend_val, ptr + GICR_PENDBASER);
+    }
+}
+
+void gicv3_lpi_set_clr_pending(int rdist, int n, bool set)
+{
+    u8 *ptr = gicv3_data.lpi_pend[rdist];
+    u8 mask = 1 << (n % 8), byte;
+
+    ptr += (n / 8);
+    byte = *ptr;
+    if (set)
+        byte |=  mask;
+    else
+        byte &= ~mask;
+    *ptr = byte;
+}
+#endif /* __aarch64__ */

Hi Eric,

lib/arm64/asm/gic-v3-its.h line 56

Quoted 5 lines · Eric Auger

lib/arm64/asm/gic-v3-its.h new file +5 -0

+#define GITS_TYPER_PLPIS                BIT(0)
+#define GITS_TYPER_VLPIS        BIT(1)
+#define GITS_TYPER_ITT_ENTRY_SIZE    GENMASK_ULL(7, 4)
+#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT    4
+#define GITS_TYPER_IDBITS        GENMASK_ULL(8, 12)

Note that this should be GENMASK_ULL(12, 8).

lib/arm64/asm/gic-v3-its.h line 58

Quoted 3 lines

lib/arm64/asm/gic-v3-its.h new file +2 -0

+#define GITS_TYPER_IDBITS_SHIFT         8
+#define GITS_TYPER_DEVBITS        GENMASK_ULL(13, 17)

(17, 13)

lib/arm64/asm/gic-v3-its.h line 61

Quoted 3 lines

lib/arm64/asm/gic-v3-its.h new file +3 -0

+#define GITS_TYPER_DEVBITS_SHIFT        13
+#define GITS_TYPER_PTA                  BIT(19)
+#define GITS_TYPER_CIDBITS        GENMASK_ULL(32, 35)

(35, 32)

lib/arm64/asm/gic-v3-its.h line 63

Quoted 2 lines

lib/arm64/asm/gic-v3-its.h new file +2 -0

+#define GITS_TYPER_CIDBITS_SHIFT    32
+#define GITS_TYPER_CIL            BIT(36)

And please use tab for all of them.

Thanks,
Zenghui

its_enable_defaults() enable LPIs at distributor level

and ITS level.

gicv3_enable_defaults must be called before.

Signed-off-by Eric Auger<[email protected]>

Implement main ITS commands. The code is largely inherited from the ITS driver.

Signed-off-by Eric Auger <[email protected]>

---

v3 -> v4:
- device's itt now is a VGA
- pass verbose to choose whether we shall print the cmd
- use printf instead of report_info

v2 -> v3:
- do not use report() anymore
- assert if cmd_write exceeds the queue capacity

v1 -> v2:
- removed its_print_cmd_state

---
  arm/Makefile.arm64         |   2 +-
  lib/arm64/asm/gic-v3-its.h |  57 +++++
  lib/arm64/gic-v3-its-cmd.c | 463 +++++++++++++++++++++++++++++++++++++
  3 files changed, 521 insertions(+), 1 deletion(-)

create mode 100644 lib/arm64/gic-v3-its-cmd.c

arm/Makefile.arm64 +1 -1

index 60182ae..dfd0c56 100644
@@ -19,7 +19,7 @@ endef
  cstart.o = $(TEST_DIR)/cstart64.o
  cflatobjs += lib/arm64/processor.o
  cflatobjs += lib/arm64/spinlock.o
-cflatobjs += lib/arm64/gic-v3-its.o
+cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o
 
  OBJDIRS += lib/arm64
 

lib/arm64/asm/gic-v3-its.h +0 -0

index 3da548b..889d6ce 100644
@@ -102,6 +102,28 @@ extern struct its_data its_data;
  #define GITS_BASER_TYPE_DEVICE		1

lib/arm64/asm/gic-v3-its.h +38 -0

+#define GITS_CMD_MAPI			0x0b
+#define GITS_CMD_MOVI			0x01
+#define GITS_CMD_DISCARD		0x0f
+#define GITS_CMD_INV			0x0c
+#define GITS_CMD_MOVALL			0x0e
+#define GITS_CMD_INVALL			0x0d
+#define GITS_CMD_INT			0x03
+#define GITS_CMD_CLEAR			0x04
+#define GITS_CMD_SYNC			0x05
+
+struct its_cmd_block {
+	u64 raw_cmd[4];
+};
+
  extern void its_parse_typer(void);
  extern void its_init(void);
  extern int its_baser_lookup(int i, struct its_baser *baser);

@@ -109,4 +131,39 @@ extern void its_enable_defaults(void);
  extern struct its_device *its_create_device(u32 dev_id, int nr_ites);
  extern struct its_collection *its_create_collection(u32 col_id, u32 target_pe);
 
+extern void __its_send_mapd(struct its_device *dev, int valid, bool verbose);
+extern void __its_send_mapc(struct its_collection *col, int valid, bool verbose);
+extern void __its_send_mapti(struct its_device *dev, u32 irq_id, u32 event_id,
+			     struct its_collection *col, bool verbose);
+extern void __its_send_int(struct its_device *dev, u32 event_id, bool verbose);
+extern void __its_send_inv(struct its_device *dev, u32 event_id, bool verbose);
+extern void __its_send_discard(struct its_device *dev, u32 event_id, bool verbose);
+extern void __its_send_clear(struct its_device *dev, u32 event_id, bool verbose);
+extern void __its_send_invall(struct its_collection *col, bool verbose);
+extern void __its_send_movi(struct its_device *dev, struct its_collection *col,
+			    u32 id, bool verbose);
+extern void __its_send_sync(struct its_collection *col, bool verbose);
+
+#define its_send_mapd(dev, valid)			__its_send_mapd(dev, valid, true)
+#define its_send_mapc(col, valid)			__its_send_mapc(col, valid, true)
+#define its_send_mapti(dev, irqid, eventid, col)	__its_send_mapti(dev, irqid, eventid, col, true)
+#define its_send_int(dev, eventid)			__its_send_int(dev, eventid, true)
+#define its_send_inv(dev, eventid)			__its_send_inv(dev, eventid, true)
+#define its_send_discard(dev, eventid)			__its_send_discard(dev, eventid, true)
+#define its_send_clear(dev, eventid)			__its_send_clear(dev, eventid, true)
+#define its_send_invall(col)				__its_send_invall(col, true)
+#define its_send_movi(dev, col, id)			__its_send_movi(dev, col, id, true)
+#define its_send_sync(col)				__its_send_sync(col, true)
+

lib/arm64/asm/gic-v3-its.h +10 -0

+
+static void its_build_invall_cmd(struct its_cmd_block *cmd,
+			      struct its_cmd_desc *desc)
+{
+	its_encode_cmd(cmd, GITS_CMD_INVALL);
+	its_encode_collection(cmd, desc->its_invall_cmd.col->col_id);
+	its_fixup_cmd(cmd);
+	if (desc->verbose)
+		printf("INVALL col_id=%d\n", desc->its_invall_cmd.col->col_id);
+}

Triggers LPIs through the INT command.

the test checks the LPI hits the right CPU and triggers the right LPI intid, ie. the translation is correct.

Updates to the config table also are tested, along with inv and invall commands.

Signed-off-by Eric Auger <[email protected]>

---

arm/gic.c +9 -0

+static void test_its_trigger(void)
+{
+	struct its_collection *col3, *col2;
+	struct its_device *dev2, *dev7;
+
+	if (its_prerequisites(4))
+		return;
+
+	dev2 = its_create_device(2 /* dev id */, 8 /* nb_ites */);

arm/gic.c +64 -0

+
+	report_prefix_push("int");
+	/*
+	 * dev=2, eventid=20  -> lpi= 8195, col=3
+	 * dev=7, eventid=255 -> lpi= 8196, col=2
+	 * Trigger dev2, eventid=20 and dev7, eventid=255
+	 * Check both LPIs hit
+	 */
+
+	its_send_mapd(dev2, true);
+	its_send_mapd(dev7, true);
+
+	its_send_mapc(col3, true);
+	its_send_mapc(col2, true);
+
+	its_send_mapti(dev2, 8195 /* lpi id */, 20 /* event id */, col3);
+	its_send_mapti(dev7, 8196 /* lpi id */, 255 /* event id */, col2);
+
+	lpi_stats_expect(3, 8195);
+	its_send_int(dev2, 20);
+	check_lpi_stats("dev=2, eventid=20  -> lpi= 8195, col=3");
+
+	lpi_stats_expect(2, 8196);
+	its_send_int(dev7, 255);
+	check_lpi_stats("dev=7, eventid=255 -> lpi= 8196, col=2");
+
+	report_prefix_pop();
+
+	report_prefix_push("inv/invall");
+
+	/*
+	 * disable 8195, check dev2/eventid=20 does not trigger the
+	 * corresponding LPI
+	 */
+	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT & ~LPI_PROP_ENABLED);
+	its_send_inv(dev2, 20);
+
+	lpi_stats_expect(-1, -1);
+	its_send_int(dev2, 20);
+	check_lpi_stats("dev2/eventid=20 does not trigger any LPI");
+
+	/*
+	 * re-enable the LPI but willingly do not call invall
+	 * so the change in config is not taken into account.
+	 * The LPI should not hit
+	 */
+	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
+	lpi_stats_expect(-1, -1);
+	its_send_int(dev2, 20);
+	check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
+
+	/* Now call the invall and check the LPI hits */
+	its_send_invall(col3);
+	lpi_stats_expect(3, 8195);
+	its_send_int(dev2, 20);
+	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
+
+	report_prefix_pop();
+
+	report_prefix_push("mapd valid=false");
+	/*
+	 * Unmap device 2 and check the eventid 20 formerly
+	 * attached to it does not hit anymore
+	 */

Hi Eric,

Quoted 11 lines · Eric Auger

Triggers LPIs through the INT command.

the test checks the LPI hits the right CPU and triggers the right LPI intid, ie. the translation is correct.

Updates to the config table also are tested, along with inv and invall commands.

Signed-off-by Eric Auger <[email protected]>

---

[...]

Quoted 15 lines

arm/gic.c +15 -0

+static void test_its_trigger(void)
+{
+    struct its_collection *col3, *col2;
+    struct its_device *dev2, *dev7;
+
+    if (its_prerequisites(4))
+        return;
+
+    dev2 = its_create_device(2 /* dev id */, 8 /* nb_ites */);
+    dev7 = its_create_device(7 /* dev id */, 8 /* nb_ites */);
+
+    col3 = its_create_collection(3 /* col id */, 3/* target PE */);
+    col2 = its_create_collection(2 /* col id */, 2/* target PE */);
+
+    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);

Quoted 4 lines

+    gicv3_lpi_set_config(8196, LPI_PROP_DEFAULT); +
+    its_send_invall(col2);
+    its_send_invall(col3);

These two INVALLs should be issued after col2 and col3 are mapped, otherwise this will cause the INVALL command error as per the spec (though KVM doesn't complain it at all).

Quoted 65 lines

arm/gic.c +64 -0

+
+    report_prefix_push("int");
+    /*
+     * dev=2, eventid=20  -> lpi= 8195, col=3
+     * dev=7, eventid=255 -> lpi= 8196, col=2
+     * Trigger dev2, eventid=20 and dev7, eventid=255
+     * Check both LPIs hit
+     */
+
+    its_send_mapd(dev2, true);
+    its_send_mapd(dev7, true);
+
+    its_send_mapc(col3, true);
+    its_send_mapc(col2, true);
+
+    its_send_mapti(dev2, 8195 /* lpi id */, 20 /* event id */, col3);
+    its_send_mapti(dev7, 8196 /* lpi id */, 255 /* event id */, col2);
+
+    lpi_stats_expect(3, 8195);
+    its_send_int(dev2, 20);
+    check_lpi_stats("dev=2, eventid=20  -> lpi= 8195, col=3");
+
+    lpi_stats_expect(2, 8196);
+    its_send_int(dev7, 255);
+    check_lpi_stats("dev=7, eventid=255 -> lpi= 8196, col=2");
+
+    report_prefix_pop();
+
+    report_prefix_push("inv/invall");
+
+    /*
+     * disable 8195, check dev2/eventid=20 does not trigger the
+     * corresponding LPI
+     */
+    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT & ~LPI_PROP_ENABLED);
+    its_send_inv(dev2, 20);
+
+    lpi_stats_expect(-1, -1);
+    its_send_int(dev2, 20);
+    check_lpi_stats("dev2/eventid=20 does not trigger any LPI");
+
+    /*
+     * re-enable the LPI but willingly do not call invall
+     * so the change in config is not taken into account.
+     * The LPI should not hit
+     */
+    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
+    lpi_stats_expect(-1, -1);
+    its_send_int(dev2, 20);
+    check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
+
+    /* Now call the invall and check the LPI hits */
+    its_send_invall(col3);
+    lpi_stats_expect(3, 8195);
+    its_send_int(dev2, 20);
+    check_lpi_stats("dev2/eventid=20 now triggers an LPI");
+
+    report_prefix_pop();
+
+    report_prefix_push("mapd valid=false");
+    /*
+     * Unmap device 2 and check the eventid 20 formerly
+     * attached to it does not hit anymore
+     */

arm/gic.c static void test_its_introspection(void) · line 753

Quoted 10 lines

arm/gic.c +10 -0

+
+    its_send_mapd(dev2, false);
+    lpi_stats_expect(-1, -1);
+    its_send_int(dev2, 20);
+    check_lpi_stats("no LPI after device unmap");
+    report_prefix_pop();
+
+    /* Unmap the collection this time and check no LPI does hit */
+    report_prefix_push("mapc valid=false");
+    its_send_mapc(col2, false);

And as for the MAPC, the spec says:

" When V is 0:
Behavior is unpredictable if there are interrupts that are mapped to the specified collection, with the restriction that further translation requests from that device are ignored. "

So this collection-unmap test may not make sense?

Quoted 6 lines

+    lpi_stats_expect(-1, -1);
+    its_send_int(dev7, 255);
+    check_lpi_stats("no LPI after collection unmap"); +    report_prefix_pop();
+}

[...]

Otherwise looks good.

Thanks,
Zenghui

Hi Zenghui,

Quoted 32 lines · Zenghui Yu

Hi Eric,

Quoted 11 lines · Eric Auger

Triggers LPIs through the INT command.

the test checks the LPI hits the right CPU and triggers the right LPI intid, ie. the translation is correct.

Updates to the config table also are tested, along with inv and invall commands.

Signed-off-by Eric Auger <[email protected]>

---

[...]

Quoted 15 lines

arm/gic.c +15 -0

+static void test_its_trigger(void)
+{
+    struct its_collection *col3, *col2;
+    struct its_device *dev2, *dev7;
+
+    if (its_prerequisites(4))
+        return;
+
+    dev2 = its_create_device(2 /* dev id */, 8 /* nb_ites */);
+    dev7 = its_create_device(7 /* dev id */, 8 /* nb_ites */);
+
+    col3 = its_create_collection(3 /* col id */, 3/* target PE */);
+    col2 = its_create_collection(2 /* col id */, 2/* target PE */);
+
+    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);

Quoted 8 lines

Quoted 4 lines

+    gicv3_lpi_set_config(8196, LPI_PROP_DEFAULT); +
+    its_send_invall(col2);
+    its_send_invall(col3);

These two INVALLs should be issued after col2 and col3 are mapped, otherwise this will cause the INVALL command error as per the spec (though KVM doesn't complain it at all).

Yes you're right. reading the spec again:

A command error occurs if any of the following apply:
../..
The collection specified by ICID has not been mapped to an RDbase using MAPC.

But as mentionned in the cover letter, no real means to retrieve the error at the moment.

That is still a problem with the ITS. There is no architectural way to report an error, even if the error numbers are architected...

One thing we could do though is to implement the stall model (as described in 5.3.2). It still doesn't give us the error, but at least the command queue would stop on detecting an error.

Quoted 7 lines · Marc Zyngier

That is still a problem with the ITS. There is no architectural way to report an error, even if the error numbers are architected...

One thing we could do though is to implement the stall model (as described
in 5.3.2). It still doesn't give us the error, but at least the command queue would stop on detecting an error.

It would be interesting to see the buggy guest's behavior under the stall mode. I've used the following diff (absolutely *not* a formal patch, don't handle CREADR.Stalled and CWRITER.Retry at all) to have a try, and caught another command error in the 'its-trigger' test.

logs/its-trigger.log:
" INT dev_id=2 event_id=20 lib/arm64/gic-v3-its-cmd.c:194: assert failed: false: INT timeout! "

dmesg:

[13297.711958] ------------[ cut here ]------------
[13297.711964] ITS command error encoding 0x10307

It's the last INT test in test_its_trigger() who has triggered this error, Eric?

Thanks.

---8<---

include/kvm/arm_vgic.h +1 -0

index 9d53f545a3d5..5717f5da0f22 100644
@@ -179,6 +179,7 @@ struct vgic_its {
      u64            cbaser;
      u32            creadr;
      u32            cwriter;
+    bool            stalled;
 
      /* migration ABI revision in use */
      u32            abi_rev;

virt/kvm/arm/vgic/vgic-its.c +3 -0

index d53d34a33e35..72422b75e627 100644
@@ -1519,6 +1519,9 @@ static void vgic_its_process_commands(struct kvm
 *kvm, struct vgic_its *its)
      if (!its->enabled)
          return;
 
+    if (unlikely(its->stalled))
+        return;
+
      cbaser = GITS_CBASER_ADDRESS(its->cbaser);
 

     while (its->cwriter != its->creadr) {

virt/kvm/arm/vgic/vgic-its.c +27 -2

@@ -1531,9 +1534,34 @@ static void vgic_its_process_commands(struct kvm
 *kvm, struct vgic_its *its)
           * According to section 6.3.2 in the GICv3 spec we can just
           * ignore that command then.
           */
-        if (!ret)
-            vgic_its_handle_command(kvm, its, cmd_buf);
+        if (ret)
+            goto done;
+
+        ret = vgic_its_handle_command(kvm, its, cmd_buf);
+
+        /*
+         * Choose the stall mode on detection of command errors.
+         * Guest still can't get the architected error numbers though...
+         */
+        if (ret) {
+            /* GITS_CREADR.Stalled is set to 1. */
+            its->stalled = true;
+
+            /*
+             * GITS_TYPER.SEIS is 0 atm, no System error will be
+             * generated.  Instead report error encodings at ITS
+             * level.
+             */
+            WARN_RATELIMIT(ret, "ITS command error encoding 0x%x", ret);
+
+            /*
+             * GITS_CREADR is not incremented and continues to
+             * point to the entry that triggered the error.
+             */
+            break;
+        }
 
+done:
          its->creadr += ITS_CMD_SIZE;
          if (its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser))

             its->creadr = 0;

Hi Zenghui,

Quoted 18 lines · Zenghui Yu

Quoted 7 lines · Marc Zyngier

That is still a problem with the ITS. There is no architectural way to report an error, even if the error numbers are architected...

One thing we could do though is to implement the stall model (as described
in 5.3.2). It still doesn't give us the error, but at least the command queue would stop on detecting an error.

It would be interesting to see the buggy guest's behavior under the stall mode. I've used the following diff (absolutely *not* a formal patch, don't handle CREADR.Stalled and CWRITER.Retry at all) to have a try, and caught another command error in the 'its-trigger' test.

logs/its-trigger.log:
" INT dev_id=2 event_id=20 lib/arm64/gic-v3-its-cmd.c:194: assert failed: false: INT timeout! "

Quoted 6 lines

dmesg:

[13297.711958] ------------[ cut here ]------------
[13297.711964] ITS command error encoding 0x10307

It's the last INT test in test_its_trigger() who has triggered this error, Eric?

Yes it may be the culprit. Anyway I removed the collection unmap in v6.

By the way are you OK now with v6? I think Drew plans to send a pull request by the end of this week.

Add two new migration tests. One testing the migration of a topology where collection were unmapped. The second test checks the migration of the pending table.

Signed-off-by Eric Auger <[email protected]>

---

v4 -> v5:
- move stub from header to arm/gic.c

v3 -> v4:
- do not talk about odd/even CPUs, use pe0 and pe1
- comment the delay

v2 -> v3:
- tests belong to both its and migration groups
- use LPI(i)
- gicv3_lpi_set_pending_table_bit renamed into gicv3_lpi_set_clr_pending

---
  arm/gic.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++
  arm/unittests.cfg |  16 +++++
  2 files changed, 162 insertions(+)

arm/gic.c +19 -0

index b8fbc13..e6ffbc3 100644
@@ -193,6 +193,7 @@ static void lpi_handler(struct pt_regs *regs __unused)
  	smp_rmb(); /* pairs with wmb in lpi_stats_expect */
  	lpi_stats.observed.cpu_id = smp_processor_id();
  	lpi_stats.observed.lpi_id = irqnr;
+	acked[lpi_stats.observed.cpu_id]++;
  	smp_wmb(); /* pairs with rmb in check_lpi_stats */
  }
 

@@ -236,6 +237,22 @@ static void secondary_lpi_test(void)
  	while (1)
  		wfi();
  }
+
+static void check_lpi_hits(int *expected, const char *msg)
+{
+	bool pass = true;
+	int i;
+
+	for (i = 0; i < nr_cpus; i++) {
+		if (acked[i] != expected[i]) {
+			report_info("expected %d LPIs on PE #%d, %d observed",
+				    expected[i], i, acked[i]);
+			pass = false;
+			break;
+		}
+	}
+	report(pass, "%s", msg);
+}
  #endif
 
  static void gicv2_ipi_send_self(void)

@@ -591,6 +608,8 @@ static void gic_test_mmio(void)
  static void test_its_introspection(void) {}
  static void test_its_trigger(void) {}
  static void test_its_migration(void) {}
+static void test_its_pending_migration(void) {}
+static void test_migrate_unmapped_collection(void) {}
 
  #else /* __aarch64__ */
 

@@ -659,6 +678,17 @@ static int its_prerequisites(int nb_cpus)
  	return 0;
  }

arm/gic.c +31 -0

+}
+
  /*
   * Setup the configuration for those mappings:
   * dev_id=2 event=20 -> vcpu 3, intid=8195

@@ -799,6 +829,114 @@ static void test_its_migration(void)
  	its_send_int(dev7, 255);
  	check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
  }
+
+static void test_migrate_unmapped_collection(void)
+{
+	struct its_collection *col;
+	struct its_device *dev2, *dev7;
+	int pe0 = nr_cpus - 1;
+	u8 config;
+
+	if (its_setup1())
+		return;
+
+	col = its_create_collection(pe0, pe0);
+	dev2 = its_get_device(2);
+	dev7 = its_get_device(7);
+
+	/* MAPTI with the collection unmapped */
+	set_lpi(dev2, 0, 8192, col);
+
+	puts("Now migrate the VM, then press a key to continue...\n");
+	(void)getchar();
+	report_info("Migration complete");
+
+	/* on the destination, map the collection */
+	its_send_mapc(col, true);
+
+	lpi_stats_expect(2, 8196);
+	its_send_int(dev7, 255);
+	check_lpi_stats("dev7/eventid= 255 triggered LPI 8196 on PE #2");
+

arm/gic.c +35 -0

+
+	lpi_stats_expect(-1, -1);
+	its_send_int(dev2, 0);
+	check_lpi_stats("no LPI triggered after collection unmapping");
+}
+
+static void test_its_pending_migration(void)
+{
+	struct its_device *dev;
+	struct its_collection *collection[2];
+	int *expected = malloc(nr_cpus * sizeof(int));
+	int pe0 = nr_cpus - 1, pe1 = nr_cpus - 2;
+	u64 pendbaser;
+	void *ptr;
+	int i;
+
+	if (its_prerequisites(4))
+		return;
+
+	dev = its_create_device(2 /* dev id */, 8 /* nb_ites */);
+	its_send_mapd(dev, true);
+
+	collection[0] = its_create_collection(pe0, pe0);
+	collection[1] = its_create_collection(pe1, pe1);
+	its_send_mapc(collection[0], true);
+	its_send_mapc(collection[1], true);
+
+	/* disable lpi at redist level */
+	gicv3_lpi_rdist_disable(pe0);
+	gicv3_lpi_rdist_disable(pe1);
+
+	/* lpis are interleaved inbetween the 2 PEs */
+	for (i = 0; i < 256; i++) {
+		struct its_collection *col = i % 2 ? collection[0] :
+						     collection[1];

arm/gic.c +14 -0

+
+	expected[pe0] = 128;
+	expected[pe1] = 128;
+
+	ptr = gicv3_data.redist_base[pe0] + GICR_PENDBASER;
+	pendbaser = readq(ptr);
+	writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
+
+	ptr = gicv3_data.redist_base[pe1] + GICR_PENDBASER;
+	pendbaser = readq(ptr);
+	writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
+
+	gicv3_lpi_rdist_enable(pe0);
+	gicv3_lpi_rdist_enable(pe1);

Hi Eric,

Quoted 71 lines · Eric Auger

Add two new migration tests. One testing the migration of a topology where collection were unmapped. The second test checks the migration of the pending table.

Signed-off-by Eric Auger <[email protected]>

---

v4 -> v5:
- move stub from header to arm/gic.c

v3 -> v4:
- do not talk about odd/even CPUs, use pe0 and pe1
- comment the delay

v2 -> v3:
- tests belong to both its and migration groups
- use LPI(i)
- gicv3_lpi_set_pending_table_bit renamed into gicv3_lpi_set_clr_pending
---
  arm/gic.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++ arm/unittests.cfg |  16 +++++

  2 files changed, 162 insertions(+)

arm/gic.c +0 -0

index b8fbc13..e6ffbc3 100644
@@ -193,6 +193,7 @@ static void lpi_handler(struct pt_regs *regs
 __unused)

      smp_rmb(); /* pairs with wmb in lpi_stats_expect */ lpi_stats.observed.cpu_id = smp_processor_id(); lpi_stats.observed.lpi_id = irqnr;
+    acked[lpi_stats.observed.cpu_id]++; smp_wmb(); /* pairs with rmb in check_lpi_stats */ }
  @@ -236,6 +237,22 @@ static void secondary_lpi_test(void) while (1)
          wfi();
  }

arm/gic.c +16 -0

+
+static void check_lpi_hits(int *expected, const char *msg)
+{
+    bool pass = true;
+    int i;
+
+    for (i = 0; i < nr_cpus; i++) {
+        if (acked[i] != expected[i]) {
+            report_info("expected %d LPIs on PE #%d, %d observed",
+                    expected[i], i, acked[i]);
+            pass = false;
+            break;
+        }
+    }
+    report(pass, "%s", msg);
+}

  #endif
    static void gicv2_ipi_send_self(void)

arm/gic.c +2 -0

@@ -591,6 +608,8 @@ static void gic_test_mmio(void)
   static void test_its_introspection(void) {}
   static void test_its_trigger(void) {}
   static void test_its_migration(void) {}
+static void test_its_pending_migration(void) {}
+static void test_migrate_unmapped_collection(void) {}
     #else /* __aarch64__ */
   @@ -659,6 +678,17 @@ static int its_prerequisites(int nb_cpus)
       return 0;

  }
  +static void set_lpi(struct its_device *dev, u32 eventid, u32 physid, +            struct its_collection *col)
+{

arm/gic.c static int its_prerequisites(int nb_cpus) · line 689

Quoted 6 lines

+    assert(dev && col);

arm/gic.c +5 -0

+
+    its_send_mapti(dev, physid, eventid, col);
+
+    gicv3_lpi_set_config(physid, LPI_PROP_DEFAULT);
+    its_send_invall(col);

Again, the col hasn't been mapped currently.

Quoted 44 lines

arm/gic.c +2 -0

+}
+

  /*
   * Setup the configuration for those mappings:
   * dev_id=2 event=20 -> vcpu 3, intid=8195

arm/gic.c +0 -0

@@ -799,6 +829,114 @@ static void test_its_migration(void)
       its_send_int(dev7, 255);

      check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
  }

arm/gic.c +33 -0

+
+static void test_migrate_unmapped_collection(void)
+{
+    struct its_collection *col;
+    struct its_device *dev2, *dev7;
+    int pe0 = nr_cpus - 1;
+    u8 config;
+
+    if (its_setup1())
+        return;
+
+    col = its_create_collection(pe0, pe0);
+    dev2 = its_get_device(2);
+    dev7 = its_get_device(7);
+
+    /* MAPTI with the collection unmapped */
+    set_lpi(dev2, 0, 8192, col);
+
+    puts("Now migrate the VM, then press a key to continue...\n");
+    (void)getchar();
+    report_info("Migration complete");
+
+    /* on the destination, map the collection */
+    its_send_mapc(col, true);
+
+    lpi_stats_expect(2, 8196);
+    its_send_int(dev7, 255);
+    check_lpi_stats("dev7/eventid= 255 triggered LPI 8196 on PE #2");
+
+    config = gicv3_lpi_get_config(8192);
+    report(config == LPI_PROP_DEFAULT,
+           "Config of LPI 8192 was properly migrated");
+

Quoted 6 lines

+    lpi_stats_expect(pe0, 8192);
+    its_send_int(dev2, 0);
+    check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0"); +
+    /* unmap the collection */
+    its_send_mapc(col, false);

Again, behavior is unpredictable.

Quoted 40 lines

arm/gic.c +39 -0

+
+    lpi_stats_expect(-1, -1);
+    its_send_int(dev2, 0);
+    check_lpi_stats("no LPI triggered after collection unmapping");
+}
+
+static void test_its_pending_migration(void)
+{
+    struct its_device *dev;
+    struct its_collection *collection[2];
+    int *expected = malloc(nr_cpus * sizeof(int));
+    int pe0 = nr_cpus - 1, pe1 = nr_cpus - 2;
+    u64 pendbaser;
+    void *ptr;
+    int i;
+
+    if (its_prerequisites(4))
+        return;
+
+    dev = its_create_device(2 /* dev id */, 8 /* nb_ites */);
+    its_send_mapd(dev, true);
+
+    collection[0] = its_create_collection(pe0, pe0);
+    collection[1] = its_create_collection(pe1, pe1);
+    its_send_mapc(collection[0], true);
+    its_send_mapc(collection[1], true);
+
+    /* disable lpi at redist level */
+    gicv3_lpi_rdist_disable(pe0);
+    gicv3_lpi_rdist_disable(pe1);
+
+    /* lpis are interleaved inbetween the 2 PEs */
+    for (i = 0; i < 256; i++) {
+        struct its_collection *col = i % 2 ? collection[0] :
+                             collection[1];
+        int vcpu = col->target_address >> 16;
+
+        its_send_mapti(dev, LPI(i), i, col);
+        gicv3_lpi_set_config(LPI(i), LPI_PROP_DEFAULT);

Quoted 6 lines

+        gicv3_lpi_set_clr_pending(vcpu, LPI(i), true); +    }
+    its_send_invall(collection[0]);
+    its_send_invall(collection[1]);
+
+    /* Set the PTZ bit on each pendbaser */

'Clear' the PTZ.

Otherwise looks good!

Quoted 24 lines

arm/gic.c +23 -0

+
+    expected[pe0] = 128;
+    expected[pe1] = 128;
+
+    ptr = gicv3_data.redist_base[pe0] + GICR_PENDBASER;
+    pendbaser = readq(ptr);
+    writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
+
+    ptr = gicv3_data.redist_base[pe1] + GICR_PENDBASER;
+    pendbaser = readq(ptr);
+    writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
+
+    gicv3_lpi_rdist_enable(pe0);
+    gicv3_lpi_rdist_enable(pe1);
+
+    puts("Now migrate the VM, then press a key to continue...\n");
+    (void)getchar();
+    report_info("Migration complete");
+
+    /* let's wait for the 256 LPIs to be handled */
+    mdelay(1000);
+
+    check_lpi_hits(expected, "128 LPIs on both PE0 and PE1 after

Quoted 2 lines

migration");
+}

Thanks,
Zenghui