[PATCH 0/4] KVM: arm/arm64: vgic-v3: Group0 SGI support

[PATCH 0/4] KVM: arm/arm64: vgic-v3: Group0 SGI support

[Marc Zyngier]

Although we now have Group0 support, we still miss support for Group0
SGIs (which amounts to handling ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
traps), and this small series adds such support.

I appreciate this is *very* late for 4.19, I'd like to take it in as
they complement Christoffer's Group0 support, and It'd be annoying to
have something incomplete in 4.19.

Please shout if you spot something that doesn't look quite right.

Thanks,

	M.

Marc Zyngier (4):
  KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding
  KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs
  KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
    accesses
  KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

 arch/arm/kvm/coproc.c            | 25 ++++++++++++++++++-
 arch/arm64/include/asm/sysreg.h  |  2 ++
 arch/arm64/kvm/sys_regs.c        | 43 +++++++++++++++++++++++++++++---
 include/kvm/arm_vgic.h           |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++---
 5 files changed, 79 insertions(+), 9 deletions(-)

-- 
2.18.0

[PATCH 0/4] KVM: arm/arm64: vgic-v3: Group0 SGI support

[Marc Zyngier]

Although we now have Group0 support, we still miss support for Group0
SGIs (which amounts to handling ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
traps), and this small series adds such support.

I appreciate this is *very* late for 4.19, I'd like to take it in as
they complement Christoffer's Group0 support, and It'd be annoying to
have something incomplete in 4.19.

Please shout if you spot something that doesn't look quite right.

Thanks,

	M.

Marc Zyngier (4):
  KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding
  KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs
  KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
    accesses
  KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

 arch/arm/kvm/coproc.c            | 25 ++++++++++++++++++-
 arch/arm64/include/asm/sysreg.h  |  2 ++
 arch/arm64/kvm/sys_regs.c        | 43 +++++++++++++++++++++++++++++---
 include/kvm/arm_vgic.h           |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++---
 5 files changed, 79 insertions(+), 9 deletions(-)

-- 
2.18.0

[PATCH 1/4] KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding

[Marc Zyngier]

ICC_SGI1R is a 64bit system register, even on AArch32. It is thus
pointless to have such an encoding in the 32bit cp15 array. Let's
drop it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 774d72155904..e04aacb2a24c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1622,8 +1622,6 @@ static const struct sys_reg_desc cp14_64_regs[] = {
  * register).
  */
 static const struct sys_reg_desc cp15_regs[] = {
-	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
-
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, c1_SCTLR },
 	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
 	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, c2_TTBR1 },
-- 
2.18.0

[PATCH 1/4] KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding

[Marc Zyngier]

ICC_SGI1R is a 64bit system register, even on AArch32. It is thus
pointless to have such an encoding in the 32bit cp15 array. Let's
drop it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 774d72155904..e04aacb2a24c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1622,8 +1622,6 @@ static const struct sys_reg_desc cp14_64_regs[] = {
  * register).
  */
 static const struct sys_reg_desc cp15_regs[] = {
-	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
-
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, c1_SCTLR },
 	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
 	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, c2_TTBR1 },
-- 
2.18.0

[PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Marc Zyngier]

Although vgic-v3 now supports Group0 interrupts, it still doesn't
deal with Group0 SGIs. As usually with the GIC, nothing is simple:

- ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
  with KVM (as per 8.1.10, Non-secure EL1 access)

- ICC_SGI0R can only generate Group0 SGIs

- ICC_ASGI1R sees its scope refocussed to generate only Group0
  SGIs (as per the note at the bottom of Table 8-14)

One way to look at this is that an SGI can be generated if the
group implied by the CPU interface is lower or equal to the
group specified by the interrupt.

We only support Group1 SGIs so far, so no material change.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/coproc.c            |  2 +-
 arch/arm64/kvm/sys_regs.c        |  2 +-
 include/kvm/arm_vgic.h           |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 3a02e76699a6..ec517992c12d 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
 	reg |= *vcpu_reg(vcpu, p->Rt1) ;
 
-	vgic_v3_dispatch_sgi(vcpu, reg);
+	vgic_v3_dispatch_sgi(vcpu, reg, 1);
 
 	return true;
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e04aacb2a24c..a09139b97e81 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	vgic_v3_dispatch_sgi(vcpu, p->regval);
+	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
 
 	return true;
 }
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c134790be32c..06a25b11efa7 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
 void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
 
-void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
+void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
 
 /**
  * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 88e78b582139..11d321f7ad71 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
  * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
  * @vcpu: The VCPU requesting a SGI
  * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
+ * @group: Interrupt group requested by the sender
  *
  * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
  * This will trap in sys_regs.c and call this function.
@@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
  * check for matching ones. If this bit is set, we signal all, but not the
  * calling VCPU.
  */
-void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
+void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
 {
 	struct kvm *kvm = vcpu->kvm;
 	struct kvm_vcpu *c_vcpu;
@@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
 
 		spin_lock_irqsave(&irq->irq_lock, flags);
-		irq->pending_latch = true;
 
-		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		/*
+		 * A Group0 access can only generate a Group0 SGI,
+		 * while a Group1 access can generate either group.
+		 */
+		if (irq->group <= group) {
+			irq->pending_latch = true;
+			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		} else {
+			spin_unlock_irqrestore(&irq->irq_lock, flags);
+		}
+
 		vgic_put_irq(vcpu->kvm, irq);
 	}
 }
-- 
2.18.0

[PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Marc Zyngier]

Although vgic-v3 now supports Group0 interrupts, it still doesn't
deal with Group0 SGIs. As usually with the GIC, nothing is simple:

- ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
  with KVM (as per 8.1.10, Non-secure EL1 access)

- ICC_SGI0R can only generate Group0 SGIs

- ICC_ASGI1R sees its scope refocussed to generate only Group0
  SGIs (as per the note at the bottom of Table 8-14)

One way to look at this is that an SGI can be generated if the
group implied by the CPU interface is lower or equal to the
group specified by the interrupt.

We only support Group1 SGIs so far, so no material change.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/coproc.c            |  2 +-
 arch/arm64/kvm/sys_regs.c        |  2 +-
 include/kvm/arm_vgic.h           |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 3a02e76699a6..ec517992c12d 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
 	reg |= *vcpu_reg(vcpu, p->Rt1) ;
 
-	vgic_v3_dispatch_sgi(vcpu, reg);
+	vgic_v3_dispatch_sgi(vcpu, reg, 1);
 
 	return true;
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e04aacb2a24c..a09139b97e81 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	vgic_v3_dispatch_sgi(vcpu, p->regval);
+	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
 
 	return true;
 }
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c134790be32c..06a25b11efa7 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
 void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
 
-void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
+void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
 
 /**
  * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 88e78b582139..11d321f7ad71 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
  * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
  * @vcpu: The VCPU requesting a SGI
  * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
+ * @group: Interrupt group requested by the sender
  *
  * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
  * This will trap in sys_regs.c and call this function.
@@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
  * check for matching ones. If this bit is set, we signal all, but not the
  * calling VCPU.
  */
-void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
+void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
 {
 	struct kvm *kvm = vcpu->kvm;
 	struct kvm_vcpu *c_vcpu;
@@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
 
 		spin_lock_irqsave(&irq->irq_lock, flags);
-		irq->pending_latch = true;
 
-		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		/*
+		 * A Group0 access can only generate a Group0 SGI,
+		 * while a Group1 access can generate either group.
+		 */
+		if (irq->group <= group) {
+			irq->pending_latch = true;
+			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+		} else {
+			spin_unlock_irqrestore(&irq->irq_lock, flags);
+		}
+
 		vgic_put_irq(vcpu->kvm, irq);
 	}
 }
-- 
2.18.0

[PATCH 3/4] KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1 accesses

[Marc Zyngier]

In order to generate Group0 SGIs, let's add some decoding logic to
access_gic_sgi(), and pass the generating group accordingly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/sysreg.h |  2 ++
 arch/arm64/kvm/sys_regs.c       | 41 +++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 98af0b37fb31..b0d2a52a71a3 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -314,6 +314,8 @@
 #define SYS_ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
 #define SYS_ICC_RPR_EL1			sys_reg(3, 0, 12, 11, 3)
 #define SYS_ICC_SGI1R_EL1		sys_reg(3, 0, 12, 11, 5)
+#define SYS_ICC_ASGI1R_EL1		sys_reg(3, 0, 12, 11, 6)
+#define SYS_ICC_SGI0R_EL1		sys_reg(3, 0, 12, 11, 7)
 #define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
 #define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
 #define SYS_ICC_HPPIR1_EL1		sys_reg(3, 0, 12, 12, 2)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a09139b97e81..b09eac49e1ac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -252,10 +252,43 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
+	int group;
+
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
+	/*
+	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R_EL1 access generates
+	 * Group0 SGIs only, while ICC_SGI1R_EL1 can generate either group,
+	 * depending on the SGI configuration. ICC_ASGI1R_EL1 is effectively
+	 * equivalent to ICC_SGI0R_EL1, as there is no "alternative" secure
+	 * group.
+	 */
+	if (p->is_aarch32) {
+		switch (p->Op1) {
+		default:		/* Keep GCC quiet */
+		case 0:			/* ICC_SGI1R */
+			group = 1;
+			break;
+		case 1:			/* ICC_ASGI1R */
+		case 2:			/* ICC_SGI0R */
+			group = 0;
+			break;
+		}
+	} else {
+		switch (p->Op2) {
+		default:		/* Keep GCC quiet */
+		case 5:			/* ICC_SGI1R_EL1 */
+			group = 1;
+			break;
+		case 6:			/* ICC_ASGI1R_EL1 */
+		case 7:			/* ICC_SGI0R_EL1 */
+			group = 0;
+			break;
+		}
+	}
+
+	vgic_v3_dispatch_sgi(vcpu, p->regval, group);
 
 	return true;
 }
@@ -1312,6 +1345,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_ICC_DIR_EL1), read_from_write_only },
 	{ SYS_DESC(SYS_ICC_RPR_EL1), write_to_read_only },
 	{ SYS_DESC(SYS_ICC_SGI1R_EL1), access_gic_sgi },
+	{ SYS_DESC(SYS_ICC_ASGI1R_EL1), access_gic_sgi },
+	{ SYS_DESC(SYS_ICC_SGI0R_EL1), access_gic_sgi },
 	{ SYS_DESC(SYS_ICC_IAR1_EL1), write_to_read_only },
 	{ SYS_DESC(SYS_ICC_EOIR1_EL1), read_from_write_only },
 	{ SYS_DESC(SYS_ICC_HPPIR1_EL1), write_to_read_only },
@@ -1744,8 +1779,10 @@ static const struct sys_reg_desc cp15_regs[] = {
 static const struct sys_reg_desc cp15_64_regs[] = {
 	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
 	{ Op1( 0), CRn( 0), CRm( 9), Op2( 0), access_pmu_evcntr },
-	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
+	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
 	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
+	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
+	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
 	{ Op1( 2), CRn( 0), CRm(14), Op2( 0), access_cntp_cval },
 };
 
-- 
2.18.0

[PATCH 3/4] KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1 accesses

[Marc Zyngier]

In order to generate Group0 SGIs, let's add some decoding logic to
access_gic_sgi(), and pass the generating group accordingly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/sysreg.h |  2 ++
 arch/arm64/kvm/sys_regs.c       | 41 +++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 98af0b37fb31..b0d2a52a71a3 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -314,6 +314,8 @@
 #define SYS_ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
 #define SYS_ICC_RPR_EL1			sys_reg(3, 0, 12, 11, 3)
 #define SYS_ICC_SGI1R_EL1		sys_reg(3, 0, 12, 11, 5)
+#define SYS_ICC_ASGI1R_EL1		sys_reg(3, 0, 12, 11, 6)
+#define SYS_ICC_SGI0R_EL1		sys_reg(3, 0, 12, 11, 7)
 #define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
 #define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
 #define SYS_ICC_HPPIR1_EL1		sys_reg(3, 0, 12, 12, 2)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a09139b97e81..b09eac49e1ac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -252,10 +252,43 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
+	int group;
+
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
+	/*
+	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R_EL1 access generates
+	 * Group0 SGIs only, while ICC_SGI1R_EL1 can generate either group,
+	 * depending on the SGI configuration. ICC_ASGI1R_EL1 is effectively
+	 * equivalent to ICC_SGI0R_EL1, as there is no "alternative" secure
+	 * group.
+	 */
+	if (p->is_aarch32) {
+		switch (p->Op1) {
+		default:		/* Keep GCC quiet */
+		case 0:			/* ICC_SGI1R */
+			group = 1;
+			break;
+		case 1:			/* ICC_ASGI1R */
+		case 2:			/* ICC_SGI0R */
+			group = 0;
+			break;
+		}
+	} else {
+		switch (p->Op2) {
+		default:		/* Keep GCC quiet */
+		case 5:			/* ICC_SGI1R_EL1 */
+			group = 1;
+			break;
+		case 6:			/* ICC_ASGI1R_EL1 */
+		case 7:			/* ICC_SGI0R_EL1 */
+			group = 0;
+			break;
+		}
+	}
+
+	vgic_v3_dispatch_sgi(vcpu, p->regval, group);
 
 	return true;
 }
@@ -1312,6 +1345,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_ICC_DIR_EL1), read_from_write_only },
 	{ SYS_DESC(SYS_ICC_RPR_EL1), write_to_read_only },
 	{ SYS_DESC(SYS_ICC_SGI1R_EL1), access_gic_sgi },
+	{ SYS_DESC(SYS_ICC_ASGI1R_EL1), access_gic_sgi },
+	{ SYS_DESC(SYS_ICC_SGI0R_EL1), access_gic_sgi },
 	{ SYS_DESC(SYS_ICC_IAR1_EL1), write_to_read_only },
 	{ SYS_DESC(SYS_ICC_EOIR1_EL1), read_from_write_only },
 	{ SYS_DESC(SYS_ICC_HPPIR1_EL1), write_to_read_only },
@@ -1744,8 +1779,10 @@ static const struct sys_reg_desc cp15_regs[] = {
 static const struct sys_reg_desc cp15_64_regs[] = {
 	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
 	{ Op1( 0), CRn( 0), CRm( 9), Op2( 0), access_pmu_evcntr },
-	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
+	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
 	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
+	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
+	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
 	{ Op1( 2), CRn( 0), CRm(14), Op2( 0), access_cntp_cval },
 };
 
-- 
2.18.0

[PATCH 4/4] KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

[Marc Zyngier]

In order to generate Group0 SGIs, let's add some decoding logic to
access_gic_sgi(), and pass the generating group accordingly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/coproc.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index ec517992c12d..0dc0ca061d78 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -246,6 +246,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 			   const struct coproc_reg *r)
 {
 	u64 reg;
+	int group;
 
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p);
@@ -253,7 +254,25 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
 	reg |= *vcpu_reg(vcpu, p->Rt1) ;
 
-	vgic_v3_dispatch_sgi(vcpu, reg, 1);
+	/*
+	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R access generates
+	 * Group0 SGIs only, while ICC_SGI1R can generate either group,
+	 * depending on the SGI configuration. ICC_ASGI1R is effectively
+	 * equivalent to ICC_SGI0R, as there is no "alternative" secure
+	 * group.
+	 */
+	switch (p->Op1) {
+	default:		/* Keep GCC quiet */
+	case 0:			/* ICC_SGI1R */
+		group = 1;
+		break;
+	case 1:			/* ICC_ASGI1R */
+	case 2:			/* ICC_SGI0R */
+		group = 0;
+		break;
+	}
+
+	vgic_v3_dispatch_sgi(vcpu, reg, group);
 
 	return true;
 }
@@ -459,6 +478,10 @@ static const struct coproc_reg cp15_regs[] = {
 
 	/* ICC_SGI1R */
 	{ CRm64(12), Op1( 0), is64, access_gic_sgi},
+	/* ICC_ASGI1R */
+	{ CRm64(12), Op1( 1), is64, access_gic_sgi},
+	/* ICC_SGI0R */
+	{ CRm64(12), Op1( 2), is64, access_gic_sgi},
 
 	/* VBAR: swapped by interrupt.S. */
 	{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
-- 
2.18.0

[PATCH 4/4] KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

[Marc Zyngier]

In order to generate Group0 SGIs, let's add some decoding logic to
access_gic_sgi(), and pass the generating group accordingly.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/coproc.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index ec517992c12d..0dc0ca061d78 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -246,6 +246,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 			   const struct coproc_reg *r)
 {
 	u64 reg;
+	int group;
 
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p);
@@ -253,7 +254,25 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
 	reg |= *vcpu_reg(vcpu, p->Rt1) ;
 
-	vgic_v3_dispatch_sgi(vcpu, reg, 1);
+	/*
+	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R access generates
+	 * Group0 SGIs only, while ICC_SGI1R can generate either group,
+	 * depending on the SGI configuration. ICC_ASGI1R is effectively
+	 * equivalent to ICC_SGI0R, as there is no "alternative" secure
+	 * group.
+	 */
+	switch (p->Op1) {
+	default:		/* Keep GCC quiet */
+	case 0:			/* ICC_SGI1R */
+		group = 1;
+		break;
+	case 1:			/* ICC_ASGI1R */
+	case 2:			/* ICC_SGI0R */
+		group = 0;
+		break;
+	}
+
+	vgic_v3_dispatch_sgi(vcpu, reg, group);
 
 	return true;
 }
@@ -459,6 +478,10 @@ static const struct coproc_reg cp15_regs[] = {
 
 	/* ICC_SGI1R */
 	{ CRm64(12), Op1( 0), is64, access_gic_sgi},
+	/* ICC_ASGI1R */
+	{ CRm64(12), Op1( 1), is64, access_gic_sgi},
+	/* ICC_SGI0R */
+	{ CRm64(12), Op1( 2), is64, access_gic_sgi},
 
 	/* VBAR: swapped by interrupt.S. */
 	{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
-- 
2.18.0

Re: [PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:14:59PM +0100, Marc Zyngier wrote:
> Although vgic-v3 now supports Group0 interrupts, it still doesn't
> deal with Group0 SGIs. As usually with the GIC, nothing is simple:
> 
> - ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
>   with KVM (as per 8.1.10, Non-secure EL1 access)
> 
> - ICC_SGI0R can only generate Group0 SGIs
> 
> - ICC_ASGI1R sees its scope refocussed to generate only Group0
>   SGIs (as per the note at the bottom of Table 8-14)
> 
> One way to look at this is that an SGI can be generated if the
> group implied by the CPU interface is lower or equal to the
> group specified by the interrupt.

This sentence hurts my brain. Another way to look at it is that with
DS=1, only SGI1R allows signaling group1 interrupts.  See below.

> 
> We only support Group1 SGIs so far, so no material change.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c            |  2 +-
>  arch/arm64/kvm/sys_regs.c        |  2 +-
>  include/kvm/arm_vgic.h           |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 3a02e76699a6..ec517992c12d 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>  
> -	vgic_v3_dispatch_sgi(vcpu, reg);
> +	vgic_v3_dispatch_sgi(vcpu, reg, 1);
>  
>  	return true;
>  }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e04aacb2a24c..a09139b97e81 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p, r);
>  
> -	vgic_v3_dispatch_sgi(vcpu, p->regval);
> +	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
>  
>  	return true;
>  }
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c134790be32c..06a25b11efa7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
>  
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
>  
>  /**
>   * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 88e78b582139..11d321f7ad71 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>   * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
>   * @vcpu: The VCPU requesting a SGI
>   * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
> + * @group: Interrupt group requested by the sender
>   *
>   * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
>   * This will trap in sys_regs.c and call this function.
> @@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>   * check for matching ones. If this bit is set, we signal all, but not the
>   * calling VCPU.
>   */
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	struct kvm_vcpu *c_vcpu;
> @@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>  
>  		spin_lock_irqsave(&irq->irq_lock, flags);
> -		irq->pending_latch = true;
>  
> -		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +		/*
> +		 * A Group0 access can only generate a Group0 SGI,
> +		 * while a Group1 access can generate either group.
> +		 */

nit: "Is a Group 0 access" a well-defined term?

I think it may be clearer if the parameter was: bool allow_group1

and the condition was:

if (irq->group && allow_group1) {
	...

At least that would match the comment.

> +		if (irq->group <= group) {
> +			irq->pending_latch = true;
> +			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +		} else {
> +			spin_unlock_irqrestore(&irq->irq_lock, flags);
> +		}
> +
>  		vgic_put_irq(vcpu->kvm, irq);
>  	}
>  }
> -- 
> 2.18.0
> 

Thanks,
-Christoffer

[PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:14:59PM +0100, Marc Zyngier wrote:
> Although vgic-v3 now supports Group0 interrupts, it still doesn't
> deal with Group0 SGIs. As usually with the GIC, nothing is simple:
> 
> - ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
>   with KVM (as per 8.1.10, Non-secure EL1 access)
> 
> - ICC_SGI0R can only generate Group0 SGIs
> 
> - ICC_ASGI1R sees its scope refocussed to generate only Group0
>   SGIs (as per the note at the bottom of Table 8-14)
> 
> One way to look at this is that an SGI can be generated if the
> group implied by the CPU interface is lower or equal to the
> group specified by the interrupt.

This sentence hurts my brain. Another way to look at it is that with
DS=1, only SGI1R allows signaling group1 interrupts.  See below.

> 
> We only support Group1 SGIs so far, so no material change.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c            |  2 +-
>  arch/arm64/kvm/sys_regs.c        |  2 +-
>  include/kvm/arm_vgic.h           |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 3a02e76699a6..ec517992c12d 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>  
> -	vgic_v3_dispatch_sgi(vcpu, reg);
> +	vgic_v3_dispatch_sgi(vcpu, reg, 1);
>  
>  	return true;
>  }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e04aacb2a24c..a09139b97e81 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p, r);
>  
> -	vgic_v3_dispatch_sgi(vcpu, p->regval);
> +	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
>  
>  	return true;
>  }
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c134790be32c..06a25b11efa7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
>  
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
>  
>  /**
>   * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 88e78b582139..11d321f7ad71 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>   * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
>   * @vcpu: The VCPU requesting a SGI
>   * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
> + * @group: Interrupt group requested by the sender
>   *
>   * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
>   * This will trap in sys_regs.c and call this function.
> @@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>   * check for matching ones. If this bit is set, we signal all, but not the
>   * calling VCPU.
>   */
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	struct kvm_vcpu *c_vcpu;
> @@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>  
>  		spin_lock_irqsave(&irq->irq_lock, flags);
> -		irq->pending_latch = true;
>  
> -		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +		/*
> +		 * A Group0 access can only generate a Group0 SGI,
> +		 * while a Group1 access can generate either group.
> +		 */

nit: "Is a Group 0 access" a well-defined term?

I think it may be clearer if the parameter was: bool allow_group1

and the condition was:

if (irq->group && allow_group1) {
	...

At least that would match the comment.

> +		if (irq->group <= group) {
> +			irq->pending_latch = true;
> +			vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +		} else {
> +			spin_unlock_irqrestore(&irq->irq_lock, flags);
> +		}
> +
>  		vgic_put_irq(vcpu->kvm, irq);
>  	}
>  }
> -- 
> 2.18.0
> 

Thanks,
-Christoffer

Re: [PATCH 3/4] KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1 accesses

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:15:00PM +0100, Marc Zyngier wrote:
> In order to generate Group0 SGIs, let's add some decoding logic to
> access_gic_sgi(), and pass the generating group accordingly.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  2 ++
>  arch/arm64/kvm/sys_regs.c       | 41 +++++++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 98af0b37fb31..b0d2a52a71a3 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -314,6 +314,8 @@
>  #define SYS_ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
>  #define SYS_ICC_RPR_EL1			sys_reg(3, 0, 12, 11, 3)
>  #define SYS_ICC_SGI1R_EL1		sys_reg(3, 0, 12, 11, 5)
> +#define SYS_ICC_ASGI1R_EL1		sys_reg(3, 0, 12, 11, 6)
> +#define SYS_ICC_SGI0R_EL1		sys_reg(3, 0, 12, 11, 7)
>  #define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
>  #define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
>  #define SYS_ICC_HPPIR1_EL1		sys_reg(3, 0, 12, 12, 2)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a09139b97e81..b09eac49e1ac 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -252,10 +252,43 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  			   struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> +	int group;
> +
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p, r);
>  
> -	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
> +	/*
> +	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R_EL1 access generates
> +	 * Group0 SGIs only, while ICC_SGI1R_EL1 can generate either group,
> +	 * depending on the SGI configuration. ICC_ASGI1R_EL1 is effectively
> +	 * equivalent to ICC_SGI0R_EL1, as there is no "alternative" secure
> +	 * group.
> +	 */
> +	if (p->is_aarch32) {
> +		switch (p->Op1) {
> +		default:		/* Keep GCC quiet */
> +		case 0:			/* ICC_SGI1R */
> +			group = 1;
> +			break;
> +		case 1:			/* ICC_ASGI1R */
> +		case 2:			/* ICC_SGI0R */
> +			group = 0;
> +			break;
> +		}
> +	} else {
> +		switch (p->Op2) {
> +		default:		/* Keep GCC quiet */
> +		case 5:			/* ICC_SGI1R_EL1 */
> +			group = 1;
> +			break;
> +		case 6:			/* ICC_ASGI1R_EL1 */
> +		case 7:			/* ICC_SGI0R_EL1 */
> +			group = 0;
> +			break;
> +		}
> +	}
> +
> +	vgic_v3_dispatch_sgi(vcpu, p->regval, group);
>  
>  	return true;
>  }
> @@ -1312,6 +1345,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_ICC_DIR_EL1), read_from_write_only },
>  	{ SYS_DESC(SYS_ICC_RPR_EL1), write_to_read_only },
>  	{ SYS_DESC(SYS_ICC_SGI1R_EL1), access_gic_sgi },
> +	{ SYS_DESC(SYS_ICC_ASGI1R_EL1), access_gic_sgi },
> +	{ SYS_DESC(SYS_ICC_SGI0R_EL1), access_gic_sgi },
>  	{ SYS_DESC(SYS_ICC_IAR1_EL1), write_to_read_only },
>  	{ SYS_DESC(SYS_ICC_EOIR1_EL1), read_from_write_only },
>  	{ SYS_DESC(SYS_ICC_HPPIR1_EL1), write_to_read_only },
> @@ -1744,8 +1779,10 @@ static const struct sys_reg_desc cp15_regs[] = {
>  static const struct sys_reg_desc cp15_64_regs[] = {
>  	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
>  	{ Op1( 0), CRn( 0), CRm( 9), Op2( 0), access_pmu_evcntr },
> -	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
> +	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
>  	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
> +	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
> +	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
>  	{ Op1( 2), CRn( 0), CRm(14), Op2( 0), access_cntp_cval },
>  };
>  
> -- 
> 2.18.0
> 

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

[PATCH 3/4] KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1 accesses

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:15:00PM +0100, Marc Zyngier wrote:
> In order to generate Group0 SGIs, let's add some decoding logic to
> access_gic_sgi(), and pass the generating group accordingly.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  2 ++
>  arch/arm64/kvm/sys_regs.c       | 41 +++++++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 98af0b37fb31..b0d2a52a71a3 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -314,6 +314,8 @@
>  #define SYS_ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
>  #define SYS_ICC_RPR_EL1			sys_reg(3, 0, 12, 11, 3)
>  #define SYS_ICC_SGI1R_EL1		sys_reg(3, 0, 12, 11, 5)
> +#define SYS_ICC_ASGI1R_EL1		sys_reg(3, 0, 12, 11, 6)
> +#define SYS_ICC_SGI0R_EL1		sys_reg(3, 0, 12, 11, 7)
>  #define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
>  #define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
>  #define SYS_ICC_HPPIR1_EL1		sys_reg(3, 0, 12, 12, 2)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a09139b97e81..b09eac49e1ac 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -252,10 +252,43 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  			   struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> +	int group;
> +
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p, r);
>  
> -	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
> +	/*
> +	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R_EL1 access generates
> +	 * Group0 SGIs only, while ICC_SGI1R_EL1 can generate either group,
> +	 * depending on the SGI configuration. ICC_ASGI1R_EL1 is effectively
> +	 * equivalent to ICC_SGI0R_EL1, as there is no "alternative" secure
> +	 * group.
> +	 */
> +	if (p->is_aarch32) {
> +		switch (p->Op1) {
> +		default:		/* Keep GCC quiet */
> +		case 0:			/* ICC_SGI1R */
> +			group = 1;
> +			break;
> +		case 1:			/* ICC_ASGI1R */
> +		case 2:			/* ICC_SGI0R */
> +			group = 0;
> +			break;
> +		}
> +	} else {
> +		switch (p->Op2) {
> +		default:		/* Keep GCC quiet */
> +		case 5:			/* ICC_SGI1R_EL1 */
> +			group = 1;
> +			break;
> +		case 6:			/* ICC_ASGI1R_EL1 */
> +		case 7:			/* ICC_SGI0R_EL1 */
> +			group = 0;
> +			break;
> +		}
> +	}
> +
> +	vgic_v3_dispatch_sgi(vcpu, p->regval, group);
>  
>  	return true;
>  }
> @@ -1312,6 +1345,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_ICC_DIR_EL1), read_from_write_only },
>  	{ SYS_DESC(SYS_ICC_RPR_EL1), write_to_read_only },
>  	{ SYS_DESC(SYS_ICC_SGI1R_EL1), access_gic_sgi },
> +	{ SYS_DESC(SYS_ICC_ASGI1R_EL1), access_gic_sgi },
> +	{ SYS_DESC(SYS_ICC_SGI0R_EL1), access_gic_sgi },
>  	{ SYS_DESC(SYS_ICC_IAR1_EL1), write_to_read_only },
>  	{ SYS_DESC(SYS_ICC_EOIR1_EL1), read_from_write_only },
>  	{ SYS_DESC(SYS_ICC_HPPIR1_EL1), write_to_read_only },
> @@ -1744,8 +1779,10 @@ static const struct sys_reg_desc cp15_regs[] = {
>  static const struct sys_reg_desc cp15_64_regs[] = {
>  	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
>  	{ Op1( 0), CRn( 0), CRm( 9), Op2( 0), access_pmu_evcntr },
> -	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
> +	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
>  	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
> +	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
> +	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
>  	{ Op1( 2), CRn( 0), CRm(14), Op2( 0), access_cntp_cval },
>  };
>  
> -- 
> 2.18.0
> 

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

Re: [PATCH 1/4] KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:14:58PM +0100, Marc Zyngier wrote:
> ICC_SGI1R is a 64bit system register, even on AArch32. It is thus
> pointless to have such an encoding in the 32bit cp15 array. Let's
> drop it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 774d72155904..e04aacb2a24c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1622,8 +1622,6 @@ static const struct sys_reg_desc cp14_64_regs[] = {
>   * register).
>   */
>  static const struct sys_reg_desc cp15_regs[] = {
> -	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
> -
>  	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, c1_SCTLR },
>  	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
>  	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, c2_TTBR1 },
> -- 
> 2.18.0
> 
Acked-by: Christoffer Dall <christoffer.dall@arm.com>

[PATCH 1/4] KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:14:58PM +0100, Marc Zyngier wrote:
> ICC_SGI1R is a 64bit system register, even on AArch32. It is thus
> pointless to have such an encoding in the 32bit cp15 array. Let's
> drop it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 774d72155904..e04aacb2a24c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1622,8 +1622,6 @@ static const struct sys_reg_desc cp14_64_regs[] = {
>   * register).
>   */
>  static const struct sys_reg_desc cp15_regs[] = {
> -	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi },
> -
>  	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, c1_SCTLR },
>  	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
>  	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, c2_TTBR1 },
> -- 
> 2.18.0
> 
Acked-by: Christoffer Dall <christoffer.dall@arm.com>

Re: [PATCH 4/4] KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:15:01PM +0100, Marc Zyngier wrote:
> In order to generate Group0 SGIs, let's add some decoding logic to
> access_gic_sgi(), and pass the generating group accordingly.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index ec517992c12d..0dc0ca061d78 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -246,6 +246,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  			   const struct coproc_reg *r)
>  {
>  	u64 reg;
> +	int group;
>  
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p);
> @@ -253,7 +254,25 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>  
> -	vgic_v3_dispatch_sgi(vcpu, reg, 1);
> +	/*
> +	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R access generates
> +	 * Group0 SGIs only, while ICC_SGI1R can generate either group,
> +	 * depending on the SGI configuration. ICC_ASGI1R is effectively
> +	 * equivalent to ICC_SGI0R, as there is no "alternative" secure
> +	 * group.
> +	 */
> +	switch (p->Op1) {
> +	default:		/* Keep GCC quiet */
> +	case 0:			/* ICC_SGI1R */
> +		group = 1;
> +		break;
> +	case 1:			/* ICC_ASGI1R */
> +	case 2:			/* ICC_SGI0R */
> +		group = 0;
> +		break;
> +	}
> +
> +	vgic_v3_dispatch_sgi(vcpu, reg, group);
>  
>  	return true;
>  }
> @@ -459,6 +478,10 @@ static const struct coproc_reg cp15_regs[] = {
>  
>  	/* ICC_SGI1R */
>  	{ CRm64(12), Op1( 0), is64, access_gic_sgi},
> +	/* ICC_ASGI1R */
> +	{ CRm64(12), Op1( 1), is64, access_gic_sgi},
> +	/* ICC_SGI0R */
> +	{ CRm64(12), Op1( 2), is64, access_gic_sgi},
>  
>  	/* VBAR: swapped by interrupt.S. */
>  	{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
> -- 
> 2.18.0
> 

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

[PATCH 4/4] KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses

[Christoffer Dall]

On Wed, Aug 08, 2018 at 02:15:01PM +0100, Marc Zyngier wrote:
> In order to generate Group0 SGIs, let's add some decoding logic to
> access_gic_sgi(), and pass the generating group accordingly.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index ec517992c12d..0dc0ca061d78 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -246,6 +246,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  			   const struct coproc_reg *r)
>  {
>  	u64 reg;
> +	int group;
>  
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p);
> @@ -253,7 +254,25 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>  
> -	vgic_v3_dispatch_sgi(vcpu, reg, 1);
> +	/*
> +	 * In a system where GICD_CTLR.DS=1, a ICC_SGI0R access generates
> +	 * Group0 SGIs only, while ICC_SGI1R can generate either group,
> +	 * depending on the SGI configuration. ICC_ASGI1R is effectively
> +	 * equivalent to ICC_SGI0R, as there is no "alternative" secure
> +	 * group.
> +	 */
> +	switch (p->Op1) {
> +	default:		/* Keep GCC quiet */
> +	case 0:			/* ICC_SGI1R */
> +		group = 1;
> +		break;
> +	case 1:			/* ICC_ASGI1R */
> +	case 2:			/* ICC_SGI0R */
> +		group = 0;
> +		break;
> +	}
> +
> +	vgic_v3_dispatch_sgi(vcpu, reg, group);
>  
>  	return true;
>  }
> @@ -459,6 +478,10 @@ static const struct coproc_reg cp15_regs[] = {
>  
>  	/* ICC_SGI1R */
>  	{ CRm64(12), Op1( 0), is64, access_gic_sgi},
> +	/* ICC_ASGI1R */
> +	{ CRm64(12), Op1( 1), is64, access_gic_sgi},
> +	/* ICC_SGI0R */
> +	{ CRm64(12), Op1( 2), is64, access_gic_sgi},
>  
>  	/* VBAR: swapped by interrupt.S. */
>  	{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
> -- 
> 2.18.0
> 

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

Re: [PATCH 0/4] KVM: arm/arm64: vgic-v3: Group0 SGI support

[Auger Eric]

Hi Marc,

On 08/08/2018 03:14 PM, Marc Zyngier wrote:
> Although we now have Group0 support, we still miss support for Group0
> SGIs (which amounts to handling ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
> traps), and this small series adds such support.
> 
> I appreciate this is *very* late for 4.19, I'd like to take it in as
> they complement Christoffer's Group0 support, and It'd be annoying to
> have something incomplete in 4.19.
> 
> Please shout if you spot something that doesn't look quite right.

Series
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 
> Marc Zyngier (4):
>   KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding
>   KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs
>   KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
>     accesses
>   KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses
> 
>  arch/arm/kvm/coproc.c            | 25 ++++++++++++++++++-
>  arch/arm64/include/asm/sysreg.h  |  2 ++
>  arch/arm64/kvm/sys_regs.c        | 43 +++++++++++++++++++++++++++++---
>  include/kvm/arm_vgic.h           |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++---
>  5 files changed, 79 insertions(+), 9 deletions(-)
> 

[PATCH 0/4] KVM: arm/arm64: vgic-v3: Group0 SGI support

[Auger Eric]

Hi Marc,

On 08/08/2018 03:14 PM, Marc Zyngier wrote:
> Although we now have Group0 support, we still miss support for Group0
> SGIs (which amounts to handling ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
> traps), and this small series adds such support.
> 
> I appreciate this is *very* late for 4.19, I'd like to take it in as
> they complement Christoffer's Group0 support, and It'd be annoying to
> have something incomplete in 4.19.
> 
> Please shout if you spot something that doesn't look quite right.

Series
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 
> Marc Zyngier (4):
>   KVM: arm64: Remove non-existent AArch32 ICC_SGI1R encoding
>   KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs
>   KVM: arm64: vgic-v3: Add support for ICC_SGI0R_EL1 and ICC_ASGI1R_EL1
>     accesses
>   KVM: arm: vgic-v3: Add support for ICC_SGI0R and ICC_ASGI1R accesses
> 
>  arch/arm/kvm/coproc.c            | 25 ++++++++++++++++++-
>  arch/arm64/include/asm/sysreg.h  |  2 ++
>  arch/arm64/kvm/sys_regs.c        | 43 +++++++++++++++++++++++++++++---
>  include/kvm/arm_vgic.h           |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++---
>  5 files changed, 79 insertions(+), 9 deletions(-)
> 

Re: [PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Marc Zyngier]

On 09/08/18 08:40, Christoffer Dall wrote:
> On Wed, Aug 08, 2018 at 02:14:59PM +0100, Marc Zyngier wrote:
>> Although vgic-v3 now supports Group0 interrupts, it still doesn't
>> deal with Group0 SGIs. As usually with the GIC, nothing is simple:
>>
>> - ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
>>   with KVM (as per 8.1.10, Non-secure EL1 access)
>>
>> - ICC_SGI0R can only generate Group0 SGIs
>>
>> - ICC_ASGI1R sees its scope refocussed to generate only Group0
>>   SGIs (as per the note at the bottom of Table 8-14)
>>
>> One way to look at this is that an SGI can be generated if the
>> group implied by the CPU interface is lower or equal to the
>> group specified by the interrupt.
> 
> This sentence hurts my brain. Another way to look at it is that with
> DS=1, only SGI1R allows signaling group1 interrupts.  See below.

Fair enough.

>>
>> We only support Group1 SGIs so far, so no material change.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kvm/coproc.c            |  2 +-
>>  arch/arm64/kvm/sys_regs.c        |  2 +-
>>  include/kvm/arm_vgic.h           |  2 +-
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 3a02e76699a6..ec517992c12d 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>>  
>> -	vgic_v3_dispatch_sgi(vcpu, reg);
>> +	vgic_v3_dispatch_sgi(vcpu, reg, 1);
>>  
>>  	return true;
>>  }
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index e04aacb2a24c..a09139b97e81 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>>  	if (!p->is_write)
>>  		return read_from_write_only(vcpu, p, r);
>>  
>> -	vgic_v3_dispatch_sgi(vcpu, p->regval);
>> +	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
>>  
>>  	return true;
>>  }
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index c134790be32c..06a25b11efa7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>>  void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
>>  
>> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
>>  
>>  /**
>>   * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 88e78b582139..11d321f7ad71 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>>   * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
>>   * @vcpu: The VCPU requesting a SGI
>>   * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
>> + * @group: Interrupt group requested by the sender
>>   *
>>   * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
>>   * This will trap in sys_regs.c and call this function.
>> @@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>>   * check for matching ones. If this bit is set, we signal all, but not the
>>   * calling VCPU.
>>   */
>> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
>>  {
>>  	struct kvm *kvm = vcpu->kvm;
>>  	struct kvm_vcpu *c_vcpu;
>> @@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>>  		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>>  
>>  		spin_lock_irqsave(&irq->irq_lock, flags);
>> -		irq->pending_latch = true;
>>  
>> -		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>> +		/*
>> +		 * A Group0 access can only generate a Group0 SGI,
>> +		 * while a Group1 access can generate either group.
>> +		 */
> 
> nit: "Is a Group 0 access" a well-defined term?
> 
> I think it may be clearer if the parameter was: bool allow_group1
> 
> and the condition was:
> 
> if (irq->group && allow_group1) {
> 	...
> 
> At least that would match the comment.

Indeed, that's better. Let me respin that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/4] KVM: arm/arm64: vgic-v3: Add core support for Group0 SGIs

[Marc Zyngier]

On 09/08/18 08:40, Christoffer Dall wrote:
> On Wed, Aug 08, 2018 at 02:14:59PM +0100, Marc Zyngier wrote:
>> Although vgic-v3 now supports Group0 interrupts, it still doesn't
>> deal with Group0 SGIs. As usually with the GIC, nothing is simple:
>>
>> - ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
>>   with KVM (as per 8.1.10, Non-secure EL1 access)
>>
>> - ICC_SGI0R can only generate Group0 SGIs
>>
>> - ICC_ASGI1R sees its scope refocussed to generate only Group0
>>   SGIs (as per the note at the bottom of Table 8-14)
>>
>> One way to look at this is that an SGI can be generated if the
>> group implied by the CPU interface is lower or equal to the
>> group specified by the interrupt.
> 
> This sentence hurts my brain. Another way to look at it is that with
> DS=1, only SGI1R allows signaling group1 interrupts.  See below.

Fair enough.

>>
>> We only support Group1 SGIs so far, so no material change.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kvm/coproc.c            |  2 +-
>>  arch/arm64/kvm/sys_regs.c        |  2 +-
>>  include/kvm/arm_vgic.h           |  2 +-
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 3a02e76699a6..ec517992c12d 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>>  	reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
>>  	reg |= *vcpu_reg(vcpu, p->Rt1) ;
>>  
>> -	vgic_v3_dispatch_sgi(vcpu, reg);
>> +	vgic_v3_dispatch_sgi(vcpu, reg, 1);
>>  
>>  	return true;
>>  }
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index e04aacb2a24c..a09139b97e81 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>>  	if (!p->is_write)
>>  		return read_from_write_only(vcpu, p, r);
>>  
>> -	vgic_v3_dispatch_sgi(vcpu, p->regval);
>> +	vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
>>  
>>  	return true;
>>  }
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index c134790be32c..06a25b11efa7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>>  void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>>  void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
>>  
>> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
>>  
>>  /**
>>   * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 88e78b582139..11d321f7ad71 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>>   * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
>>   * @vcpu: The VCPU requesting a SGI
>>   * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
>> + * @group: Interrupt group requested by the sender
>>   *
>>   * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
>>   * This will trap in sys_regs.c and call this function.
>> @@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
>>   * check for matching ones. If this bit is set, we signal all, but not the
>>   * calling VCPU.
>>   */
>> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
>>  {
>>  	struct kvm *kvm = vcpu->kvm;
>>  	struct kvm_vcpu *c_vcpu;
>> @@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>>  		irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>>  
>>  		spin_lock_irqsave(&irq->irq_lock, flags);
>> -		irq->pending_latch = true;
>>  
>> -		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>> +		/*
>> +		 * A Group0 access can only generate a Group0 SGI,
>> +		 * while a Group1 access can generate either group.
>> +		 */
> 
> nit: "Is a Group 0 access" a well-defined term?
> 
> I think it may be clearer if the parameter was: bool allow_group1
> 
> and the condition was:
> 
> if (irq->group && allow_group1) {
> 	...
> 
> At least that would match the comment.

Indeed, that's better. Let me respin that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...