irqchip/gic-v4: GICv4.1 architecture support

In a system that is only sparsly populated with CPUs, we can end-up with redistributors structures that are not initialized. Let's make sure we don't try and access those when iterating over them (in this case when checking we have a L2 VPE table).

Fixes 4e6437f12d6e ("irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level")

Signed-off-by Marc Zyngier <[email protected]>

---
  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
  1 file changed, 4 insertions(+)

drivers/irqchip/irq-gic-v3-its.c +0 -0

index 83b1186ffcad..da883a691028 100644

drivers/irqchip/irq-gic-v3-its.c +0 -0

  	val  = gicr_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);
 
  	esz  = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;

To allow the direct injection of SGIs into a guest, the GICv4.1 architecture has to sacrifice the Active state so that SGIs look a lot like LPIs (they are injected by the same mechanism).

In order not to break existing software, the architecture gives offers guests OSs the choice: SGIs with or without an active state. It is the hypervisors duty to honor the guest's choice.

For this, the architecture offers a discovery bit indicating whether the GIC supports GICv4.1 SGIs (GICD_TYPER2.nASSGIcap), and another bit indicating whether the guest wants Active-less SGIs or not (controlled by GICD_CTLR.nASSGIreq).

A hypervisor not supporting GICv4.1 SGIs would leave nASSGIcap clear, and a guest not knowing about GICv4.1 SGIs (or definitely wanting an Active state) would leave nASSGIreq clear (both being thankfully backward compatible with oler revisions of the GIC).

Since Linux is perfectly happy without an active state on SGIs, inform the hypervisor that we'll use that if offered.

Signed-off-by Marc Zyngier <[email protected]>

Tell KVM that we support v4.1. Nothing uses this information so far.

Signed-off-by Marc Zyngier <[email protected]>

One of the new features of GICv4.1 is to allow virtual SGIs to be directly signaled to a VPE. For that, the ITS has grown a new 64kB page containing only a single register that is used to signal a SGI to a given VPE.

Add a second mapping covering this new 64kB range, and take this opportunity to limit the original mapping to 64kB, which is enough to cover the span of the ITS registers.

Signed-off-by Marc Zyngier<[email protected]>

Since GICv4.1 has the capability to inject 16 SGIs into each VPE, and that I'm keen not to invent too many specific interfaces to manupulate these interrupts, let's pretend that each of these SGIs

is an actual Linux interrupt.

For that matter, let's introduce a minimal irqchip and irqdomain setup that will get fleshed up in the following patches.

Signed-off-by Marc Zyngier<[email protected]>

The GICv4.1 ITS has yet another new command (VSGI) which allows a VPE-targeted SGI to be configured (or have its pending state cleared). Add support for this command and plumb it into the activate irqdomain callback so that it is ready to be used.

Signed-off-by Marc Zyngier <[email protected]>

---
  drivers/irqchip/irq-gic-v3-its.c   | 76 +++++++++++++++++++++++++++++-
  include/linux/irqchip/arm-gic-v3.h |  3 +-
  2 files changed, 77 insertions(+), 2 deletions(-)

drivers/irqchip/irq-gic-v3-its.c +71 -0

index 6121c8f2a8ce..229e4ae9c59b 100644
@@ -354,6 +354,15 @@ struct its_cmd_desc {
  		struct {
  			struct its_vpe *vpe;
  		} its_invdb_cmd;
+
+		struct {
+			struct its_vpe *vpe;
+			u8 sgi;
+			u8 priority;
+			bool enable;
+			bool group;
+			bool clear;
+		} its_vsgi_cmd;
  	};
  };
 

@@ -502,6 +511,31 @@ static void its_encode_db(struct its_cmd_block *cmd, bool db)
  	its_mask_encode(&cmd->raw_cmd[2], db, 63, 63);
  }
 
+static void its_encode_sgi_intid(struct its_cmd_block *cmd, u8 sgi)
+{
+	its_mask_encode(&cmd->raw_cmd[0], sgi, 35, 32);
+}
+
+static void its_encode_sgi_priority(struct its_cmd_block *cmd, u8 prio)
+{
+	its_mask_encode(&cmd->raw_cmd[0], prio >> 4, 23, 20);
+}
+
+static void its_encode_sgi_group(struct its_cmd_block *cmd, bool grp)
+{
+	its_mask_encode(&cmd->raw_cmd[0], grp, 10, 10);
+}
+
+static void its_encode_sgi_clear(struct its_cmd_block *cmd, bool clr)
+{
+	its_mask_encode(&cmd->raw_cmd[0], clr, 9, 9);
+}
+
+static void its_encode_sgi_enable(struct its_cmd_block *cmd, bool en)
+{
+	its_mask_encode(&cmd->raw_cmd[0], en, 8, 8);
+}
+
  static inline void its_fixup_cmd(struct its_cmd_block *cmd)
  {
  	/* Let's fixup BE commands */

@@ -867,6 +901,26 @@ static struct its_vpe *its_build_invdb_cmd(struct its_node *its,
  	return valid_vpe(its, desc->its_invdb_cmd.vpe);
  }
 
+static struct its_vpe *its_build_vsgi_cmd(struct its_node *its,
+					  struct its_cmd_block *cmd,
+					  struct its_cmd_desc *desc)
+{
+	if (WARN_ON(!is_v4_1(its)))
+		return NULL;
+
+	its_encode_cmd(cmd, GITS_CMD_VSGI);
+	its_encode_vpeid(cmd, desc->its_vsgi_cmd.vpe->vpe_id);
+	its_encode_sgi_intid(cmd, desc->its_vsgi_cmd.sgi);
+	its_encode_sgi_priority(cmd, desc->its_vsgi_cmd.priority);
+	its_encode_sgi_group(cmd, desc->its_vsgi_cmd.group);
+	its_encode_sgi_clear(cmd, desc->its_vsgi_cmd.clear);
+	its_encode_sgi_enable(cmd, desc->its_vsgi_cmd.enable);
+
+	its_fixup_cmd(cmd);
+
+	return valid_vpe(its, desc->its_vsgi_cmd.vpe);
+}
+
  static u64 its_cmd_ptr_to_offset(struct its_node *its,
  				 struct its_cmd_block *ptr)
  {

@@ -3823,6 +3877,21 @@ static struct irq_chip its_vpe_4_1_irq_chip = {
  	.irq_set_vcpu_affinity	= its_vpe_4_1_set_vcpu_affinity,
  };
 
+static void its_configure_sgi(struct irq_data *d, bool clear)
+{
+	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+	struct its_cmd_desc desc;
+
+	desc.its_vsgi_cmd.vpe = vpe;
+	desc.its_vsgi_cmd.sgi = d->hwirq;
+	desc.its_vsgi_cmd.priority = vpe->sgi_config[d->hwirq].priority;
+	desc.its_vsgi_cmd.enable = vpe->sgi_config[d->hwirq].enabled;
+	desc.its_vsgi_cmd.group = vpe->sgi_config[d->hwirq].group;
+	desc.its_vsgi_cmd.clear = clear;
+
+	its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
+}
+
  static int its_sgi_set_affinity(struct irq_data *d,
  				const struct cpumask *mask_val,
  				bool force)

@@ -3868,13 +3937,18 @@ static void its_sgi_irq_domain_free(struct irq_domain *domain,
  static int its_sgi_irq_domain_activate(struct irq_domain *domain,
  				       struct irq_data *d, bool reserve)
  {
+	/* Write out the initial SGI configuration */
+	its_configure_sgi(d, false);
  	return 0;

drivers/irqchip/irq-gic-v3-its.c +0 -0

  }
 
  static struct irq_domain_ops its_sgi_domain_ops = {

include/linux/irqchip/arm-gic-v3.h +2 -1

index c29a02678a6f..a89578884263 100644
@@ -502,8 +502,9 @@
  #define GITS_CMD_VMAPTI			GITS_CMD_GICv4(GITS_CMD_MAPTI)
  #define GITS_CMD_VMOVI			GITS_CMD_GICv4(GITS_CMD_MOVI)
  #define GITS_CMD_VSYNC			GITS_CMD_GICv4(GITS_CMD_SYNC)
-/* VMOVP and INVDB are the odd ones, as they dont have a physical counterpart */
+/* VMOVP, VSGI and INVDB are the odd ones, as they dont have a physical counterpart */
  #define GITS_CMD_VMOVP			GITS_CMD_GICv4(2)
+#define GITS_CMD_VSGI			GITS_CMD_GICv4(3)
  #define GITS_CMD_INVDB			GITS_CMD_GICv4(0xe)
 
  /*

Hi Marc,

Quoted 5 lines

static void its_sgi_irq_domain_deactivate(struct irq_domain
*domain,
struct irq_data *d)
{

drivers/irqchip/irq-gic-v3-its.c +0 -1

-	/* Nothing to do */

drivers/irqchip/irq-gic-v3-its.c static void its_sgi_irq_domain_free(struct irq_domain *domain, · line 3951

Quoted 4 lines

drivers/irqchip/irq-gic-v3-its.c +4 -0

+	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+	vpe->sgi_config[d->hwirq].enabled = false;
+	its_configure_sgi(d, true);

The spec says, when C==1, VSGI clears the pending state of the vSGI, leaving the configuration unchanged. So should we first clear the pending state and then disable vSGI (let E==0)?

Hi Zenghui,

Quoted 1 line · Zenghui Yu

Hi Marc,

[...]

Quoted 9 lines

Quoted 8 lines

    static void its_sgi_irq_domain_deactivate(struct irq_domain *domain, struct irq_data *d)
  {
-    /* Nothing to do */
+    struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+    vpe->sgi_config[d->hwirq].enabled = false;
+    its_configure_sgi(d, true);

Quoted 3 lines

The spec says, when C==1, VSGI clears the pending state of the vSGI, leaving the configuration unchanged.  So should we first clear the pending state and then disable vSGI (let E==0)?

Right you are again. We need two commands, not just one (the pseudocode is pretty explicit).

Implement mask/unmask for virtual SGIs by calling into the configuration helper.

Signed-off-by Marc Zyngier <[email protected]>

To implement the get/set_irqchip_state callbacks (limited to the PENDING state), we have to use a particular set of hacks:

- Reading the pending state is done by using a pair of new redistributor registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16 interrupts state to be retrieved.
- Setting the pending state is done by generating it as we'd otherwise do for a guest (writing to GITS_SGIR)
- Clearing the pending state is done by emiting a VSGI command with the "clear" bit set.

Signed-off-by Marc Zyngier <[email protected]>

---
  drivers/irqchip/irq-gic-v3-its.c   | 56 ++++++++++++++++++++++++++++++
  include/linux/irqchip/arm-gic-v3.h | 14 ++++++++
  2 files changed, 70 insertions(+)

drivers/irqchip/irq-gic-v3-its.c +17 -0

index 1e448d9a16ea..a9753435c4ff 100644
@@ -3915,11 +3915,67 @@ static int its_sgi_set_affinity(struct irq_data *d,
  	return -EINVAL;
  }
 
+static int its_sgi_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which,
+				     bool state)
+{
+	if (which != IRQCHIP_STATE_PENDING)
+		return -EINVAL;
+
+	if (state) {
+		struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+		struct its_node *its = find_4_1_its();
+		u64 val;
+
+		val  = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
+		val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
+		writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
+	} else {
+		its_configure_sgi(d, true);

drivers/irqchip/irq-gic-v3-its.c +29 -0

+	u32 count = 1000000;	/* 1s! */
+	u32 status;
+
+	if (which != IRQCHIP_STATE_PENDING)
+		return -EINVAL;
+
+	writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
+	do {
+		status = readl_relaxed(base + GICR_VSGIPENDR);
+		if (!(status & GICR_VSGIPENDR_BUSY))
+			goto out;
+
+		count--;
+		if (!count) {
+			pr_err_ratelimited("Unable to get SGI status\n");
+			goto out;
+		}
+		cpu_relax();
+		udelay(1);
+	} while(count);
+
+out:
+	*val = !!(status & (1 << d->hwirq));
+
+	return 0;
+}
+
  static struct irq_chip its_sgi_irq_chip = {
  	.name			= "GICv4.1-sgi",
  	.irq_mask		= its_sgi_mask_irq,
  	.irq_unmask		= its_sgi_unmask_irq,
  	.irq_set_affinity	= its_sgi_set_affinity,
+	.irq_set_irqchip_state	= its_sgi_set_irqchip_state,
+	.irq_get_irqchip_state	= its_sgi_get_irqchip_state,
  };
 
  static int its_sgi_irq_domain_alloc(struct irq_domain *domain,

include/linux/irqchip/arm-gic-v3.h +9 -0

index a89578884263..64da945486ac 100644
@@ -345,6 +345,15 @@
  #define GICR_VPENDBASER_4_1_VGRP1EN	(1ULL << 58)
  #define GICR_VPENDBASER_4_1_VPEID	GENMASK_ULL(15, 0)
 
+#define GICR_VSGIR			0x0080
+
+#define GICR_VSGIR_VPEID		GENMASK(15, 0)
+
+#define GICR_VSGIPENDR			0x0088
+
+#define GICR_VSGIPENDR_BUSY		(1U << 31)
+#define GICR_VSGIPENDR_PENDING		GENMASK(15, 0)
+
  /*
   * ITS registers, offsets from ITS_base
   */

include/linux/irqchip/arm-gic-v3.h +1 -0

+
  #define GITS_CTLR_ENABLE		(1U << 0)
  #define GITS_CTLR_ImDe			(1U << 1)
  #define	GITS_CTLR_ITS_NUMBER_SHIFT	4

Hi Marc,

Quoted 55 lines · Marc Zyngier

To implement the get/set_irqchip_state callbacks (limited to the PENDING state), we have to use a particular set of hacks:

- Reading the pending state is done by using a pair of new redistributor
registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16
interrupts
state to be retrieved.
- Setting the pending state is done by generating it as we'd otherwise do
for a guest (writing to GITS_SGIR)
- Clearing the pending state is done by emiting a VSGI command with the
"clear" bit set.

Signed-off-by Marc Zyngier <[email protected]>

---
  drivers/irqchip/irq-gic-v3-its.c   | 56 
++++++++++++++++++++++++++++++
  include/linux/irqchip/arm-gic-v3.h | 14 ++++++++
  2 files changed, 70 insertions(+)

drivers/irqchip/irq-gic-v3-its.c +24 -0

index 1e448d9a16ea..a9753435c4ff 100644
@@ -3915,11 +3915,67 @@ static int its_sgi_set_affinity(struct 
 irq_data *d,
  	return -EINVAL;
  }
  +static int its_sgi_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which,
+				     bool state)
+{
+	if (which != IRQCHIP_STATE_PENDING)
+		return -EINVAL;
+
+	if (state) {
+		struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+		struct its_node *its = find_4_1_its();
+		u64 val;
+
+		val  = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
+		val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
+		writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
+	} else {
+		its_configure_sgi(d, true);
+	}
+
+	return 0;
+}
+
+static int its_sgi_get_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool *val)
+{

drivers/irqchip/irq-gic-v3-its.c static int its_sgi_set_affinity(struct · line 3945

Quoted 3 lines

drivers/irqchip/irq-gic-v3-its.c +2 -0

+	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+	void __iomem *base = gic_data_rdist_cpu(vpe->col_idx)->rd_base + 
 SZ_128K;

There might be a race on reading the 'vpe->col_idx' against a concurrent vPE schedule (col_idx will be modified in its_vpe_set_affinity)? Will we end up accessing the GICR_VSGI* registers of the old redistributor, while the vPE is now resident on the new one? Or is it harmful?

The same question for direct_lpi_inv(), where 'vpe->col_idx' will be used in irq_to_cpuid().

Quoted 56 lines

drivers/irqchip/irq-gic-v3-its.c +29 -0

+	u32 count = 1000000;	/* 1s! */
+	u32 status;
+
+	if (which != IRQCHIP_STATE_PENDING)
+		return -EINVAL;
+
+	writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
+	do {
+		status = readl_relaxed(base + GICR_VSGIPENDR);
+		if (!(status & GICR_VSGIPENDR_BUSY))
+			goto out;
+
+		count--;
+		if (!count) {
+			pr_err_ratelimited("Unable to get SGI status\n");
+			goto out;
+		}
+		cpu_relax();
+		udelay(1);
+	} while(count);
+
+out:
+	*val = !!(status & (1 << d->hwirq));
+
+	return 0;
+}
+
  static struct irq_chip its_sgi_irq_chip = {
  	.name			= "GICv4.1-sgi",
  	.irq_mask		= its_sgi_mask_irq,
  	.irq_unmask		= its_sgi_unmask_irq,
  	.irq_set_affinity	= its_sgi_set_affinity,
+	.irq_set_irqchip_state	= its_sgi_set_irqchip_state,
+	.irq_get_irqchip_state	= its_sgi_get_irqchip_state,
  };
    static int its_sgi_irq_domain_alloc(struct irq_domain *domain,

include/linux/irqchip/arm-gic-v3.h +8 -0

index a89578884263..64da945486ac 100644
@@ -345,6 +345,15 @@
  #define GICR_VPENDBASER_4_1_VGRP1EN	(1ULL << 58)
  #define GICR_VPENDBASER_4_1_VPEID	GENMASK_ULL(15, 0)
  +#define GICR_VSGIR			0x0080
+
+#define GICR_VSGIR_VPEID		GENMASK(15, 0)
+
+#define GICR_VSGIPENDR			0x0088
+
+#define GICR_VSGIPENDR_BUSY		(1U << 31)
+#define GICR_VSGIPENDR_PENDING		GENMASK(15, 0)
+
  /*
   * ITS registers, offsets from ITS_base
   */

include/linux/irqchip/arm-gic-v3.h line 381

Quoted 6 lines

include/linux/irqchip/arm-gic-v3.h +3 -0

@@ -368,6 +377,11 @@
    #define GITS_TRANSLATER			0x10040
  +#define GITS_SGIR			0x20020
+
+#define GITS_SGIR_VPEID			GENMASK_ULL(47, 32)
+#define GITS_SGIR_VINTID		GENMASK_ULL(7, 0)

The spec says vINTID field is [3:0] of the GITS_SGIR.

Hi Zenghui,

Quoted 1 line · Zenghui Yu

Hi Marc,

Quoted 4 lines

There might be a race on reading the 'vpe->col_idx' against a concurrent vPE schedule (col_idx will be modified in its_vpe_set_affinity)? Will we end up accessing the GICR_VSGI* registers of the old redistributor, while the vPE is now resident on the new one? Or is it harmful?

Very well spotted. There is a potential problem if old and new RDs are not part of the same CommonLPIAff group.

Quoted 2 lines

The same question for direct_lpi_inv(), where 'vpe->col_idx' will be used in irq_to_cpuid().

Same problem indeed. We need to ensure that no VMOVP operation can occur whilst we use col_idx to access a redistributor. This means a vPE lock of some sort that will protect the affinity.

But I think there is a slightly more general problem here, which we failed to see initially: the same issue exists for physical LPIs, as col_map[] can be updated (its_set_affinity()) in parallel with a direct invalidate.

The good old invalidation through the ITS does guarantee that the two operation don't overlap, but direct invalidation breaks it.

Let me have a think about it.

Hi Zenghui,

Quoted 4 lines · Marc Zyngier

Hi Zenghui,

Quoted 1 line · Zenghui Yu

Hi Marc,

[...]

Quoted 14 lines

Quoted 4 lines

There might be a race on reading the 'vpe->col_idx' against a concurrent vPE schedule (col_idx will be modified in its_vpe_set_affinity)? Will we end up accessing the GICR_VSGI* registers of the old redistributor, while the vPE is now resident on the new one? Or is it harmful?

Very well spotted. There is a potential problem if old and new RDs are not part of the same CommonLPIAff group.

Quoted 2 lines

The same question for direct_lpi_inv(), where 'vpe->col_idx' will be used in irq_to_cpuid().

Same problem indeed. We need to ensure that no VMOVP operation can occur whilst we use col_idx to access a redistributor. This means a vPE lock of some sort that will protect the affinity.

Quoted 6 lines

But I think there is a slightly more general problem here, which we failed to see initially: the same issue exists for physical LPIs, as col_map[] can be updated (its_set_affinity()) in parallel with a direct invalidate.

The good old invalidation through the ITS does guarantee that the two operation don't overlap, but direct invalidation breaks it.

Quoted 1 line

Let me have a think about it.

So I've thought about it, wrote a patch, and I don't really like the look of it. This is pretty invasive, and we end-up serializing a lot more than we used to (the repurposing of vlpi_lock to a general "lpi mapping lock" is probably too coarse).

It of course needs splitting over at least three patches, but it'd be good if you could have a look (applies on top of the whole series).

3. it looks like KVM makes the assumption that the per-RD MMIO region will only be accessed by the associated VCPU?  But I think this's not restricted by the architecture, we can do it better.  Or I've just missed some important points here.

drivers/irqchip/irq-gic-v3-its.c +4 -4

      struct its_vm        *vm;
      struct its_vlpi_map    *vlpi_maps;
      int            nr_vlpis;

@@ -240,15 +240,33 @@ static struct its_vlpi_map *get_vlpi_map(struct 
 irq_data *d)
      return NULL;
  }
 
-static int irq_to_cpuid(struct irq_data *d)
+static int irq_to_cpuid_lock(struct irq_data *d, unsigned long *flags)
  {
-    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
      struct its_vlpi_map *map = get_vlpi_map(d);
+    int cpu;
 
-    if (map)
-        return map->vpe->col_idx;
+    if (map) {
+        raw_spin_lock_irqsave(&map->vpe->vpe_lock, *flags);

drivers/irqchip/irq-gic-v3-its.c +7 -3

  static struct its_collection *valid_col(struct its_collection *col)

@@ -1384,6 +1402,8 @@ static void direct_lpi_inv(struct irq_data *d)
  {
      struct its_vlpi_map *map = get_vlpi_map(d);
      void __iomem *rdbase;
+    unsigned long flags;
+    int cpu;
      u64 val;
 
      if (map) {

@@ -1399,10 +1419,12 @@ static void direct_lpi_inv(struct irq_data *d)
      }
 
      /* Target the redistributor this LPI is currently routed to */
-    rdbase = per_cpu_ptr(gic_rdists->rdist, irq_to_cpuid(d))->rd_base;
+    cpu = irq_to_cpuid_lock(d, &flags);
+    rdbase = per_cpu_ptr(gic_rdists->rdist, cpu)->rd_base;
      gic_write_lpir(val, rdbase + GICR_INVLPIR);
 
      wait_for_syncr(rdbase);
+    irq_to_cpuid_unlock(d, flags);
  }
 
  static void lpi_update_config(struct irq_data *d, u8 clr, u8 set)

@@ -1471,11 +1493,11 @@ static void its_unmask_irq(struct irq_data *d)
  static int its_set_affinity(struct irq_data *d, const struct cpumask 
 *mask_val,
                  bool force)
  {
-    unsigned int cpu;
      const struct cpumask *cpu_mask = cpu_online_mask;
      struct its_device *its_dev = irq_data_get_irq_chip_data(d);
      struct its_collection *target_col;
-    u32 id = its_get_event_id(d);
+    unsigned int from, cpu;
+    unsigned long flags;
 
      /* A forwarded interrupt should use irq_set_vcpu_affinity */

     if (irqd_is_forwarded_to_vcpu(d))

drivers/irqchip/irq-gic-v3-its.c +0 -0

@@ -1496,12 +1518,16 @@ static int its_set_affinity(struct irq_data *d, 
 const struct cpumask *mask_val,
          return -EINVAL;
 
      /* don't set the affinity when the target cpu is same as current 

one */
-    if (cpu != its_dev->event_map.col_map[id]) {
+    from = irq_to_cpuid_lock(d, &flags);
+    if (cpu != from) {
+        u32 id = its_get_event_id(d);

drivers/irqchip/irq-gic-v3-its.c +3 -1

+
          target_col = &its_dev->its->collections[cpu];
          its_send_movi(its_dev, target_col, id);
          its_dev->event_map.col_map[id] = cpu;
          irq_data_update_effective_affinity(d, cpumask_of(cpu));
      }
+    irq_to_cpuid_unlock(d, flags);
 
      return IRQ_SET_MASK_OK_DONE;
  }

@@ -1636,7 +1662,7 @@ static int its_vlpi_map(struct irq_data *d, struct 
 its_cmd_info *info)
      if (!info->map)
          return -EINVAL;
 
-    raw_spin_lock(&its_dev->event_map.vlpi_lock);
+    raw_spin_lock(&its_dev->event_map.map_lock);
 
      if (!its_dev->event_map.vm) {

         struct its_vlpi_map *maps;

drivers/irqchip/irq-gic-v3-its.c +11 -6

@@ -1685,7 +1711,7 @@ static int its_vlpi_map(struct irq_data *d, struct 
 its_cmd_info *info)
      }
 
  out:
-    raw_spin_unlock(&its_dev->event_map.vlpi_lock);
+    raw_spin_unlock(&its_dev->event_map.map_lock);
      return ret;
  }

@@ -1695,7 +1721,7 @@ static int its_vlpi_get(struct irq_data *d, struct 
 its_cmd_info *info)
      struct its_vlpi_map *map;
      int ret = 0;
 
-    raw_spin_lock(&its_dev->event_map.vlpi_lock);
+    raw_spin_lock(&its_dev->event_map.map_lock);
 
      map = get_vlpi_map(d);

@@ -1708,7 +1734,7 @@ static int its_vlpi_get(struct irq_data *d, struct 
 its_cmd_info *info)
      *info->map = *map;
 
  out:
-    raw_spin_unlock(&its_dev->event_map.vlpi_lock);
+    raw_spin_unlock(&its_dev->event_map.map_lock);
      return ret;
  }

@@ -1718,7 +1744,7 @@ static int its_vlpi_unmap(struct irq_data *d)
      u32 event = its_get_event_id(d);
      int ret = 0;
 
-    raw_spin_lock(&its_dev->event_map.vlpi_lock);
+    raw_spin_lock(&its_dev->event_map.map_lock);
 
      if (!its_dev->event_map.vm || !irqd_is_forwarded_to_vcpu(d)) {
          ret = -EINVAL;

@@ -1748,7 +1774,7 @@ static int its_vlpi_unmap(struct irq_data *d)
      }
 
  out:
-    raw_spin_unlock(&its_dev->event_map.vlpi_lock);
+    raw_spin_unlock(&its_dev->event_map.map_lock);
      return ret;
  }
 

@@ -3193,7 +3219,7 @@ static struct its_device *its_create_device(struct 
 its_node *its, u32 dev_id,
      dev->event_map.col_map = col_map;
      dev->event_map.lpi_base = lpi_base;
      dev->event_map.nr_lpis = nr_lpis;
-    raw_spin_lock_init(&dev->event_map.vlpi_lock);
+    raw_spin_lock_init(&dev->event_map.map_lock);
      dev->device_id = dev_id;
      INIT_LIST_HEAD(&dev->entry);

@@ -3560,6 +3586,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
  {
      struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
      int from, cpu = cpumask_first(mask_val);
+    unsigned long flags;
 
      /*
       * Changing affinity is mega expensive, so let's be as lazy as

@@ -3567,6 +3594,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
       * into the proxy device, we need to move the doorbell
       * interrupt to its new location.
       */
+    raw_spin_lock_irqsave(&vpe->vpe_lock, flags);
      if (vpe->col_idx == cpu)
          goto out;
 

@@ -3586,6 +3614,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
 
  out:
      irq_data_update_effective_affinity(d, cpumask_of(cpu));
+    raw_spin_unlock_irqrestore(&vpe->vpe_lock, flags);
 
      return IRQ_SET_MASK_OK_DONE;
  }

@@ -3695,11 +3724,15 @@ static void its_vpe_send_inv(struct irq_data *d)
 
      if (gic_rdists->has_direct_lpi) {
          void __iomem *rdbase;
+        unsigned long flags;
+        int cpu;
 

drivers/irqchip/irq-gic-v3-its.c +5 -1

@@ -3735,14 +3768,18 @@ static int its_vpe_set_irqchip_state(struct 
 irq_data *d,
 
      if (gic_rdists->has_direct_lpi) {
          void __iomem *rdbase;
+        unsigned long flags;
+        int cpu;
 
-        rdbase = per_cpu_ptr(gic_rdists->rdist, vpe->col_idx)->rd_base;
+        cpu = irq_to_cpuid_lock(d, &flags);
+        rdbase = per_cpu_ptr(gic_rdists->rdist, cpu)->rd_base;
          if (state) {
              gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_SETLPIR);
          } else {
              gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_CLRLPIR);
              wait_for_syncr(rdbase);
          }
+        irq_to_cpuid_unlock(d, flags);
      } else {
          if (state)

             its_vpe_send_cmd(vpe, its_send_int);

drivers/irqchip/irq-gic-v3-its.c +3 -0

@@ -3854,14 +3891,17 @@ static void its_vpe_4_1_deschedule(struct 
 its_vpe *vpe,
  static void its_vpe_4_1_invall(struct its_vpe *vpe)
  {
      void __iomem *rdbase;
+    unsigned long flags;
      u64 val;
 
      val  = GICR_INVALLR_V;
      val |= FIELD_PREP(GICR_INVALLR_VPEID, vpe->vpe_id);
 
      /* Target the redistributor this vPE is currently known on */
+    raw_spin_lock_irqsave(&vpe->vpe_lock, flags);
      rdbase = per_cpu_ptr(gic_rdists->rdist, vpe->col_idx)->rd_base;
      gic_write_lpir(val, rdbase + GICR_INVALLR);
+    raw_spin_unlock_irqrestore(&vpe->vpe_lock, flags);
  }
 

 static int its_vpe_4_1_set_vcpu_affinity(struct irq_data *d, void
*vcpu_info)

drivers/irqchip/irq-gic-v3-its.c +5 -1

@@ -3960,13 +4000,17 @@ static int its_sgi_get_irqchip_state(struct 
 irq_data *d,
                       enum irqchip_irq_state which, bool *val)
  {
      struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
-    void __iomem *base = gic_data_rdist_cpu(vpe->col_idx)->rd_base + 
 SZ_128K;
+    void __iomem *base;
+    unsigned long flags;
      u32 count = 1000000;    /* 1s! */
      u32 status;
+    int cpu;
 
      if (which != IRQCHIP_STATE_PENDING)
          return -EINVAL;
 
+    cpu = irq_to_cpuid_lock(d, &flags);
+    base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
      writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);

     do {
         status = readl_relaxed(base + GICR_VSGIPENDR);

drivers/irqchip/irq-gic-v3-its.c +1 -0

@@ -3983,6 +4027,7 @@ static int its_sgi_get_irqchip_state(struct 
 irq_data *d,
      } while(count);
 
  out:
+    irq_to_cpuid_unlock(d, flags);
      *val = !!(status & (1 << d->hwirq));
 

     return 0;

drivers/irqchip/irq-gic-v3-its.c +1 -0

@@ -4102,6 +4147,7 @@ static int its_vpe_init(struct its_vpe *vpe)
          return -ENOMEM;
      }
 
+    raw_spin_lock_init(&vpe->vpe_lock);
      vpe->vpe_id = vpe_id;
      vpe->vpt_page = vpt_page;
      if (gic_rdists->has_rvpeid)

include/linux/irqchip/arm-gic-v4.h +0 -0

index 46c167a6349f..fc43a63875a3 100644

Hi Marc,

drivers/irqchip/irq-gic-v3-its.c struct event_lpi_map { · line 147

Quoted 10 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c +1 -1

index 7656b353a95f..0ed286dba827 100644
@@ -144,7 +144,7 @@ struct event_lpi_map {
      u16            *col_map;
      irq_hw_number_t        lpi_base;
      int            nr_lpis;
-    raw_spinlock_t        vlpi_lock;
+    raw_spinlock_t        map_lock;

So we use map_lock to protect both LPI's and VLPI's mapping affinity of a device, and use vpe_lock to protect vPE's affinity, OK.

Quoted 27 lines

drivers/irqchip/irq-gic-v3-its.c +10 -4

      struct its_vm        *vm;
      struct its_vlpi_map    *vlpi_maps;
      int            nr_vlpis;

@@ -240,15 +240,33 @@ static struct its_vlpi_map *get_vlpi_map(struct 
 irq_data *d)
      return NULL;
  }
 
-static int irq_to_cpuid(struct irq_data *d)
+static int irq_to_cpuid_lock(struct irq_data *d, unsigned long 
 *flags)
  {
-    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
      struct its_vlpi_map *map = get_vlpi_map(d);
+    int cpu;
 
-    if (map)
-        return map->vpe->col_idx;
+    if (map) {
+        raw_spin_lock_irqsave(&map->vpe->vpe_lock, *flags);
+        cpu = map->vpe->col_idx;
+    } else {
+        struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+        raw_spin_lock_irqsave(&its_dev->event_map.map_lock, *flags);
+        cpu = its_dev->event_map.col_map[its_get_event_id(d)];
+    }
 

drivers/irqchip/irq-gic-v3-its.c static struct its_vlpi_map *get_vlpi_map(struct irq_data *d) · line 258

Quoted 3 lines

drivers/irqchip/irq-gic-v3-its.c +2 -1

-    return its_dev->event_map.col_map[its_get_event_id(d)];
+    return cpu;
+}

This helper is correct for normal LPIs and VLPIs, but wrong for per-vPE IRQ (doorbell) and vSGIs. irq_data_get_irq_chip_data() gets confused by both of them.

Quoted 11 lines

drivers/irqchip/irq-gic-v3-its.c +0 -1

-        rdbase = per_cpu_ptr(gic_rdists->rdist, 

vpe->col_idx)->rd_base;
+        cpu = irq_to_cpuid_lock(d, &flags);
+        rdbase = per_cpu_ptr(gic_rdists->rdist, cpu)->rd_base;
         gic_write_lpir(d->parent_data->hwirq, rdbase +
GICR_INVLPIR);
         wait_for_syncr(rdbase);
+        irq_to_cpuid_unlock(d, flags);
     } else {
         its_vpe_send_cmd(vpe, its_send_inv);
     }

Do we really need to grab the vpe_lock for those which are belong to the same irqchip with its_vpe_set_affinity()? The IRQ core code should already ensure the mutual exclusion among them, wrong?

Quoted 3 lines · Zenghui Yu

Do we really need to grab the vpe_lock for those which are belong to the same irqchip with its_vpe_set_affinity()? The IRQ core code should already ensure the mutual exclusion among them, wrong?

I've been trying to think about that, but jet-lag keeps getting in the way. I empirically think that you are right, but I need to go and check the various code paths to be sure. Hopefully I'll have a bit more brain space next week.

Quoted 9 lines · Marc Zyngier

Quoted 3 lines · Zenghui Yu

Do we really need to grab the vpe_lock for those which are belong to the same irqchip with its_vpe_set_affinity()? The IRQ core code should already ensure the mutual exclusion among them, wrong?

I've been trying to think about that, but jet-lag keeps getting in the way. I empirically think that you are right, but I need to go and check the various code paths to be sure. Hopefully I'll have a bit more brain space next week.

So I slept on it and came back to my senses. The only case we actually need to deal with is when an affinity change impacts *another* interrupt.

There is only two instances of this issue:

- vLPIs have their *physical* affinity impacted by the affinity of the vPE. Their virtual affinity is of course unchanged, but the physical one becomes important with direct invalidation. Taking a per-VPE lock in such context should address the issue.

- vSGIs have the exact same issue, plus the matter of requiring some *extra* one when reading the pending state, which requires a RMW on two different registers. This requires an extra per-RD lock.

My original patch was stupidly complex, and the irq_desc lock is perfectly enough to deal with anything that only affects the interrupt state itself.

GICv4 + direct invalidation for vLPIs breaks this by bypassing the serialization initially provided by the ITS, as the RD is completely out of band. The per-vPE lock brings back this serialization.

I've updated the branch, which seems to run OK on D05. I still need to run the usual tests on the FVP model though.

Quoted 23 lines · Marc Zyngier

Quoted 9 lines · Marc Zyngier

Quoted 3 lines · Zenghui Yu

Do we really need to grab the vpe_lock for those which are belong to the same irqchip with its_vpe_set_affinity()? The IRQ core code should already ensure the mutual exclusion among them, wrong?

I've been trying to think about that, but jet-lag keeps getting in the way. I empirically think that you are right, but I need to go and check the various code paths to be sure. Hopefully I'll have a bit more brain space next week.

So I slept on it and came back to my senses. The only case we actually need to deal with is when an affinity change impacts *another* interrupt.

There is only two instances of this issue:

- vLPIs have their *physical* affinity impacted by the affinity of the vPE. Their virtual affinity is of course unchanged, but the
physical
  one becomes important with direct invalidation. Taking a per-VPE
lock
  in such context should address the issue.

Quoted 3 lines

- vSGIs have the exact same issue, plus the matter of requiring some *extra* one when reading the pending state, which requires a RMW on two different registers. This requires an extra per-RD lock.

Agreed with both!

Quoted 11 lines

My original patch was stupidly complex, and the irq_desc lock is perfectly enough to deal with anything that only affects the interrupt state itself.

GICv4 + direct invalidation for vLPIs breaks this by bypassing the serialization initially provided by the ITS, as the RD is completely out of band. The per-vPE lock brings back this serialization.

I've updated the branch, which seems to run OK on D05. I still need to run the usual tests on the FVP model though.

I have pulled the latest branch and it looks good to me, except for one remaining concern:

GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently by multiple direct invalidation, should we also use the per-RD lock to ensure mutual exclusion? It looks not so harmful though, as this will only increase one's polling time against the Busy bit (in my view).

But I point it out again for confirmation.

Hi Zenghui,

Quoted 50 lines · Zenghui Yu

Quoted 28 lines · Marc Zyngier

Quoted 7 lines · Marc Zyngier

Quoted 4 lines · Zenghui Yu

Do we really need to grab the vpe_lock for those which are belong to the same irqchip with its_vpe_set_affinity()? The IRQ core code should
already ensure the mutual exclusion among them, wrong?

Quoted 6 lines

I've been trying to think about that, but jet-lag keeps getting in the way.
I empirically think that you are right, but I need to go and check the various
code paths to be sure. Hopefully I'll have a bit more brain space next week.

So I slept on it and came back to my senses. The only case we actually need
to deal with is when an affinity change impacts *another* interrupt.

There is only two instances of this issue:

- vLPIs have their *physical* affinity impacted by the affinity of the vPE. Their virtual affinity is of course unchanged, but the
physical
one becomes important with direct invalidation. Taking a per-VPE
lock
in such context should address the issue.

Quoted 3 lines

- vSGIs have the exact same issue, plus the matter of requiring some *extra* one when reading the pending state, which requires a RMW on two different registers. This requires an extra per-RD lock.

Agreed with both!

Quoted 11 lines

My original patch was stupidly complex, and the irq_desc lock is perfectly enough to deal with anything that only affects the interrupt state itself.

GICv4 + direct invalidation for vLPIs breaks this by bypassing the serialization initially provided by the ITS, as the RD is completely out of band. The per-vPE lock brings back this serialization.

I've updated the branch, which seems to run OK on D05. I still need to run the usual tests on the FVP model though.

I have pulled the latest branch and it looks good to me, except for one remaining concern:

Quoted 7 lines

GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently by multiple direct invalidation, should we also use the per-RD lock to ensure mutual exclusion? It looks not so harmful though, as this will only increase one's polling time against the Busy bit (in my view).

But I point it out again for confirmation.

I was about to say that it doesn't really matter because it is only a performance optimisation (and we're noty quite there yet), until I spotted
this great nugget in the spec:

<quote>
Writing GICR_INVLPIR or GICR_INVALLR when GICR_SYNCR.Busy==1 is CONSTRAINED
UNPREDICTABLE:
- The write is IGNORED .
- The invalidate specified by the write is performed. </quote>

So we really need some form of mutual exclusion on a per-RD basis to ensure
that no two invalidations occur at the same time, ensuring that Busy clears
between the two.

Thanks for the heads up,

         M.

--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

As for VLPIs, there is a number of configuration bits that cannot

be directly communicated through the normal irqchip API, and we have to use our good old friend set_vcpu_affinity.

This is used to configure group and priority for a given vSGI.

Signed-off-by Marc Zyngier <[email protected]>

---
  drivers/irqchip/irq-gic-v3-its.c   | 18 ++++++++++++++++++
  include/linux/irqchip/arm-gic-v4.h |  5 +++++
  2 files changed, 23 insertions(+)

drivers/irqchip/irq-gic-v3-its.c +18 -0

index a9753435c4ff..a2e824eae43f 100644
@@ -3969,6 +3969,23 @@ static int its_sgi_get_irqchip_state(struct irq_data *d,
  	return 0;
  }
 
+static int its_sgi_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
+{
+	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+	struct its_cmd_info *info = vcpu_info;
+
+	switch (info->cmd_type) {
+	case PROP_UPDATE_SGI:
+		vpe->sgi_config[d->hwirq].priority = info->priority;
+		vpe->sgi_config[d->hwirq].group = info->group;
+		its_configure_sgi(d, false);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
  static struct irq_chip its_sgi_irq_chip = {
  	.name			= "GICv4.1-sgi",
  	.irq_mask		= its_sgi_mask_irq,

@@ -3976,6 +3993,7 @@ static struct irq_chip its_sgi_irq_chip = {
  	.irq_set_affinity	= its_sgi_set_affinity,
  	.irq_set_irqchip_state	= its_sgi_set_irqchip_state,
  	.irq_get_irqchip_state	= its_sgi_get_irqchip_state,
+	.irq_set_vcpu_affinity	= its_sgi_set_vcpu_affinity,
  };
 
  static int its_sgi_irq_domain_alloc(struct irq_domain *domain,

include/linux/irqchip/arm-gic-v4.h +0 -0

index 30b4855bf766..a1a9d40266f5 100644

include/linux/irqchip/arm-gic-v4.h +4 -0

  };
 
  struct its_cmd_info {

@@ -110,6 +111,10 @@ struct its_cmd_info {
  			bool		g0en;
  			bool		g1en;
  		};
+		struct {
+			u8		priority;
+			bool		group;
+		};
  	};
  };
 

Hi Marc,

Quoted 1 line · Marc Zyngier

As for VLPIs, there is a number of configuration bits that cannot

As for vSGIs,

Quoted 7 lines

be directly communicated through the normal irqchip API, and we have to use our good old friend set_vcpu_affinity.

This is used to configure group and priority for a given vSGI.

Signed-off-by Marc Zyngier <[email protected]>

Reviewed-by Zenghui Yu <[email protected]>

Quoted 44 lines

---
  drivers/irqchip/irq-gic-v3-its.c   | 18 ++++++++++++++++++
  include/linux/irqchip/arm-gic-v4.h |  5 +++++
  2 files changed, 23 insertions(+)

drivers/irqchip/irq-gic-v3-its.c +17 -0

index a9753435c4ff..a2e824eae43f 100644
@@ -3969,6 +3969,23 @@ static int its_sgi_get_irqchip_state(struct 
 irq_data *d,
  	return 0;
  }
  +static int its_sgi_set_vcpu_affinity(struct irq_data *d, void 
 *vcpu_info)
+{
+	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+	struct its_cmd_info *info = vcpu_info;
+
+	switch (info->cmd_type) {
+	case PROP_UPDATE_SGI:
+		vpe->sgi_config[d->hwirq].priority = info->priority;
+		vpe->sgi_config[d->hwirq].group = info->group;
+		its_configure_sgi(d, false);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
  static struct irq_chip its_sgi_irq_chip = {
  	.name			= "GICv4.1-sgi",
  	.irq_mask		= its_sgi_mask_irq,

@@ -3976,6 +3993,7 @@ static struct irq_chip its_sgi_irq_chip = {
  	.irq_set_affinity	= its_sgi_set_affinity,
  	.irq_set_irqchip_state	= its_sgi_set_irqchip_state,
  	.irq_get_irqchip_state	= its_sgi_get_irqchip_state,
+	.irq_set_vcpu_affinity	= its_sgi_set_vcpu_affinity,
  };
    static int its_sgi_irq_domain_alloc(struct irq_domain *domain,

include/linux/irqchip/arm-gic-v4.h +0 -1

index 30b4855bf766..a1a9d40266f5 100644
--- a/include/linux/irqchip/arm-gic-v4.h

include/linux/irqchip/arm-gic-v4.h enum its_vcpu_info_cmd_type { · line 101

Quoted 6 lines

+++ b/include/linux/irqchip/arm-gic-v4.h

include/linux/irqchip/arm-gic-v4.h +1 -0

@@ -98,6 +98,7 @@ enum its_vcpu_info_cmd_type {
  	SCHEDULE_VPE,
  	DESCHEDULE_VPE,
  	INVALL_VPE,
+	PROP_UPDATE_SGI,

Maybe better to use 'PROP_UPDATE_VSGI'?

Most of the GICv3 emulation code that deals with SGIs now has to be aware of the v4.1 capabilities in order to benefit from it.

Add such support, keyed on the interrupt having the hw flag set and being a SGI.

Signed-off-by Marc Zyngier <[email protected]>

---
  virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
  virt/kvm/arm/vgic/vgic-mmio.c    | 88 ++++++++++++++++++++++++++++++--
  2 files changed, 96 insertions(+), 7 deletions(-)

virt/kvm/arm/vgic/vgic-mmio-v3.c +13 -2

index ebc218840fc2..de89da76a379 100644
@@ -6,6 +6,7 @@
  #include <linux/irqchip/arm-gic-v3.h>
  #include <linux/kvm.h>
  #include <linux/kvm_host.h>
+#include <linux/interrupt.h>
  #include <kvm/iodev.h>
  #include <kvm/arm_vgic.h>
 

@@ -942,8 +943,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, bool allow_group1)
  		 * generate interrupts of either group.
  		 */
  		if (!irq->group || allow_group1) {
-			irq->pending_latch = true;
-			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+			if (!irq->hw) {
+				irq->pending_latch = true;
+				vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+			} else {
+				/* HW SGI? Ask the GIC to inject it */
+				int err;
+				err = irq_set_irqchip_state(irq->host_irq,
+							    IRQCHIP_STATE_PENDING,
+							    true);
+				WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+				raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			}
  		} else {
  			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
  		}

virt/kvm/arm/vgic/vgic-mmio.c +52 -4

index d656ebd5f9d4..0a1fb61e5b89 100644
@@ -5,6 +5,8 @@
 
  #include <linux/bitops.h>
  #include <linux/bsearch.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
  #include <linux/kvm.h>
  #include <linux/kvm_host.h>
  #include <kvm/iodev.h>

@@ -59,6 +61,11 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
  	return value;
  }
 
+static void vgic_update_vsgi(struct vgic_irq *irq)
+{
+	WARN_ON(its_prop_update_vsgi(irq->host_irq, irq->priority, irq->group));
+}
+
  void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
  			   unsigned int len, unsigned long val)
  {

@@ -71,7 +78,12 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
 
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
  		irq->group = !!(val & BIT(i));
-		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			vgic_update_vsgi(irq);
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+		} else {
+			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		}
 
  		vgic_put_irq(vcpu->kvm, irq);
  	}

@@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		if (vgic_irq_is_mapped_level(irq)) {
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			if (!irq->enabled) {
+				struct irq_data *data;
+
+				irq->enabled = true;
+				data = &irq_to_desc(irq->host_irq)->irq_data;
+				while (irqd_irq_disabled(data))
+					enable_irq(irq->host_irq);
+			}
+
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			vgic_put_irq(vcpu->kvm, irq);
+
+			continue;
+		} else if (vgic_irq_is_mapped_level(irq)) {
  			bool was_high = irq->line_level;
 
  			/*

@@ -148,6 +174,8 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid) && irq->enabled)
+			disable_irq_nosync(irq->host_irq);
 
  		irq->enabled = false;
 

@@ -167,10 +195,22 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
  	for (i = 0; i < len * 8; i++) {
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  		unsigned long flags;
+		bool val;
 
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		if (irq_is_pending(irq))
-			value |= (1U << i);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			int err;
+
+			val = false;
+			err = irq_get_irqchip_state(irq->host_irq,
+						    IRQCHIP_STATE_PENDING,
+						    &val);
+			WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+		} else {
+			val = irq_is_pending(irq);
+		}
+
+		value |= ((u32)val << i);
  		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 
  		vgic_put_irq(vcpu->kvm, irq);

@@ -227,6 +267,21 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
  		}
 
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
+
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			/* HW SGI? Ask the GIC to inject it */
+			int err;
+			err = irq_set_irqchip_state(irq->host_irq,
+						    IRQCHIP_STATE_PENDING,
+						    true);
+			WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);

virt/kvm/arm/vgic/vgic-mmio.c +22 -1

+			int err;
+			err = irq_set_irqchip_state(irq->host_irq,
+						    IRQCHIP_STATE_PENDING,
+						    false);
+			WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			vgic_put_irq(vcpu->kvm, irq);
+
+			continue;
+		}
+
  		if (irq->hw)
  			vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
  		else

@@ -330,8 +399,15 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 
  	raw_spin_lock_irqsave(&irq->irq_lock, flags);
 
-	if (irq->hw) {
+	if (irq->hw && !vgic_irq_is_sgi(irq->intid)) {
  		vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
+	} else if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+		/*
+		 * GICv4.1 VSGI feature doesn't track an active state,
+		 * so let's not kid ourselves, there is nothing we can
+		 * do here.
+		 */
+		irq->active = false;
  	} else {
  		u32 model = vcpu->kvm->arch.vgic.vgic_model;
  		u8 active_source;

@@ -505,6 +581,8 @@ void vgic_mmio_write_priority(struct kvm_vcpu *vcpu,
  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
  		/* Narrow the priority range to what we actually support */
  		irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid))
+			vgic_update_vsgi(irq);
  		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 
  		vgic_put_irq(vcpu->kvm, irq);

Hi Marc,

Quoted 149 lines · Marc Zyngier

Most of the GICv3 emulation code that deals with SGIs now has to be aware of the v4.1 capabilities in order to benefit from it.

Add such support, keyed on the interrupt having the hw flag set and being a SGI.

Signed-off-by Marc Zyngier <[email protected]>

---
  virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
  virt/kvm/arm/vgic/vgic-mmio.c    | 88 
++++++++++++++++++++++++++++++--
  2 files changed, 96 insertions(+), 7 deletions(-)

virt/kvm/arm/vgic/vgic-mmio-v3.c +1 -0

index ebc218840fc2..de89da76a379 100644
@@ -6,6 +6,7 @@
  #include <linux/irqchip/arm-gic-v3.h>
  #include <linux/kvm.h>
  #include <linux/kvm_host.h>
+#include <linux/interrupt.h>
  #include <kvm/iodev.h>
  #include <kvm/arm_vgic.h>
  @@ -942,8 +943,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu 

*vcpu, u64 reg, bool allow_group1)
* generate interrupts of either group. */
if (!irq->group || allow_group1) {

virt/kvm/arm/vgic/vgic-mmio-v3.c +12 -2

-			irq->pending_latch = true;
-			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+			if (!irq->hw) {
+				irq->pending_latch = true;
+				vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+			} else {
+				/* HW SGI? Ask the GIC to inject it */
+				int err;
+				err = irq_set_irqchip_state(irq->host_irq,
+							    IRQCHIP_STATE_PENDING,
+							    true);
+				WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+				raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			}
  		} else {
  			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
  		}

virt/kvm/arm/vgic/vgic-mmio.c +29 -2

index d656ebd5f9d4..0a1fb61e5b89 100644
@@ -5,6 +5,8 @@
    #include <linux/bitops.h>
  #include <linux/bsearch.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
  #include <linux/kvm.h>
  #include <linux/kvm_host.h>
  #include <kvm/iodev.h>

@@ -59,6 +61,11 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu 
 *vcpu,
  	return value;
  }
  +static void vgic_update_vsgi(struct vgic_irq *irq)
+{
+	WARN_ON(its_prop_update_vsgi(irq->host_irq, irq->priority, 
 irq->group));
+}
+
  void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
  			   unsigned int len, unsigned long val)
  {

@@ -71,7 +78,12 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, 
 gpa_t addr,
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
  		irq->group = !!(val & BIT(i));
-		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			vgic_update_vsgi(irq);
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+		} else {
+			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		}
    		vgic_put_irq(vcpu->kvm, irq);
  	}

@@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu 
 *vcpu,
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		if (vgic_irq_is_mapped_level(irq)) {
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			if (!irq->enabled) {
+				struct irq_data *data;
+
+				irq->enabled = true;
+				data = &irq_to_desc(irq->host_irq)->irq_data;
+				while (irqd_irq_disabled(data))
+					enable_irq(irq->host_irq);
+			}
+
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			vgic_put_irq(vcpu->kvm, irq);
+
+			continue;
+		} else if (vgic_irq_is_mapped_level(irq)) {
  			bool was_high = irq->line_level;
    			/*

@@ -148,6 +174,8 @@ void vgic_mmio_write_cenable(struct kvm_vcpu 
 *vcpu,
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid) && irq->enabled)
+			disable_irq_nosync(irq->host_irq);
    		irq->enabled = false;
  @@ -167,10 +195,22 @@ unsigned long vgic_mmio_read_pending(struct 
 kvm_vcpu *vcpu,

  	for (i = 0; i < len * 8; i++) {
  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  		unsigned long flags;

virt/kvm/arm/vgic/vgic-mmio.c +22 -2

+		bool val;
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		if (irq_is_pending(irq))
-			value |= (1U << i);
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			int err;
+
+			val = false;
+			err = irq_get_irqchip_state(irq->host_irq,
+						    IRQCHIP_STATE_PENDING,
+						    &val);
+			WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+		} else {
+			val = irq_is_pending(irq);
+		}
+
+		value |= ((u32)val << i);
  		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
    		vgic_put_irq(vcpu->kvm, irq);

@@ -227,6 +267,21 @@ void vgic_mmio_write_spending(struct kvm_vcpu 
 *vcpu,
  		}
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
+
+		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			/* HW SGI? Ask the GIC to inject it */
+			int err;
+			err = irq_set_irqchip_state(irq->host_irq,
+						    IRQCHIP_STATE_PENDING,
+						    true);
+			WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);

virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_spending(struct kvm_vcpu · line 287

Quoted 10 lines

virt/kvm/arm/vgic/vgic-mmio.c +7 -0

+
+			raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+			vgic_put_irq(vcpu->kvm, irq);
+
+			continue;
+		}
+
  		if (irq->hw)
  			vgic_hw_irq_spending(vcpu, irq, is_uaccess);
  		else

Should we consider taking the GICv4.1 support into uaccess_{read/write} callbacks for GICR_ISPENDR0 so that userspace can properly save/restore the pending state of GICv4.1 vSGIs?

I *think* we can do it because on restoration, GICD_CTLR(.nASSGIreq) is restored before GICR_ISPENDR0. So we know whether we're restoring pending for vSGIs, and we can restore it to the HW level if v4.1 is supported by GIC. Otherwise restore it by the normal way.

And saving is easy with the get_irqchip_state callback, right?

virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cpending(struct kvm_vcpu · line 339

Quoted 6 lines

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

@@ -281,6 +336,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu 
 *vcpu,
    		raw_spin_lock_irqsave(&irq->irq_lock, flags);
  +		if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+			/* HW SGI? Ask the GIC to inject it */

"Ask the GIC to clear its pending state" :-)

In order to let a guest buy in the new, active-less SGIs, we need to be able to switch between the two modes.

Handle this by stopping all guest activity, transfer the state from one mode to the other, and resume the guest.

Signed-off-by Marc Zyngier <[email protected]>

Hi Marc,

Quoted 7 lines · Marc Zyngier

In order to let a guest buy in the new, active-less SGIs, we need to be able to switch between the two modes.

Handle this by stopping all guest activity, transfer the state from one mode to the other, and resume the guest.

Signed-off-by Marc Zyngier <[email protected]>

[...]

virt/kvm/arm/vgic/vgic-v3.c int vgic_v3_map_resources(struct kvm *kvm) · line 545

Quoted 11 lines

virt/kvm/arm/vgic/vgic-v3.c +1 -0

index 1bc09b523486..2c9fc13e2c59 100644
@@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
  		goto out;
  	}
  +	if (kvm_vgic_global_state.has_gicv4_1)
+		vgic_v4_configure_vsgis(kvm);
  	dist->ready = true;
    out:

Is there any reason to invoke vgic_v4_configure_vsgis() here? This is called on the first VCPU run, through kvm_vgic_map_resources(). Shouldn't the vSGI configuration only driven by a GICD_CTLR.nASSGIreq writing (from guest, or from userspace maybe)?

Hi Zenghui,

Quoted 13 lines · Zenghui Yu

Hi Marc,

Quoted 7 lines · Marc Zyngier

In order to let a guest buy in the new, active-less SGIs, we need to be able to switch between the two modes.

Handle this by stopping all guest activity, transfer the state from one mode to the other, and resume the guest.

Signed-off-by Marc Zyngier <[email protected]>

[...]

Quoted 16 lines

Quoted 11 lines

virt/kvm/arm/vgic/vgic-v3.c +1 -0

index 1bc09b523486..2c9fc13e2c59 100644
@@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
           goto out;
       }
   +    if (kvm_vgic_global_state.has_gicv4_1)
+        vgic_v4_configure_vsgis(kvm);
       dist->ready = true;

    out:

Is there any reason to invoke vgic_v4_configure_vsgis() here? This is called on the first VCPU run, through kvm_vgic_map_resources(). Shouldn't the vSGI configuration only driven by a GICD_CTLR.nASSGIreq writing (from guest, or from userspace maybe)?

What I'm trying to catch here is the guest that has been restored with nASSGIreq set. At the moment, we don't do anything on the userspace side, because the vmm could decide to write that particular bit multiple times, and switching between the two modes is expensive (not to mention that all the vcpus may not have been created yet).

Moving it to the first run makes all these pitfalls go away (we have the final nASSSGIreq value, and all the vcpus are accounted for).

Does this make sense to you?