arm/arm64: Add ITS tests

Hi Eric,

lib/arm64/gic-v3-its.c line 73

Quoted 6 lines · Eric Auger

lib/arm64/gic-v3-its.c new file +6 -0

+static void its_cmd_queue_init(void)
+{
+    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
+    u64 cbaser;
+
+    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));

Shouldn't the cmd_base (and the cmd_write) be set as a GVA?

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

lib/arm64/gic-v3-its.c line 81

Quoted 10 lines

+
+    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    | GITS_CBASER_VALID);

lib/arm64/gic-v3-its.c new file +6 -0

+
+    writeq(cbaser, its_data.base + GITS_CBASER);
+
+    its_data.cmd_write = its_data.cmd_base;
+    writeq(0, its_data.base + GITS_CWRITER);
+}

Otherwise this looks good,

Reviewed-by Zenghui Yu <[email protected]>

Thanks

Hi Zenghui,

Quoted 11 lines · Zenghui Yu

Hi Eric,

lib/arm64/gic-v3-its.c line 73

Quoted 6 lines · Eric Auger

lib/arm64/gic-v3-its.c new file +6 -0

+static void its_cmd_queue_init(void)
+{
+    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
+    u64 cbaser;
+
+    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));

Shouldn't the cmd_base (and the cmd_write) be set as a GVA?

yes it should

Quoted 4 lines

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

I was told by Paolo that the VA/PA memory map is flat in kvmunit test.

Quoted 13 lines

lib/arm64/gic-v3-its.c line 81

Quoted 11 lines

+
+    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    | GITS_CBASER_VALID);

lib/arm64/gic-v3-its.c new file +6 -0

+
+    writeq(cbaser, its_data.base + GITS_CBASER);
+
+    its_data.cmd_write = its_data.cmd_base;
+    writeq(0, its_data.base + GITS_CWRITER);
+}

Otherwise this looks good,

Quoted 1 line

Reviewed-by Zenghui Yu <[email protected]>

Thanks!

Eric

Quoted 4 lines

Thanks

Quoted 14 lines · Auger Eric

Hi Zenghui,

Quoted 11 lines · Zenghui Yu

Hi Eric,

lib/arm64/gic-v3-its.c line 73

Quoted 6 lines · Eric Auger

lib/arm64/gic-v3-its.c new file +6 -0

+static void its_cmd_queue_init(void)
+{
+    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
+    u64 cbaser;
+
+    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));

Shouldn't the cmd_base (and the cmd_write) be set as a GVA?

Quoted 1 line

yes it should

If it's supposed to be a virtual address, when why do the virt_to_phys?

Quoted 6 lines

Quoted 5 lines

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

I was told by Paolo that the VA/PA memory map is flat in kvmunit test.

What does flat mean?

an identity map of all physical memory, which explains why the above is working.

phys addr, but when the ITS uses it as a virt addr it works because we *also* have a virt addr == phys addr mapping in the default page table, which is named "idmap" for good reason.

I think it would be better to test with the non-identity mapped addresses though.

Thanks,
drew

Quoted 23 lines

Quoted 13 lines

lib/arm64/gic-v3-its.c line 81

Quoted 11 lines

+
+    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    | GITS_CBASER_VALID);

lib/arm64/gic-v3-its.c new file +6 -0

+
+    writeq(cbaser, its_data.base + GITS_CBASER);
+
+    its_data.cmd_write = its_data.cmd_base;
+    writeq(0, its_data.base + GITS_CWRITER);
+}

Otherwise this looks good,

Quoted 1 line

Reviewed-by Zenghui Yu <[email protected]>

Thanks!

Eric

Quoted 4 lines

Thanks

Hi,

Quoted 15 lines · Andrew Jones

Quoted 14 lines · Auger Eric

Hi Zenghui,

Quoted 11 lines · Zenghui Yu

Hi Eric,

lib/arm64/gic-v3-its.c line 73

Quoted 6 lines · Eric Auger

lib/arm64/gic-v3-its.c new file +6 -0

+static void its_cmd_queue_init(void)
+{
+    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
+    u64 cbaser;
+
+    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));

Shouldn't the cmd_base (and the cmd_write) be set as a GVA?

Quoted 3 lines

Quoted 1 line

yes it should

If it's supposed to be a virtual address, when why do the virt_to_phys?

What is programmed in CBASER register is a physical address. So the virt_to_phys() is relevant. The inconsistency is in its_allocate_entry() introduced later on where I return the physical address instead of the virtual address. I will fix that.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

I was told by Paolo that the VA/PA memory map is flat in kvmunit test.

What does flat mean?

Yes I meant an identity map.

kvm-unit-tests, at least arm/arm64, does prepare

Quoted 2 lines

an identity map of all physical memory, which explains why the above is working.

should be the same on x86

It's doing virt_to_phys(some-virt-addr), which gets a

Quoted 6 lines

phys addr, but when the ITS uses it as a virt addr it works because we *also* have a virt addr == phys addr mapping in the default page table, which is named "idmap" for good reason.

I think it would be better to test with the non-identity mapped addresses though.

is there any way to exercise a non idmap?

Thanks

Eric

Quoted 27 lines

Thanks,
drew

Quoted 23 lines

Quoted 13 lines

lib/arm64/gic-v3-its.c line 81

Quoted 11 lines

+
+    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    | GITS_CBASER_VALID);

lib/arm64/gic-v3-its.c new file +6 -0

+
+    writeq(cbaser, its_data.base + GITS_CBASER);
+
+    its_data.cmd_write = its_data.cmd_base;
+    writeq(0, its_data.base + GITS_CWRITER);
+}

Otherwise this looks good,

Quoted 1 line

Reviewed-by Zenghui Yu <[email protected]>

Thanks!

Eric

Quoted 4 lines

Thanks

Quoted 36 lines · Auger Eric

Hi,

Quoted 15 lines · Andrew Jones

Quoted 14 lines · Auger Eric

Hi Zenghui,

Quoted 11 lines · Zenghui Yu

Hi Eric,

lib/arm64/gic-v3-its.c line 73

Quoted 6 lines · Eric Auger

lib/arm64/gic-v3-its.c new file +6 -0

+static void its_cmd_queue_init(void)
+{
+    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
+    u64 cbaser;
+
+    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));

Shouldn't the cmd_base (and the cmd_write) be set as a GVA?

Quoted 3 lines

Quoted 1 line

yes it should

If it's supposed to be a virtual address, when why do the virt_to_phys?

What is programmed in CBASER register is a physical address. So the virt_to_phys() is relevant. The inconsistency is in its_allocate_entry() introduced later on where I return the physical address instead of the virtual address. I will fix that.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

I was told by Paolo that the VA/PA memory map is flat in kvmunit test.

What does flat mean?

Quoted 7 lines

Yes I meant an identity map.

kvm-unit-tests, at least arm/arm64, does prepare

Quoted 2 lines

an identity map of all physical memory, which explains why the above is working.

should be the same on x86

Maybe, but I didn't write or review how x86 does their default map, so I don't know.

Quoted 10 lines

It's doing virt_to_phys(some-virt-addr), which gets a

Quoted 6 lines

phys addr, but when the ITS uses it as a virt addr it works because we *also* have a virt addr == phys addr mapping in the default page table, which is named "idmap" for good reason.

I think it would be better to test with the non-identity mapped addresses though.

is there any way to exercise a non idmap?

You could create your own map and then switch to it. See lib/arm/asm/mmu-api.h

But, you don't have to switch the whole map to use non-identity mapped addresses. Just create new virt mappings to the phys addresses you're interested in, and then use those new virt addresses instead. If you're worried that somewhere an identity mapped virt address is getting used because of a phys/virt address mix up, then you could probably sprinkle some assert(virt_to_phys(addr) != addr)'s around to ensure it.

Thanks,
drew

Quoted 33 lines

Thanks

Eric

Quoted 27 lines

Thanks,
drew

Quoted 23 lines

Quoted 13 lines

lib/arm64/gic-v3-its.c line 81

Quoted 11 lines

+
+    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    | GITS_CBASER_VALID);

lib/arm64/gic-v3-its.c new file +6 -0

+
+    writeq(cbaser, its_data.base + GITS_CBASER);
+
+    its_data.cmd_write = its_data.cmd_base;
+    writeq(0, its_data.base + GITS_CWRITER);
+}

Otherwise this looks good,

Quoted 1 line

Reviewed-by Zenghui Yu <[email protected]>

Thanks!

Eric

Quoted 4 lines

Thanks

Quoted 3 lines · Auger Eric

Hi Zenghui,

On 3/30/20 10:30 AM, Zenghui Yu wrote:

Quoted 4 lines

Quoted 3 lines

Otherwise I think we will end-up with memory corruption when writing the command queue.  But it seems that everything just works fine ... So I'm really confused here :-/

I was told by Paolo that the VA/PA memory map is flat in kvmunit test.

What does flat mean? kvm-unit-tests, at least arm/arm64, does prepare an identity map of all physical memory, which explains why the above is working. It's doing virt_to_phys(some-virt-addr), which gets a phys addr, but when the ITS uses it as a virt addr it works because we *also* have a virt addr == phys addr mapping in the default page table, which is named "idmap" for good reason.

Introduce an helper functions to register
- a new device, characterized by its device id and the max number of event IDs that dimension its ITT (Interrupt Translation Table). The function allocates the ITT.

- a new collection, characterized by its ID and the target processing engine (PE).

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

---

v3 -> v4:
- remove unused its_baser variable from its_create_device()
- use get_order()
- device->itt becomes a GVA instead of GPA

v2 -> v3:
- s/report_abort/assert

v1 -> v2:
- s/nb_/nr_

---
  lib/arm64/asm/gic-v3-its.h | 19 +++++++++++++++++++
  lib/arm64/gic-v3-its.c     | 38 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 57 insertions(+)

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

index 4683011..adcb642 100644
@@ -31,6 +31,19 @@ struct its_baser {
  };
 
  #define GITS_BASER_NR_REGS		8
+#define GITS_MAX_DEVICES		8
+#define GITS_MAX_COLLECTIONS		8
+
+struct its_device {
+	u32 device_id;	/* device ID */
+	u32 nr_ites;	/* Max Interrupt Translation Entries */
+	void *itt;	/* Interrupt Translation Table GVA */
+};
+
+struct its_collection {
+	u64 target_address;
+	u16 col_id;
+};
 
  struct its_data {
  	void *base;

@@ -39,6 +52,10 @@ struct its_data {
  	struct its_baser coll_baser;
  	struct its_cmd_block *cmd_base;
  	struct its_cmd_block *cmd_write;
+	struct its_device devices[GITS_MAX_DEVICES];
+	u32 nr_devices;		/* Allocated Devices */
+	struct its_collection collections[GITS_MAX_COLLECTIONS];
+	u32 nr_collections;	/* Allocated Collections */
  };
 
  extern struct its_data its_data;

Hi Eric,

Quoted 60 lines · Eric Auger

Introduce an helper functions to register
- a new device, characterized by its device id and the max number of event IDs that dimension its ITT (Interrupt Translation Table).  The function allocates the ITT.

- a new collection, characterized by its ID and the target processing engine (PE).

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

---

v3 -> v4:
- remove unused its_baser variable from its_create_device()
- use get_order()
- device->itt becomes a GVA instead of GPA

v2 -> v3:
- s/report_abort/assert

v1 -> v2:
- s/nb_/nr_
---
  lib/arm64/asm/gic-v3-its.h | 19 +++++++++++++++++++ lib/arm64/gic-v3-its.c     | 38 ++++++++++++++++++++++++++++++++++++++

  2 files changed, 57 insertions(+)

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

index 4683011..adcb642 100644
@@ -31,6 +31,19 @@ struct its_baser {
   };
     #define GITS_BASER_NR_REGS        8
+#define GITS_MAX_DEVICES        8
+#define GITS_MAX_COLLECTIONS        8
+
+struct its_device {
+    u32 device_id;    /* device ID */
+    u32 nr_ites;    /* Max Interrupt Translation Entries */
+    void *itt;    /* Interrupt Translation Table GVA */
+};
+
+struct its_collection {
+    u64 target_address;
+    u16 col_id;
+};
     struct its_data {
       void *base;

@@ -39,6 +52,10 @@ struct its_data {
       struct its_baser coll_baser;
       struct its_cmd_block *cmd_base;
       struct its_cmd_block *cmd_write;
+    struct its_device devices[GITS_MAX_DEVICES];
+    u32 nr_devices;        /* Allocated Devices */
+    struct its_collection collections[GITS_MAX_COLLECTIONS];
+    u32 nr_collections;    /* Allocated Collections */
   };
     extern struct its_data its_data;

@@ -93,5 +110,7 @@ extern void its_parse_typer(void);

lib/arm64/asm/gic-v3-its.h extern void its_parse_typer(void);

Quoted 6 lines

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

   extern void its_init(void);
   extern int its_baser_lookup(int i, struct its_baser *baser);
   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);

Maybe use 'u16 col_id'?

Besides,

Reviewed-by Zenghui Yu <[email protected]>

Thanks

Hi Zenghui,

Quoted 63 lines · Zenghui Yu

Hi Eric,

Quoted 60 lines · Eric Auger

Introduce an helper functions to register
- a new device, characterized by its device id and the max number of event IDs that dimension its ITT (Interrupt Translation Table).  The function allocates the ITT.

- a new collection, characterized by its ID and the target processing engine (PE).

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

---

v3 -> v4:
- remove unused its_baser variable from its_create_device()
- use get_order()
- device->itt becomes a GVA instead of GPA

v2 -> v3:
- s/report_abort/assert

v1 -> v2:
- s/nb_/nr_
---
  lib/arm64/asm/gic-v3-its.h | 19 +++++++++++++++++++ lib/arm64/gic-v3-its.c     | 38 ++++++++++++++++++++++++++++++++++++++

  2 files changed, 57 insertions(+)

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

index 4683011..adcb642 100644
@@ -31,6 +31,19 @@ struct its_baser {
   };
     #define GITS_BASER_NR_REGS        8
+#define GITS_MAX_DEVICES        8
+#define GITS_MAX_COLLECTIONS        8
+
+struct its_device {
+    u32 device_id;    /* device ID */
+    u32 nr_ites;    /* Max Interrupt Translation Entries */
+    void *itt;    /* Interrupt Translation Table GVA */
+};
+
+struct its_collection {
+    u64 target_address;
+    u16 col_id;
+};
     struct its_data {
       void *base;

@@ -39,6 +52,10 @@ struct its_data {
       struct its_baser coll_baser;
       struct its_cmd_block *cmd_base;
       struct its_cmd_block *cmd_write;
+    struct its_device devices[GITS_MAX_DEVICES];
+    u32 nr_devices;        /* Allocated Devices */
+    struct its_collection collections[GITS_MAX_COLLECTIONS];
+    u32 nr_collections;    /* Allocated Collections */
   };
     extern struct its_data its_data;

@@ -93,5 +110,7 @@ extern void its_parse_typer(void);

Quoted 8 lines

lib/arm64/asm/gic-v3-its.h extern void its_parse_typer(void);

Quoted 6 lines

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

   extern void its_init(void);
   extern int its_baser_lookup(int i, struct its_baser *baser);
   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);

Maybe use 'u16 col_id'?

fair enough. At this point, not sure this is worth a respin though ;-)

Quoted 3 lines

Besides,

Reviewed-by Zenghui Yu <[email protected]>

Thanks!

Eric

Quoted 5 lines

Thanks

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

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

lib/arm64/gic-v3-its-cmd.c new file +45 -0

+/* ITS COMMANDS */
+
+static void its_encode_cmd(struct its_cmd_block *cmd, u8 cmd_nr)
+{
+	cmd->raw_cmd[0] &= ~0xffUL;
+	cmd->raw_cmd[0] |= cmd_nr;
+}
+
+static void its_encode_devid(struct its_cmd_block *cmd, u32 devid)
+{
+	cmd->raw_cmd[0] &= BIT_ULL(32) - 1;
+	cmd->raw_cmd[0] |= ((u64)devid) << 32;
+}
+
+static void its_encode_event_id(struct its_cmd_block *cmd, u32 id)
+{
+	cmd->raw_cmd[1] &= ~0xffffffffUL;
+	cmd->raw_cmd[1] |= id;
+}
+
+static void its_encode_phys_id(struct its_cmd_block *cmd, u32 phys_id)
+{
+	cmd->raw_cmd[1] &= 0xffffffffUL;
+	cmd->raw_cmd[1] |= ((u64)phys_id) << 32;
+}
+
+static void its_encode_size(struct its_cmd_block *cmd, u8 size)
+{
+	cmd->raw_cmd[1] &= ~0x1fUL;
+	cmd->raw_cmd[1] |= size & 0x1f;
+}
+
+static void its_encode_itt(struct its_cmd_block *cmd, u64 itt_addr)
+{
+	cmd->raw_cmd[2] &= ~0xffffffffffffUL;
+	cmd->raw_cmd[2] |= itt_addr & 0xffffffffff00UL;
+}
+
+static void its_encode_valid(struct its_cmd_block *cmd, int valid)
+{
+	cmd->raw_cmd[2] &= ~(1UL << 63);
+	cmd->raw_cmd[2] |= ((u64)!!valid) << 63;
+}
+
+static void its_encode_target(struct its_cmd_block *cmd, u64 target_addr)

lib/arm64/gic-v3-its-cmd.c new file +5 -0

+
+	itt_addr = (unsigned long)(virt_to_phys(desc->its_mapd_cmd.dev->itt));
+	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
+
+	its_encode_cmd(cmd, GITS_CMD_MAPD);

Hi Eric,

Quoted 4 lines · Eric Auger

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

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

[...]

Quoted 46 lines

lib/arm64/gic-v3-its-cmd.c new file +45 -0

+/* ITS COMMANDS */
+
+static void its_encode_cmd(struct its_cmd_block *cmd, u8 cmd_nr)
+{
+    cmd->raw_cmd[0] &= ~0xffUL;
+    cmd->raw_cmd[0] |= cmd_nr;
+}
+
+static void its_encode_devid(struct its_cmd_block *cmd, u32 devid)
+{
+    cmd->raw_cmd[0] &= BIT_ULL(32) - 1;
+    cmd->raw_cmd[0] |= ((u64)devid) << 32;
+}
+
+static void its_encode_event_id(struct its_cmd_block *cmd, u32 id)
+{
+    cmd->raw_cmd[1] &= ~0xffffffffUL;
+    cmd->raw_cmd[1] |= id;
+}
+
+static void its_encode_phys_id(struct its_cmd_block *cmd, u32 phys_id)
+{
+    cmd->raw_cmd[1] &= 0xffffffffUL;
+    cmd->raw_cmd[1] |= ((u64)phys_id) << 32;
+}
+
+static void its_encode_size(struct its_cmd_block *cmd, u8 size)
+{
+    cmd->raw_cmd[1] &= ~0x1fUL;
+    cmd->raw_cmd[1] |= size & 0x1f;
+}
+
+static void its_encode_itt(struct its_cmd_block *cmd, u64 itt_addr)
+{
+    cmd->raw_cmd[2] &= ~0xffffffffffffUL;
+    cmd->raw_cmd[2] |= itt_addr & 0xffffffffff00UL;
+}
+
+static void its_encode_valid(struct its_cmd_block *cmd, int valid)
+{
+    cmd->raw_cmd[2] &= ~(1UL << 63);
+    cmd->raw_cmd[2] |= ((u64)!!valid) << 63;
+}
+
+static void its_encode_target(struct its_cmd_block *cmd, u64

target_addr)

lib/arm64/gic-v3-its-cmd.c line 141

Quoted 10 lines

lib/arm64/gic-v3-its-cmd.c new file +10 -0

+{
+    cmd->raw_cmd[2] &= ~(0xfffffffffUL << 16);
+    cmd->raw_cmd[2] |= (target_addr & (0xffffffffUL << 16));
+}
+
+static void its_encode_collection(struct its_cmd_block *cmd, u16 col)
+{
+    cmd->raw_cmd[2] &= ~0xffffUL;
+    cmd->raw_cmd[2] |= col;
+}

The command encoding can be refactored like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4d36f136d57aea6f6440886106e246bb7e5918d8

which will look much clearer.

[...]

lib/arm64/gic-v3-its-cmd.c line 211

Quoted 13 lines

lib/arm64/gic-v3-its-cmd.c new file +13 -0

+static void its_send_single_command(its_cmd_builder_t builder,
+                    struct its_cmd_desc *desc)
+{
+    struct its_cmd_block *cmd, *next_cmd;
+
+    cmd = its_allocate_entry();
+    builder(cmd, desc);
+    next_cmd = its_post_commands();
+
+    its_wait_for_range_completion(cmd, next_cmd);
+}
+
+

extra line.

lib/arm64/gic-v3-its-cmd.c line 216

Quoted 6 lines

lib/arm64/gic-v3-its-cmd.c new file +5 -0

+static void its_build_mapd_cmd(struct its_cmd_block *cmd,
+                   struct its_cmd_desc *desc)
+{
+    unsigned long itt_addr;
+    u8 size = 12; /* 4096 eventids */

Maybe use desc->its_mapd_cmd.dev->nr_ites instead as we already have it?

Quoted 7 lines

+
+    itt_addr = (unsigned
long)(virt_to_phys(desc->its_mapd_cmd.dev->itt));
+    itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);

lib/arm64/gic-v3-its-cmd.c new file +2 -0

+
+    its_encode_cmd(cmd, GITS_CMD_MAPD);

lib/arm64/gic-v3-its-cmd.c line 231

Quoted 10 lines

lib/arm64/gic-v3-its-cmd.c new file +10 -0

+    its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
+    its_encode_size(cmd, size - 1);
+    its_encode_itt(cmd, itt_addr);
+    its_encode_valid(cmd, desc->its_mapd_cmd.valid);
+    its_fixup_cmd(cmd);
+    if (desc->verbose)
+        printf("ITS: MAPD devid=%d size = 0x%x itt=0x%lx valid=%d\n",
+            desc->its_mapd_cmd.dev->device_id,
+            size, itt_addr, desc->its_mapd_cmd.valid);
+

extra line.

All of these are trivial things and feel free to ignore them,

Reviewed-by Zenghui Yu <[email protected]>

Thanks

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 +8 -0

+	smp_rmb(); /* pairs with wmb in lpi_handler */
+	if (lpi_stats.observed.cpu_id != lpi_stats.expected.cpu_id ||
+	    lpi_stats.observed.lpi_id != lpi_stats.expected.lpi_id) {
+		if (lpi_stats.observed.cpu_id == -1 &&
+		    lpi_stats.observed.lpi_id == -1) {
+			report_info("No LPI received whereas (cpuid=%d, intid=%d) "
+				    "was expected", lpi_stats.expected.cpu_id,
+				    lpi_stats.expected.lpi_id);

Hi Eric,

Quoted 9 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]>

[...]

So I've tested this series and found that the "INT" test will sometimes fail.

"not ok 12 - gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
not ok 13 - gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

From logs:
"INFO: gicv3: its-migration: Migration complete
INT dev_id=2 event_id=20
INFO: gicv3: its-migration: No LPI received whereas (cpuid=3, intid=8195) was expected
FAIL: gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
INT dev_id=7 event_id=255
INFO: gicv3: its-migration: No LPI received whereas (cpuid=2, intid=8196) was expected
FAIL: gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

arm/gic.c static void ipi_handler(struct pt_regs *regs __unused) · line 212

Quoted 5 lines

arm/gic.c +5 -0

+static void check_lpi_stats(const char *msg)
+{
+    bool pass = false;
+
+    mdelay(100);

After changing this to 'mdelay(1000)', the above error doesn't show up anymore. But it sounds strange that 100ms is not enough to deliver a single LPI. I haven't dig it further but will get back here later.

Quoted 11 lines

+    smp_rmb(); /* pairs with wmb in lpi_handler */
+    if (lpi_stats.observed.cpu_id != lpi_stats.expected.cpu_id || +        lpi_stats.observed.lpi_id != lpi_stats.expected.lpi_id) { +        if (lpi_stats.observed.cpu_id == -1 &&
+            lpi_stats.observed.lpi_id == -1) {
+            report_info("No LPI received whereas (cpuid=%d, intid=%d) " +                    "was expected", lpi_stats.expected.cpu_id, +                    lpi_stats.expected.lpi_id);
+        } else {
+            report_info("Unexpected LPI (cpuid=%d, intid=%d)",

Quoted 8 lines

+                    lpi_stats.observed.cpu_id,
+                    lpi_stats.observed.lpi_id);
+        }
+    } else {
+        pass = true;
+    }
+    report(pass, "%s", msg);
+}

This patch itself looks good to me,

Reviewed-by Zenghui Yu <[email protected]>

Thanks

Hi Zenghui,

Quoted 36 lines · Zenghui Yu

Hi Eric,

Quoted 9 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]>

[...]

So I've tested this series and found that the "INT" test will sometimes fail.

"not ok 12 - gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
not ok 13 - gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

From logs:
"INFO: gicv3: its-migration: Migration complete
INT dev_id=2 event_id=20
INFO: gicv3: its-migration: No LPI received whereas (cpuid=3, intid=8195) was expected
FAIL: gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
INT dev_id=7 event_id=255
INFO: gicv3: its-migration: No LPI received whereas (cpuid=2, intid=8196) was expected
FAIL: gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

Quoted 9 lines

arm/gic.c static void ipi_handler(struct pt_regs *regs __unused) · line 212

Quoted 5 lines

arm/gic.c +5 -0

+static void check_lpi_stats(const char *msg)
+{
+    bool pass = false;
+
+    mdelay(100);

After changing this to 'mdelay(1000)', the above error doesn't show up anymore. But it sounds strange that 100ms is not enough to deliver a single LPI. I haven't dig it further but will get back here later.

Did you find some time to investigate this issue. Changing 100 to 1000 has a huge impact on the overall test duration and I don't think it is sensible. Could you see what is your minimal value that pass the tests?

Hi Eric,

Quoted 49 lines · Auger Eric

Hi Zenghui,

Quoted 36 lines · Zenghui Yu

Hi Eric,

Quoted 9 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]>

[...]

So I've tested this series and found that the "INT" test will sometimes fail.

"not ok 12 - gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
not ok 13 - gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

From logs:
"INFO: gicv3: its-migration: Migration complete
INT dev_id=2 event_id=20
INFO: gicv3: its-migration: No LPI received whereas (cpuid=3, intid=8195) was expected
FAIL: gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 en PE #3 after migration
INT dev_id=7 event_id=255
INFO: gicv3: its-migration: No LPI received whereas (cpuid=2, intid=8196) was expected
FAIL: gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after migration"

Quoted 9 lines

arm/gic.c static void ipi_handler(struct pt_regs *regs __unused) · line 212

Quoted 5 lines

arm/gic.c +5 -0

+static void check_lpi_stats(const char *msg)
+{
+    bool pass = false;
+
+    mdelay(100);

After changing this to 'mdelay(1000)', the above error doesn't show up anymore. But it sounds strange that 100ms is not enough to deliver a single LPI. I haven't dig it further but will get back here later.

Quoted 3 lines

Did you find some time to investigate this issue. Changing 100 to 1000 has a huge impact on the overall test duration and I don't think it is sensible. Could you see what is your minimal value that pass the tests?

I can reproduce this issue with a very *low* probability so I failed to investigate it :-(. (It might because the LPI was delivered to a busy vcpu...)

You can leave it as it is until someone else complain about it again. Or take the similar approach as check_acked() - wait up to 5s for the interrupt to be delivered, and bail out as soon as we see it.

Thanks,
Zenghui

This test maps LPIs (populates the device table, the collection table, interrupt translation tables, configuration table), migrates and make sure the translation is correct on the destination.

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

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

v3 -> v4:
- assert in its_get_device/collection if the id is not found

---
  arm/gic.c                  | 58 ++++++++++++++++++++++++++++++++++----
  arm/unittests.cfg          |  8 ++++++
  lib/arm64/asm/gic-v3-its.h |  3 ++
  lib/arm64/gic-v3-its.c     | 22 +++++++++++++++
  4 files changed, 85 insertions(+), 6 deletions(-)

arm/gic.c +33 -6

index 5f1e595..6ecfdbc 100644
@@ -590,6 +590,7 @@ static void gic_test_mmio(void)
 
  static void test_its_introspection(void) {}
  static void test_its_trigger(void) {}
+static void test_its_migration(void) {}
 
  #else /* __aarch64__ */
 

@@ -658,13 +659,19 @@ static int its_prerequisites(int nb_cpus)
  	return 0;
  }
 
-static void test_its_trigger(void)
+/*
+ * Setup the configuration for those mappings:
+ * dev_id=2 event=20 -> vcpu 3, intid=8195
+ * dev_id=7 event=255 -> vcpu 2, intid=8196
+ * LPIs ready to hit
+ */
+static int its_setup1(void)
  {
  	struct its_collection *col3, *col2;
  	struct its_device *dev2, *dev7;
 
  	if (its_prerequisites(4))
-		return;
+		return -1;
 
  	dev2 = its_create_device(2 /* dev id */, 8 /* nb_ites */);
  	dev7 = its_create_device(7 /* dev id */, 8 /* nb_ites */);

@@ -675,14 +682,10 @@ static void test_its_trigger(void)
  	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
  	gicv3_lpi_set_config(8196, LPI_PROP_DEFAULT);
 
-	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);
 

@@ -694,6 +697,22 @@ static void test_its_trigger(void)
 
  	its_send_mapti(dev2, 8195 /* lpi id */, 20 /* event id */, col3);
  	its_send_mapti(dev7, 8196 /* lpi id */, 255 /* event id */, col2);
+	return 0;
+}
+
+static void test_its_trigger(void)
+{
+	struct its_collection *col3;
+	struct its_device *dev2, *dev7;
+
+	if (its_setup1())
+		return;
+
+	col3 = its_get_collection(3);
+	dev2 = its_get_device(2);
+	dev7 = its_get_device(7);
+
+	report_prefix_push("int");
 
  	lpi_stats_expect(3, 8195);
  	its_send_int(dev2, 20);

@@ -748,6 +767,29 @@ static void test_its_trigger(void)
  	check_lpi_stats("no LPI after device unmap");
  	report_prefix_pop();
  }
+
+static void test_its_migration(void)
+{
+	struct its_device *dev2, *dev7;
+
+	if (its_setup1())
+		return;
+

arm/gic.c +9 -0

+
+	lpi_stats_expect(2, 8196);
+	its_send_int(dev7, 255);
+	check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
+}
  #endif
 
  int main(int argc, char **argv)

@@ -785,6 +827,10 @@ int main(int argc, char **argv)
  		report_prefix_push(argv[1]);
  		test_its_trigger();
  		report_prefix_pop();
+	} else if (!strcmp(argv[1], "its-migration")) {
+		report_prefix_push(argv[1]);
+		test_its_migration();
+		report_prefix_pop();
  	} else if (strcmp(argv[1], "its-introspection") == 0) {
  		report_prefix_push(argv[1]);
  		test_its_introspection();

arm/unittests.cfg +8 -0

index b9a7a2c..480adec 100644
@@ -136,6 +136,14 @@ extra_params = -machine gic-version=3 -append 'its-trigger'
  groups = its
  arch = arm64
 
+[its-migration]
+file = gic.flat
+smp = $MAX_SMP
+accel = kvm
+extra_params = -machine gic-version=3 -append 'its-migration'
+groups = its migration
+arch = arm64
+
  # Test PSCI emulation
  [psci]
  file = psci.flat

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

index e1e42c3..412f438 100644
@@ -168,4 +168,7 @@ extern void __its_send_sync(struct its_collection *col, bool verbose);
  #define its_send_movi_nv(dev, col, id)			__its_send_movi(dev, col, id, false)
  #define its_send_sync_nv(col)				__its_send_sync(col, false)
 
+extern struct its_device *its_get_device(u32 id);
+extern struct its_collection *its_get_collection(u32 id);
+
  #endif /* _ASMARM64_GIC_V3_ITS_H_ */

lib/arm64/gic-v3-its.c +12 -0

index f0a0381..c7755d9 100644
@@ -147,3 +147,25 @@ struct its_collection *its_create_collection(u32 col_id, u32 pe)
  	its_data.nr_collections++;
  	return new;
  }
+
+struct its_device *its_get_device(u32 id)
+{
+	int i;
+
+	for (i = 0; i < GITS_MAX_DEVICES; i++) {
+		if (its_data.devices[i].device_id == id)
+			return &its_data.devices[i];
+	}
+	assert(0);
+}
+

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]>

arm/gic.c +8 -0

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

@@ -790,6 +818,108 @@ 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 = 0;
+	u8 config;

arm/gic.c +54 -0

+
+	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);
+	its_send_invall(col);
+
+	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");
+
+	lpi_stats_expect(pe0, 8192);
+	its_send_int(dev2, 0);
+	check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0");
+}
+
+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);

Hi Eric,

Quoted 5 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]>

[...]

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

Quoted 11 lines

arm/gic.c +7 -0

@@ -659,6 +678,15 @@ 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)
+{
+    assert(dev && col);
+
+    its_send_mapti(dev, physid, eventid, col);
+    gicv3_lpi_set_config(physid, LPI_PROP_DEFAULT);
+}

I'd say we can just drop this helper and open-code it anywhere necessarily. The name 'set_lpi' doesn't tell me too much about what has been implemented inside the helper.

Quoted 16 lines

arm/gic.c +1 -0

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

@@ -790,6 +818,108 @@ 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 +7 -0

+
+static void test_migrate_unmapped_collection(void)
+{
+    struct its_collection *col;
+    struct its_device *dev2, *dev7;
+    int pe0 = 0;
+    u8 config;

arm/gic.c static void test_its_migration(void) · line 837

Quoted 10 lines

arm/gic.c +10 -0

+
+    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);

... and it's only invoked here.

Quoted 54 lines

arm/gic.c +54 -0

+
+    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);
+    its_send_invall(col);
+
+    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");
+
+    lpi_stats_expect(pe0, 8192);
+    its_send_int(dev2, 0);
+    check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0");
+}
+
+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);

arm/gic.c static void test_its_migration(void) · line 901

Quoted 10 lines

arm/gic.c +10 -0

+        gicv3_lpi_set_config(LPI(i), LPI_PROP_DEFAULT);
+        gicv3_lpi_set_clr_pending(vcpu, LPI(i), true);
+    }
+    its_send_invall(collection[0]);
+    its_send_invall(collection[1]);
+
+    /* Clear the PTZ bit on each pendbaser */
+
+    expected[pe0] = 128;
+    expected[pe1] = 128;

Do we need to initialize expected[] for other PEs? Or it has always been zeroed by the kvm-unit-tests implementation of malloc()?

Quoted 9 lines

arm/gic.c +9 -0

+
+    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);
+

Quoted 2 lines

+    gicv3_lpi_rdist_enable(pe0);
+    gicv3_lpi_rdist_enable(pe1);

I don't know how the migration gets implemented in kvm-unit-tests. But is there any guarantee that the LPIs will only be triggered on the destination side? As once the EnableLPIs bit becomes 1, VGIC will start reading the pending bit in guest memory and potentially injecting LPIs into the target vcpu (in the source side).

Quoted 10 lines

arm/gic.c +9 -0

+
+    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

Hi Zenghui,

Quoted 9 lines

Quoted 9 lines

arm/gic.c +9 -0

+
+    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);
+

Quoted 8 lines

Quoted 2 lines

+    gicv3_lpi_rdist_enable(pe0);
+    gicv3_lpi_rdist_enable(pe1);

I don't know how the migration gets implemented in kvm-unit-tests. But is there any guarantee that the LPIs will only be triggered on the destination side? As once the EnableLPIs bit becomes 1, VGIC will start reading the pending bit in guest memory and potentially injecting LPIs into the target vcpu (in the source side).

I expect some LPIs to hit on source and some others to hit on the destination. To me, this does not really matter as long as the handlers gets called and accumulate the stats. Given the number of LPIs, we will at least test the migration of some of the pending bits and especially adjacent ones. It does work as it allows to test your fix:

ca185b260951 KVM: arm/arm64: vgic: Don't rely on the wrong pending table