[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

This small series addresses a peculiarity of the current VGIC
implementation, namely that we don't support interrupt grouping.

KVM either implements a GICv2 without support for the security
extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
security extensions, group 0 interrupts can be configured to be either
signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
and group 0 interrupts are always FIQs, and there is no concept of
secure vs. non-secure group 1 interrupts when DS=1.

We were treating all interrupts on GICv2 as group 0, but yet telling the
geust that they were group 1.  The first patch changes this behavior,
which seems to have no effect on no known guests, but still.

The remaining patches introduce proper interrupt grouping support, along
with MMIO accessors for the VM and userspace to retrieve and set the
which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
interrupts as per the architecture, and there is no way to modify this
configuration (no IGROUPR registers for LPIs or equivalent ITS
commands).

Lightly tested on Seattle, TX1, and the foundation model.  I've run a
GICv2 guest on a GICv3 host on the foundation model.

Applies to v4.18-rc1.

Christoffer Dall (4):
  KVM: arm/arm64: GICv2 IGROUPR should read as zero
  KVM: arm/arm64: vgic: Add group field to struct irq
  KVM: arm/arm64: Signal IRQs using their configured group
  KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

 include/kvm/arm_vgic.h           |  1 +
 include/linux/irqchip/arm-gic.h  |  1 +
 virt/kvm/arm/vgic/vgic-debug.c   |  8 ++++---
 virt/kvm/arm/vgic/vgic-init.c    | 11 +++++++--
 virt/kvm/arm/vgic/vgic-its.c     |  1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
 virt/kvm/arm/vgic/vgic-v2.c      |  3 +++
 virt/kvm/arm/vgic/vgic-v3.c      |  6 +----
 11 files changed, 67 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

This small series addresses a peculiarity of the current VGIC
implementation, namely that we don't support interrupt grouping.

KVM either implements a GICv2 without support for the security
extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
security extensions, group 0 interrupts can be configured to be either
signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
and group 0 interrupts are always FIQs, and there is no concept of
secure vs. non-secure group 1 interrupts when DS=1.

We were treating all interrupts on GICv2 as group 0, but yet telling the
geust that they were group 1.  The first patch changes this behavior,
which seems to have no effect on no known guests, but still.

The remaining patches introduce proper interrupt grouping support, along
with MMIO accessors for the VM and userspace to retrieve and set the
which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
interrupts as per the architecture, and there is no way to modify this
configuration (no IGROUPR registers for LPIs or equivalent ITS
commands).

Lightly tested on Seattle, TX1, and the foundation model.  I've run a
GICv2 guest on a GICv3 host on the foundation model.

Applies to v4.18-rc1.

Christoffer Dall (4):
  KVM: arm/arm64: GICv2 IGROUPR should read as zero
  KVM: arm/arm64: vgic: Add group field to struct irq
  KVM: arm/arm64: Signal IRQs using their configured group
  KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

 include/kvm/arm_vgic.h           |  1 +
 include/linux/irqchip/arm-gic.h  |  1 +
 virt/kvm/arm/vgic/vgic-debug.c   |  8 ++++---
 virt/kvm/arm/vgic/vgic-init.c    | 11 +++++++--
 virt/kvm/arm/vgic/vgic-its.c     |  1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
 virt/kvm/arm/vgic/vgic-v2.c      |  3 +++
 virt/kvm/arm/vgic/vgic-v3.c      |  6 +----
 11 files changed, 67 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/4] KVM: arm/arm64: GICv2 IGROUPR should read as zero

[Christoffer Dall]

We currently don't support grouping in the emulated VGIC, which is a
known defect on KVM (not hurting any currently used guests as far as
we're aware). This is currently handled by treating all interrupts as
group 0 interrupts for an emulated GICv2 and always signaling interrupts
as group 0 to the virtual CPU interface.

However, when reading which group interrupts belongs to in the guest
from the emulated VGIC, the VGIC currently reports group 1 instead of
group 0, which is misleading.  Fix this temporarily before introducing
full group support by changing the hander to _raz instead of _rao.

Fixes: fb848db39661a "KVM: arm/arm64: vgic-new: Add GICv2 MMIO handling framework"
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ffc587bf4742..8d18f89397d3 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
-- 
2.17.1

[PATCH 1/4] KVM: arm/arm64: GICv2 IGROUPR should read as zero

[Christoffer Dall]

We currently don't support grouping in the emulated VGIC, which is a
known defect on KVM (not hurting any currently used guests as far as
we're aware). This is currently handled by treating all interrupts as
group 0 interrupts for an emulated GICv2 and always signaling interrupts
as group 0 to the virtual CPU interface.

However, when reading which group interrupts belongs to in the guest
from the emulated VGIC, the VGIC currently reports group 1 instead of
group 0, which is misleading.  Fix this temporarily before introducing
full group support by changing the hander to _raz instead of _rao.

Fixes: fb848db39661a "KVM: arm/arm64: vgic-new: Add GICv2 MMIO handling framework"
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ffc587bf4742..8d18f89397d3 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
-- 
2.17.1

[PATCH 2/4] KVM: arm/arm64: vgic: Add group field to struct irq

[Christoffer Dall]

In preparation for proper group 0 and group 1 support in the vgic, we
add a field in the struct irq to store the group of all interrupts.

We initialize the group to group 0 when emulating GICv2 and to group 1
when emulating GICv3, just like we treat them today.  LPIs are always
group 1.  We also continue to ignore writes from the guest, preserving
existing functionality, for now.

Finally, we also add this field to the vgic debug logic to show the
group for all interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h         |  1 +
 virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
 virt/kvm/arm/vgic/vgic-init.c  | 11 +++++++++--
 virt/kvm/arm/vgic/vgic-its.c   |  1 +
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cfdd2484cc42..d2d578d099de 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -133,6 +133,7 @@ struct vgic_irq {
 	u8 source;			/* GICv2 SGIs only */
 	u8 active_source;		/* GICv2 SGIs only */
 	u8 priority;
+	u8 group;			/* 0 == group 0, 1 == group 1 */
 	enum vgic_irq_config config;	/* Level or edge */
 
 	/*
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index c589d4c2b478..d3a403f63010 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -148,6 +148,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)
 
 	seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
 	seq_printf(s, "E=enabled, H=hw, C=config (level=1, edge=0)\n");
+	seq_printf(s, "G=group\n");
 }
 
 static void print_header(struct seq_file *s, struct vgic_irq *irq,
@@ -162,8 +163,8 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
 	}
 
 	seq_printf(s, "\n");
-	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
-	seq_printf(s, "---------------------------------------------------------------\n");
+	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHCG     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
+	seq_printf(s, "----------------------------------------------------------------\n");
 }
 
 static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
@@ -182,7 +183,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
 
 	seq_printf(s, "       %s %4d "
 		      "    %2d "
-		      "%d%d%d%d%d%d "
+		      "%d%d%d%d%d%d%d "
 		      "%8d "
 		      "%8x "
 		      " %2x "
@@ -197,6 +198,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
 			irq->enabled,
 			irq->hw,
 			irq->config == VGIC_CONFIG_LEVEL,
+			irq->group,
 			irq->hwintid,
 			irq->mpidr,
 			irq->source,
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 2673efce65f3..146a1bbb05c8 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -175,10 +175,13 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 		irq->vcpu = NULL;
 		irq->target_vcpu = vcpu0;
 		kref_init(&irq->refcount);
-		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
 			irq->targets = 0;
-		else
+			irq->group = 0;
+		} else {
 			irq->mpidr = 0;
+			irq->group = 1;
+		}
 	}
 	return 0;
 }
@@ -227,6 +230,10 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			irq->group = 0;
+		else
+			irq->group = 1;
 	}
 
 	if (!irqchip_in_kernel(vcpu->kvm))
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 4ed79c939fb4..92840c06fcd7 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -71,6 +71,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
 	kref_init(&irq->refcount);
 	irq->intid = intid;
 	irq->target_vcpu = vcpu;
+	irq->group = 1;
 
 	spin_lock_irqsave(&dist->lpi_list_lock, flags);
 
-- 
2.17.1

[PATCH 2/4] KVM: arm/arm64: vgic: Add group field to struct irq

[Christoffer Dall]

In preparation for proper group 0 and group 1 support in the vgic, we
add a field in the struct irq to store the group of all interrupts.

We initialize the group to group 0 when emulating GICv2 and to group 1
when emulating GICv3, just like we treat them today.  LPIs are always
group 1.  We also continue to ignore writes from the guest, preserving
existing functionality, for now.

Finally, we also add this field to the vgic debug logic to show the
group for all interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h         |  1 +
 virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
 virt/kvm/arm/vgic/vgic-init.c  | 11 +++++++++--
 virt/kvm/arm/vgic/vgic-its.c   |  1 +
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cfdd2484cc42..d2d578d099de 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -133,6 +133,7 @@ struct vgic_irq {
 	u8 source;			/* GICv2 SGIs only */
 	u8 active_source;		/* GICv2 SGIs only */
 	u8 priority;
+	u8 group;			/* 0 == group 0, 1 == group 1 */
 	enum vgic_irq_config config;	/* Level or edge */
 
 	/*
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index c589d4c2b478..d3a403f63010 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -148,6 +148,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)
 
 	seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
 	seq_printf(s, "E=enabled, H=hw, C=config (level=1, edge=0)\n");
+	seq_printf(s, "G=group\n");
 }
 
 static void print_header(struct seq_file *s, struct vgic_irq *irq,
@@ -162,8 +163,8 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
 	}
 
 	seq_printf(s, "\n");
-	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
-	seq_printf(s, "---------------------------------------------------------------\n");
+	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHCG     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
+	seq_printf(s, "----------------------------------------------------------------\n");
 }
 
 static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
@@ -182,7 +183,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
 
 	seq_printf(s, "       %s %4d "
 		      "    %2d "
-		      "%d%d%d%d%d%d "
+		      "%d%d%d%d%d%d%d "
 		      "%8d "
 		      "%8x "
 		      " %2x "
@@ -197,6 +198,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
 			irq->enabled,
 			irq->hw,
 			irq->config == VGIC_CONFIG_LEVEL,
+			irq->group,
 			irq->hwintid,
 			irq->mpidr,
 			irq->source,
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 2673efce65f3..146a1bbb05c8 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -175,10 +175,13 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 		irq->vcpu = NULL;
 		irq->target_vcpu = vcpu0;
 		kref_init(&irq->refcount);
-		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
 			irq->targets = 0;
-		else
+			irq->group = 0;
+		} else {
 			irq->mpidr = 0;
+			irq->group = 1;
+		}
 	}
 	return 0;
 }
@@ -227,6 +230,10 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			irq->group = 0;
+		else
+			irq->group = 1;
 	}
 
 	if (!irqchip_in_kernel(vcpu->kvm))
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 4ed79c939fb4..92840c06fcd7 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -71,6 +71,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
 	kref_init(&irq->refcount);
 	irq->intid = intid;
 	irq->target_vcpu = vcpu;
+	irq->group = 1;
 
 	spin_lock_irqsave(&dist->lpi_list_lock, flags);
 
-- 
2.17.1

[PATCH 3/4] KVM: arm/arm64: Signal IRQs using their configured group

[Christoffer Dall]

Now when we have a group configuration on the struct IRQ, use this state
when populating the LR and signaling interrupts as either group 0 or
group 1 to the VM.  Depending on the model of the emulated GIC, and the
guest's configuration of the VMCR, interrupts may be signaled as IRQs or
FIQs to the VM.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/linux/irqchip/arm-gic.h | 1 +
 virt/kvm/arm/vgic/vgic-v2.c     | 3 +++
 virt/kvm/arm/vgic/vgic-v3.c     | 6 +-----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 68d8b1f73682..3fa74b917a82 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -94,6 +94,7 @@
 #define GICH_LR_PENDING_BIT		(1 << 28)
 #define GICH_LR_ACTIVE_BIT		(1 << 29)
 #define GICH_LR_EOI			(1 << 19)
+#define GICH_LR_GROUP1			(1 << 30)
 #define GICH_LR_HW			(1 << 31)
 
 #define GICH_VMCR_ENABLE_GRP0_SHIFT	0
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index a5f2e44f1c33..df5e6a6e3186 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -159,6 +159,9 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 		}
 	}
 
+	if (irq->group)
+		val |= GICH_LR_GROUP1;
+
 	if (irq->hw) {
 		val |= GICH_LR_HW;
 		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index ff7dc890941a..4f64228e4b3d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -197,11 +197,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
 		irq->line_level = false;
 
-	/*
-	 * We currently only support Group1 interrupts, which is a
-	 * known defect. This needs to be addressed at some point.
-	 */
-	if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
+	if (irq->group)
 		val |= ICH_LR_GROUP;
 
 	val |= (u64)irq->priority << ICH_LR_PRIORITY_SHIFT;
-- 
2.17.1

[PATCH 3/4] KVM: arm/arm64: Signal IRQs using their configured group

[Christoffer Dall]

Now when we have a group configuration on the struct IRQ, use this state
when populating the LR and signaling interrupts as either group 0 or
group 1 to the VM.  Depending on the model of the emulated GIC, and the
guest's configuration of the VMCR, interrupts may be signaled as IRQs or
FIQs to the VM.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/linux/irqchip/arm-gic.h | 1 +
 virt/kvm/arm/vgic/vgic-v2.c     | 3 +++
 virt/kvm/arm/vgic/vgic-v3.c     | 6 +-----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 68d8b1f73682..3fa74b917a82 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -94,6 +94,7 @@
 #define GICH_LR_PENDING_BIT		(1 << 28)
 #define GICH_LR_ACTIVE_BIT		(1 << 29)
 #define GICH_LR_EOI			(1 << 19)
+#define GICH_LR_GROUP1			(1 << 30)
 #define GICH_LR_HW			(1 << 31)
 
 #define GICH_VMCR_ENABLE_GRP0_SHIFT	0
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index a5f2e44f1c33..df5e6a6e3186 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -159,6 +159,9 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 		}
 	}
 
+	if (irq->group)
+		val |= GICH_LR_GROUP1;
+
 	if (irq->hw) {
 		val |= GICH_LR_HW;
 		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index ff7dc890941a..4f64228e4b3d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -197,11 +197,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
 		irq->line_level = false;
 
-	/*
-	 * We currently only support Group1 interrupts, which is a
-	 * known defect. This needs to be addressed at some point.
-	 */
-	if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
+	if (irq->group)
 		val |= ICH_LR_GROUP;
 
 	val |= (u64)irq->priority << ICH_LR_PRIORITY_SHIFT;
-- 
2.17.1

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

Implement the required MMIO accessors for GICv2 and GICv3 for the
IGROUPR distributor and redistributor registers.

This can allow guests to change behavior compared to running on previous
versions of KVM, but only to align with the architecture and hardware
implementations.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 8d18f89397d3..ff3834d16ac9 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 287784095b5b..76e422859745 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
 		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index ff9655cfeb2f..ae31bd0dd365 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	/* Ignore */
 }
 
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
+				   gpa_t addr, unsigned int len)
+{
+	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+	u32 value = 0;
+	int i;
+
+	/* Loop over all IRQs affected by this read */
+	for (i = 0; i < len * 8; i++) {
+		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+		if (irq->group)
+			value |= BIT(i);
+
+		vgic_put_irq(vcpu->kvm, irq);
+	}
+
+	return value;
+}
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+			   unsigned int len, unsigned long val)
+{
+	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+	int i;
+	unsigned long flags;
+
+	for (i = 0; i < len * 8; i++) {
+		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+		spin_lock_irqsave(&irq->irq_lock, flags);
+		irq->group = !!(val & BIT(i));
+		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+
+		vgic_put_irq(vcpu->kvm, irq);
+	}
+}
+
 /*
  * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
  * of the enabled bit, so there is only one function for both here.
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df45ec..10798625f9ce 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
 void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 			unsigned int len, unsigned long val);
 
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
+				   unsigned int len);
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+			   unsigned int len, unsigned long val);
+
 unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
 				    gpa_t addr, unsigned int len);
 
-- 
2.17.1

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

Implement the required MMIO accessors for GICv2 and GICv3 for the
IGROUPR distributor and redistributor registers.

This can allow guests to change behavior compared to running on previous
versions of KVM, but only to align with the architecture and hardware
implementations.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
 virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 8d18f89397d3..ff3834d16ac9 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 287784095b5b..76e422859745 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
 		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index ff9655cfeb2f..ae31bd0dd365 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	/* Ignore */
 }
 
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
+				   gpa_t addr, unsigned int len)
+{
+	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+	u32 value = 0;
+	int i;
+
+	/* Loop over all IRQs affected by this read */
+	for (i = 0; i < len * 8; i++) {
+		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+		if (irq->group)
+			value |= BIT(i);
+
+		vgic_put_irq(vcpu->kvm, irq);
+	}
+
+	return value;
+}
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+			   unsigned int len, unsigned long val)
+{
+	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+	int i;
+	unsigned long flags;
+
+	for (i = 0; i < len * 8; i++) {
+		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+		spin_lock_irqsave(&irq->irq_lock, flags);
+		irq->group = !!(val & BIT(i));
+		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+
+		vgic_put_irq(vcpu->kvm, irq);
+	}
+}
+
 /*
  * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
  * of the enabled bit, so there is only one function for both here.
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df45ec..10798625f9ce 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
 void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 			unsigned int len, unsigned long val);
 
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
+				   unsigned int len);
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+			   unsigned int len, unsigned long val);
+
 unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
 				    gpa_t addr, unsigned int len);
 
-- 
2.17.1

Re: [PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Marc Zyngier]

On 24/06/18 23:11, Christoffer Dall wrote:
> Implement the required MMIO accessors for GICv2 and GICv3 for the
> IGROUPR distributor and redistributor registers.
> 
> This can allow guests to change behavior compared to running on previous
> versions of KVM, but only to align with the architecture and hardware
> implementations.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>  4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 8d18f89397d3..ff3834d16ac9 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 287784095b5b..76e422859745 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,

I think you're missing the GICR_IGROUPR accessor in the redistributor
(despite mentioning it in the commit message).

> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index ff9655cfeb2f..ae31bd0dd365 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
>  	/* Ignore */
>  }
>  
> +unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
> +				   gpa_t addr, unsigned int len)
> +{
> +	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +	u32 value = 0;
> +	int i;
> +
> +	/* Loop over all IRQs affected by this read */
> +	for (i = 0; i < len * 8; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		if (irq->group)
> +			value |= BIT(i);
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}
> +
> +	return value;
> +}
> +
> +void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +			   unsigned int len, unsigned long val)
> +{
> +	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +	int i;
> +	unsigned long flags;
> +
> +	for (i = 0; i < len * 8; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		spin_lock_irqsave(&irq->irq_lock, flags);
> +		irq->group = !!(val & BIT(i));
> +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}
> +}
> +
>  /*
>   * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
>   * of the enabled bit, so there is only one function for both here.
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 5693f6df45ec..10798625f9ce 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
>  void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
>  			unsigned int len, unsigned long val);
>  
> +unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +				   unsigned int len);
> +
> +void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +			   unsigned int len, unsigned long val);
> +
>  unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
>  				    gpa_t addr, unsigned int len);
>  
> 

Otherwise looks good.

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

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Marc Zyngier]

On 24/06/18 23:11, Christoffer Dall wrote:
> Implement the required MMIO accessors for GICv2 and GICv3 for the
> IGROUPR distributor and redistributor registers.
> 
> This can allow guests to change behavior compared to running on previous
> versions of KVM, but only to align with the architecture and hardware
> implementations.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>  4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 8d18f89397d3..ff3834d16ac9 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 287784095b5b..76e422859745 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,

I think you're missing the GICR_IGROUPR accessor in the redistributor
(despite mentioning it in the commit message).

> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index ff9655cfeb2f..ae31bd0dd365 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
>  	/* Ignore */
>  }
>  
> +unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
> +				   gpa_t addr, unsigned int len)
> +{
> +	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +	u32 value = 0;
> +	int i;
> +
> +	/* Loop over all IRQs affected by this read */
> +	for (i = 0; i < len * 8; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		if (irq->group)
> +			value |= BIT(i);
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}
> +
> +	return value;
> +}
> +
> +void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +			   unsigned int len, unsigned long val)
> +{
> +	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +	int i;
> +	unsigned long flags;
> +
> +	for (i = 0; i < len * 8; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		spin_lock_irqsave(&irq->irq_lock, flags);
> +		irq->group = !!(val & BIT(i));
> +		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}
> +}
> +
>  /*
>   * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
>   * of the enabled bit, so there is only one function for both here.
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 5693f6df45ec..10798625f9ce 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
>  void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
>  			unsigned int len, unsigned long val);
>  
> +unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +				   unsigned int len);
> +
> +void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
> +			   unsigned int len, unsigned long val);
> +
>  unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
>  				    gpa_t addr, unsigned int len);
>  
> 

Otherwise looks good.

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

Re: [PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
> On 24/06/18 23:11, Christoffer Dall wrote:
> > Implement the required MMIO accessors for GICv2 and GICv3 for the
> > IGROUPR distributor and redistributor registers.
> > 
> > This can allow guests to change behavior compared to running on previous
> > versions of KVM, but only to align with the architecture and hardware
> > implementations.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
> >  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
> >  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
> >  4 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 8d18f89397d3..ff3834d16ac9 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> >  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> > -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 287784095b5b..76e422859745 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> >  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> > -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> 
> I think you're missing the GICR_IGROUPR accessor in the redistributor
> (despite mentioning it in the commit message).
> 

Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
the fix is pretty trivial:

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 76e422859745..d2acad07dd30 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
 
 static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
+		vgic_mmio_read_group, vgic_mmio_write_group, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,

I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
happy still.

Do you want me to send a v2, or do you prefer to fix this up locally?

Thanks,
-Christoffer

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
> On 24/06/18 23:11, Christoffer Dall wrote:
> > Implement the required MMIO accessors for GICv2 and GICv3 for the
> > IGROUPR distributor and redistributor registers.
> > 
> > This can allow guests to change behavior compared to running on previous
> > versions of KVM, but only to align with the architecture and hardware
> > implementations.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
> >  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
> >  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
> >  4 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 8d18f89397d3..ff3834d16ac9 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> >  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> > -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 287784095b5b..76e422859745 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> >  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> > -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> 
> I think you're missing the GICR_IGROUPR accessor in the redistributor
> (despite mentioning it in the commit message).
> 

Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
the fix is pretty trivial:

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 76e422859745..d2acad07dd30 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
 
 static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
-		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
+		vgic_mmio_read_group, vgic_mmio_write_group, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
 		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,

I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
happy still.

Do you want me to send a v2, or do you prefer to fix this up locally?

Thanks,
-Christoffer

Re: [PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Marc Zyngier]

On 25/06/18 10:34, Christoffer Dall wrote:
> On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
>> On 24/06/18 23:11, Christoffer Dall wrote:
>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>> IGROUPR distributor and redistributor registers.
>>>
>>> This can allow guests to change behavior compared to running on previous
>>> versions of KVM, but only to align with the architecture and hardware
>>> implementations.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>>>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>>>  4 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> index 8d18f89397d3..ff3834d16ac9 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
>>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 287784095b5b..76e422859745 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>>>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
>>> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
>>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
>>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>>
>> I think you're missing the GICR_IGROUPR accessor in the redistributor
>> (despite mentioning it in the commit message).
>>
> 
> Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
> the fix is pretty trivial:
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 76e422859745..d2acad07dd30 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>  
>  static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
> -		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> +		vgic_mmio_read_group, vgic_mmio_write_group, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,
> 
> I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
> happy still.
> 
> Do you want me to send a v2, or do you prefer to fix this up locally?
I can fix that myself. But this raises another question: As this change
is obviously observable by the guest, should we consider bumping up the
IIDR revision field?

Thanks,

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

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Marc Zyngier]

On 25/06/18 10:34, Christoffer Dall wrote:
> On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
>> On 24/06/18 23:11, Christoffer Dall wrote:
>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>> IGROUPR distributor and redistributor registers.
>>>
>>> This can allow guests to change behavior compared to running on previous
>>> versions of KVM, but only to align with the architecture and hardware
>>> implementations.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>>>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>>>  4 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> index 8d18f89397d3..ff3834d16ac9 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
>>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 287784095b5b..76e422859745 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>>>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
>>> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
>>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
>>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>>
>> I think you're missing the GICR_IGROUPR accessor in the redistributor
>> (despite mentioning it in the commit message).
>>
> 
> Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
> the fix is pretty trivial:
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 76e422859745..d2acad07dd30 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>  
>  static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
> -		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> +		vgic_mmio_read_group, vgic_mmio_write_group, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
>  		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,
> 
> I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
> happy still.
> 
> Do you want me to send a v2, or do you prefer to fix this up locally?
I can fix that myself. But this raises another question: As this change
is obviously observable by the guest, should we consider bumping up the
IIDR revision field?

Thanks,

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

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Peter Maydell]

On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> This small series addresses a peculiarity of the current VGIC
> implementation, namely that we don't support interrupt grouping.
>
> KVM either implements a GICv2 without support for the security
> extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> security extensions, group 0 interrupts can be configured to be either
> signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> and group 0 interrupts are always FIQs, and there is no concept of
> secure vs. non-secure group 1 interrupts when DS=1.
>
> We were treating all interrupts on GICv2 as group 0, but yet telling the
> geust that they were group 1.  The first patch changes this behavior,
> which seems to have no effect on no known guests, but still.
>
> The remaining patches introduce proper interrupt grouping support, along
> with MMIO accessors for the VM and userspace to retrieve and set the
> which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> interrupts as per the architecture, and there is no way to modify this
> configuration (no IGROUPR registers for LPIs or equivalent ITS
> commands).

How do we handle migration compatibility here? For instance if
we have a guest running on an old kernel with GICv2, presumably
it reports the GICD_IGROUPRn to userspace as being all-1 (but
they behave as group 0); then when we load on the new kernel
the new kernel will honour the IGROUPRn values and the interrupts
will start behaving like group 1 ? Maybe this works because the
guest is expecting them to be signalled as IRQ either way...

thanks
-- PMM

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Peter Maydell]

On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> This small series addresses a peculiarity of the current VGIC
> implementation, namely that we don't support interrupt grouping.
>
> KVM either implements a GICv2 without support for the security
> extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> security extensions, group 0 interrupts can be configured to be either
> signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> and group 0 interrupts are always FIQs, and there is no concept of
> secure vs. non-secure group 1 interrupts when DS=1.
>
> We were treating all interrupts on GICv2 as group 0, but yet telling the
> geust that they were group 1.  The first patch changes this behavior,
> which seems to have no effect on no known guests, but still.
>
> The remaining patches introduce proper interrupt grouping support, along
> with MMIO accessors for the VM and userspace to retrieve and set the
> which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> interrupts as per the architecture, and there is no way to modify this
> configuration (no IGROUPR registers for LPIs or equivalent ITS
> commands).

How do we handle migration compatibility here? For instance if
we have a guest running on an old kernel with GICv2, presumably
it reports the GICD_IGROUPRn to userspace as being all-1 (but
they behave as group 0); then when we load on the new kernel
the new kernel will honour the IGROUPRn values and the interrupts
will start behaving like group 1 ? Maybe this works because the
guest is expecting them to be signalled as IRQ either way...

thanks
-- PMM

Re: [PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:44:17AM +0100, Marc Zyngier wrote:
> On 25/06/18 10:34, Christoffer Dall wrote:
> > On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
> >> On 24/06/18 23:11, Christoffer Dall wrote:
> >>> Implement the required MMIO accessors for GICv2 and GICv3 for the
> >>> IGROUPR distributor and redistributor registers.
> >>>
> >>> This can allow guests to change behavior compared to running on previous
> >>> versions of KVM, but only to align with the architecture and hardware
> >>> implementations.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> >>> ---
> >>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
> >>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
> >>>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
> >>>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
> >>>  4 files changed, 46 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> index 8d18f89397d3..ff3834d16ac9 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> >>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> >>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> >>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
> >>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> index 287784095b5b..76e422859745 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> >>>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> >>> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> >>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> >>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> >>
> >> I think you're missing the GICR_IGROUPR accessor in the redistributor
> >> (despite mentioning it in the commit message).
> >>
> > 
> > Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
> > the fix is pretty trivial:
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 76e422859745..d2acad07dd30 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
> >  
> >  static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> >  	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
> > -		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, 4,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,
> > 
> > I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
> > happy still.
> > 
> > Do you want me to send a v2, or do you prefer to fix this up locally?
> I can fix that myself. But this raises another question: As this change
> is obviously observable by the guest, should we consider bumping up the
> IIDR revision field?
> 

Hmm, yeah, good idea.  To really nit pick: Should we make this change
along with the change of visibility of the group of GICv2 interrupts, or
when the guest can actually modify the grouping, or should we bump the
revision twice, one for GICv2 only, and the second time for both GICv2
and GICv3?

I suggest the latter, making it:

Current:
  GICv2 revision: 0
  GICv3 revision: 0

GICv2 fix:
  GICv2 revision: 1
  GICv3 revision: 0

Support both groups:
  GICv2 revision: 2
  GICv3 revision: 2

Thanks,
-Christoffer

[PATCH 4/4] KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:44:17AM +0100, Marc Zyngier wrote:
> On 25/06/18 10:34, Christoffer Dall wrote:
> > On Mon, Jun 25, 2018 at 09:19:07AM +0100, Marc Zyngier wrote:
> >> On 24/06/18 23:11, Christoffer Dall wrote:
> >>> Implement the required MMIO accessors for GICv2 and GICv3 for the
> >>> IGROUPR distributor and redistributor registers.
> >>>
> >>> This can allow guests to change behavior compared to running on previous
> >>> versions of KVM, but only to align with the architecture and hardware
> >>> implementations.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> >>> ---
> >>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
> >>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
> >>>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
> >>>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
> >>>  4 files changed, 46 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> index 8d18f89397d3..ff3834d16ac9 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >>> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> >>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> >>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> >>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
> >>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> index 287784095b5b..76e422859745 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>> @@ -451,7 +451,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> >>>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> >>> -		vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> >>> +		vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
> >>>  		VGIC_ACCESS_32bit),
> >>>  	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> >>>  		vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> >>
> >> I think you're missing the GICR_IGROUPR accessor in the redistributor
> >> (despite mentioning it in the commit message).
> >>
> > 
> > Indeed.  I fixed this up on my kernel.org branch (vgic-group-fixes), and
> > the fix is pretty trivial:
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 76e422859745..d2acad07dd30 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -524,7 +524,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
> >  
> >  static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> >  	REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
> > -		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> > +		vgic_mmio_read_group, vgic_mmio_write_group, 4,
> >  		VGIC_ACCESS_32bit),
> >  	REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
> >  		vgic_mmio_read_enable, vgic_mmio_write_senable, 4,
> > 
> > I did a test on the model using both GICv3 and GICv2-on-GICv3, seems
> > happy still.
> > 
> > Do you want me to send a v2, or do you prefer to fix this up locally?
> I can fix that myself. But this raises another question: As this change
> is obviously observable by the guest, should we consider bumping up the
> IIDR revision field?
> 

Hmm, yeah, good idea.  To really nit pick: Should we make this change
along with the change of visibility of the group of GICv2 interrupts, or
when the guest can actually modify the grouping, or should we bump the
revision twice, one for GICv2 only, and the second time for both GICv2
and GICv3?

I suggest the latter, making it:

Current:
  GICv2 revision: 0
  GICv3 revision: 0

GICv2 fix:
  GICv2 revision: 1
  GICv3 revision: 0

Support both groups:
  GICv2 revision: 2
  GICv3 revision: 2

Thanks,
-Christoffer

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Andre Przywara]

Hi,

On 24/06/18 23:10, Christoffer Dall wrote:
> This small series addresses a peculiarity of the current VGIC
> implementation, namely that we don't support interrupt grouping.

Nice one and seems straight forward enough.
Just curious: Were there any complaints or even users of FIQs and/or
interrupt grouping?

> KVM either implements a GICv2 without support for the security
> extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> security extensions, group 0 interrupts can be configured to be either
> signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> and group 0 interrupts are always FIQs, and there is no concept of
> secure vs. non-secure group 1 interrupts when DS=1.
> 
> We were treating all interrupts on GICv2 as group 0, but yet telling the
> geust that they were group 1.  The first patch changes this behavior,
> which seems to have no effect on no known guests, but still.
> 
> The remaining patches introduce proper interrupt grouping support, along
> with MMIO accessors for the VM and userspace to retrieve and set the
> which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> interrupts as per the architecture, and there is no way to modify this
> configuration (no IGROUPR registers for LPIs or equivalent ITS
> commands).
> 
> Lightly tested on Seattle, TX1, and the foundation model.  I've run a
> GICv2 guest on a GICv3 host on the foundation model.

I take it those were regression tests with Linux guests?
Do we have any means of testing this with guests which actually use
different groups or FIQs? Does kvm-unit-tests cover this?

Did you do any tests on 32 bit? I am going to fire up something on my
Midway later today ...

Cheers,
Andre.

> Applies to v4.18-rc1.
> 
> Christoffer Dall (4):
>   KVM: arm/arm64: GICv2 IGROUPR should read as zero
>   KVM: arm/arm64: vgic: Add group field to struct irq
>   KVM: arm/arm64: Signal IRQs using their configured group
>   KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups
> 
>  include/kvm/arm_vgic.h           |  1 +
>  include/linux/irqchip/arm-gic.h  |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c   |  8 ++++---
>  virt/kvm/arm/vgic/vgic-init.c    | 11 +++++++--
>  virt/kvm/arm/vgic/vgic-its.c     |  1 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>  virt/kvm/arm/vgic/vgic-v2.c      |  3 +++
>  virt/kvm/arm/vgic/vgic-v3.c      |  6 +----
>  11 files changed, 67 insertions(+), 12 deletions(-)
> 

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Andre Przywara]

Hi,

On 24/06/18 23:10, Christoffer Dall wrote:
> This small series addresses a peculiarity of the current VGIC
> implementation, namely that we don't support interrupt grouping.

Nice one and seems straight forward enough.
Just curious: Were there any complaints or even users of FIQs and/or
interrupt grouping?

> KVM either implements a GICv2 without support for the security
> extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> security extensions, group 0 interrupts can be configured to be either
> signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> and group 0 interrupts are always FIQs, and there is no concept of
> secure vs. non-secure group 1 interrupts when DS=1.
> 
> We were treating all interrupts on GICv2 as group 0, but yet telling the
> geust that they were group 1.  The first patch changes this behavior,
> which seems to have no effect on no known guests, but still.
> 
> The remaining patches introduce proper interrupt grouping support, along
> with MMIO accessors for the VM and userspace to retrieve and set the
> which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> interrupts as per the architecture, and there is no way to modify this
> configuration (no IGROUPR registers for LPIs or equivalent ITS
> commands).
> 
> Lightly tested on Seattle, TX1, and the foundation model.  I've run a
> GICv2 guest on a GICv3 host on the foundation model.

I take it those were regression tests with Linux guests?
Do we have any means of testing this with guests which actually use
different groups or FIQs? Does kvm-unit-tests cover this?

Did you do any tests on 32 bit? I am going to fire up something on my
Midway later today ...

Cheers,
Andre.

> Applies to v4.18-rc1.
> 
> Christoffer Dall (4):
>   KVM: arm/arm64: GICv2 IGROUPR should read as zero
>   KVM: arm/arm64: vgic: Add group field to struct irq
>   KVM: arm/arm64: Signal IRQs using their configured group
>   KVM: arm/arm64: vgic: Allow VMs to configure interrupt groups
> 
>  include/kvm/arm_vgic.h           |  1 +
>  include/linux/irqchip/arm-gic.h  |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c   |  8 ++++---
>  virt/kvm/arm/vgic/vgic-init.c    | 11 +++++++--
>  virt/kvm/arm/vgic/vgic-its.c     |  1 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 +++++
>  virt/kvm/arm/vgic/vgic-v2.c      |  3 +++
>  virt/kvm/arm/vgic/vgic-v3.c      |  6 +----
>  11 files changed, 67 insertions(+), 12 deletions(-)
> 

Re: [PATCH 1/4] KVM: arm/arm64: GICv2 IGROUPR should read as zero

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> We currently don't support grouping in the emulated VGIC, which is a
> known defect on KVM (not hurting any currently used guests as far as
> we're aware). This is currently handled by treating all interrupts as
> group 0 interrupts for an emulated GICv2 and always signaling interrupts
> as group 0 to the virtual CPU interface.
>
> However, when reading which group interrupts belongs to in the guest
> from the emulated VGIC, the VGIC currently reports group 1 instead of
> group 0, which is misleading.  Fix this temporarily before introducing
> full group support by changing the hander to _raz instead of _rao.
>
> Fixes: fb848db39661a "KVM: arm/arm64: vgic-new: Add GICv2 MMIO handling framework"
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index ffc587bf4742..8d18f89397d3 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>               vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>               VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> -             vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> +             vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>               VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>               vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 1/4] KVM: arm/arm64: GICv2 IGROUPR should read as zero

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> We currently don't support grouping in the emulated VGIC, which is a
> known defect on KVM (not hurting any currently used guests as far as
> we're aware). This is currently handled by treating all interrupts as
> group 0 interrupts for an emulated GICv2 and always signaling interrupts
> as group 0 to the virtual CPU interface.
>
> However, when reading which group interrupts belongs to in the guest
> from the emulated VGIC, the VGIC currently reports group 1 instead of
> group 0, which is misleading.  Fix this temporarily before introducing
> full group support by changing the hander to _raz instead of _rao.
>
> Fixes: fb848db39661a "KVM: arm/arm64: vgic-new: Add GICv2 MMIO handling framework"
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index ffc587bf4742..8d18f89397d3 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -362,7 +362,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>               vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>               VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
> -             vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> +             vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
>               VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
>               vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 2/4] KVM: arm/arm64: vgic: Add group field to struct irq

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> In preparation for proper group 0 and group 1 support in the vgic, we
> add a field in the struct irq to store the group of all interrupts.
> 
> We initialize the group to group 0 when emulating GICv2 and to group 1
> when emulating GICv3, just like we treat them today.  LPIs are always
> group 1.  We also continue to ignore writes from the guest, preserving
> existing functionality, for now.
> 
> Finally, we also add this field to the vgic debug logic to show the
> group for all interrupts.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  include/kvm/arm_vgic.h         |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
>  virt/kvm/arm/vgic/vgic-init.c  | 11 +++++++++--
>  virt/kvm/arm/vgic/vgic-its.c   |  1 +
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index cfdd2484cc42..d2d578d099de 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -133,6 +133,7 @@ struct vgic_irq {
>  	u8 source;			/* GICv2 SGIs only */
>  	u8 active_source;		/* GICv2 SGIs only */
>  	u8 priority;
> +	u8 group;			/* 0 == group 0, 1 == group 1 */
>  	enum vgic_irq_config config;	/* Level or edge */
>  
>  	/*
> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> index c589d4c2b478..d3a403f63010 100644
> --- a/virt/kvm/arm/vgic/vgic-debug.c
> +++ b/virt/kvm/arm/vgic/vgic-debug.c
> @@ -148,6 +148,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)
>  
>  	seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
>  	seq_printf(s, "E=enabled, H=hw, C=config (level=1, edge=0)\n");
> +	seq_printf(s, "G=group\n");
>  }
>  
>  static void print_header(struct seq_file *s, struct vgic_irq *irq,
> @@ -162,8 +163,8 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>  	}
>  
>  	seq_printf(s, "\n");
> -	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
> -	seq_printf(s, "---------------------------------------------------------------\n");
> +	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHCG     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
> +	seq_printf(s, "----------------------------------------------------------------\n");
>  }
>  
>  static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> @@ -182,7 +183,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>  
>  	seq_printf(s, "       %s %4d "
>  		      "    %2d "
> -		      "%d%d%d%d%d%d "
> +		      "%d%d%d%d%d%d%d "
>  		      "%8d "
>  		      "%8x "
>  		      " %2x "
> @@ -197,6 +198,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>  			irq->enabled,
>  			irq->hw,
>  			irq->config == VGIC_CONFIG_LEVEL,
> +			irq->group,
>  			irq->hwintid,
>  			irq->mpidr,
>  			irq->source,
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 2673efce65f3..146a1bbb05c8 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -175,10 +175,13 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  		irq->vcpu = NULL;
>  		irq->target_vcpu = vcpu0;
>  		kref_init(&irq->refcount);
> -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
>  			irq->targets = 0;
> -		else
> +			irq->group = 0;
> +		} else {
>  			irq->mpidr = 0;
> +			irq->group = 1;
> +		}
>  	}
>  	return 0;
>  }
> @@ -227,6 +230,10 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  			/* PPIs */
>  			irq->config = VGIC_CONFIG_LEVEL;
>  		}
> +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +			irq->group = 0;
> +		else
> +			irq->group = 1;
>  	}
>  
>  	if (!irqchip_in_kernel(vcpu->kvm))
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 4ed79c939fb4..92840c06fcd7 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -71,6 +71,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
>  	kref_init(&irq->refcount);
>  	irq->intid = intid;
>  	irq->target_vcpu = vcpu;
> +	irq->group = 1;
>  
>  	spin_lock_irqsave(&dist->lpi_list_lock, flags);
>  
> 

[PATCH 2/4] KVM: arm/arm64: vgic: Add group field to struct irq

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> In preparation for proper group 0 and group 1 support in the vgic, we
> add a field in the struct irq to store the group of all interrupts.
> 
> We initialize the group to group 0 when emulating GICv2 and to group 1
> when emulating GICv3, just like we treat them today.  LPIs are always
> group 1.  We also continue to ignore writes from the guest, preserving
> existing functionality, for now.
> 
> Finally, we also add this field to the vgic debug logic to show the
> group for all interrupts.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  include/kvm/arm_vgic.h         |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
>  virt/kvm/arm/vgic/vgic-init.c  | 11 +++++++++--
>  virt/kvm/arm/vgic/vgic-its.c   |  1 +
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index cfdd2484cc42..d2d578d099de 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -133,6 +133,7 @@ struct vgic_irq {
>  	u8 source;			/* GICv2 SGIs only */
>  	u8 active_source;		/* GICv2 SGIs only */
>  	u8 priority;
> +	u8 group;			/* 0 == group 0, 1 == group 1 */
>  	enum vgic_irq_config config;	/* Level or edge */
>  
>  	/*
> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> index c589d4c2b478..d3a403f63010 100644
> --- a/virt/kvm/arm/vgic/vgic-debug.c
> +++ b/virt/kvm/arm/vgic/vgic-debug.c
> @@ -148,6 +148,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)
>  
>  	seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
>  	seq_printf(s, "E=enabled, H=hw, C=config (level=1, edge=0)\n");
> +	seq_printf(s, "G=group\n");
>  }
>  
>  static void print_header(struct seq_file *s, struct vgic_irq *irq,
> @@ -162,8 +163,8 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>  	}
>  
>  	seq_printf(s, "\n");
> -	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
> -	seq_printf(s, "---------------------------------------------------------------\n");
> +	seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHCG     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
> +	seq_printf(s, "----------------------------------------------------------------\n");
>  }
>  
>  static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> @@ -182,7 +183,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>  
>  	seq_printf(s, "       %s %4d "
>  		      "    %2d "
> -		      "%d%d%d%d%d%d "
> +		      "%d%d%d%d%d%d%d "
>  		      "%8d "
>  		      "%8x "
>  		      " %2x "
> @@ -197,6 +198,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>  			irq->enabled,
>  			irq->hw,
>  			irq->config == VGIC_CONFIG_LEVEL,
> +			irq->group,
>  			irq->hwintid,
>  			irq->mpidr,
>  			irq->source,
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 2673efce65f3..146a1bbb05c8 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -175,10 +175,13 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  		irq->vcpu = NULL;
>  		irq->target_vcpu = vcpu0;
>  		kref_init(&irq->refcount);
> -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
>  			irq->targets = 0;
> -		else
> +			irq->group = 0;
> +		} else {
>  			irq->mpidr = 0;
> +			irq->group = 1;
> +		}
>  	}
>  	return 0;
>  }
> @@ -227,6 +230,10 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  			/* PPIs */
>  			irq->config = VGIC_CONFIG_LEVEL;
>  		}
> +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +			irq->group = 0;
> +		else
> +			irq->group = 1;
>  	}
>  
>  	if (!irqchip_in_kernel(vcpu->kvm))
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 4ed79c939fb4..92840c06fcd7 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -71,6 +71,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
>  	kref_init(&irq->refcount);
>  	irq->intid = intid;
>  	irq->target_vcpu = vcpu;
> +	irq->group = 1;
>  
>  	spin_lock_irqsave(&dist->lpi_list_lock, flags);
>  
> 

Re: [PATCH 3/4] KVM: arm/arm64: Signal IRQs using their configured group

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> Now when we have a group configuration on the struct IRQ, use this state
> when populating the LR and signaling interrupts as either group 0 or
> group 1 to the VM.  Depending on the model of the emulated GIC, and the
> guest's configuration of the VMCR, interrupts may be signaled as IRQs or
> FIQs to the VM.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  include/linux/irqchip/arm-gic.h | 1 +
>  virt/kvm/arm/vgic/vgic-v2.c     | 3 +++
>  virt/kvm/arm/vgic/vgic-v3.c     | 6 +-----
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 68d8b1f73682..3fa74b917a82 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -94,6 +94,7 @@
>  #define GICH_LR_PENDING_BIT		(1 << 28)
>  #define GICH_LR_ACTIVE_BIT		(1 << 29)
>  #define GICH_LR_EOI			(1 << 19)
> +#define GICH_LR_GROUP1			(1 << 30)
>  #define GICH_LR_HW			(1 << 31)
>  
>  #define GICH_VMCR_ENABLE_GRP0_SHIFT	0
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index a5f2e44f1c33..df5e6a6e3186 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -159,6 +159,9 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  		}
>  	}
>  
> +	if (irq->group)
> +		val |= GICH_LR_GROUP1;
> +
>  	if (irq->hw) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index ff7dc890941a..4f64228e4b3d 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -197,11 +197,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
>  		irq->line_level = false;
>  
> -	/*
> -	 * We currently only support Group1 interrupts, which is a
> -	 * known defect. This needs to be addressed at some point.
> -	 */
> -	if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +	if (irq->group)
>  		val |= ICH_LR_GROUP;
>  
>  	val |= (u64)irq->priority << ICH_LR_PRIORITY_SHIFT;
> 

[PATCH 3/4] KVM: arm/arm64: Signal IRQs using their configured group

[Andre Przywara]

Hi,

On 24/06/18 23:11, Christoffer Dall wrote:
> Now when we have a group configuration on the struct IRQ, use this state
> when populating the LR and signaling interrupts as either group 0 or
> group 1 to the VM.  Depending on the model of the emulated GIC, and the
> guest's configuration of the VMCR, interrupts may be signaled as IRQs or
> FIQs to the VM.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  include/linux/irqchip/arm-gic.h | 1 +
>  virt/kvm/arm/vgic/vgic-v2.c     | 3 +++
>  virt/kvm/arm/vgic/vgic-v3.c     | 6 +-----
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 68d8b1f73682..3fa74b917a82 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -94,6 +94,7 @@
>  #define GICH_LR_PENDING_BIT		(1 << 28)
>  #define GICH_LR_ACTIVE_BIT		(1 << 29)
>  #define GICH_LR_EOI			(1 << 19)
> +#define GICH_LR_GROUP1			(1 << 30)
>  #define GICH_LR_HW			(1 << 31)
>  
>  #define GICH_VMCR_ENABLE_GRP0_SHIFT	0
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index a5f2e44f1c33..df5e6a6e3186 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -159,6 +159,9 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  		}
>  	}
>  
> +	if (irq->group)
> +		val |= GICH_LR_GROUP1;
> +
>  	if (irq->hw) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index ff7dc890941a..4f64228e4b3d 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -197,11 +197,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
>  		irq->line_level = false;
>  
> -	/*
> -	 * We currently only support Group1 interrupts, which is a
> -	 * known defect. This needs to be addressed at some point.
> -	 */
> -	if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +	if (irq->group)
>  		val |= ICH_LR_GROUP;
>  
>  	val |= (u64)irq->priority << ICH_LR_PRIORITY_SHIFT;
> 

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:59:06AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 24/06/18 23:10, Christoffer Dall wrote:
> > This small series addresses a peculiarity of the current VGIC
> > implementation, namely that we don't support interrupt grouping.
> 
> Nice one and seems straight forward enough.
> Just curious: Were there any complaints or even users of FIQs and/or
> interrupt grouping?
> 

No, but the behavior wasn't strictly architecturally compliant so it was
on the table for the GICv2-on-GICv3 fixes that Marc was working on a
while back.

> > KVM either implements a GICv2 without support for the security
> > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > security extensions, group 0 interrupts can be configured to be either
> > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > and group 0 interrupts are always FIQs, and there is no concept of
> > secure vs. non-secure group 1 interrupts when DS=1.
> > 
> > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > geust that they were group 1.  The first patch changes this behavior,
> > which seems to have no effect on no known guests, but still.
> > 
> > The remaining patches introduce proper interrupt grouping support, along
> > with MMIO accessors for the VM and userspace to retrieve and set the
> > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > interrupts as per the architecture, and there is no way to modify this
> > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > commands).
> > 
> > Lightly tested on Seattle, TX1, and the foundation model.  I've run a
> > GICv2 guest on a GICv3 host on the foundation model.
> 
> I take it those were regression tests with Linux guests?
> Do we have any means of testing this with guests which actually use
> different groups or FIQs? Does kvm-unit-tests cover this?

I don't think so.  I did run kvm-unit-tests on a GICv3 system and didn't
observe anything.  I think I forgot to run it on a GICv2 system, I'll
expand coverage for the next round.

> 
> Did you do any tests on 32 bit? I am going to fire up something on my
> Midway later today ...
> 

Yes, I ran it on my cubietruck.

Thanks,
-Christoffer

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:59:06AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 24/06/18 23:10, Christoffer Dall wrote:
> > This small series addresses a peculiarity of the current VGIC
> > implementation, namely that we don't support interrupt grouping.
> 
> Nice one and seems straight forward enough.
> Just curious: Were there any complaints or even users of FIQs and/or
> interrupt grouping?
> 

No, but the behavior wasn't strictly architecturally compliant so it was
on the table for the GICv2-on-GICv3 fixes that Marc was working on a
while back.

> > KVM either implements a GICv2 without support for the security
> > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > security extensions, group 0 interrupts can be configured to be either
> > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > and group 0 interrupts are always FIQs, and there is no concept of
> > secure vs. non-secure group 1 interrupts when DS=1.
> > 
> > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > geust that they were group 1.  The first patch changes this behavior,
> > which seems to have no effect on no known guests, but still.
> > 
> > The remaining patches introduce proper interrupt grouping support, along
> > with MMIO accessors for the VM and userspace to retrieve and set the
> > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > interrupts as per the architecture, and there is no way to modify this
> > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > commands).
> > 
> > Lightly tested on Seattle, TX1, and the foundation model.  I've run a
> > GICv2 guest on a GICv3 host on the foundation model.
> 
> I take it those were regression tests with Linux guests?
> Do we have any means of testing this with guests which actually use
> different groups or FIQs? Does kvm-unit-tests cover this?

I don't think so.  I did run kvm-unit-tests on a GICv3 system and didn't
observe anything.  I think I forgot to run it on a GICv2 system, I'll
expand coverage for the next round.

> 
> Did you do any tests on 32 bit? I am going to fire up something on my
> Midway later today ...
> 

Yes, I ran it on my cubietruck.

Thanks,
-Christoffer

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > This small series addresses a peculiarity of the current VGIC
> > implementation, namely that we don't support interrupt grouping.
> >
> > KVM either implements a GICv2 without support for the security
> > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > security extensions, group 0 interrupts can be configured to be either
> > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > and group 0 interrupts are always FIQs, and there is no concept of
> > secure vs. non-secure group 1 interrupts when DS=1.
> >
> > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > geust that they were group 1.  The first patch changes this behavior,
> > which seems to have no effect on no known guests, but still.
> >
> > The remaining patches introduce proper interrupt grouping support, along
> > with MMIO accessors for the VM and userspace to retrieve and set the
> > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > interrupts as per the architecture, and there is no way to modify this
> > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > commands).
> 
> How do we handle migration compatibility here? For instance if
> we have a guest running on an old kernel with GICv2, presumably
> it reports the GICD_IGROUPRn to userspace as being all-1 (but
> they behave as group 0); then when we load on the new kernel
> the new kernel will honour the IGROUPRn values and the interrupts
> will start behaving like group 1 ? Maybe this works because the
> guest is expecting them to be signalled as IRQ either way...
> 

Interesting.  For GICv2, I think this will actually break, and the
migrated Linux guest won't see any interrupts anymore (Linux only sets
bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
guest is specifically trying to configure group 0 for something.

I suppose this means we need to let userspace request
ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?

Thanks,
-Christoffer

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > This small series addresses a peculiarity of the current VGIC
> > implementation, namely that we don't support interrupt grouping.
> >
> > KVM either implements a GICv2 without support for the security
> > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > security extensions, group 0 interrupts can be configured to be either
> > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > and group 0 interrupts are always FIQs, and there is no concept of
> > secure vs. non-secure group 1 interrupts when DS=1.
> >
> > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > geust that they were group 1.  The first patch changes this behavior,
> > which seems to have no effect on no known guests, but still.
> >
> > The remaining patches introduce proper interrupt grouping support, along
> > with MMIO accessors for the VM and userspace to retrieve and set the
> > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > interrupts as per the architecture, and there is no way to modify this
> > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > commands).
> 
> How do we handle migration compatibility here? For instance if
> we have a guest running on an old kernel with GICv2, presumably
> it reports the GICD_IGROUPRn to userspace as being all-1 (but
> they behave as group 0); then when we load on the new kernel
> the new kernel will honour the IGROUPRn values and the interrupts
> will start behaving like group 1 ? Maybe this works because the
> guest is expecting them to be signalled as IRQ either way...
> 

Interesting.  For GICv2, I think this will actually break, and the
migrated Linux guest won't see any interrupts anymore (Linux only sets
bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
guest is specifically trying to configure group 0 for something.

I suppose this means we need to let userspace request
ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?

Thanks,
-Christoffer

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > This small series addresses a peculiarity of the current VGIC
> > > implementation, namely that we don't support interrupt grouping.
> > >
> > > KVM either implements a GICv2 without support for the security
> > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > security extensions, group 0 interrupts can be configured to be either
> > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > and group 0 interrupts are always FIQs, and there is no concept of
> > > secure vs. non-secure group 1 interrupts when DS=1.
> > >
> > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > geust that they were group 1.  The first patch changes this behavior,
> > > which seems to have no effect on no known guests, but still.
> > >
> > > The remaining patches introduce proper interrupt grouping support, along
> > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > interrupts as per the architecture, and there is no way to modify this
> > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > commands).
> > 
> > How do we handle migration compatibility here? For instance if
> > we have a guest running on an old kernel with GICv2, presumably
> > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > they behave as group 0); then when we load on the new kernel
> > the new kernel will honour the IGROUPRn values and the interrupts
> > will start behaving like group 1 ? Maybe this works because the
> > guest is expecting them to be signalled as IRQ either way...
> > 
> 
> Interesting.  For GICv2, I think this will actually break, and the
> migrated Linux guest won't see any interrupts anymore (Linux only sets
> bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> guest is specifically trying to configure group 0 for something.
> 
> I suppose this means we need to let userspace request
> ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> 
Alternatively, we could deny (fail) migration where the
GICD_IIDR.Revision does not match the source, and just let userspace fix
things up.  Thoughts?

Thanks,
-Christoffer

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > This small series addresses a peculiarity of the current VGIC
> > > implementation, namely that we don't support interrupt grouping.
> > >
> > > KVM either implements a GICv2 without support for the security
> > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > security extensions, group 0 interrupts can be configured to be either
> > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > and group 0 interrupts are always FIQs, and there is no concept of
> > > secure vs. non-secure group 1 interrupts when DS=1.
> > >
> > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > geust that they were group 1.  The first patch changes this behavior,
> > > which seems to have no effect on no known guests, but still.
> > >
> > > The remaining patches introduce proper interrupt grouping support, along
> > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > interrupts as per the architecture, and there is no way to modify this
> > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > commands).
> > 
> > How do we handle migration compatibility here? For instance if
> > we have a guest running on an old kernel with GICv2, presumably
> > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > they behave as group 0); then when we load on the new kernel
> > the new kernel will honour the IGROUPRn values and the interrupts
> > will start behaving like group 1 ? Maybe this works because the
> > guest is expecting them to be signalled as IRQ either way...
> > 
> 
> Interesting.  For GICv2, I think this will actually break, and the
> migrated Linux guest won't see any interrupts anymore (Linux only sets
> bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> guest is specifically trying to configure group 0 for something.
> 
> I suppose this means we need to let userspace request
> ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> 
Alternatively, we could deny (fail) migration where the
GICD_IIDR.Revision does not match the source, and just let userspace fix
things up.  Thoughts?

Thanks,
-Christoffer

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On Mon, 25 Jun 2018 12:56:03 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > > This small series addresses a peculiarity of the current VGIC
> > > > implementation, namely that we don't support interrupt grouping.
> > > >
> > > > KVM either implements a GICv2 without support for the security
> > > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > > security extensions, group 0 interrupts can be configured to be either
> > > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > > and group 0 interrupts are always FIQs, and there is no concept of
> > > > secure vs. non-secure group 1 interrupts when DS=1.
> > > >
> > > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > > geust that they were group 1.  The first patch changes this behavior,
> > > > which seems to have no effect on no known guests, but still.
> > > >
> > > > The remaining patches introduce proper interrupt grouping support, along
> > > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > > interrupts as per the architecture, and there is no way to modify this
> > > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > > commands).
> > > 
> > > How do we handle migration compatibility here? For instance if
> > > we have a guest running on an old kernel with GICv2, presumably
> > > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > > they behave as group 0); then when we load on the new kernel
> > > the new kernel will honour the IGROUPRn values and the interrupts
> > > will start behaving like group 1 ? Maybe this works because the
> > > guest is expecting them to be signalled as IRQ either way...
> > > 
> > 
> > Interesting.  For GICv2, I think this will actually break, and the
> > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > guest is specifically trying to configure group 0 for something.
> > 
> > I suppose this means we need to let userspace request
> > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > 
> Alternatively, we could deny (fail) migration where the
> GICD_IIDR.Revision does not match the source, and just let userspace fix
> things up.  Thoughts?

That's exactly the kind of thing I had in mind when I suggested to
bump up the IIDR. It allows us to push the knowledge of the quirks to
both the guest and userspace, instead of adding more esoteric API
features.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On Mon, 25 Jun 2018 12:56:03 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > > This small series addresses a peculiarity of the current VGIC
> > > > implementation, namely that we don't support interrupt grouping.
> > > >
> > > > KVM either implements a GICv2 without support for the security
> > > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > > security extensions, group 0 interrupts can be configured to be either
> > > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > > and group 0 interrupts are always FIQs, and there is no concept of
> > > > secure vs. non-secure group 1 interrupts when DS=1.
> > > >
> > > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > > geust that they were group 1.  The first patch changes this behavior,
> > > > which seems to have no effect on no known guests, but still.
> > > >
> > > > The remaining patches introduce proper interrupt grouping support, along
> > > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > > interrupts as per the architecture, and there is no way to modify this
> > > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > > commands).
> > > 
> > > How do we handle migration compatibility here? For instance if
> > > we have a guest running on an old kernel with GICv2, presumably
> > > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > > they behave as group 0); then when we load on the new kernel
> > > the new kernel will honour the IGROUPRn values and the interrupts
> > > will start behaving like group 1 ? Maybe this works because the
> > > guest is expecting them to be signalled as IRQ either way...
> > > 
> > 
> > Interesting.  For GICv2, I think this will actually break, and the
> > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > guest is specifically trying to configure group 0 for something.
> > 
> > I suppose this means we need to let userspace request
> > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > 
> Alternatively, we could deny (fail) migration where the
> GICD_IIDR.Revision does not match the source, and just let userspace fix
> things up.  Thoughts?

That's exactly the kind of thing I had in mind when I suggested to
bump up the IIDR. It allows us to push the knowledge of the quirks to
both the guest and userspace, instead of adding more esoteric API
features.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> On Mon, 25 Jun 2018 12:56:03 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > > > This small series addresses a peculiarity of the current VGIC
> > > > > implementation, namely that we don't support interrupt grouping.
> > > > >
> > > > > KVM either implements a GICv2 without support for the security
> > > > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > > > security extensions, group 0 interrupts can be configured to be either
> > > > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > > > and group 0 interrupts are always FIQs, and there is no concept of
> > > > > secure vs. non-secure group 1 interrupts when DS=1.
> > > > >
> > > > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > > > geust that they were group 1.  The first patch changes this behavior,
> > > > > which seems to have no effect on no known guests, but still.
> > > > >
> > > > > The remaining patches introduce proper interrupt grouping support, along
> > > > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > > > interrupts as per the architecture, and there is no way to modify this
> > > > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > > > commands).
> > > >
> > > > How do we handle migration compatibility here? For instance if
> > > > we have a guest running on an old kernel with GICv2, presumably
> > > > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > > > they behave as group 0); then when we load on the new kernel
> > > > the new kernel will honour the IGROUPRn values and the interrupts
> > > > will start behaving like group 1 ? Maybe this works because the
> > > > guest is expecting them to be signalled as IRQ either way...
> > > >
> > >
> > > Interesting.  For GICv2, I think this will actually break, and the
> > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > guest is specifically trying to configure group 0 for something.
> > >
> > > I suppose this means we need to let userspace request
> > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > >
> > Alternatively, we could deny (fail) migration where the
> > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > things up.  Thoughts?
>
> That's exactly the kind of thing I had in mind when I suggested to
> bump up the IIDR. It allows us to push the knowledge of the quirks to
> both the guest and userspace, instead of adding more esoteric API
> features.
>

The problem with that is that upgrading the kernel (but not QEMU) on
your destination machine now breaks migration.  Is that acceptable?


Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> On Mon, 25 Jun 2018 12:56:03 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > On 24 June 2018 at 23:10, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > > > This small series addresses a peculiarity of the current VGIC
> > > > > implementation, namely that we don't support interrupt grouping.
> > > > >
> > > > > KVM either implements a GICv2 without support for the security
> > > > > extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
> > > > > security extensions, group 0 interrupts can be configured to be either
> > > > > signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
> > > > > always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
> > > > > and group 0 interrupts are always FIQs, and there is no concept of
> > > > > secure vs. non-secure group 1 interrupts when DS=1.
> > > > >
> > > > > We were treating all interrupts on GICv2 as group 0, but yet telling the
> > > > > geust that they were group 1.  The first patch changes this behavior,
> > > > > which seems to have no effect on no known guests, but still.
> > > > >
> > > > > The remaining patches introduce proper interrupt grouping support, along
> > > > > with MMIO accessors for the VM and userspace to retrieve and set the
> > > > > which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
> > > > > interrupts as per the architecture, and there is no way to modify this
> > > > > configuration (no IGROUPR registers for LPIs or equivalent ITS
> > > > > commands).
> > > >
> > > > How do we handle migration compatibility here? For instance if
> > > > we have a guest running on an old kernel with GICv2, presumably
> > > > it reports the GICD_IGROUPRn to userspace as being all-1 (but
> > > > they behave as group 0); then when we load on the new kernel
> > > > the new kernel will honour the IGROUPRn values and the interrupts
> > > > will start behaving like group 1 ? Maybe this works because the
> > > > guest is expecting them to be signalled as IRQ either way...
> > > >
> > >
> > > Interesting.  For GICv2, I think this will actually break, and the
> > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > guest is specifically trying to configure group 0 for something.
> > >
> > > I suppose this means we need to let userspace request
> > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > >
> > Alternatively, we could deny (fail) migration where the
> > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > things up.  Thoughts?
>
> That's exactly the kind of thing I had in mind when I suggested to
> bump up the IIDR. It allows us to push the knowledge of the quirks to
> both the guest and userspace, instead of adding more esoteric API
> features.
>

The problem with that is that upgrading the kernel (but not QEMU) on
your destination machine now breaks migration.  Is that acceptable?


Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On Fri, 29 Jun 2018 10:28:16 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> > On Mon, 25 Jun 2018 12:56:03 +0100,
> > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > 
> > > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > Interesting.  For GICv2, I think this will actually break, and the
> > > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > > guest is specifically trying to configure group 0 for something.
> > > > 
> > > > I suppose this means we need to let userspace request
> > > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > > > 
> > > Alternatively, we could deny (fail) migration where the
> > > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > > things up.  Thoughts?
> > 
> > That's exactly the kind of thing I had in mind when I suggested to
> > bump up the IIDR. It allows us to push the knowledge of the quirks to
> > both the guest and userspace, instead of adding more esoteric API
> > features.
> > 
> 
> The problem with that is that upgrading the kernel (but not QEMU) on
> your destination machine now breaks migration.  Is that acceptable?

Each time we add a new sysreg, we do break migration as well (in the
other direction though), and I'm not sure we can do much about it
(that's a QEMU policy, and not the kernel's).

What we could do is to allow IIDR to be written from userspace, and
pick one behaviour or the other. This would allow compatibility in the
forward direction, at least, and we have the ITS as a precedent for
this behaviour.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On Fri, 29 Jun 2018 10:28:16 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> > On Mon, 25 Jun 2018 12:56:03 +0100,
> > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > 
> > > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > Interesting.  For GICv2, I think this will actually break, and the
> > > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > > guest is specifically trying to configure group 0 for something.
> > > > 
> > > > I suppose this means we need to let userspace request
> > > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > > > 
> > > Alternatively, we could deny (fail) migration where the
> > > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > > things up.  Thoughts?
> > 
> > That's exactly the kind of thing I had in mind when I suggested to
> > bump up the IIDR. It allows us to push the knowledge of the quirks to
> > both the guest and userspace, instead of adding more esoteric API
> > features.
> > 
> 
> The problem with that is that upgrading the kernel (but not QEMU) on
> your destination machine now breaks migration.  Is that acceptable?

Each time we add a new sysreg, we do break migration as well (in the
other direction though), and I'm not sure we can do much about it
(that's a QEMU policy, and not the kernel's).

What we could do is to allow IIDR to be written from userspace, and
pick one behaviour or the other. This would allow compatibility in the
forward direction, at least, and we have the ITS as a precedent for
this behaviour.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Fri, Jun 29, 2018 at 11:12:44AM +0100, Marc Zyngier wrote:
> On Fri, 29 Jun 2018 10:28:16 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> > > On Mon, 25 Jun 2018 12:56:03 +0100,
> > > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > >
> > > > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > > Interesting.  For GICv2, I think this will actually break, and the
> > > > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > > > guest is specifically trying to configure group 0 for something.
> > > > >
> > > > > I suppose this means we need to let userspace request
> > > > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > > > >
> > > > Alternatively, we could deny (fail) migration where the
> > > > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > > > things up.  Thoughts?
> > >
> > > That's exactly the kind of thing I had in mind when I suggested to
> > > bump up the IIDR. It allows us to push the knowledge of the quirks to
> > > both the guest and userspace, instead of adding more esoteric API
> > > features.
> > >
> >
> > The problem with that is that upgrading the kernel (but not QEMU) on
> > your destination machine now breaks migration.  Is that acceptable?
>
> Each time we add a new sysreg, we do break migration as well (in the
> other direction though), and I'm not sure we can do much about it
> (that's a QEMU policy, and not the kernel's).
>
> What we could do is to allow IIDR to be written from userspace, and
> pick one behaviour or the other. This would allow compatibility in the
> forward direction, at least, and we have the ITS as a precedent for
> this behaviour.
>

To make things work without imposing an ordering requirement on the
order of GIC register restore, we could then zero all groups when
writing the old IIDR and ignore future writes to GICD_IGROUPR from that
point, and otherwise not do anything for the IIDR.  Esssentially doing
the conversion in the kernel.  Might not be too bad actually.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 0/4] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

On Fri, Jun 29, 2018 at 11:12:44AM +0100, Marc Zyngier wrote:
> On Fri, 29 Jun 2018 10:28:16 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Fri, Jun 29, 2018 at 10:11:27AM +0100, Marc Zyngier wrote:
> > > On Mon, 25 Jun 2018 12:56:03 +0100,
> > > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > >
> > > > On Mon, Jun 25, 2018 at 12:13:47PM +0200, Christoffer Dall wrote:
> > > > > On Mon, Jun 25, 2018 at 10:52:41AM +0100, Peter Maydell wrote:
> > > > > Interesting.  For GICv2, I think this will actually break, and the
> > > > > migrated Linux guest won't see any interrupts anymore (Linux only sets
> > > > > bit 0 in GICD_CTLR).  For GICv3, everything stays group 1 unless the
> > > > > guest is specifically trying to configure group 0 for something.
> > > > >
> > > > > I suppose this means we need to let userspace request
> > > > > ARCH_COMPLIANT_GICV2 and otherwise ignore userspace writes to IGROUPR?
> > > > >
> > > > Alternatively, we could deny (fail) migration where the
> > > > GICD_IIDR.Revision does not match the source, and just let userspace fix
> > > > things up.  Thoughts?
> > >
> > > That's exactly the kind of thing I had in mind when I suggested to
> > > bump up the IIDR. It allows us to push the knowledge of the quirks to
> > > both the guest and userspace, instead of adding more esoteric API
> > > features.
> > >
> >
> > The problem with that is that upgrading the kernel (but not QEMU) on
> > your destination machine now breaks migration.  Is that acceptable?
>
> Each time we add a new sysreg, we do break migration as well (in the
> other direction though), and I'm not sure we can do much about it
> (that's a QEMU policy, and not the kernel's).
>
> What we could do is to allow IIDR to be written from userspace, and
> pick one behaviour or the other. This would allow compatibility in the
> forward direction, at least, and we have the ITS as a precedent for
> this behaviour.
>

To make things work without imposing an ordering requirement on the
order of GIC register restore, we could then zero all groups when
writing the old IIDR and ignore future writes to GICD_IGROUPR from that
point, and otherwise not do anything for the IIDR.  Esssentially doing
the conversion in the kernel.  Might not be too bad actually.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.