* [PATCH v3 0/9] KVM: arm/arm64: vgic: Virtual interrupt grouping support
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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-rc3.
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 (9):
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: Allow configuration of interrupt groups
KVM: arm/arm64: vgic: Permit uaccess writes to return errors
KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
KVM: arm/arm64: vgic: Update documentation of the GICv2 device
Documentation/virtual/kvm/devices/arm-vgic.txt | 12 +++---
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 | 60 +++++++++++++++++++++++---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 51 ++++++++++++++++------
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 +--
13 files changed, 223 insertions(+), 47 deletions(-)
--
2.7.4
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 1/9] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 1/9] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 2/9] KVM: arm/arm64: vgic: Keep track of implementation revision
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 2/9] KVM: arm/arm64: vgic: Keep track of implementation revision
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 3/9] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 3/9] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 4/9] KVM: arm/arm64: vgic: Add group field to struct irq
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 4/9] KVM: arm/arm64: vgic: Add group field to struct irq
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 5/9] KVM: arm/arm64: vgic: Signal IRQs using their configured group
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 5/9] KVM: arm/arm64: vgic: Signal IRQs using their configured group
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 6/9] KVM: arm/arm64: vgic: Allow configuration of interrupt groups
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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 | 12 +++++++++++-
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, 65 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 db646f1..34e36fc 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,
@@ -75,6 +77,13 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
}
}
+static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len,
+ unsigned long val)
+{
+ /* Ignore writes from userspace */
+}
+
static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -371,7 +380,8 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
- vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+ vgic_mmio_read_group, vgic_mmio_write_group,
+ NULL, 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 ebe10a0..49df2a1 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)
{
@@ -454,7 +461,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,
@@ -527,7 +534,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 ff9655c..ae31bd0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
/* Ignore */
}
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ u32 value = 0;
+ int i;
+
+ /* Loop over all IRQs affected by this read */
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ if (irq->group)
+ value |= BIT(i);
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+
+ return value;
+}
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len, unsigned long val)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ int i;
+ unsigned long flags;
+
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ spin_lock_irqsave(&irq->irq_lock, flags);
+ irq->group = !!(val & BIT(i));
+ vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+}
+
/*
* Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
* of the enabled bit, so there is only one function for both here.
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df..1079862 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int len, unsigned long val);
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len);
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len, unsigned long val);
+
unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
--
2.7.4
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 6/9] KVM: arm/arm64: vgic: Allow configuration of interrupt groups
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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 | 12 +++++++++++-
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, 65 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 db646f1..34e36fc 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,
@@ -75,6 +77,13 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
}
}
+static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len,
+ unsigned long val)
+{
+ /* Ignore writes from userspace */
+}
+
static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -371,7 +380,8 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
- vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+ vgic_mmio_read_group, vgic_mmio_write_group,
+ NULL, 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 ebe10a0..49df2a1 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)
{
@@ -454,7 +461,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,
@@ -527,7 +534,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 ff9655c..ae31bd0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
/* Ignore */
}
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ u32 value = 0;
+ int i;
+
+ /* Loop over all IRQs affected by this read */
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ if (irq->group)
+ value |= BIT(i);
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+
+ return value;
+}
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len, unsigned long val)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ int i;
+ unsigned long flags;
+
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ spin_lock_irqsave(&irq->irq_lock, flags);
+ irq->group = !!(val & BIT(i));
+ vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+}
+
/*
* Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
* of the enabled bit, so there is only one function for both here.
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df..1079862 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int len, unsigned long val);
+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len);
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len, unsigned long val);
+
unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
--
2.7.4
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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-v2.c | 26 +++++++++++++++++++++-----
virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
4 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 34e36fc..b79de42 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
}
}
-static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len,
- unsigned long val)
+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 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,
@@ -376,8 +391,9 @@ 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,
+ 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_group, vgic_mmio_write_group,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 49df2a1..fabec2b 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -120,6 +120,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)
{
@@ -256,9 +270,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;
@@ -283,6 +297,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. */
@@ -454,8 +470,9 @@ 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,
+ 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,
@@ -475,7 +492,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,
@@ -548,7 +565,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 ae31bd0..f56ff1c 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;
+}
+
unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len)
{
@@ -401,11 +408,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,
@@ -437,11 +445,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,
@@ -773,10 +782,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 1079862..a07f90a 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_group(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int len);
@@ -173,13 +176,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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-v2.c | 26 +++++++++++++++++++++-----
virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
4 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 34e36fc..b79de42 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
}
}
-static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len,
- unsigned long val)
+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 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,
@@ -376,8 +391,9 @@ 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,
+ 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_group, vgic_mmio_write_group,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 49df2a1..fabec2b 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -120,6 +120,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)
{
@@ -256,9 +270,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;
@@ -283,6 +297,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. */
@@ -454,8 +470,9 @@ 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,
+ 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,
@@ -475,7 +492,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,
@@ -548,7 +565,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 ae31bd0..f56ff1c 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;
+}
+
unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len)
{
@@ -401,11 +408,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,
@@ -437,11 +445,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,
@@ -773,10 +782,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 1079862..a07f90a 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_group(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int len);
@@ -173,13 +176,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-16 8:28 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:28 UTC (permalink / raw)
To: Christoffer Dall, kvmarm, linux-arm-kernel; +Cc: kvm, Andre Przywara
On 14/07/18 18:05, 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-v2.c | 26 +++++++++++++++++++++-----
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
> virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
> virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
> 4 files changed, 69 insertions(+), 25 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 34e36fc..b79de42 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
> }
> }
>
> -static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len,
> - unsigned long val)
> +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 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;
> }
I think it'd make more sense if this patch came before the previous one
(change the prototype of the userspace accessor, and only then add a new
accessor that can return an error). It would avoid churn in the above
function, and you could move vgic_mmio_uaccess_write_v2_misc into patch 6.
Not a big deal though, as the code is otherwise perfectly fine.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
@ 2018-07-16 8:28 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:28 UTC (permalink / raw)
To: linux-arm-kernel
On 14/07/18 18:05, 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-v2.c | 26 +++++++++++++++++++++-----
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
> virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
> virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
> 4 files changed, 69 insertions(+), 25 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 34e36fc..b79de42 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
> }
> }
>
> -static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len,
> - unsigned long val)
> +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 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;
> }
I think it'd make more sense if this patch came before the previous one
(change the prototype of the userspace accessor, and only then add a new
accessor that can return an error). It would avoid churn in the above
function, and you could move vgic_mmio_uaccess_write_v2_misc into patch 6.
Not a big deal though, as the code is otherwise perfectly fine.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
2018-07-16 8:28 ` Marc Zyngier
@ 2018-07-16 9:59 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:59 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Jul 16, 2018 at 09:28:00AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, 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-v2.c | 26 +++++++++++++++++++++-----
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
> > virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
> > virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
> > 4 files changed, 69 insertions(+), 25 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 34e36fc..b79de42 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > -static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len,
> > - unsigned long val)
> > +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 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;
> > }
>
> I think it'd make more sense if this patch came before the previous one
> (change the prototype of the userspace accessor, and only then add a new
> accessor that can return an error). It would avoid churn in the above
> function, and you could move vgic_mmio_uaccess_write_v2_misc into patch 6.
The intention was that the first six patches dealt with the basic
interrupt grouping support, pretending that we don't have this
save/restore issue with GICv2, and then let the following patches deal
with the GICv2 problem.
>
> Not a big deal though, as the code is otherwise perfectly fine.
>
That said, I'm happy to try to rework this if you think the git history
will be easier to carry as a result.
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 7/9] KVM: arm/arm64: vgic: Permit uaccess writes to return errors
@ 2018-07-16 9:59 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:59 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 09:28:00AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, 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-v2.c | 26 +++++++++++++++++++++-----
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++++++++++-------
> > virt/kvm/arm/vgic/vgic-mmio.c | 18 +++++++++++++-----
> > virt/kvm/arm/vgic/vgic-mmio.h | 19 +++++++++++--------
> > 4 files changed, 69 insertions(+), 25 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 34e36fc..b79de42 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -77,11 +77,26 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > -static void vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len,
> > - unsigned long val)
> > +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 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;
> > }
>
> I think it'd make more sense if this patch came before the previous one
> (change the prototype of the userspace accessor, and only then add a new
> accessor that can return an error). It would avoid churn in the above
> function, and you could move vgic_mmio_uaccess_write_v2_misc into patch 6.
The intention was that the first six patches dealt with the basic
interrupt grouping support, pretending that we don't have this
save/restore issue with GICv2, and then let the following patches deal
with the GICv2 problem.
>
> Not a big deal though, as the code is otherwise perfectly fine.
>
That said, I'm happy to try to rework this if you think the git history
will be easier to carry as a result.
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
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 | 15 ++++++++++++++-
2 files changed, 17 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 b79de42..9b6b758 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -85,6 +85,17 @@ 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 gruops 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;
}
vgic_mmio_write_v2_misc(vcpu, addr, len, val);
@@ -95,7 +106,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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 | 15 ++++++++++++++-
2 files changed, 17 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 b79de42..9b6b758 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -85,6 +85,17 @@ 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 gruops 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;
}
vgic_mmio_write_v2_misc(vcpu, addr, len, val);
@@ -95,7 +106,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-16 8:38 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:38 UTC (permalink / raw)
To: Christoffer Dall, kvmarm, linux-arm-kernel; +Cc: kvm, Andre Przywara
On 14/07/18 18:05, 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 | 15 ++++++++++++++-
> 2 files changed, 17 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 b79de42..9b6b758 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -85,6 +85,17 @@ 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 gruops to
groups
> + * 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; ?
I think it'd be safer, just in case we decide to do something bizarre in
vgic_mmio_write_v2_misc...
> }
>
> vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> @@ -95,7 +106,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;
> }
>
>
Otherwise looks good..
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
@ 2018-07-16 8:38 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:38 UTC (permalink / raw)
To: linux-arm-kernel
On 14/07/18 18:05, 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 | 15 ++++++++++++++-
> 2 files changed, 17 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 b79de42..9b6b758 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -85,6 +85,17 @@ 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 gruops to
groups
> + * 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; ?
I think it'd be safer, just in case we decide to do something bizarre in
vgic_mmio_write_v2_misc...
> }
>
> vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> @@ -95,7 +106,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;
> }
>
>
Otherwise looks good..
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
2018-07-16 8:38 ` Marc Zyngier
@ 2018-07-16 9:46 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:46 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Jul 16, 2018 at 09:38:36AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, 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 | 15 ++++++++++++++-
> > 2 files changed, 17 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 b79de42..9b6b758 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -85,6 +85,17 @@ 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 gruops to
>
> groups
>
> > + * 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; ?
>
> I think it'd be safer, just in case we decide to do something bizarre in
> vgic_mmio_write_v2_misc...
>
That one was on purpose, as I figured we handle setting values from
userspace and the guest for the other regs in the other function, and it
would be a bit strange that one the one register was special-cased, but
I can see it from the other point of view as well, so I don't mind
changing it.
> > }
> >
> > vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> > @@ -95,7 +106,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;
> > }
> >
> >
>
> Otherwise looks good..
>
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
@ 2018-07-16 9:46 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:46 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 09:38:36AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, 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 | 15 ++++++++++++++-
> > 2 files changed, 17 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 b79de42..9b6b758 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -85,6 +85,17 @@ 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 gruops to
>
> groups
>
> > + * 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; ?
>
> I think it'd be safer, just in case we decide to do something bizarre in
> vgic_mmio_write_v2_misc...
>
That one was on purpose, as I figured we handle setting values from
userspace and the guest for the other regs in the other function, and it
would be a bit strange that one the one register was special-cased, but
I can see it from the other point of view as well, so I don't mind
changing it.
> > }
> >
> > vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> > @@ -95,7 +106,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;
> > }
> >
> >
>
> Otherwise looks good..
>
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
2018-07-16 9:46 ` Christoffer Dall
@ 2018-07-16 11:39 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 11:39 UTC (permalink / raw)
To: Marc Zyngier; +Cc: linux-arm-kernel, Andre Przywara, kvmarm, kvm
On Mon, Jul 16, 2018 at 11:46:57AM +0200, Christoffer Dall wrote:
>
> Thanks,
> -Christoffer
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Apologies on the disclaimer, mutt configuration snafu.
Please disregard the disclaimer as it clearly doesn't apply here on a
public mailing list.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 8/9] KVM: arm/arm64: vgic: Let userspace opt-in to writable v2 IGROUPR
@ 2018-07-16 11:39 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 11:39 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 11:46:57AM +0200, Christoffer Dall wrote:
>
> Thanks,
> -Christoffer
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Apologies on the disclaimer, mutt configuration snafu.
Please disregard the disclaimer as it clearly doesn't apply here on a
public mailing list.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-14 17:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: kvm, Marc Zyngier, Andre Przywara
Update the documentation to reflect the ordering requirements of
restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b3ce126..c9a6393 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
+ the read value to confirm its expected behavior is aligned with the KVM
+ implementation. To properly restore the interrupt group configuration,
+ GICD_IIDR should be written before any other registers.
Errors:
-ENXIO: Getting or setting this register is not yet supported
-EBUSY: One or more VCPUs are running
@@ -94,9 +97,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-14 17:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-14 17:05 UTC (permalink / raw)
To: linux-arm-kernel
Update the documentation to reflect the ordering requirements of
restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b3ce126..c9a6393 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
+ the read value to confirm its expected behavior is aligned with the KVM
+ implementation. To properly restore the interrupt group configuration,
+ GICD_IIDR should be written before any other registers.
Errors:
-ENXIO: Getting or setting this register is not yet supported
-EBUSY: One or more VCPUs are running
@@ -94,9 +97,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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-14 17:05 ` Christoffer Dall
@ 2018-07-16 8:52 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:52 UTC (permalink / raw)
To: Christoffer Dall, kvmarm, linux-arm-kernel; +Cc: kvm, Andre Przywara
On 14/07/18 18:05, Christoffer Dall wrote:
> Update the documentation to reflect the ordering requirements of
> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..c9a6393 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> + the read value to confirm its expected behavior is aligned with the KVM
> + implementation. To properly restore the interrupt group configuration,
> + GICD_IIDR should be written before any other registers.
I'd like to make it crystal clear that writing to IIDR doesn't allow to
select a behaviour, and is merely a confirmation that host and guest do
agree on either revision 0 (no write) or revision n (n being read and
written back).
Also, on ordering: does the write to IIDR have to happen before any
write to other distributor registers? Or to any GIC register? I think
you mean the former, but I wonder if we shouldn't mandate the latter,
just to be sure...
> Errors:
> -ENXIO: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
> @@ -94,9 +97,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
>
Finally, do we want to to extend the same requirements to GICv3, for the
sake of uniformity?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-16 8:52 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 8:52 UTC (permalink / raw)
To: linux-arm-kernel
On 14/07/18 18:05, Christoffer Dall wrote:
> Update the documentation to reflect the ordering requirements of
> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b3ce126..c9a6393 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> + the read value to confirm its expected behavior is aligned with the KVM
> + implementation. To properly restore the interrupt group configuration,
> + GICD_IIDR should be written before any other registers.
I'd like to make it crystal clear that writing to IIDR doesn't allow to
select a behaviour, and is merely a confirmation that host and guest do
agree on either revision 0 (no write) or revision n (n being read and
written back).
Also, on ordering: does the write to IIDR have to happen before any
write to other distributor registers? Or to any GIC register? I think
you mean the former, but I wonder if we shouldn't mandate the latter,
just to be sure...
> Errors:
> -ENXIO: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
> @@ -94,9 +97,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
>
Finally, do we want to to extend the same requirements to GICv3, for the
sake of uniformity?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-16 8:52 ` Marc Zyngier
@ 2018-07-16 9:54 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:54 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..c9a6393 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> > + the read value to confirm its expected behavior is aligned with the KVM
> > + implementation. To properly restore the interrupt group configuration,
> > + GICD_IIDR should be written before any other registers.
>
> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> select a behaviour, and is merely a confirmation that host and guest do
> agree on either revision 0 (no write) or revision n (n being read and
> written back).
>
> Also, on ordering: does the write to IIDR have to happen before any
> write to other distributor registers? Or to any GIC register? I think
> you mean the former, but I wonder if we shouldn't mandate the latter,
> just to be sure...
>
I was thinking that IIDR should simply be the very first thing you
write, I'll clarify the intent.
> > Errors:
> > -ENXIO: Getting or setting this register is not yet supported
> > -EBUSY: One or more VCPUs are running
> > @@ -94,9 +97,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
> >
>
> Finally, do we want to to extend the same requirements to GICv3, for the
> sake of uniformity?
Probably. I'll check the userspace implementation if that would break
compatibility with today.
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-16 9:54 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 9:54 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..c9a6393 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> > + the read value to confirm its expected behavior is aligned with the KVM
> > + implementation. To properly restore the interrupt group configuration,
> > + GICD_IIDR should be written before any other registers.
>
> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> select a behaviour, and is merely a confirmation that host and guest do
> agree on either revision 0 (no write) or revision n (n being read and
> written back).
>
> Also, on ordering: does the write to IIDR have to happen before any
> write to other distributor registers? Or to any GIC register? I think
> you mean the former, but I wonder if we shouldn't mandate the latter,
> just to be sure...
>
I was thinking that IIDR should simply be the very first thing you
write, I'll clarify the intent.
> > Errors:
> > -ENXIO: Getting or setting this register is not yet supported
> > -EBUSY: One or more VCPUs are running
> > @@ -94,9 +97,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
> >
>
> Finally, do we want to to extend the same requirements to GICv3, for the
> sake of uniformity?
Probably. I'll check the userspace implementation if that would break
compatibility with today.
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-16 8:52 ` Marc Zyngier
@ 2018-07-16 10:34 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 10:34 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..c9a6393 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> > + the read value to confirm its expected behavior is aligned with the KVM
> > + implementation. To properly restore the interrupt group configuration,
> > + GICD_IIDR should be written before any other registers.
>
> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> select a behaviour, and is merely a confirmation that host and guest do
> agree on either revision 0 (no write) or revision n (n being read and
> written back).
>
But that's not really true though, because userspace can read rev 0, and
still have "no write", or read and write n for n >= 1 and have "write".
I guess I'm not completely sure which wording you'd like me to add?
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-16 10:34 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 10:34 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> On 14/07/18 18:05, Christoffer Dall wrote:
> > Update the documentation to reflect the ordering requirements of
> > restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > index b3ce126..c9a6393 100644
> > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> > @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> > + the read value to confirm its expected behavior is aligned with the KVM
> > + implementation. To properly restore the interrupt group configuration,
> > + GICD_IIDR should be written before any other registers.
>
> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> select a behaviour, and is merely a confirmation that host and guest do
> agree on either revision 0 (no write) or revision n (n being read and
> written back).
>
But that's not really true though, because userspace can read rev 0, and
still have "no write", or read and write n for n >= 1 and have "write".
I guess I'm not completely sure which wording you'd like me to add?
Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-16 10:34 ` Christoffer Dall
@ 2018-07-16 11:07 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 11:07 UTC (permalink / raw)
To: Christoffer Dall; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On 16/07/18 11:34, Christoffer Dall wrote:
> On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
>> On 14/07/18 18:05, Christoffer Dall wrote:
>>> Update the documentation to reflect the ordering requirements of
>>> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> index b3ce126..c9a6393 100644
>>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
>>> + the read value to confirm its expected behavior is aligned with the KVM
>>> + implementation. To properly restore the interrupt group configuration,
>>> + GICD_IIDR should be written before any other registers.
>>
>> I'd like to make it crystal clear that writing to IIDR doesn't allow to
>> select a behaviour, and is merely a confirmation that host and guest do
>> agree on either revision 0 (no write) or revision n (n being read and
>> written back).
>>
>
> But that's not really true though, because userspace can read rev 0, and
> still have "no write", or read and write n for n >= 1 and have "write".
>
> I guess I'm not completely sure which wording you'd like me to add?
I'm not sure either, because I find the behaviour of this register a bit
odd. We always default to rev 0, and allow the behaviour to be switched
to rev 2 if we write 2 to it. But we can't select rev 1.
So in a way, it is not a revision selection API (like we have with the
ITS), but a "I don't want rev 0" API. It works for what we have now, but
I'm not sure how future proof it is (I guess we can cross that bridge
when we get there).
Another thing is that the guest will always see rev 2, even when it runs
with the rev 0 behaviour, which is quite bizarre.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-16 11:07 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2018-07-16 11:07 UTC (permalink / raw)
To: linux-arm-kernel
On 16/07/18 11:34, Christoffer Dall wrote:
> On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
>> On 14/07/18 18:05, Christoffer Dall wrote:
>>> Update the documentation to reflect the ordering requirements of
>>> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> index b3ce126..c9a6393 100644
>>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
>>> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
>>> + the read value to confirm its expected behavior is aligned with the KVM
>>> + implementation. To properly restore the interrupt group configuration,
>>> + GICD_IIDR should be written before any other registers.
>>
>> I'd like to make it crystal clear that writing to IIDR doesn't allow to
>> select a behaviour, and is merely a confirmation that host and guest do
>> agree on either revision 0 (no write) or revision n (n being read and
>> written back).
>>
>
> But that's not really true though, because userspace can read rev 0, and
> still have "no write", or read and write n for n >= 1 and have "write".
>
> I guess I'm not completely sure which wording you'd like me to add?
I'm not sure either, because I find the behaviour of this register a bit
odd. We always default to rev 0, and allow the behaviour to be switched
to rev 2 if we write 2 to it. But we can't select rev 1.
So in a way, it is not a revision selection API (like we have with the
ITS), but a "I don't want rev 0" API. It works for what we have now, but
I'm not sure how future proof it is (I guess we can cross that bridge
when we get there).
Another thing is that the guest will always see rev 2, even when it runs
with the rev 0 behaviour, which is quite bizarre.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
2018-07-16 11:07 ` Marc Zyngier
@ 2018-07-16 11:38 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 11:38 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Jul 16, 2018 at 12:07:04PM +0100, Marc Zyngier wrote:
> On 16/07/18 11:34, Christoffer Dall wrote:
> > On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> >> On 14/07/18 18:05, Christoffer Dall wrote:
> >>> Update the documentation to reflect the ordering requirements of
> >>> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> >>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> index b3ce126..c9a6393 100644
> >>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> >>> + the read value to confirm its expected behavior is aligned with the KVM
> >>> + implementation. To properly restore the interrupt group configuration,
> >>> + GICD_IIDR should be written before any other registers.
> >>
> >> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> >> select a behaviour, and is merely a confirmation that host and guest do
> >> agree on either revision 0 (no write) or revision n (n being read and
> >> written back).
> >>
> >
> > But that's not really true though, because userspace can read rev 0, and
> > still have "no write", or read and write n for n >= 1 and have "write".
> >
> > I guess I'm not completely sure which wording you'd like me to add?
> I'm not sure either, because I find the behaviour of this register a bit
> odd. We always default to rev 0,
Not quite. From the PoV of the guest, we default to rev 2 (which it
cannot differentiate from rev 1), becasue it is itself allowed to change
the groups, which it couldn't do in rev 0.
However, from the PoV of userspace...
> and allow the behaviour to be switched
> to rev 2 if we write 2 to it. But we can't select rev 1.
For future changes, which are likely to be unrelated to IGROUPR, you get
rev N behavior where N is the most recent N, except that IGROUPR won't
be writable unless userspace confirms that it knows about revisions.
>
> So in a way, it is not a revision selection API (like we have with the
> ITS), but a "I don't want rev 0" API. It works for what we have now, but
> I'm not sure how future proof it is (I guess we can cross that bridge
> when we get there).
This is by design. I would like to avoid having
switch (rev) {
case 2: logic2();
case 3: logic3();
case 4: logic4();
}
spread out over all uaccess writes.
Instead, I'd like for the GICD_IIDR write to fail, and let userspace
deal with things for the future. However, the problem is that we don't
have any such logic to use right now, and therefore we have to support
the backwards compat case now.
We could introduce a separate attribute on the GIC (IGROUPR_WRITABLE)
instead, I just found it less noisy to piggy back on the GICD_IIDR.
Is that acceptable?
>
> Another thing is that the guest will always see rev 2, even when it runs
> with the rev 0 behaviour, which is quite bizarre.
>
I don't think this is bizarre. As it stand now, it will see rev 1
behavior, which is identical from the guest PoV to rev 2 behavior.
And I don't think there's anything in the architecture that says that a
revision change must be directly observable by the guest?
In any case, I've come up with a new proposal for some documentation and
I hope it will be more to your liking.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 9/9] KVM: arm/arm64: vgic: Update documentation of the GICv2 device
@ 2018-07-16 11:38 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2018-07-16 11:38 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2018 at 12:07:04PM +0100, Marc Zyngier wrote:
> On 16/07/18 11:34, Christoffer Dall wrote:
> > On Mon, Jul 16, 2018 at 09:52:04AM +0100, Marc Zyngier wrote:
> >> On 14/07/18 18:05, Christoffer Dall wrote:
> >>> Update the documentation to reflect the ordering requirements of
> >>> restoring GICv2 distributor registers and remove 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.txt | 12 ++++++------
> >>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> index b3ce126..c9a6393 100644
> >>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> >>> @@ -49,9 +49,12 @@ 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. Userspace should read GICD_IIDR from KVM and write back
> >>> + the read value to confirm its expected behavior is aligned with the KVM
> >>> + implementation. To properly restore the interrupt group configuration,
> >>> + GICD_IIDR should be written before any other registers.
> >>
> >> I'd like to make it crystal clear that writing to IIDR doesn't allow to
> >> select a behaviour, and is merely a confirmation that host and guest do
> >> agree on either revision 0 (no write) or revision n (n being read and
> >> written back).
> >>
> >
> > But that's not really true though, because userspace can read rev 0, and
> > still have "no write", or read and write n for n >= 1 and have "write".
> >
> > I guess I'm not completely sure which wording you'd like me to add?
> I'm not sure either, because I find the behaviour of this register a bit
> odd. We always default to rev 0,
Not quite. From the PoV of the guest, we default to rev 2 (which it
cannot differentiate from rev 1), becasue it is itself allowed to change
the groups, which it couldn't do in rev 0.
However, from the PoV of userspace...
> and allow the behaviour to be switched
> to rev 2 if we write 2 to it. But we can't select rev 1.
For future changes, which are likely to be unrelated to IGROUPR, you get
rev N behavior where N is the most recent N, except that IGROUPR won't
be writable unless userspace confirms that it knows about revisions.
>
> So in a way, it is not a revision selection API (like we have with the
> ITS), but a "I don't want rev 0" API. It works for what we have now, but
> I'm not sure how future proof it is (I guess we can cross that bridge
> when we get there).
This is by design. I would like to avoid having
switch (rev) {
case 2: logic2();
case 3: logic3();
case 4: logic4();
}
spread out over all uaccess writes.
Instead, I'd like for the GICD_IIDR write to fail, and let userspace
deal with things for the future. However, the problem is that we don't
have any such logic to use right now, and therefore we have to support
the backwards compat case now.
We could introduce a separate attribute on the GIC (IGROUPR_WRITABLE)
instead, I just found it less noisy to piggy back on the GICD_IIDR.
Is that acceptable?
>
> Another thing is that the guest will always see rev 2, even when it runs
> with the rev 0 behaviour, which is quite bizarre.
>
I don't think this is bizarre. As it stand now, it will see rev 1
behavior, which is identical from the guest PoV to rev 2 behavior.
And I don't think there's anything in the architecture that says that a
revision change must be directly observable by the guest?
In any case, I've come up with a new proposal for some documentation and
I hope it will be more to your liking.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread