[PATCH v4 00/10] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[PATCH v4 00/10] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

This 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
guest 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).

Tested on Seattle, TX1, and the foundation model.  I've run a
GICv2 guest on a GICv3 host on the foundation model.  I've tested
migration on Seattle, and migration from an old kernel to a newer kernel
with an unmodified QEMU binary continues to work.  I also tested a
modified userspace that sets some interrupts to group 1, and that is
reflected in the vgic debug output.

Applies to v4.18-rc5.

Changes since v3:
 - Rebased to v4.18-rc5
 - Reworked documentation to clarify order of register restore and
   include GICv3
 - Re-ordered patches to add infrastructure for returning uaccess write
   errors first, and factored out GIC_IIDR write checking to a separate
   patch.
 - Added a return 0; in the GICv2 GICD_IIDR uaccess write handler and
   fixed a typo in the comment.

Changes since v2:
 - Patch 6 doesn't allow configuraiton of groups from userspace for
   GICv2.
 - Patches 7 through 9 are new in v3 of this series, and prevents
   userspace from restoring an incompatible IIDR without seeing an error
   in the future, and supports legacy userspace migration from old to
   new kernels.

Changes since v1:
 - Bumped implementation revision field in GICD_IIDR along with changes
 - Changed logic in init code to correctly detect vgic emulation type
 - Rebased to v4.18-rc3

Christoffer Dall (10):
  KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3
  KVM: arm/arm64: vgic: Keep track of implementation revision
  KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero
  KVM: arm/arm64: vgic: Add group field to struct irq
  KVM: arm/arm64: vgic: Signal IRQs using their configured group
  KVM: arm/arm64: vgic: Permit uaccess writes to return errors
  KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR
    writes
  KVM: arm/arm64: vgic: Allow configuration of interrupt groups
  KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
  KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 +++
 Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++---
 include/kvm/arm_vgic.h                            |  7 +++
 include/linux/irqchip/arm-gic-v3.h                | 10 ++++
 include/linux/irqchip/arm-gic.h                   | 11 ++++
 virt/kvm/arm/vgic/vgic-debug.c                    |  8 +--
 virt/kvm/arm/vgic/vgic-init.c                     | 20 ++++++-
 virt/kvm/arm/vgic/vgic-its.c                      |  1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c                  | 63 ++++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-mmio-v3.c                  | 53 ++++++++++++++-----
 virt/kvm/arm/vgic/vgic-mmio.c                     | 56 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.h                     | 25 ++++++---
 virt/kvm/arm/vgic/vgic-v2.c                       |  3 ++
 virt/kvm/arm/vgic/vgic-v3.c                       |  6 +--
 14 files changed, 237 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH v4 00/10] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

This 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
guest 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).

Tested on Seattle, TX1, and the foundation model.  I've run a
GICv2 guest on a GICv3 host on the foundation model.  I've tested
migration on Seattle, and migration from an old kernel to a newer kernel
with an unmodified QEMU binary continues to work.  I also tested a
modified userspace that sets some interrupts to group 1, and that is
reflected in the vgic debug output.

Applies to v4.18-rc5.

Changes since v3:
 - Rebased to v4.18-rc5
 - Reworked documentation to clarify order of register restore and
   include GICv3
 - Re-ordered patches to add infrastructure for returning uaccess write
   errors first, and factored out GIC_IIDR write checking to a separate
   patch.
 - Added a return 0; in the GICv2 GICD_IIDR uaccess write handler and
   fixed a typo in the comment.

Changes since v2:
 - Patch 6 doesn't allow configuraiton of groups from userspace for
   GICv2.
 - Patches 7 through 9 are new in v3 of this series, and prevents
   userspace from restoring an incompatible IIDR without seeing an error
   in the future, and supports legacy userspace migration from old to
   new kernels.

Changes since v1:
 - Bumped implementation revision field in GICD_IIDR along with changes
 - Changed logic in init code to correctly detect vgic emulation type
 - Rebased to v4.18-rc3

Christoffer Dall (10):
  KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3
  KVM: arm/arm64: vgic: Keep track of implementation revision
  KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero
  KVM: arm/arm64: vgic: Add group field to struct irq
  KVM: arm/arm64: vgic: Signal IRQs using their configured group
  KVM: arm/arm64: vgic: Permit uaccess writes to return errors
  KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR
    writes
  KVM: arm/arm64: vgic: Allow configuration of interrupt groups
  KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
  KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 +++
 Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++---
 include/kvm/arm_vgic.h                            |  7 +++
 include/linux/irqchip/arm-gic-v3.h                | 10 ++++
 include/linux/irqchip/arm-gic.h                   | 11 ++++
 virt/kvm/arm/vgic/vgic-debug.c                    |  8 +--
 virt/kvm/arm/vgic/vgic-init.c                     | 20 ++++++-
 virt/kvm/arm/vgic/vgic-its.c                      |  1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c                  | 63 ++++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-mmio-v3.c                  | 53 ++++++++++++++-----
 virt/kvm/arm/vgic/vgic-mmio.c                     | 56 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.h                     | 25 ++++++---
 virt/kvm/arm/vgic/vgic-v2.c                       |  3 ++
 virt/kvm/arm/vgic/vgic-v3.c                       |  6 +--
 14 files changed, 237 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH v4 01/10] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3

[Christoffer Dall]

Instead of hardcoding the shifts and masks in the GICD_IIDR register
emulation, let's add the definition of these fields to the GIC header
files and use them.

This will make things more obvious when we're going to bump the revision
in the IIDR when we'll make guest-visible changes to the implementation.

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

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index cbb872c..b22f9df 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -61,6 +61,16 @@
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_IIDR_IMPLEMENTER_SHIFT	0
+#define GICD_IIDR_IMPLEMENTER_MASK	(0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT	12
+#define GICD_IIDR_REVISION_MASK		(0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT		16
+#define GICD_IIDR_VARIANT_MASK		(0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT	24
+#define GICD_IIDR_PRODUCT_ID_MASK	(0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 /*
  * In systems with a single security state (what we emulate in KVM)
  * the meaning of the interrupt group enable bits is slightly different
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 68d8b1f..484f5bf 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -71,6 +71,16 @@
 					(GICD_INT_DEF_PRI << 8) |\
 					GICD_INT_DEF_PRI)
 
+#define GICD_IIDR_IMPLEMENTER_SHIFT	0
+#define GICD_IIDR_IMPLEMENTER_MASK	(0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT	12
+#define GICD_IIDR_REVISION_MASK		(0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT		16
+#define GICD_IIDR_VARIANT_MASK		(0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT	24
+#define GICD_IIDR_PRODUCT_ID_MASK	(0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 #define GICH_HCR			0x0
 #define GICH_VTR			0x4
 #define GICH_VMCR			0x8
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ffc587b..af44e569 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -37,7 +37,8 @@ static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 		value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
 		break;
 	case GIC_DIST_IIDR:
-		value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
 		return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 2877840..c03f424 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -81,7 +81,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		}
 		break;
 	case GICD_IIDR:
-		value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
 		return 0;
-- 
2.7.4

[PATCH v4 01/10] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3

[Christoffer Dall]

Instead of hardcoding the shifts and masks in the GICD_IIDR register
emulation, let's add the definition of these fields to the GIC header
files and use them.

This will make things more obvious when we're going to bump the revision
in the IIDR when we'll make guest-visible changes to the implementation.

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

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index cbb872c..b22f9df 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -61,6 +61,16 @@
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_IIDR_IMPLEMENTER_SHIFT	0
+#define GICD_IIDR_IMPLEMENTER_MASK	(0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT	12
+#define GICD_IIDR_REVISION_MASK		(0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT		16
+#define GICD_IIDR_VARIANT_MASK		(0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT	24
+#define GICD_IIDR_PRODUCT_ID_MASK	(0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 /*
  * In systems with a single security state (what we emulate in KVM)
  * the meaning of the interrupt group enable bits is slightly different
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 68d8b1f..484f5bf 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -71,6 +71,16 @@
 					(GICD_INT_DEF_PRI << 8) |\
 					GICD_INT_DEF_PRI)
 
+#define GICD_IIDR_IMPLEMENTER_SHIFT	0
+#define GICD_IIDR_IMPLEMENTER_MASK	(0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT	12
+#define GICD_IIDR_REVISION_MASK		(0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT		16
+#define GICD_IIDR_VARIANT_MASK		(0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT	24
+#define GICD_IIDR_PRODUCT_ID_MASK	(0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 #define GICH_HCR			0x0
 #define GICH_VTR			0x4
 #define GICH_VMCR			0x8
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ffc587b..af44e569 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -37,7 +37,8 @@ static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 		value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
 		break;
 	case GIC_DIST_IIDR:
-		value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
 		return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 2877840..c03f424 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -81,7 +81,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		}
 		break;
 	case GICD_IIDR:
-		value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
 		return 0;
-- 
2.7.4

[PATCH v4 02/10] KVM: arm/arm64: vgic: Keep track of implementation revision

[Christoffer Dall]

As we are about to tweak implementation aspects of the VGIC emulation,
while still preserving some level of backwards compatibility support,
add a field to keep track of the implementation revision field which is
reported to the VM and to userspace.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cfdd248..7e64c46 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -217,6 +217,9 @@ struct vgic_dist {
 	/* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
 	u32			vgic_model;
 
+	/* Implementation revision as reported in the GICD_IIDR */
+	u32			implementation_rev;
+
 	/* Do injected MSIs require an additional device ID? */
 	bool			msis_require_devid;
 
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index b714179..8b6fc45 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,6 +298,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
+	dist->implementation_rev = 0;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index af44e569..f0c5351 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -25,19 +25,21 @@
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
+	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
 	u32 value;
 
 	switch (addr & 0x0c) {
 	case GIC_DIST_CTRL:
-		value = vcpu->kvm->arch.vgic.enabled ? GICD_ENABLE : 0;
+		value = vgic->enabled ? GICD_ENABLE : 0;
 		break;
 	case GIC_DIST_CTR:
-		value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+		value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
 		value = (value >> 5) - 1;
 		value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
 		break;
 	case GIC_DIST_IIDR:
 		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
 			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index c03f424..ebe10a0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -62,16 +62,17 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
+	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
 	u32 value = 0;
 
 	switch (addr & 0x0c) {
 	case GICD_CTLR:
-		if (vcpu->kvm->arch.vgic.enabled)
+		if (vgic->enabled)
 			value |= GICD_CTLR_ENABLE_SS_G1;
 		value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
 		break;
 	case GICD_TYPER:
-		value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+		value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
 		value = (value >> 5) - 1;
 		if (vgic_has_its(vcpu->kvm)) {
 			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
@@ -82,6 +83,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		break;
 	case GICD_IIDR:
 		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
 			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
-- 
2.7.4

[PATCH v4 02/10] KVM: arm/arm64: vgic: Keep track of implementation revision

[Christoffer Dall]

As we are about to tweak implementation aspects of the VGIC emulation,
while still preserving some level of backwards compatibility support,
add a field to keep track of the implementation revision field which is
reported to the VM and to userspace.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cfdd248..7e64c46 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -217,6 +217,9 @@ struct vgic_dist {
 	/* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
 	u32			vgic_model;
 
+	/* Implementation revision as reported in the GICD_IIDR */
+	u32			implementation_rev;
+
 	/* Do injected MSIs require an additional device ID? */
 	bool			msis_require_devid;
 
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index b714179..8b6fc45 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,6 +298,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
+	dist->implementation_rev = 0;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index af44e569..f0c5351 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -25,19 +25,21 @@
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
+	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
 	u32 value;
 
 	switch (addr & 0x0c) {
 	case GIC_DIST_CTRL:
-		value = vcpu->kvm->arch.vgic.enabled ? GICD_ENABLE : 0;
+		value = vgic->enabled ? GICD_ENABLE : 0;
 		break;
 	case GIC_DIST_CTR:
-		value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+		value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
 		value = (value >> 5) - 1;
 		value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
 		break;
 	case GIC_DIST_IIDR:
 		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
 			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index c03f424..ebe10a0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -62,16 +62,17 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
+	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
 	u32 value = 0;
 
 	switch (addr & 0x0c) {
 	case GICD_CTLR:
-		if (vcpu->kvm->arch.vgic.enabled)
+		if (vgic->enabled)
 			value |= GICD_CTLR_ENABLE_SS_G1;
 		value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
 		break;
 	case GICD_TYPER:
-		value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+		value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
 		value = (value >> 5) - 1;
 		if (vgic_has_its(vcpu->kvm)) {
 			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
@@ -82,6 +83,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		break;
 	case GICD_IIDR:
 		value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+			(vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
 			(IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
 		break;
 	default:
-- 
2.7.4

[PATCH v4 03/10] KVM: arm/arm64: vgic: 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-init.c    | 2 +-
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 8b6fc45..230c922 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
-	dist->implementation_rev = 0;
+	dist->implementation_rev = 1;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index f0c5351..db646f1 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -22,6 +22,12 @@
 #include "vgic.h"
 #include "vgic-mmio.h"
 
+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ */
+
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -365,7 +371,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.7.4

[PATCH v4 03/10] KVM: arm/arm64: vgic: 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-init.c    | 2 +-
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 8b6fc45..230c922 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
-	dist->implementation_rev = 0;
+	dist->implementation_rev = 1;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index f0c5351..db646f1 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -22,6 +22,12 @@
 #include "vgic.h"
 #include "vgic-mmio.h"
 
+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ */
+
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -365,7 +371,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.7.4

[PATCH v4 04/10] 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  | 19 +++++++++++++++++--
 virt/kvm/arm/vgic/vgic-its.c   |  1 +
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7e64c46..c661d0e 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 c589d4c..d3a403f 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 230c922..a7c19cd 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,18 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
+
+		/*
+		 * GICv3 can only be created via the KVM_DEVICE_CREATE API and
+		 * so we always know the emulation type at this point as it's
+		 * either explicitly configured as GICv3, or explicitly
+		 * configured as GICv2, or not configured yet which also
+		 * implies GICv2.
+		 */
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+			irq->group = 1;
+		else
+			irq->group = 0;
 	}
 
 	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 4ed79c9..92840c0 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.7.4

[PATCH v4 04/10] 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  | 19 +++++++++++++++++--
 virt/kvm/arm/vgic/vgic-its.c   |  1 +
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7e64c46..c661d0e 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 c589d4c..d3a403f 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 230c922..a7c19cd 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,18 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
+
+		/*
+		 * GICv3 can only be created via the KVM_DEVICE_CREATE API and
+		 * so we always know the emulation type at this point as it's
+		 * either explicitly configured as GICv3, or explicitly
+		 * configured as GICv2, or not configured yet which also
+		 * implies GICv2.
+		 */
+		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+			irq->group = 1;
+		else
+			irq->group = 0;
 	}
 
 	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 4ed79c9..92840c0 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.7.4

[PATCH v4 05/10] KVM: arm/arm64: vgic: 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 484f5bf..6c4aaf0 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -104,6 +104,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 a5f2e44..df5e6a6 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 cdce653..530b849 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.7.4

[PATCH v4 05/10] KVM: arm/arm64: vgic: 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 484f5bf..6c4aaf0 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -104,6 +104,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 a5f2e44..df5e6a6 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 cdce653..530b849 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.7.4

[PATCH v4 06/10] KVM: arm/arm64: vgic: Permit uaccess writes to return errors

[Christoffer Dall]

Currently we do not allow any vgic mmio write operations to fail, which
makes sense from mmio traps from the guest.  However, we should be able
to report failures to userspace, if userspace writes incompatible values
to read-only registers.  Rework the internal interface to allow errors
to be returned on the write side for userspace writes.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++++++-----
 virt/kvm/arm/vgic/vgic-mmio.c    | 18 +++++++++++++-----
 virt/kvm/arm/vgic/vgic-mmio.h    | 19 +++++++++++--------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ebe10a0..ef57a1a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -249,9 +249,9 @@ static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
 	return value;
 }
 
-static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
-					  gpa_t addr, unsigned int len,
-					  unsigned long val)
+static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
+					 gpa_t addr, unsigned int len,
+					 unsigned long val)
 {
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
@@ -276,6 +276,8 @@ static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
 
 		vgic_put_irq(vcpu->kvm, irq);
 	}
+
+	return 0;
 }
 
 /* We want to avoid outer shareable. */
@@ -468,7 +470,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
 		vgic_mmio_read_pending, vgic_mmio_write_cpending,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+		vgic_mmio_read_raz, vgic_mmio_uaccess_write_wi, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
 		vgic_mmio_read_active, vgic_mmio_write_sactive,
@@ -541,7 +543,7 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
 		vgic_mmio_read_pending, vgic_mmio_write_cpending,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
+		vgic_mmio_read_raz, vgic_mmio_uaccess_write_wi, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISACTIVER0,
 		vgic_mmio_read_active, vgic_mmio_write_sactive,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index ff9655c..e1e7998 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,13 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	/* Ignore */
 }
 
+int vgic_mmio_uaccess_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
+			       unsigned int len, unsigned long val)
+{
+	/* Ignore */
+	return 0;
+}
+
 /*
  * 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.
@@ -363,11 +370,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 	mutex_unlock(&vcpu->kvm->lock);
 }
 
-void vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
+int vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
 				     gpa_t addr, unsigned int len,
 				     unsigned long val)
 {
 	__vgic_mmio_write_cactive(vcpu, addr, len, val);
+	return 0;
 }
 
 static void __vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
@@ -399,11 +407,12 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 	mutex_unlock(&vcpu->kvm->lock);
 }
 
-void vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
+int vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
 				     gpa_t addr, unsigned int len,
 				     unsigned long val)
 {
 	__vgic_mmio_write_sactive(vcpu, addr, len, val);
+	return 0;
 }
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
@@ -735,10 +744,9 @@ static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 
 	r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
 	if (region->uaccess_write)
-		region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
-	else
-		region->write(r_vcpu, addr, sizeof(u32), *val);
+		return region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
 
+	region->write(r_vcpu, addr, sizeof(u32), *val);
 	return 0;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df..9c3d6d3 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -37,8 +37,8 @@ struct vgic_register_region {
 	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
 				      unsigned int len);
 	union {
-		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
-				      unsigned int len, unsigned long val);
+		int (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
+				     unsigned int len, unsigned long val);
 		int (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
 					 gpa_t addr, unsigned int len,
 					 unsigned long val);
@@ -134,6 +134,9 @@ 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);
 
+int vgic_mmio_uaccess_write_wi(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);
 
@@ -167,13 +170,13 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 			     gpa_t addr, unsigned int len,
 			     unsigned long val);
 
-void vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
-				     gpa_t addr, unsigned int len,
-				     unsigned long val);
+int vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
+				    gpa_t addr, unsigned int len,
+				    unsigned long val);
 
-void vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
-				     gpa_t addr, unsigned int len,
-				     unsigned long val);
+int vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
+				    gpa_t addr, unsigned int len,
+				    unsigned long val);
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
 				      gpa_t addr, unsigned int len);
-- 
2.7.4

[PATCH v4 06/10] KVM: arm/arm64: vgic: Permit uaccess writes to return errors

[Christoffer Dall]

Currently we do not allow any vgic mmio write operations to fail, which
makes sense from mmio traps from the guest.  However, we should be able
to report failures to userspace, if userspace writes incompatible values
to read-only registers.  Rework the internal interface to allow errors
to be returned on the write side for userspace writes.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++++++-----
 virt/kvm/arm/vgic/vgic-mmio.c    | 18 +++++++++++++-----
 virt/kvm/arm/vgic/vgic-mmio.h    | 19 +++++++++++--------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ebe10a0..ef57a1a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -249,9 +249,9 @@ static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
 	return value;
 }
 
-static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
-					  gpa_t addr, unsigned int len,
-					  unsigned long val)
+static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
+					 gpa_t addr, unsigned int len,
+					 unsigned long val)
 {
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
@@ -276,6 +276,8 @@ static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
 
 		vgic_put_irq(vcpu->kvm, irq);
 	}
+
+	return 0;
 }
 
 /* We want to avoid outer shareable. */
@@ -468,7 +470,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
 		vgic_mmio_read_pending, vgic_mmio_write_cpending,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+		vgic_mmio_read_raz, vgic_mmio_uaccess_write_wi, 1,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
 		vgic_mmio_read_active, vgic_mmio_write_sactive,
@@ -541,7 +543,7 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
 		vgic_mmio_read_pending, vgic_mmio_write_cpending,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
+		vgic_mmio_read_raz, vgic_mmio_uaccess_write_wi, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISACTIVER0,
 		vgic_mmio_read_active, vgic_mmio_write_sactive,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index ff9655c..e1e7998 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,13 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	/* Ignore */
 }
 
+int vgic_mmio_uaccess_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
+			       unsigned int len, unsigned long val)
+{
+	/* Ignore */
+	return 0;
+}
+
 /*
  * 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.
@@ -363,11 +370,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 	mutex_unlock(&vcpu->kvm->lock);
 }
 
-void vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
+int vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
 				     gpa_t addr, unsigned int len,
 				     unsigned long val)
 {
 	__vgic_mmio_write_cactive(vcpu, addr, len, val);
+	return 0;
 }
 
 static void __vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
@@ -399,11 +407,12 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 	mutex_unlock(&vcpu->kvm->lock);
 }
 
-void vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
+int vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
 				     gpa_t addr, unsigned int len,
 				     unsigned long val)
 {
 	__vgic_mmio_write_sactive(vcpu, addr, len, val);
+	return 0;
 }
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
@@ -735,10 +744,9 @@ static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 
 	r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
 	if (region->uaccess_write)
-		region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
-	else
-		region->write(r_vcpu, addr, sizeof(u32), *val);
+		return region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
 
+	region->write(r_vcpu, addr, sizeof(u32), *val);
 	return 0;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df..9c3d6d3 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -37,8 +37,8 @@ struct vgic_register_region {
 	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
 				      unsigned int len);
 	union {
-		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
-				      unsigned int len, unsigned long val);
+		int (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
+				     unsigned int len, unsigned long val);
 		int (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
 					 gpa_t addr, unsigned int len,
 					 unsigned long val);
@@ -134,6 +134,9 @@ 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);
 
+int vgic_mmio_uaccess_write_wi(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);
 
@@ -167,13 +170,13 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 			     gpa_t addr, unsigned int len,
 			     unsigned long val);
 
-void vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
-				     gpa_t addr, unsigned int len,
-				     unsigned long val);
+int vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
+				    gpa_t addr, unsigned int len,
+				    unsigned long val);
 
-void vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
-				     gpa_t addr, unsigned int len,
-				     unsigned long val);
+int vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
+				    gpa_t addr, unsigned int len,
+				    unsigned long val);
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
 				      gpa_t addr, unsigned int len);
-- 
2.7.4

[PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Christoffer Dall]

If userspace attempts to write a GICD_IIDR that does not match the
kernel version, return an error to userspace.  The intention is to allow
implementation changes inside KVM while avoiding silently breaking
migration resulting in guests not running without any clear indication
of what went wrong.

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

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index db646f1..4f0f2c4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
 	}
 }
 
+static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
+					   gpa_t addr, unsigned int len,
+					   unsigned long val)
+{
+	switch (addr & 0x0c) {
+	case GIC_DIST_IIDR:
+		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
+			return -EINVAL;
+	}
+
+	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
+	return 0;
+}
+
 static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
 				 gpa_t addr, unsigned int len,
 				 unsigned long val)
@@ -367,9 +381,10 @@ static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
 }
 
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
-	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
-		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
-		VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_DIST_CTRL,
+		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc,
+		NULL, vgic_mmio_uaccess_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_ACCESS_32bit),
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ef57a1a..abdb0ec 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -113,6 +113,20 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
 	}
 }
 
+static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
+					   gpa_t addr, unsigned int len,
+					   unsigned long val)
+{
+	switch (addr & 0x0c) {
+	case GICD_IIDR:
+		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
+			return -EINVAL;
+	}
+
+	vgic_mmio_write_v3_misc(vcpu, addr, len, val);
+	return 0;
+}
+
 static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -449,9 +463,10 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
 	}
 
 static const struct vgic_register_region vgic_v3_dist_registers[] = {
-	REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
-		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
-		VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH_UACCESS(GICD_CTLR,
+		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc,
+		NULL, vgic_mmio_uaccess_write_v3_misc,
+		16, VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
 		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),
-- 
2.7.4

[PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Christoffer Dall]

If userspace attempts to write a GICD_IIDR that does not match the
kernel version, return an error to userspace.  The intention is to allow
implementation changes inside KVM while avoiding silently breaking
migration resulting in guests not running without any clear indication
of what went wrong.

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

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index db646f1..4f0f2c4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
 	}
 }
 
+static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
+					   gpa_t addr, unsigned int len,
+					   unsigned long val)
+{
+	switch (addr & 0x0c) {
+	case GIC_DIST_IIDR:
+		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
+			return -EINVAL;
+	}
+
+	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
+	return 0;
+}
+
 static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
 				 gpa_t addr, unsigned int len,
 				 unsigned long val)
@@ -367,9 +381,10 @@ static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
 }
 
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
-	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
-		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
-		VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_DIST_CTRL,
+		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc,
+		NULL, vgic_mmio_uaccess_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_ACCESS_32bit),
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ef57a1a..abdb0ec 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -113,6 +113,20 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
 	}
 }
 
+static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
+					   gpa_t addr, unsigned int len,
+					   unsigned long val)
+{
+	switch (addr & 0x0c) {
+	case GICD_IIDR:
+		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
+			return -EINVAL;
+	}
+
+	vgic_mmio_write_v3_misc(vcpu, addr, len, val);
+	return 0;
+}
+
 static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -449,9 +463,10 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
 	}
 
 static const struct vgic_register_region vgic_v3_dist_registers[] = {
-	REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
-		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
-		VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH_UACCESS(GICD_CTLR,
+		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc,
+		NULL, vgic_mmio_uaccess_write_v3_misc,
+		16, VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
 		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),
-- 
2.7.4

[PATCH v4 08/10] KVM: arm/arm64: vgic: Allow configuration of 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.

This also allows userspace to configure the interrupts groups for GICv3.
We don't allow userspace to write the groups on GICv2 just yet, because
that would result in GICv2 guests not receiving interrupts after
migrating from an older kernel that exposes GICv2 interrupts as group 1.

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

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index a7c19cd..c0c0b88 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -313,7 +313,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
-	dist->implementation_rev = 1;
+	dist->implementation_rev = 2;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 4f0f2c4..ee164f8 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -26,6 +26,8 @@
  * The Revision field in the IIDR have the following meanings:
  *
  * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ * 	       their configured groups.
  */
 
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
@@ -89,6 +91,14 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
+					    gpa_t addr, unsigned int len,
+					    unsigned long val)
+{
+	/* Ignore writes from userspace */
+	return 0;
+}
+
 static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
 				 gpa_t addr, unsigned int len,
 				 unsigned long val)
@@ -386,7 +396,8 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		NULL, vgic_mmio_uaccess_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, vgic_mmio_uaccess_write_v2_group, 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 abdb0ec..88e78b5 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -59,6 +59,13 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
 	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
 }
 
+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ * 	       their configured groups.
+ */
+
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -471,7 +478,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,
@@ -544,7 +551,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,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index e1e7998..f56ff1c 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -47,6 +47,44 @@ int vgic_mmio_uaccess_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	return 0;
 }
 
+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 9c3d6d3..a07f90a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -137,6 +137,12 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 int vgic_mmio_uaccess_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.7.4

[PATCH v4 08/10] KVM: arm/arm64: vgic: Allow configuration of 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.

This also allows userspace to configure the interrupts groups for GICv3.
We don't allow userspace to write the groups on GICv2 just yet, because
that would result in GICv2 guests not receiving interrupts after
migrating from an older kernel that exposes GICv2 interrupts as group 1.

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

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index a7c19cd..c0c0b88 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -313,7 +313,7 @@ int vgic_init(struct kvm *kvm)
 
 	vgic_debug_init(kvm);
 
-	dist->implementation_rev = 1;
+	dist->implementation_rev = 2;
 	dist->initialized = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 4f0f2c4..ee164f8 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -26,6 +26,8 @@
  * The Revision field in the IIDR have the following meanings:
  *
  * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ * 	       their configured groups.
  */
 
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
@@ -89,6 +91,14 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
+					    gpa_t addr, unsigned int len,
+					    unsigned long val)
+{
+	/* Ignore writes from userspace */
+	return 0;
+}
+
 static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
 				 gpa_t addr, unsigned int len,
 				 unsigned long val)
@@ -386,7 +396,8 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
 		NULL, vgic_mmio_uaccess_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, vgic_mmio_uaccess_write_v2_group, 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 abdb0ec..88e78b5 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -59,6 +59,13 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
 	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
 }
 
+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ * 	       their configured groups.
+ */
+
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len)
 {
@@ -471,7 +478,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,
@@ -544,7 +551,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,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index e1e7998..f56ff1c 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -47,6 +47,44 @@ int vgic_mmio_uaccess_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 	return 0;
 }
 
+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 9c3d6d3..a07f90a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -137,6 +137,12 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
 int vgic_mmio_uaccess_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.7.4

[PATCH v4 09/10] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR

[Christoffer Dall]

Simply letting IGROUPR be writable from userspace would break
migration from old kernels to newer kernels, because old kernels
incorrectly report interrupt groups as group 1.  This would not be a big
problem if userspace wrote GICD_IIDR as read from the kernel, because we
could detect the incompatibility and return an error to userspace.
Unfortunately, this is not the case with current userspace
implementations and simply letting IGROUPR be writable from userspace for
an emulated GICv2 silently breaks migration and causes the destination
VM to no longer run after migration.

We now encourage userspace to write the read and expected value of
GICD_IIDR as the first part of a GIC register restore, and if we observe
a write to GICD_IIDR we know that userspace has been updated and has had
a chance to cope with older kernels (VGICv2 IIDR.Revision == 0)
incorrectly reporting interrupts as group 1, and therefore we now allow
groups to be user writable.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h           |  3 +++
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c661d0e..c134790 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -221,6 +221,9 @@ struct vgic_dist {
 	/* Implementation revision as reported in the GICD_IIDR */
 	u32			implementation_rev;
 
+	/* Userspace can write to GICv2 IGROUPR */
+	bool			v2_groups_user_writable;
+
 	/* Do injected MSIs require an additional device ID? */
 	bool			msis_require_devid;
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ee164f8..26654f4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -85,6 +85,18 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 	case GIC_DIST_IIDR:
 		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
 			return -EINVAL;
+
+		/*
+		 * If we observe a write to GICD_IIDR we know that userspace
+		 * has been updated and has had a chance to cope with older
+		 * kernels (VGICv2 IIDR.Revision == 0) incorrectly reporting
+		 * interrupts as group 1, and therefore we now allow groups to
+		 * be user writable.  Doing this by default would break
+		 * migration from old kernels to new kernels with legacy
+		 * userspace.
+		 */
+		vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
+		return 0;
 	}
 
 	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
@@ -95,7 +107,9 @@ static int vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len,
 					    unsigned long val)
 {
-	/* Ignore writes from userspace */
+	if (vcpu->kvm->arch.vgic.v2_groups_user_writable)
+		vgic_mmio_write_group(vcpu, addr, len, val);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH v4 09/10] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR

[Christoffer Dall]

Simply letting IGROUPR be writable from userspace would break
migration from old kernels to newer kernels, because old kernels
incorrectly report interrupt groups as group 1.  This would not be a big
problem if userspace wrote GICD_IIDR as read from the kernel, because we
could detect the incompatibility and return an error to userspace.
Unfortunately, this is not the case with current userspace
implementations and simply letting IGROUPR be writable from userspace for
an emulated GICv2 silently breaks migration and causes the destination
VM to no longer run after migration.

We now encourage userspace to write the read and expected value of
GICD_IIDR as the first part of a GIC register restore, and if we observe
a write to GICD_IIDR we know that userspace has been updated and has had
a chance to cope with older kernels (VGICv2 IIDR.Revision == 0)
incorrectly reporting interrupts as group 1, and therefore we now allow
groups to be user writable.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h           |  3 +++
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c661d0e..c134790 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -221,6 +221,9 @@ struct vgic_dist {
 	/* Implementation revision as reported in the GICD_IIDR */
 	u32			implementation_rev;
 
+	/* Userspace can write to GICv2 IGROUPR */
+	bool			v2_groups_user_writable;
+
 	/* Do injected MSIs require an additional device ID? */
 	bool			msis_require_devid;
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ee164f8..26654f4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -85,6 +85,18 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 	case GIC_DIST_IIDR:
 		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
 			return -EINVAL;
+
+		/*
+		 * If we observe a write to GICD_IIDR we know that userspace
+		 * has been updated and has had a chance to cope with older
+		 * kernels (VGICv2 IIDR.Revision == 0) incorrectly reporting
+		 * interrupts as group 1, and therefore we now allow groups to
+		 * be user writable.  Doing this by default would break
+		 * migration from old kernels to new kernels with legacy
+		 * userspace.
+		 */
+		vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
+		return 0;
 	}
 
 	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
@@ -95,7 +107,9 @@ static int vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
 					    gpa_t addr, unsigned int len,
 					    unsigned long val)
 {
-	/* Ignore writes from userspace */
+	if (vcpu->kvm->arch.vgic.v2_groups_user_writable)
+		vgic_mmio_write_group(vcpu, addr, len, val);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

Update the documentation to reflect the ordering requirements of
restoring the GICD_IIDR register before any other registers and the
effects this has on restoring the interrupt groups for an emulated GICv2
insttance.

Also remove some outdated limitations in the documentation while we're
at it.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
 Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
index 2408ab7..eba20ae 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
@@ -100,6 +100,14 @@ Groups:
     Note that distributor fields are not banked, but return the same value
     regardless of the mpidr used to access the register.
 
+    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
+    way directly observable by the guest or userspace.  Userspace should read
+    GICD_IIDR from KVM and write back the read value to confirm its expected
+    behavior is aligned with the KVM implementation.  Userspace should set
+    GICD_IIDR before setting any other registers.  to ensure the expected
+    behavior.
+
+
     The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
     that a write of a clear bit has no effect, whereas a write with a set bit
     clears that value.  To allow userspace to freely set the values of these two
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b3ce126..4ff7635 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -49,9 +49,15 @@ Groups:
     index is specified with the vcpu_index field.  Note that most distributor
     fields are not banked, but return the same value regardless of the
     vcpu_index used to access the register.
-  Limitations:
-    - Priorities are not implemented, and registers are RAZ/WI
-    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
+
+    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
+    GICv2 is changed in a way directly observable by the guest or userspace.
+    Userspace should read GICD_IIDR from KVM and write back the read value to
+    confirm its expected behavior is aligned with the KVM implementation.
+    Userspace should set GICD_IIDR before setting any other registers (both
+    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
+    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
+    to the interrupt group registers (GICD_IGROUPR) are ignored.
   Errors:
     -ENXIO: Getting or setting this register is not yet supported
     -EBUSY: One or more VCPUs are running
@@ -94,9 +100,6 @@ Groups:
     use the lower 5 bits to communicate with the KVM device and must shift the
     value left by 3 places to obtain the actual priority mask level.
 
-  Limitations:
-    - Priorities are not implemented, and registers are RAZ/WI
-    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
   Errors:
     -ENXIO: Getting or setting this register is not yet supported
     -EBUSY: One or more VCPUs are running
-- 
2.7.4

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

Update the documentation to reflect the ordering requirements of
restoring the GICD_IIDR register before any other registers and the
effects this has on restoring the interrupt groups for an emulated GICv2
insttance.

Also remove some outdated limitations in the documentation while we're
at it.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
 Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
index 2408ab7..eba20ae 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
@@ -100,6 +100,14 @@ Groups:
     Note that distributor fields are not banked, but return the same value
     regardless of the mpidr used to access the register.
 
+    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
+    way directly observable by the guest or userspace.  Userspace should read
+    GICD_IIDR from KVM and write back the read value to confirm its expected
+    behavior is aligned with the KVM implementation.  Userspace should set
+    GICD_IIDR before setting any other registers.  to ensure the expected
+    behavior.
+
+
     The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
     that a write of a clear bit has no effect, whereas a write with a set bit
     clears that value.  To allow userspace to freely set the values of these two
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b3ce126..4ff7635 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -49,9 +49,15 @@ Groups:
     index is specified with the vcpu_index field.  Note that most distributor
     fields are not banked, but return the same value regardless of the
     vcpu_index used to access the register.
-  Limitations:
-    - Priorities are not implemented, and registers are RAZ/WI
-    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
+
+    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
+    GICv2 is changed in a way directly observable by the guest or userspace.
+    Userspace should read GICD_IIDR from KVM and write back the read value to
+    confirm its expected behavior is aligned with the KVM implementation.
+    Userspace should set GICD_IIDR before setting any other registers (both
+    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
+    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
+    to the interrupt group registers (GICD_IGROUPR) are ignored.
   Errors:
     -ENXIO: Getting or setting this register is not yet supported
     -EBUSY: One or more VCPUs are running
@@ -94,9 +100,6 @@ Groups:
     use the lower 5 bits to communicate with the KVM device and must shift the
     value left by 3 places to obtain the actual priority mask level.
 
-  Limitations:
-    - Priorities are not implemented, and registers are RAZ/WI
-    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
   Errors:
     -ENXIO: Getting or setting this register is not yet supported
     -EBUSY: One or more VCPUs are running
-- 
2.7.4

RE: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Bharat Bhushan]

Hi Christoffer,

> -----Original Message-----
> From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-
> bounces@lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> Sent: Monday, July 16, 2018 6:36 PM
> To: kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org
> Cc: kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on
> incompatible uaccess GICD_IIDR writes
> 
> If userspace attempts to write a GICD_IIDR that does not match the
> kernel version, return an error to userspace.  The intention is to allow
> implementation changes inside KVM while avoiding silently breaking
> migration resulting in guests not running without any clear indication
> of what went wrong.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-
> mmio-v2.c
> index db646f1..4f0f2c4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu
> *vcpu,
>  	}
>  }
> 
> +static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> +					   gpa_t addr, unsigned int len,
> +					   unsigned long val)
> +{
> +	switch (addr & 0x0c) {

I am just understanding the code, not sure if it make sense to replace hardcoded "0x0c".

Thanks
-Bharat

> +	case GIC_DIST_IIDR:
> +		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
> +			return -EINVAL;
> +	}
> +
> +	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> +	return 0;
> +}
> +
>  static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
>  				 gpa_t addr, unsigned int len,
>  				 unsigned long val)
> @@ -367,9 +381,10 @@ static void vgic_mmio_write_apr(struct kvm_vcpu
> *vcpu,
>  }
> 
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
> -	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
> -		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> -		VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_DIST_CTRL,
> +		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc,
> +		NULL, vgic_mmio_uaccess_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_ACCESS_32bit),
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-
> mmio-v3.c
> index ef57a1a..abdb0ec 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -113,6 +113,20 @@ static void vgic_mmio_write_v3_misc(struct
> kvm_vcpu *vcpu,
>  	}
>  }
> 
> +static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
> +					   gpa_t addr, unsigned int len,
> +					   unsigned long val)
> +{
> +	switch (addr & 0x0c) {
> +	case GICD_IIDR:
> +		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
> +			return -EINVAL;
> +	}
> +
> +	vgic_mmio_write_v3_misc(vcpu, addr, len, val);
> +	return 0;
> +}
> +
>  static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu *vcpu,
>  					    gpa_t addr, unsigned int len)
>  {
> @@ -449,9 +463,10 @@ static void vgic_mmio_write_pendbase(struct
> kvm_vcpu *vcpu,
>  	}
> 
>  static const struct vgic_register_region vgic_v3_dist_registers[] = {
> -	REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> -		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> -		VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GICD_CTLR,
> +		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc,
> +		NULL, vgic_mmio_uaccess_write_v3_misc,
> +		16, VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>  		VGIC_ACCESS_32bit),
> --
> 2.7.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.cs.columbia.edu%2Fmailman%2Flistinfo%2Fkvmarm&amp;data=02%7C01%
> 7Cbharat.bhushan%40nxp.com%7Cf2d3e98a8d1a48166ce108d5eb1d06f4%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636673432268886197&am
> p;sdata=DitjaxtCqfVUge823Qw9IpT%2Fg9EoN2xI%2FIlj6mCdZ9k%3D&amp;r
> eserved=0

[PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Bharat Bhushan]

Hi Christoffer,

> -----Original Message-----
> From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-
> bounces at lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> Sent: Monday, July 16, 2018 6:36 PM
> To: kvmarm at lists.cs.columbia.edu; linux-arm-kernel at lists.infradead.org
> Cc: kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on
> incompatible uaccess GICD_IIDR writes
> 
> If userspace attempts to write a GICD_IIDR that does not match the
> kernel version, return an error to userspace.  The intention is to allow
> implementation changes inside KVM while avoiding silently breaking
> migration resulting in guests not running without any clear indication
> of what went wrong.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-
> mmio-v2.c
> index db646f1..4f0f2c4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu
> *vcpu,
>  	}
>  }
> 
> +static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> +					   gpa_t addr, unsigned int len,
> +					   unsigned long val)
> +{
> +	switch (addr & 0x0c) {

I am just understanding the code, not sure if it make sense to replace hardcoded "0x0c".

Thanks
-Bharat

> +	case GIC_DIST_IIDR:
> +		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
> +			return -EINVAL;
> +	}
> +
> +	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> +	return 0;
> +}
> +
>  static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
>  				 gpa_t addr, unsigned int len,
>  				 unsigned long val)
> @@ -367,9 +381,10 @@ static void vgic_mmio_write_apr(struct kvm_vcpu
> *vcpu,
>  }
> 
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
> -	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
> -		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> -		VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_DIST_CTRL,
> +		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc,
> +		NULL, vgic_mmio_uaccess_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_ACCESS_32bit),
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-
> mmio-v3.c
> index ef57a1a..abdb0ec 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -113,6 +113,20 @@ static void vgic_mmio_write_v3_misc(struct
> kvm_vcpu *vcpu,
>  	}
>  }
> 
> +static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
> +					   gpa_t addr, unsigned int len,
> +					   unsigned long val)
> +{
> +	switch (addr & 0x0c) {
> +	case GICD_IIDR:
> +		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
> +			return -EINVAL;
> +	}
> +
> +	vgic_mmio_write_v3_misc(vcpu, addr, len, val);
> +	return 0;
> +}
> +
>  static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu *vcpu,
>  					    gpa_t addr, unsigned int len)
>  {
> @@ -449,9 +463,10 @@ static void vgic_mmio_write_pendbase(struct
> kvm_vcpu *vcpu,
>  	}
> 
>  static const struct vgic_register_region vgic_v3_dist_registers[] = {
> -	REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> -		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> -		VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GICD_CTLR,
> +		vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc,
> +		NULL, vgic_mmio_uaccess_write_v3_misc,
> +		16, VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
>  		vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
>  		VGIC_ACCESS_32bit),
> --
> 2.7.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.cs.columbia.edu%2Fmailman%2Flistinfo%2Fkvmarm&amp;data=02%7C01%
> 7Cbharat.bhushan%40nxp.com%7Cf2d3e98a8d1a48166ce108d5eb1d06f4%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636673432268886197&am
> p;sdata=DitjaxtCqfVUge823Qw9IpT%2Fg9EoN2xI%2FIlj6mCdZ9k%3D&amp;r
> eserved=0

RE: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Bharat Bhushan]

Hi Christoffer,

> -----Original Message-----
> From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-
> bounces@lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> Sent: Monday, July 16, 2018 6:36 PM
> To: kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org
> Cc: kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of
> the GIC devices wrt IIDR
> 
> Update the documentation to reflect the ordering requirements of
> restoring the GICD_IIDR register before any other registers and the
> effects this has on restoring the interrupt groups for an emulated GICv2
> insttance.
> 
> Also remove some outdated limitations in the documentation while we're
> at it.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
>  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 2408ab7..eba20ae 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -100,6 +100,14 @@ Groups:
>      Note that distributor fields are not banked, but return the same value
>      regardless of the mpidr used to access the register.
> 
> +    GICD_IIDR.Revision is updated when the KVM implementation is changed
> in a
> +    way directly observable by the guest or userspace.  Userspace should
> read
> +    GICD_IIDR from KVM and write back the read value to confirm its
> expected
> +    behavior is aligned with the KVM implementation.  Userspace should set
> +    GICD_IIDR before setting any other registers.  to ensure the expected
> +    behavior.
> +
> +
>      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values of these two
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..4ff7635 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,15 @@ Groups:
>      index is specified with the vcpu_index field.  Note that most distributor
>      fields are not banked, but return the same value regardless of the
>      vcpu_index used to access the register.
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> +
> +    GICD_IIDR.Revision is updated when the KVM implementation of an
> emulated
> +    GICv2 is changed in a way directly observable by the guest or userspace.
> +    Userspace should read GICD_IIDR from KVM and write back the read
> value to
> +    confirm its expected behavior is aligned with the KVM implementation.
> +    Userspace should set GICD_IIDR before setting any other registers (both
> +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> +    the expected behavior.  Unless GICD_IIDR has been set from userspace,
> writes
> +    to the interrupt group registers (GICD_IGROUPR) are ignored.

Newer user-space will write to GICD_IIDR, that mean interrupt-groups are supported otherwise not.
And when interrupt-groups are not supported then writes to GICD_IGROUPR are ignored (backward compatibility)

Is this understanding correct?

Thanks
-Bharat

>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> @@ -94,9 +100,6 @@ Groups:
>      use the lower 5 bits to communicate with the KVM device and must shift
> the
>      value left by 3 places to obtain the actual priority mask level.
> 
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> --
> 2.7.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.cs.columbia.edu%2Fmailman%2Flistinfo%2Fkvmarm&amp;data=02%7C01%
> 7Cbharat.bhushan%40nxp.com%7C55b2ec9f9b7b4d6e530c08d5eb1d0faf%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636673432404483929&am
> p;sdata=RSQwNKkEYC02igGgPVIQmp40VsC9hU%2FFeiMcQBvYJOA%3D&am
> p;reserved=0

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Bharat Bhushan]

Hi Christoffer,

> -----Original Message-----
> From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-
> bounces at lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> Sent: Monday, July 16, 2018 6:36 PM
> To: kvmarm at lists.cs.columbia.edu; linux-arm-kernel at lists.infradead.org
> Cc: kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of
> the GIC devices wrt IIDR
> 
> Update the documentation to reflect the ordering requirements of
> restoring the GICD_IIDR register before any other registers and the
> effects this has on restoring the interrupt groups for an emulated GICv2
> insttance.
> 
> Also remove some outdated limitations in the documentation while we're
> at it.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
>  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 2408ab7..eba20ae 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -100,6 +100,14 @@ Groups:
>      Note that distributor fields are not banked, but return the same value
>      regardless of the mpidr used to access the register.
> 
> +    GICD_IIDR.Revision is updated when the KVM implementation is changed
> in a
> +    way directly observable by the guest or userspace.  Userspace should
> read
> +    GICD_IIDR from KVM and write back the read value to confirm its
> expected
> +    behavior is aligned with the KVM implementation.  Userspace should set
> +    GICD_IIDR before setting any other registers.  to ensure the expected
> +    behavior.
> +
> +
>      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values of these two
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..4ff7635 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,15 @@ Groups:
>      index is specified with the vcpu_index field.  Note that most distributor
>      fields are not banked, but return the same value regardless of the
>      vcpu_index used to access the register.
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> +
> +    GICD_IIDR.Revision is updated when the KVM implementation of an
> emulated
> +    GICv2 is changed in a way directly observable by the guest or userspace.
> +    Userspace should read GICD_IIDR from KVM and write back the read
> value to
> +    confirm its expected behavior is aligned with the KVM implementation.
> +    Userspace should set GICD_IIDR before setting any other registers (both
> +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> +    the expected behavior.  Unless GICD_IIDR has been set from userspace,
> writes
> +    to the interrupt group registers (GICD_IGROUPR) are ignored.

Newer user-space will write to GICD_IIDR, that mean interrupt-groups are supported otherwise not.
And when interrupt-groups are not supported then writes to GICD_IGROUPR are ignored (backward compatibility)

Is this understanding correct?

Thanks
-Bharat

>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> @@ -94,9 +100,6 @@ Groups:
>      use the lower 5 bits to communicate with the KVM device and must shift
> the
>      value left by 3 places to obtain the actual priority mask level.
> 
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> --
> 2.7.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.cs.columbia.edu%2Fmailman%2Flistinfo%2Fkvmarm&amp;data=02%7C01%
> 7Cbharat.bhushan%40nxp.com%7C55b2ec9f9b7b4d6e530c08d5eb1d0faf%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636673432404483929&am
> p;sdata=RSQwNKkEYC02igGgPVIQmp40VsC9hU%2FFeiMcQBvYJOA%3D&am
> p;reserved=0

Re: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Christoffer Dall]

On Mon, Jul 16, 2018 at 03:43:28PM +0000, Bharat Bhushan wrote:
> Hi Christoffer,
> 
> > -----Original Message-----
> > From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-
> > bounces@lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > Sent: Monday, July 16, 2018 6:36 PM
> > To: kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org
> > Cc: kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> > Przywara <andre.przywara@arm.com>
> > Subject: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on
> > incompatible uaccess GICD_IIDR writes
> > 
> > If userspace attempts to write a GICD_IIDR that does not match the
> > kernel version, return an error to userspace.  The intention is to allow
> > implementation changes inside KVM while avoiding silently breaking
> > migration resulting in guests not running without any clear indication
> > of what went wrong.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
> >  2 files changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-
> > mmio-v2.c
> > index db646f1..4f0f2c4 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu
> > *vcpu,
> >  	}
> >  }
> > 
> > +static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> > +					   gpa_t addr, unsigned int len,
> > +					   unsigned long val)
> > +{
> > +	switch (addr & 0x0c) {
> 
> I am just understanding the code, not sure if it make sense to replace hardcoded "0x0c".
> 

We could encode it, but we use the hardcoded value elsewhere so I just
followed the current pattern.

Thanks,
-Christoffer

[PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Christoffer Dall]

On Mon, Jul 16, 2018 at 03:43:28PM +0000, Bharat Bhushan wrote:
> Hi Christoffer,
> 
> > -----Original Message-----
> > From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-
> > bounces at lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > Sent: Monday, July 16, 2018 6:36 PM
> > To: kvmarm at lists.cs.columbia.edu; linux-arm-kernel at lists.infradead.org
> > Cc: kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> > Przywara <andre.przywara@arm.com>
> > Subject: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on
> > incompatible uaccess GICD_IIDR writes
> > 
> > If userspace attempts to write a GICD_IIDR that does not match the
> > kernel version, return an error to userspace.  The intention is to allow
> > implementation changes inside KVM while avoiding silently breaking
> > migration resulting in guests not running without any clear indication
> > of what went wrong.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
> >  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
> >  2 files changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-
> > mmio-v2.c
> > index db646f1..4f0f2c4 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -75,6 +75,20 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu
> > *vcpu,
> >  	}
> >  }
> > 
> > +static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> > +					   gpa_t addr, unsigned int len,
> > +					   unsigned long val)
> > +{
> > +	switch (addr & 0x0c) {
> 
> I am just understanding the code, not sure if it make sense to replace hardcoded "0x0c".
> 

We could encode it, but we use the hardcoded value elsewhere so I just
followed the current pattern.

Thanks,
-Christoffer

Re: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

On Mon, Jul 16, 2018 at 03:59:42PM +0000, Bharat Bhushan wrote:
> Hi Christoffer,
> 
> > -----Original Message-----
> > From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-
> > bounces@lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > Sent: Monday, July 16, 2018 6:36 PM
> > To: kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org
> > Cc: kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> > Przywara <andre.przywara@arm.com>
> > Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of
> > the GIC devices wrt IIDR
> > 
> > Update the documentation to reflect the ordering requirements of
> > restoring the GICD_IIDR register before any other registers and the
> > effects this has on restoring the interrupt groups for an emulated GICv2
> > insttance.
> > 
> > Also remove some outdated limitations in the documentation while we're
> > at it.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> >  2 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > index 2408ab7..eba20ae 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > @@ -100,6 +100,14 @@ Groups:
> >      Note that distributor fields are not banked, but return the same value
> >      regardless of the mpidr used to access the register.
> > 
> > +    GICD_IIDR.Revision is updated when the KVM implementation is changed
> > in a
> > +    way directly observable by the guest or userspace.  Userspace should
> > read
> > +    GICD_IIDR from KVM and write back the read value to confirm its
> > expected
> > +    behavior is aligned with the KVM implementation.  Userspace should set
> > +    GICD_IIDR before setting any other registers.  to ensure the expected
> > +    behavior.
> > +
> > +
> >      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> > defined such
> >      that a write of a clear bit has no effect, whereas a write with a set bit
> >      clears that value.  To allow userspace to freely set the values of these two
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..4ff7635 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,15 @@ Groups:
> >      index is specified with the vcpu_index field.  Note that most distributor
> >      fields are not banked, but return the same value regardless of the
> >      vcpu_index used to access the register.
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > +
> > +    GICD_IIDR.Revision is updated when the KVM implementation of an
> > emulated
> > +    GICv2 is changed in a way directly observable by the guest or userspace.
> > +    Userspace should read GICD_IIDR from KVM and write back the read
> > value to
> > +    confirm its expected behavior is aligned with the KVM implementation.
> > +    Userspace should set GICD_IIDR before setting any other registers (both
> > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> > KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > +    the expected behavior.  Unless GICD_IIDR has been set from userspace,
> > writes
> > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> 
> Newer user-space will write to GICD_IIDR, that mean interrupt-groups are supported otherwise not.

It only means that userspace can write interrupt groups, interrupt
groups will always be supported (and writable) from the guest's point of
view with newer kernels.

An updated userspace (which writes IIDR) must know to either raise an
error if it sees a migration stream from an older QEMU or translate the
interrupt groups from 1 to 0 before writing them.

> And when interrupt-groups are not supported then writes to GICD_IGROUPR are ignored (backward compatibility)

Legacy userspace which doesn't write IIDR will not have the above logic
so to preserve migration support between an old kernel and a new kernel,
both with legacy userspace, we ignore userspace writes to the IGROUPRs
in the KVM interface for userspace which hasn't written IIDR.

Hope this helps,
-Christoffer

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

On Mon, Jul 16, 2018 at 03:59:42PM +0000, Bharat Bhushan wrote:
> Hi Christoffer,
> 
> > -----Original Message-----
> > From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-
> > bounces at lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > Sent: Monday, July 16, 2018 6:36 PM
> > To: kvmarm at lists.cs.columbia.edu; linux-arm-kernel at lists.infradead.org
> > Cc: kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> > Przywara <andre.przywara@arm.com>
> > Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of
> > the GIC devices wrt IIDR
> > 
> > Update the documentation to reflect the ordering requirements of
> > restoring the GICD_IIDR register before any other registers and the
> > effects this has on restoring the interrupt groups for an emulated GICv2
> > insttance.
> > 
> > Also remove some outdated limitations in the documentation while we're
> > at it.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> >  2 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > index 2408ab7..eba20ae 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > @@ -100,6 +100,14 @@ Groups:
> >      Note that distributor fields are not banked, but return the same value
> >      regardless of the mpidr used to access the register.
> > 
> > +    GICD_IIDR.Revision is updated when the KVM implementation is changed
> > in a
> > +    way directly observable by the guest or userspace.  Userspace should
> > read
> > +    GICD_IIDR from KVM and write back the read value to confirm its
> > expected
> > +    behavior is aligned with the KVM implementation.  Userspace should set
> > +    GICD_IIDR before setting any other registers.  to ensure the expected
> > +    behavior.
> > +
> > +
> >      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> > defined such
> >      that a write of a clear bit has no effect, whereas a write with a set bit
> >      clears that value.  To allow userspace to freely set the values of these two
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..4ff7635 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,15 @@ Groups:
> >      index is specified with the vcpu_index field.  Note that most distributor
> >      fields are not banked, but return the same value regardless of the
> >      vcpu_index used to access the register.
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > +
> > +    GICD_IIDR.Revision is updated when the KVM implementation of an
> > emulated
> > +    GICv2 is changed in a way directly observable by the guest or userspace.
> > +    Userspace should read GICD_IIDR from KVM and write back the read
> > value to
> > +    confirm its expected behavior is aligned with the KVM implementation.
> > +    Userspace should set GICD_IIDR before setting any other registers (both
> > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> > KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > +    the expected behavior.  Unless GICD_IIDR has been set from userspace,
> > writes
> > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> 
> Newer user-space will write to GICD_IIDR, that mean interrupt-groups are supported otherwise not.

It only means that userspace can write interrupt groups, interrupt
groups will always be supported (and writable) from the guest's point of
view with newer kernels.

An updated userspace (which writes IIDR) must know to either raise an
error if it sees a migration stream from an older QEMU or translate the
interrupt groups from 1 to 0 before writing them.

> And when interrupt-groups are not supported then writes to GICD_IGROUPR are ignored (backward compatibility)

Legacy userspace which doesn't write IIDR will not have the above logic
so to preserve migration support between an old kernel and a new kernel,
both with legacy userspace, we ignore userspace writes to the IGROUPRs
in the KVM interface for userspace which hasn't written IIDR.

Hope this helps,
-Christoffer

RE: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Bharat Bhushan]

> -----Original Message-----
> From: Christoffer Dall [mailto:christoffer.dall@arm.com]
> Sent: Tuesday, July 17, 2018 2:44 PM
> To: Bharat Bhushan <bharat.bhushan@nxp.com>
> Cc: kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org;
> kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: Re: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation
> of the GIC devices wrt IIDR
> 
> On Mon, Jul 16, 2018 at 03:59:42PM +0000, Bharat Bhushan wrote:
> > Hi Christoffer,
> >
> > > -----Original Message-----
> > > From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-
> > > bounces@lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > > Sent: Monday, July 16, 2018 6:36 PM
> > > To: kvmarm@lists.cs.columbia.edu;
> > > linux-arm-kernel@lists.infradead.org
> > > Cc: kvm@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>;
> Andre
> > > Przywara <andre.przywara@arm.com>
> > > Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation
> > > of the GIC devices wrt IIDR
> > >
> > > Update the documentation to reflect the ordering requirements of
> > > restoring the GICD_IIDR register before any other registers and the
> > > effects this has on restoring the interrupt groups for an emulated
> > > GICv2 insttance.
> > >
> > > Also remove some outdated limitations in the documentation while
> > > we're at it.
> > >
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > ---
> > >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> > >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> > >  2 files changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > index 2408ab7..eba20ae 100644
> > > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > @@ -100,6 +100,14 @@ Groups:
> > >      Note that distributor fields are not banked, but return the same value
> > >      regardless of the mpidr used to access the register.
> > >
> > > +    GICD_IIDR.Revision is updated when the KVM implementation is
> > > + changed
> > > in a
> > > +    way directly observable by the guest or userspace.  Userspace
> > > + should
> > > read
> > > +    GICD_IIDR from KVM and write back the read value to confirm its
> > > expected
> > > +    behavior is aligned with the KVM implementation.  Userspace should
> set
> > > +    GICD_IIDR before setting any other registers.  to ensure the expected
> > > +    behavior.
> > > +
> > > +
> > >      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> > > defined such
> > >      that a write of a clear bit has no effect, whereas a write with a set bit
> > >      clears that value.  To allow userspace to freely set the values
> > > of these two diff --git
> > > a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > index b3ce126..4ff7635 100644
> > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > @@ -49,9 +49,15 @@ Groups:
> > >      index is specified with the vcpu_index field.  Note that most distributor
> > >      fields are not banked, but return the same value regardless of the
> > >      vcpu_index used to access the register.
> > > -  Limitations:
> > > -    - Priorities are not implemented, and registers are RAZ/WI
> > > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > > +
> > > +    GICD_IIDR.Revision is updated when the KVM implementation of an
> > > emulated
> > > +    GICv2 is changed in a way directly observable by the guest or
> userspace.
> > > +    Userspace should read GICD_IIDR from KVM and write back the
> > > + read
> > > value to
> > > +    confirm its expected behavior is aligned with the KVM
> implementation.
> > > +    Userspace should set GICD_IIDR before setting any other registers
> (both
> > > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> > > KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > > +    the expected behavior.  Unless GICD_IIDR has been set from
> > > + userspace,
> > > writes
> > > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> >
> > Newer user-space will write to GICD_IIDR, that mean interrupt-groups are
> supported otherwise not.
> 
> It only means that userspace can write interrupt groups, interrupt groups will
> always be supported (and writable) from the guest's point of view with
> newer kernels.
> 
> An updated userspace (which writes IIDR) must know to either raise an error
> if it sees a migration stream from an older QEMU or translate the interrupt
> groups from 1 to 0 before writing them.
> 
> > And when interrupt-groups are not supported then writes to
> > GICD_IGROUPR are ignored (backward compatibility)
> 
> Legacy userspace which doesn't write IIDR will not have the above logic so to
> preserve migration support between an old kernel and a new kernel, both
> with legacy userspace, we ignore userspace writes to the IGROUPRs in the
> KVM interface for userspace which hasn't written IIDR.

Thanks Christoffer,


> 
> Hope this helps,
> -Christoffer

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Bharat Bhushan]

> -----Original Message-----
> From: Christoffer Dall [mailto:christoffer.dall at arm.com]
> Sent: Tuesday, July 17, 2018 2:44 PM
> To: Bharat Bhushan <bharat.bhushan@nxp.com>
> Cc: kvmarm at lists.cs.columbia.edu; linux-arm-kernel at lists.infradead.org;
> kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Andre
> Przywara <andre.przywara@arm.com>
> Subject: Re: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation
> of the GIC devices wrt IIDR
> 
> On Mon, Jul 16, 2018 at 03:59:42PM +0000, Bharat Bhushan wrote:
> > Hi Christoffer,
> >
> > > -----Original Message-----
> > > From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-
> > > bounces at lists.cs.columbia.edu] On Behalf Of Christoffer Dall
> > > Sent: Monday, July 16, 2018 6:36 PM
> > > To: kvmarm at lists.cs.columbia.edu;
> > > linux-arm-kernel at lists.infradead.org
> > > Cc: kvm at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>;
> Andre
> > > Przywara <andre.przywara@arm.com>
> > > Subject: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation
> > > of the GIC devices wrt IIDR
> > >
> > > Update the documentation to reflect the ordering requirements of
> > > restoring the GICD_IIDR register before any other registers and the
> > > effects this has on restoring the interrupt groups for an emulated
> > > GICv2 insttance.
> > >
> > > Also remove some outdated limitations in the documentation while
> > > we're at it.
> > >
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > ---
> > >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> > >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> > >  2 files changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > index 2408ab7..eba20ae 100644
> > > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > > @@ -100,6 +100,14 @@ Groups:
> > >      Note that distributor fields are not banked, but return the same value
> > >      regardless of the mpidr used to access the register.
> > >
> > > +    GICD_IIDR.Revision is updated when the KVM implementation is
> > > + changed
> > > in a
> > > +    way directly observable by the guest or userspace.  Userspace
> > > + should
> > > read
> > > +    GICD_IIDR from KVM and write back the read value to confirm its
> > > expected
> > > +    behavior is aligned with the KVM implementation.  Userspace should
> set
> > > +    GICD_IIDR before setting any other registers.  to ensure the expected
> > > +    behavior.
> > > +
> > > +
> > >      The GICD_STATUSR and GICR_STATUSR registers are architecturally
> > > defined such
> > >      that a write of a clear bit has no effect, whereas a write with a set bit
> > >      clears that value.  To allow userspace to freely set the values
> > > of these two diff --git
> > > a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > index b3ce126..4ff7635 100644
> > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > > @@ -49,9 +49,15 @@ Groups:
> > >      index is specified with the vcpu_index field.  Note that most distributor
> > >      fields are not banked, but return the same value regardless of the
> > >      vcpu_index used to access the register.
> > > -  Limitations:
> > > -    - Priorities are not implemented, and registers are RAZ/WI
> > > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > > +
> > > +    GICD_IIDR.Revision is updated when the KVM implementation of an
> > > emulated
> > > +    GICv2 is changed in a way directly observable by the guest or
> userspace.
> > > +    Userspace should read GICD_IIDR from KVM and write back the
> > > + read
> > > value to
> > > +    confirm its expected behavior is aligned with the KVM
> implementation.
> > > +    Userspace should set GICD_IIDR before setting any other registers
> (both
> > > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and
> > > KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > > +    the expected behavior.  Unless GICD_IIDR has been set from
> > > + userspace,
> > > writes
> > > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> >
> > Newer user-space will write to GICD_IIDR, that mean interrupt-groups are
> supported otherwise not.
> 
> It only means that userspace can write interrupt groups, interrupt groups will
> always be supported (and writable) from the guest's point of view with
> newer kernels.
> 
> An updated userspace (which writes IIDR) must know to either raise an error
> if it sees a migration stream from an older QEMU or translate the interrupt
> groups from 1 to 0 before writing them.
> 
> > And when interrupt-groups are not supported then writes to
> > GICD_IGROUPR are ignored (backward compatibility)
> 
> Legacy userspace which doesn't write IIDR will not have the above logic so to
> preserve migration support between an old kernel and a new kernel, both
> with legacy userspace, we ignore userspace writes to the IGROUPRs in the
> KVM interface for userspace which hasn't written IIDR.

Thanks Christoffer,


> 
> Hope this helps,
> -Christoffer

Re: [PATCH v4 01/10] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:18PM +0200, Christoffer Dall wrote:
> Instead of hardcoding the shifts and masks in the GICD_IIDR register
> emulation, let's add the definition of these fields to the GIC header
> files and use them.
> 
> This will make things more obvious when we're going to bump the revision
> in the IIDR when we'll make guest-visible changes to the implementation.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/linux/irqchip/arm-gic-v3.h | 10 ++++++++++
>  include/linux/irqchip/arm-gic.h    | 10 ++++++++++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c   |  3 ++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c   |  3 ++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 01/10] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:18PM +0200, Christoffer Dall wrote:
> Instead of hardcoding the shifts and masks in the GICD_IIDR register
> emulation, let's add the definition of these fields to the GIC header
> files and use them.
> 
> This will make things more obvious when we're going to bump the revision
> in the IIDR when we'll make guest-visible changes to the implementation.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/linux/irqchip/arm-gic-v3.h | 10 ++++++++++
>  include/linux/irqchip/arm-gic.h    | 10 ++++++++++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c   |  3 ++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c   |  3 ++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 02/10] KVM: arm/arm64: vgic: Keep track of implementation revision

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:19PM +0200, Christoffer Dall wrote:
> As we are about to tweak implementation aspects of the VGIC emulation,
> while still preserving some level of backwards compatibility support,
> add a field to keep track of the implementation revision field which is
> reported to the VM and to userspace.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/kvm/arm_vgic.h           | 3 +++
>  virt/kvm/arm/vgic/vgic-init.c    | 1 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 6 ++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 ++++--
>  4 files changed, 12 insertions(+), 4 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 02/10] KVM: arm/arm64: vgic: Keep track of implementation revision

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:19PM +0200, Christoffer Dall wrote:
> As we are about to tweak implementation aspects of the VGIC emulation,
> while still preserving some level of backwards compatibility support,
> add a field to keep track of the implementation revision field which is
> reported to the VM and to userspace.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/kvm/arm_vgic.h           | 3 +++
>  virt/kvm/arm/vgic/vgic-init.c    | 1 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 6 ++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 ++++--
>  4 files changed, 12 insertions(+), 4 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 04/10] KVM: arm/arm64: vgic: Add group field to struct irq

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:21PM +0200, 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>
> ---
>  include/kvm/arm_vgic.h         |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
>  virt/kvm/arm/vgic/vgic-init.c  | 19 +++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-its.c   |  1 +
>  4 files changed, 24 insertions(+), 5 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 04/10] KVM: arm/arm64: vgic: Add group field to struct irq

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:21PM +0200, 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>
> ---
>  include/kvm/arm_vgic.h         |  1 +
>  virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
>  virt/kvm/arm/vgic/vgic-init.c  | 19 +++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-its.c   |  1 +
>  4 files changed, 24 insertions(+), 5 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 05/10] KVM: arm/arm64: vgic: Signal IRQs using their configured group

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:22PM +0200, 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>
> ---
>  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(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 05/10] KVM: arm/arm64: vgic: Signal IRQs using their configured group

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:22PM +0200, 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>
> ---
>  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(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 06/10] KVM: arm/arm64: vgic: Permit uaccess writes to return errors

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:23PM +0200, Christoffer Dall wrote:
> Currently we do not allow any vgic mmio write operations to fail, which
> makes sense from mmio traps from the guest.  However, we should be able
> to report failures to userspace, if userspace writes incompatible values
> to read-only registers.  Rework the internal interface to allow errors
> to be returned on the write side for userspace writes.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++++++-----
>  virt/kvm/arm/vgic/vgic-mmio.c    | 18 +++++++++++++-----
>  virt/kvm/arm/vgic/vgic-mmio.h    | 19 +++++++++++--------
>  3 files changed, 31 insertions(+), 18 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 06/10] KVM: arm/arm64: vgic: Permit uaccess writes to return errors

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:23PM +0200, Christoffer Dall wrote:
> Currently we do not allow any vgic mmio write operations to fail, which
> makes sense from mmio traps from the guest.  However, we should be able
> to report failures to userspace, if userspace writes incompatible values
> to read-only registers.  Rework the internal interface to allow errors
> to be returned on the write side for userspace writes.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++++++-----
>  virt/kvm/arm/vgic/vgic-mmio.c    | 18 +++++++++++++-----
>  virt/kvm/arm/vgic/vgic-mmio.h    | 19 +++++++++++--------
>  3 files changed, 31 insertions(+), 18 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:27PM +0200, Christoffer Dall wrote:
> Update the documentation to reflect the ordering requirements of
> restoring the GICD_IIDR register before any other registers and the
> effects this has on restoring the interrupt groups for an emulated GICv2
> insttance.

instance

> 
> Also remove some outdated limitations in the documentation while we're
> at it.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
>  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 2408ab7..eba20ae 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -100,6 +100,14 @@ Groups:
>      Note that distributor fields are not banked, but return the same value
>      regardless of the mpidr used to access the register.
>  
> +    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
> +    way directly observable by the guest or userspace.  Userspace should read
> +    GICD_IIDR from KVM and write back the read value to confirm its expected
> +    behavior is aligned with the KVM implementation.  Userspace should set
> +    GICD_IIDR before setting any other registers.  to ensure the expected

 s/. //

> +    behavior.
> +
> +
>      The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values of these two
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..4ff7635 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,15 @@ Groups:
>      index is specified with the vcpu_index field.  Note that most distributor
>      fields are not banked, but return the same value regardless of the
>      vcpu_index used to access the register.
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> +
> +    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
> +    GICv2 is changed in a way directly observable by the guest or userspace.
> +    Userspace should read GICD_IIDR from KVM and write back the read value to
> +    confirm its expected behavior is aligned with the KVM implementation.
> +    Userspace should set GICD_IIDR before setting any other registers (both
> +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> +    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
> +    to the interrupt group registers (GICD_IGROUPR) are ignored.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> @@ -94,9 +100,6 @@ Groups:
>      use the lower 5 bits to communicate with the KVM device and must shift the
>      value left by 3 places to obtain the actual priority mask level.
>  
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> -- 
> 2.7.4
>

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:27PM +0200, Christoffer Dall wrote:
> Update the documentation to reflect the ordering requirements of
> restoring the GICD_IIDR register before any other registers and the
> effects this has on restoring the interrupt groups for an emulated GICv2
> insttance.

instance

> 
> Also remove some outdated limitations in the documentation while we're
> at it.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
>  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index 2408ab7..eba20ae 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -100,6 +100,14 @@ Groups:
>      Note that distributor fields are not banked, but return the same value
>      regardless of the mpidr used to access the register.
>  
> +    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
> +    way directly observable by the guest or userspace.  Userspace should read
> +    GICD_IIDR from KVM and write back the read value to confirm its expected
> +    behavior is aligned with the KVM implementation.  Userspace should set
> +    GICD_IIDR before setting any other registers.  to ensure the expected

 s/. //

> +    behavior.
> +
> +
>      The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values of these two
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..4ff7635 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,15 @@ Groups:
>      index is specified with the vcpu_index field.  Note that most distributor
>      fields are not banked, but return the same value regardless of the
>      vcpu_index used to access the register.
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> +
> +    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
> +    GICv2 is changed in a way directly observable by the guest or userspace.
> +    Userspace should read GICD_IIDR from KVM and write back the read value to
> +    confirm its expected behavior is aligned with the KVM implementation.
> +    Userspace should set GICD_IIDR before setting any other registers (both
> +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> +    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
> +    to the interrupt group registers (GICD_IGROUPR) are ignored.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> @@ -94,9 +100,6 @@ Groups:
>      use the lower 5 bits to communicate with the KVM device and must shift the
>      value left by 3 places to obtain the actual priority mask level.
>  
> -  Limitations:
> -    - Priorities are not implemented, and registers are RAZ/WI
> -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
>    Errors:
>      -ENXIO: Getting or setting this register is not yet supported
>      -EBUSY: One or more VCPUs are running
> -- 
> 2.7.4
>

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:24PM +0200, Christoffer Dall wrote:
> If userspace attempts to write a GICD_IIDR that does not match the
> kernel version, return an error to userspace.  The intention is to allow
> implementation changes inside KVM while avoiding silently breaking
> migration resulting in guests not running without any clear indication
> of what went wrong.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 07/10] KVM: arm/arm64: vgic: Return error on incompatible uaccess GICD_IIDR writes

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:24PM +0200, Christoffer Dall wrote:
> If userspace attempts to write a GICD_IIDR that does not match the
> kernel version, return an error to userspace.  The intention is to allow
> implementation changes inside KVM while avoiding silently breaking
> migration resulting in guests not running without any clear indication
> of what went wrong.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 21 ++++++++++++++++++---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 21 ++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 08/10] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:25PM +0200, 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.
> 
> This also allows userspace to configure the interrupts groups for GICv3.
> We don't allow userspace to write the groups on GICv2 just yet, because
> that would result in GICv2 guests not receiving interrupts after
> migrating from an older kernel that exposes GICv2 interrupts as group 1.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-init.c    |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 13 ++++++++++++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 11 +++++++++--
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 ++++++
>  5 files changed, 66 insertions(+), 4 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 08/10] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:25PM +0200, 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.
> 
> This also allows userspace to configure the interrupts groups for GICv3.
> We don't allow userspace to write the groups on GICv2 just yet, because
> that would result in GICv2 guests not receiving interrupts after
> migrating from an older kernel that exposes GICv2 interrupts as group 1.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-init.c    |  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 13 ++++++++++++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 11 +++++++++--
>  virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h    |  6 ++++++
>  5 files changed, 66 insertions(+), 4 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 03/10] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:20PM +0200, 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>
> ---
>  virt/kvm/arm/vgic/vgic-init.c    | 2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 8b6fc45..230c922 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
>  
>  	vgic_debug_init(kvm);
>  
> -	dist->implementation_rev = 0;
> +	dist->implementation_rev = 1;
>  	dist->initialized = true;
>  
>  out:
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index f0c5351..db646f1 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -22,6 +22,12 @@
>  #include "vgic.h"
>  #include "vgic-mmio.h"
>  
> +/*
> + * The Revision field in the IIDR have the following meanings:
> + *
> + * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
> + */
> +
>  static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
>  					    gpa_t addr, unsigned int len)
>  {
> @@ -365,7 +371,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.7.4
>

I realize that this is fixing the emulation, so any guest that would find
this change a problem is broken, but guest-visible changes without user
consent always seem a bit wrong.

We could drop this patch and only allow gicv2 guests to read ones from the
IGROUP registers until v2_groups_user_writable is true. Then, if a broken
guest needs to run on a host with an updated kernel, userspace, which must
have been updated too, can give the guest the broken group view it needs.

That said, I'm not overly concerned about this guest-visible issue. So

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 03/10] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:20PM +0200, 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>
> ---
>  virt/kvm/arm/vgic/vgic-init.c    | 2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 8b6fc45..230c922 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
>  
>  	vgic_debug_init(kvm);
>  
> -	dist->implementation_rev = 0;
> +	dist->implementation_rev = 1;
>  	dist->initialized = true;
>  
>  out:
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index f0c5351..db646f1 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -22,6 +22,12 @@
>  #include "vgic.h"
>  #include "vgic-mmio.h"
>  
> +/*
> + * The Revision field in the IIDR have the following meanings:
> + *
> + * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
> + */
> +
>  static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
>  					    gpa_t addr, unsigned int len)
>  {
> @@ -365,7 +371,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.7.4
>

I realize that this is fixing the emulation, so any guest that would find
this change a problem is broken, but guest-visible changes without user
consent always seem a bit wrong.

We could drop this patch and only allow gicv2 guests to read ones from the
IGROUP registers until v2_groups_user_writable is true. Then, if a broken
guest needs to run on a host with an updated kernel, userspace, which must
have been updated too, can give the guest the broken group view it needs.

That said, I'm not overly concerned about this guest-visible issue. So

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 09/10] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:26PM +0200, Christoffer Dall wrote:
> Simply letting IGROUPR be writable from userspace would break
> migration from old kernels to newer kernels, because old kernels
> incorrectly report interrupt groups as group 1.  This would not be a big
> problem if userspace wrote GICD_IIDR as read from the kernel, because we
> could detect the incompatibility and return an error to userspace.
> Unfortunately, this is not the case with current userspace
> implementations and simply letting IGROUPR be writable from userspace for
> an emulated GICv2 silently breaks migration and causes the destination
> VM to no longer run after migration.
> 
> We now encourage userspace to write the read and expected value of
> GICD_IIDR as the first part of a GIC register restore, and if we observe
> a write to GICD_IIDR we know that userspace has been updated and has had
> a chance to cope with older kernels (VGICv2 IIDR.Revision == 0)
> incorrectly reporting interrupts as group 1, and therefore we now allow
> groups to be user writable.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/kvm/arm_vgic.h           |  3 +++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

[PATCH v4 09/10] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR

[Andrew Jones]

On Mon, Jul 16, 2018 at 03:06:26PM +0200, Christoffer Dall wrote:
> Simply letting IGROUPR be writable from userspace would break
> migration from old kernels to newer kernels, because old kernels
> incorrectly report interrupt groups as group 1.  This would not be a big
> problem if userspace wrote GICD_IIDR as read from the kernel, because we
> could detect the incompatibility and return an error to userspace.
> Unfortunately, this is not the case with current userspace
> implementations and simply letting IGROUPR be writable from userspace for
> an emulated GICv2 silently breaks migration and causes the destination
> VM to no longer run after migration.
> 
> We now encourage userspace to write the read and expected value of
> GICD_IIDR as the first part of a GIC register restore, and if we observe
> a write to GICD_IIDR we know that userspace has been updated and has had
> a chance to cope with older kernels (VGICv2 IIDR.Revision == 0)
> incorrectly reporting interrupts as group 1, and therefore we now allow
> groups to be user writable.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  include/kvm/arm_vgic.h           |  3 +++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v4 03/10] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero

[Christoffer Dall]

On Thu, Jul 19, 2018 at 08:16:36PM +0200, Andrew Jones wrote:
> On Mon, Jul 16, 2018 at 03:06:20PM +0200, 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>
> > ---
> >  virt/kvm/arm/vgic/vgic-init.c    | 2 +-
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> > index 8b6fc45..230c922 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
> >  
> >  	vgic_debug_init(kvm);
> >  
> > -	dist->implementation_rev = 0;
> > +	dist->implementation_rev = 1;
> >  	dist->initialized = true;
> >  
> >  out:
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index f0c5351..db646f1 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -22,6 +22,12 @@
> >  #include "vgic.h"
> >  #include "vgic-mmio.h"
> >  
> > +/*
> > + * The Revision field in the IIDR have the following meanings:
> > + *
> > + * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
> > + */
> > +
> >  static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
> >  					    gpa_t addr, unsigned int len)
> >  {
> > @@ -365,7 +371,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.7.4
> >
> 
> I realize that this is fixing the emulation, so any guest that would find
> this change a problem is broken, but guest-visible changes without user
> consent always seem a bit wrong.
> 
> We could drop this patch and only allow gicv2 guests to read ones from the
> IGROUP registers until v2_groups_user_writable is true. Then, if a broken
> guest needs to run on a host with an updated kernel, userspace, which must
> have been updated too, can give the guest the broken group view it needs.

That won't really help because those interrupts will no longer be
injected as IRQs, but as FIQs, which is also a departure from what the
guest expects.

> 
> That said, I'm not overly concerned about this guest-visible issue. So

Any guests relying on this broken behavior will not run on hardwre, so
I'm not going to change this.

> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks!
-Christoffer

[PATCH v4 03/10] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero

[Christoffer Dall]

On Thu, Jul 19, 2018 at 08:16:36PM +0200, Andrew Jones wrote:
> On Mon, Jul 16, 2018 at 03:06:20PM +0200, 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>
> > ---
> >  virt/kvm/arm/vgic/vgic-init.c    | 2 +-
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> > index 8b6fc45..230c922 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)
> >  
> >  	vgic_debug_init(kvm);
> >  
> > -	dist->implementation_rev = 0;
> > +	dist->implementation_rev = 1;
> >  	dist->initialized = true;
> >  
> >  out:
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index f0c5351..db646f1 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -22,6 +22,12 @@
> >  #include "vgic.h"
> >  #include "vgic-mmio.h"
> >  
> > +/*
> > + * The Revision field in the IIDR have the following meanings:
> > + *
> > + * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
> > + */
> > +
> >  static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
> >  					    gpa_t addr, unsigned int len)
> >  {
> > @@ -365,7 +371,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.7.4
> >
> 
> I realize that this is fixing the emulation, so any guest that would find
> this change a problem is broken, but guest-visible changes without user
> consent always seem a bit wrong.
> 
> We could drop this patch and only allow gicv2 guests to read ones from the
> IGROUP registers until v2_groups_user_writable is true. Then, if a broken
> guest needs to run on a host with an updated kernel, userspace, which must
> have been updated too, can give the guest the broken group view it needs.

That won't really help because those interrupts will no longer be
injected as IRQs, but as FIQs, which is also a departure from what the
guest expects.

> 
> That said, I'm not overly concerned about this guest-visible issue. So

Any guests relying on this broken behavior will not run on hardwre, so
I'm not going to change this.

> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks!
-Christoffer

Re: [PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

On Thu, Jul 19, 2018 at 07:08:30PM +0200, Andrew Jones wrote:
> On Mon, Jul 16, 2018 at 03:06:27PM +0200, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring the GICD_IIDR register before any other registers and the
> > effects this has on restoring the interrupt groups for an emulated GICv2
> > insttance.
> 
> instance
> 
> > 
> > Also remove some outdated limitations in the documentation while we're
> > at it.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> >  2 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > index 2408ab7..eba20ae 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > @@ -100,6 +100,14 @@ Groups:
> >      Note that distributor fields are not banked, but return the same value
> >      regardless of the mpidr used to access the register.
> >  
> > +    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
> > +    way directly observable by the guest or userspace.  Userspace should read
> > +    GICD_IIDR from KVM and write back the read value to confirm its expected
> > +    behavior is aligned with the KVM implementation.  Userspace should set
> > +    GICD_IIDR before setting any other registers.  to ensure the expected
> 
>  s/. //
> 
> > +    behavior.
> > +
> > +
> >      The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
> >      that a write of a clear bit has no effect, whereas a write with a set bit
> >      clears that value.  To allow userspace to freely set the values of these two
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..4ff7635 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,15 @@ Groups:
> >      index is specified with the vcpu_index field.  Note that most distributor
> >      fields are not banked, but return the same value regardless of the
> >      vcpu_index used to access the register.
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > +
> > +    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
> > +    GICv2 is changed in a way directly observable by the guest or userspace.
> > +    Userspace should read GICD_IIDR from KVM and write back the read value to
> > +    confirm its expected behavior is aligned with the KVM implementation.
> > +    Userspace should set GICD_IIDR before setting any other registers (both
> > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > +    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
> > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> >    Errors:
> >      -ENXIO: Getting or setting this register is not yet supported
> >      -EBUSY: One or more VCPUs are running
> > @@ -94,9 +100,6 @@ Groups:
> >      use the lower 5 bits to communicate with the KVM device and must shift the
> >      value left by 3 places to obtain the actual priority mask level.
> >  
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> >    Errors:
> >      -ENXIO: Getting or setting this register is not yet supported
> >      -EBUSY: One or more VCPUs are running
> > -- 
> > 2.7.4
> >
> 
> Otherwise
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks.  I'm hoping that Marc won't mind fixing this up when applying
the patches.

-Christoffer

[PATCH v4 10/10] KVM: arm/arm64: vgic: Update documentation of the GIC devices wrt IIDR

[Christoffer Dall]

On Thu, Jul 19, 2018 at 07:08:30PM +0200, Andrew Jones wrote:
> On Mon, Jul 16, 2018 at 03:06:27PM +0200, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring the GICD_IIDR register before any other registers and the
> > effects this has on restoring the interrupt groups for an emulated GICv2
> > insttance.
> 
> instance
> 
> > 
> > Also remove some outdated limitations in the documentation while we're
> > at it.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  Documentation/virtual/kvm/devices/arm-vgic-v3.txt |  8 ++++++++
> >  Documentation/virtual/kvm/devices/arm-vgic.txt    | 15 +++++++++------
> >  2 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > index 2408ab7..eba20ae 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> > @@ -100,6 +100,14 @@ Groups:
> >      Note that distributor fields are not banked, but return the same value
> >      regardless of the mpidr used to access the register.
> >  
> > +    GICD_IIDR.Revision is updated when the KVM implementation is changed in a
> > +    way directly observable by the guest or userspace.  Userspace should read
> > +    GICD_IIDR from KVM and write back the read value to confirm its expected
> > +    behavior is aligned with the KVM implementation.  Userspace should set
> > +    GICD_IIDR before setting any other registers.  to ensure the expected
> 
>  s/. //
> 
> > +    behavior.
> > +
> > +
> >      The GICD_STATUSR and GICR_STATUSR registers are architecturally defined such
> >      that a write of a clear bit has no effect, whereas a write with a set bit
> >      clears that value.  To allow userspace to freely set the values of these two
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..4ff7635 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,15 @@ Groups:
> >      index is specified with the vcpu_index field.  Note that most distributor
> >      fields are not banked, but return the same value regardless of the
> >      vcpu_index used to access the register.
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> > +
> > +    GICD_IIDR.Revision is updated when the KVM implementation of an emulated
> > +    GICv2 is changed in a way directly observable by the guest or userspace.
> > +    Userspace should read GICD_IIDR from KVM and write back the read value to
> > +    confirm its expected behavior is aligned with the KVM implementation.
> > +    Userspace should set GICD_IIDR before setting any other registers (both
> > +    KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> > +    the expected behavior.  Unless GICD_IIDR has been set from userspace, writes
> > +    to the interrupt group registers (GICD_IGROUPR) are ignored.
> >    Errors:
> >      -ENXIO: Getting or setting this register is not yet supported
> >      -EBUSY: One or more VCPUs are running
> > @@ -94,9 +100,6 @@ Groups:
> >      use the lower 5 bits to communicate with the KVM device and must shift the
> >      value left by 3 places to obtain the actual priority mask level.
> >  
> > -  Limitations:
> > -    - Priorities are not implemented, and registers are RAZ/WI
> > -    - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> >    Errors:
> >      -ENXIO: Getting or setting this register is not yet supported
> >      -EBUSY: One or more VCPUs are running
> > -- 
> > 2.7.4
> >
> 
> Otherwise
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks.  I'm hoping that Marc won't mind fixing this up when applying
the patches.

-Christoffer

Re: [PATCH v4 00/10] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On 16/07/18 14:06, Christoffer Dall wrote:
> This 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
> guest 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).
> 
> Tested on Seattle, TX1, and the foundation model.  I've run a
> GICv2 guest on a GICv3 host on the foundation model.  I've tested
> migration on Seattle, and migration from an old kernel to a newer kernel
> with an unmodified QEMU binary continues to work.  I also tested a
> modified userspace that sets some interrupts to group 1, and that is
> reflected in the vgic debug output.

Applied to kvmarm/next

Thanks,

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

[PATCH v4 00/10] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Marc Zyngier]

On 16/07/18 14:06, Christoffer Dall wrote:
> This 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
> guest 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).
> 
> Tested on Seattle, TX1, and the foundation model.  I've run a
> GICv2 guest on a GICv3 host on the foundation model.  I've tested
> migration on Seattle, and migration from an old kernel to a newer kernel
> with an unmodified QEMU binary continues to work.  I also tested a
> modified userspace that sets some interrupts to group 1, and that is
> reflected in the vgic debug output.

Applied to kvmarm/next

Thanks,

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