* [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D, R}_I{S, C}ACTIVER read
@ 2020-04-14 10:35 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-14 10:35 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: Julien Grall, Suzuki K Poulose, Andre Przywara, Eric Auger,
James Morse, Zenghui Yu, Julien Thierry
When a guest tries to read the active state of its interrupts,
we currently just return whatever state we have in memory. This
means that if such an interrupt lives in a List Register on another
CPU, we fail to obsertve the latest active state for this interrupt.
In order to remedy this, stop all the other vcpus so that they exit
and we can observe the most recent value for the state.
Reported-by: Julien Grall <julien@xen.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
virt/kvm/arm/vgic/vgic-mmio.h | 3 +
4 files changed, 71 insertions(+), 40 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 5945f062d749..d63881f60e1a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
vgic_mmio_read_active, vgic_mmio_write_sactive,
- NULL, vgic_mmio_uaccess_write_sactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
vgic_mmio_read_active, vgic_mmio_write_cactive,
- NULL, vgic_mmio_uaccess_write_cactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index e72dcc454247..77c8ba1a2535 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
vgic_mmio_read_active, vgic_mmio_write_sactive,
- NULL, vgic_mmio_uaccess_write_sactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
vgic_mmio_read_active, vgic_mmio_write_cactive,
- NULL, vgic_mmio_uaccess_write_cactive,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
1, VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 2199302597fa..4012cd68ac93 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
}
}
-unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
+
+/*
+ * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
+ * is not queued on some running VCPU's LRs, because then the change to the
+ * active state can be overwritten when the VCPU's state is synced coming back
+ * from the guest.
+ *
+ * For shared interrupts as well as GICv3 private interrupts, we have to
+ * stop all the VCPUs because interrupts can be migrated while we don't hold
+ * the IRQ locks and we don't want to be chasing moving targets.
+ *
+ * For GICv2 private interrupts we don't have to do anything because
+ * userspace accesses to the VGIC state already require all VCPUs to be
+ * stopped, and only the VCPU itself can modify its private interrupts
+ * active state, which guarantees that the VCPU is not running.
+ */
+static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
+{
+ if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+ intid > VGIC_NR_PRIVATE_IRQS)
+ kvm_arm_halt_guest(vcpu->kvm);
+}
+
+/* See vgic_access_active_prepare */
+static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
+{
+ if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+ intid > VGIC_NR_PRIVATE_IRQS)
+ kvm_arm_resume_guest(vcpu->kvm);
+}
+
+static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
u32 value = 0;
@@ -359,6 +390,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+ /*
+ * Even for HW interrupts, don't evaluate the HW state as
+ * all the guest is interested in is the virtual state.
+ */
if (irq->active)
value |= (1U << i);
@@ -368,6 +403,29 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
return value;
}
+unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ u32 val;
+
+ mutex_lock(&vcpu->kvm->lock);
+ vgic_access_active_prepare(vcpu, intid);
+
+ val = __vgic_mmio_read_active(vcpu, addr, len);
+
+ vgic_access_active_finish(vcpu, intid);
+ mutex_unlock(&vcpu->kvm->lock);
+
+ return val;
+}
+
+unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ return __vgic_mmio_read_active(vcpu, addr, len);
+}
+
/* Must be called with irq->irq_lock held */
static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
bool active, bool is_uaccess)
@@ -426,36 +484,6 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
}
-/*
- * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
- * is not queued on some running VCPU's LRs, because then the change to the
- * active state can be overwritten when the VCPU's state is synced coming back
- * from the guest.
- *
- * For shared interrupts, we have to stop all the VCPUs because interrupts can
- * be migrated while we don't hold the IRQ locks and we don't want to be
- * chasing moving targets.
- *
- * For private interrupts we don't have to do anything because userspace
- * accesses to the VGIC state already require all VCPUs to be stopped, and
- * only the VCPU itself can modify its private interrupts active state, which
- * guarantees that the VCPU is not running.
- */
-static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
-{
- if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
- intid > VGIC_NR_PRIVATE_IRQS)
- kvm_arm_halt_guest(vcpu->kvm);
-}
-
-/* See vgic_change_active_prepare */
-static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
-{
- if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
- intid > VGIC_NR_PRIVATE_IRQS)
- kvm_arm_resume_guest(vcpu->kvm);
-}
-
static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -477,11 +505,11 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
mutex_lock(&vcpu->kvm->lock);
- vgic_change_active_prepare(vcpu, intid);
+ vgic_access_active_prepare(vcpu, intid);
__vgic_mmio_write_cactive(vcpu, addr, len, val);
- vgic_change_active_finish(vcpu, intid);
+ vgic_access_active_finish(vcpu, intid);
mutex_unlock(&vcpu->kvm->lock);
}
@@ -514,11 +542,11 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
mutex_lock(&vcpu->kvm->lock);
- vgic_change_active_prepare(vcpu, intid);
+ vgic_access_active_prepare(vcpu, intid);
__vgic_mmio_write_sactive(vcpu, addr, len, val);
- vgic_change_active_finish(vcpu, intid);
+ vgic_access_active_finish(vcpu, intid);
mutex_unlock(&vcpu->kvm->lock);
}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5af2aefad435..30713a44e3fa 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -152,6 +152,9 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
+unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len);
+
void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D, R}_I{S, C}ACTIVER read
@ 2020-04-14 10:35 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-14 10:35 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm; +Cc: Julien Grall, Andre Przywara
When a guest tries to read the active state of its interrupts,
we currently just return whatever state we have in memory. This
means that if such an interrupt lives in a List Register on another
CPU, we fail to obsertve the latest active state for this interrupt.
In order to remedy this, stop all the other vcpus so that they exit
and we can observe the most recent value for the state.
Reported-by: Julien Grall <julien@xen.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
virt/kvm/arm/vgic/vgic-mmio.h | 3 +
4 files changed, 71 insertions(+), 40 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 5945f062d749..d63881f60e1a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
vgic_mmio_read_active, vgic_mmio_write_sactive,
- NULL, vgic_mmio_uaccess_write_sactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
vgic_mmio_read_active, vgic_mmio_write_cactive,
- NULL, vgic_mmio_uaccess_write_cactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index e72dcc454247..77c8ba1a2535 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
vgic_mmio_read_active, vgic_mmio_write_sactive,
- NULL, vgic_mmio_uaccess_write_sactive, 1,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
vgic_mmio_read_active, vgic_mmio_write_cactive,
- NULL, vgic_mmio_uaccess_write_cactive,
+ vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
1, VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 2199302597fa..4012cd68ac93 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
}
}
-unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
+
+/*
+ * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
+ * is not queued on some running VCPU's LRs, because then the change to the
+ * active state can be overwritten when the VCPU's state is synced coming back
+ * from the guest.
+ *
+ * For shared interrupts as well as GICv3 private interrupts, we have to
+ * stop all the VCPUs because interrupts can be migrated while we don't hold
+ * the IRQ locks and we don't want to be chasing moving targets.
+ *
+ * For GICv2 private interrupts we don't have to do anything because
+ * userspace accesses to the VGIC state already require all VCPUs to be
+ * stopped, and only the VCPU itself can modify its private interrupts
+ * active state, which guarantees that the VCPU is not running.
+ */
+static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
+{
+ if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+ intid > VGIC_NR_PRIVATE_IRQS)
+ kvm_arm_halt_guest(vcpu->kvm);
+}
+
+/* See vgic_access_active_prepare */
+static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
+{
+ if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+ intid > VGIC_NR_PRIVATE_IRQS)
+ kvm_arm_resume_guest(vcpu->kvm);
+}
+
+static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
u32 value = 0;
@@ -359,6 +390,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+ /*
+ * Even for HW interrupts, don't evaluate the HW state as
+ * all the guest is interested in is the virtual state.
+ */
if (irq->active)
value |= (1U << i);
@@ -368,6 +403,29 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
return value;
}
+unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ u32 val;
+
+ mutex_lock(&vcpu->kvm->lock);
+ vgic_access_active_prepare(vcpu, intid);
+
+ val = __vgic_mmio_read_active(vcpu, addr, len);
+
+ vgic_access_active_finish(vcpu, intid);
+ mutex_unlock(&vcpu->kvm->lock);
+
+ return val;
+}
+
+unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ return __vgic_mmio_read_active(vcpu, addr, len);
+}
+
/* Must be called with irq->irq_lock held */
static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
bool active, bool is_uaccess)
@@ -426,36 +484,6 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
}
-/*
- * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
- * is not queued on some running VCPU's LRs, because then the change to the
- * active state can be overwritten when the VCPU's state is synced coming back
- * from the guest.
- *
- * For shared interrupts, we have to stop all the VCPUs because interrupts can
- * be migrated while we don't hold the IRQ locks and we don't want to be
- * chasing moving targets.
- *
- * For private interrupts we don't have to do anything because userspace
- * accesses to the VGIC state already require all VCPUs to be stopped, and
- * only the VCPU itself can modify its private interrupts active state, which
- * guarantees that the VCPU is not running.
- */
-static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
-{
- if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
- intid > VGIC_NR_PRIVATE_IRQS)
- kvm_arm_halt_guest(vcpu->kvm);
-}
-
-/* See vgic_change_active_prepare */
-static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
-{
- if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
- intid > VGIC_NR_PRIVATE_IRQS)
- kvm_arm_resume_guest(vcpu->kvm);
-}
-
static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -477,11 +505,11 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
mutex_lock(&vcpu->kvm->lock);
- vgic_change_active_prepare(vcpu, intid);
+ vgic_access_active_prepare(vcpu, intid);
__vgic_mmio_write_cactive(vcpu, addr, len, val);
- vgic_change_active_finish(vcpu, intid);
+ vgic_access_active_finish(vcpu, intid);
mutex_unlock(&vcpu->kvm->lock);
}
@@ -514,11 +542,11 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
mutex_lock(&vcpu->kvm->lock);
- vgic_change_active_prepare(vcpu, intid);
+ vgic_access_active_prepare(vcpu, intid);
__vgic_mmio_write_sactive(vcpu, addr, len, val);
- vgic_change_active_finish(vcpu, intid);
+ vgic_access_active_finish(vcpu, intid);
mutex_unlock(&vcpu->kvm->lock);
}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5af2aefad435..30713a44e3fa 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -152,6 +152,9 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
+unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len);
+
void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
--
2.25.1
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
2020-04-14 10:35 ` Marc Zyngier
(?)
@ 2020-04-14 11:16 ` André Przywara
-1 siblings, 0 replies; 24+ messages in thread
From: André Przywara @ 2020-04-14 11:16 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm
Cc: Zenghui Yu, Eric Auger, Julien Grall, James Morse,
Julien Thierry, Suzuki K Poulose
On 14/04/2020 11:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
^^^^^^^^
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
Maybe worth mentioning that this copies the approach we already deal
with write accesses (split userland and guess accessors). This is in the
cover letter, but until I found it there it took me a while to grasp
what this patch really does.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 2199302597fa..4012cd68ac93 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> }
> }
>
> -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> +
> +/*
> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> + * is not queued on some running VCPU's LRs, because then the change to the
> + * active state can be overwritten when the VCPU's state is synced coming back
> + * from the guest.
> + *
> + * For shared interrupts as well as GICv3 private interrupts, we have to
> + * stop all the VCPUs because interrupts can be migrated while we don't hold
> + * the IRQ locks and we don't want to be chasing moving targets.
> + *
> + * For GICv2 private interrupts we don't have to do anything because
> + * userspace accesses to the VGIC state already require all VCPUs to be
> + * stopped, and only the VCPU itself can modify its private interrupts
> + * active state, which guarantees that the VCPU is not running.
> + */
> +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
I understand that this is just moved from existing code below, but
shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
"intid > VGIC_MAX_PRIVATE"?
Rest looks alright.
Cheers,
Andre
> + kvm_arm_halt_guest(vcpu->kvm);
> +}
> +
> +/* See vgic_access_active_prepare */
> +static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
> + kvm_arm_resume_guest(vcpu->kvm);
> +}
> +
> +static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> {
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> u32 value = 0;
> @@ -359,6 +390,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> for (i = 0; i < len * 8; i++) {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> + /*
> + * Even for HW interrupts, don't evaluate the HW state as
> + * all the guest is interested in is the virtual state.
> + */
> if (irq->active)
> value |= (1U << i);
>
> @@ -368,6 +403,29 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 val;
> +
> + mutex_lock(&vcpu->kvm->lock);
> + vgic_access_active_prepare(vcpu, intid);
> +
> + val = __vgic_mmio_read_active(vcpu, addr, len);
> +
> + vgic_access_active_finish(vcpu, intid);
> + mutex_unlock(&vcpu->kvm->lock);
> +
> + return val;
> +}
> +
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __vgic_mmio_read_active(vcpu, addr, len);
> +}
> +
> /* Must be called with irq->irq_lock held */
> static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> bool active, bool is_uaccess)
> @@ -426,36 +484,6 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> }
>
> -/*
> - * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> - * is not queued on some running VCPU's LRs, because then the change to the
> - * active state can be overwritten when the VCPU's state is synced coming back
> - * from the guest.
> - *
> - * For shared interrupts, we have to stop all the VCPUs because interrupts can
> - * be migrated while we don't hold the IRQ locks and we don't want to be
> - * chasing moving targets.
> - *
> - * For private interrupts we don't have to do anything because userspace
> - * accesses to the VGIC state already require all VCPUs to be stopped, and
> - * only the VCPU itself can modify its private interrupts active state, which
> - * guarantees that the VCPU is not running.
> - */
> -static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_halt_guest(vcpu->kvm);
> -}
> -
> -/* See vgic_change_active_prepare */
> -static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_resume_guest(vcpu->kvm);
> -}
> -
> static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -477,11 +505,11 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_cactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> @@ -514,11 +542,11 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_sactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 5af2aefad435..30713a44e3fa 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -152,6 +152,9 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len);
>
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-14 11:16 ` André Przywara
0 siblings, 0 replies; 24+ messages in thread
From: André Przywara @ 2020-04-14 11:16 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm
Cc: Julien Grall, Suzuki K Poulose, Eric Auger, James Morse,
Zenghui Yu, Julien Thierry
On 14/04/2020 11:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
^^^^^^^^
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
Maybe worth mentioning that this copies the approach we already deal
with write accesses (split userland and guess accessors). This is in the
cover letter, but until I found it there it took me a while to grasp
what this patch really does.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 2199302597fa..4012cd68ac93 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> }
> }
>
> -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> +
> +/*
> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> + * is not queued on some running VCPU's LRs, because then the change to the
> + * active state can be overwritten when the VCPU's state is synced coming back
> + * from the guest.
> + *
> + * For shared interrupts as well as GICv3 private interrupts, we have to
> + * stop all the VCPUs because interrupts can be migrated while we don't hold
> + * the IRQ locks and we don't want to be chasing moving targets.
> + *
> + * For GICv2 private interrupts we don't have to do anything because
> + * userspace accesses to the VGIC state already require all VCPUs to be
> + * stopped, and only the VCPU itself can modify its private interrupts
> + * active state, which guarantees that the VCPU is not running.
> + */
> +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
I understand that this is just moved from existing code below, but
shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
"intid > VGIC_MAX_PRIVATE"?
Rest looks alright.
Cheers,
Andre
> + kvm_arm_halt_guest(vcpu->kvm);
> +}
> +
> +/* See vgic_access_active_prepare */
> +static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
> + kvm_arm_resume_guest(vcpu->kvm);
> +}
> +
> +static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> {
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> u32 value = 0;
> @@ -359,6 +390,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> for (i = 0; i < len * 8; i++) {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> + /*
> + * Even for HW interrupts, don't evaluate the HW state as
> + * all the guest is interested in is the virtual state.
> + */
> if (irq->active)
> value |= (1U << i);
>
> @@ -368,6 +403,29 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 val;
> +
> + mutex_lock(&vcpu->kvm->lock);
> + vgic_access_active_prepare(vcpu, intid);
> +
> + val = __vgic_mmio_read_active(vcpu, addr, len);
> +
> + vgic_access_active_finish(vcpu, intid);
> + mutex_unlock(&vcpu->kvm->lock);
> +
> + return val;
> +}
> +
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __vgic_mmio_read_active(vcpu, addr, len);
> +}
> +
> /* Must be called with irq->irq_lock held */
> static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> bool active, bool is_uaccess)
> @@ -426,36 +484,6 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> }
>
> -/*
> - * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> - * is not queued on some running VCPU's LRs, because then the change to the
> - * active state can be overwritten when the VCPU's state is synced coming back
> - * from the guest.
> - *
> - * For shared interrupts, we have to stop all the VCPUs because interrupts can
> - * be migrated while we don't hold the IRQ locks and we don't want to be
> - * chasing moving targets.
> - *
> - * For private interrupts we don't have to do anything because userspace
> - * accesses to the VGIC state already require all VCPUs to be stopped, and
> - * only the VCPU itself can modify its private interrupts active state, which
> - * guarantees that the VCPU is not running.
> - */
> -static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_halt_guest(vcpu->kvm);
> -}
> -
> -/* See vgic_change_active_prepare */
> -static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_resume_guest(vcpu->kvm);
> -}
> -
> static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -477,11 +505,11 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_cactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> @@ -514,11 +542,11 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_sactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 5af2aefad435..30713a44e3fa 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -152,6 +152,9 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len);
>
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-14 11:16 ` André Przywara
0 siblings, 0 replies; 24+ messages in thread
From: André Przywara @ 2020-04-14 11:16 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm; +Cc: Julien Grall
On 14/04/2020 11:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
^^^^^^^^
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
Maybe worth mentioning that this copies the approach we already deal
with write accesses (split userland and guess accessors). This is in the
cover letter, but until I found it there it took me a while to grasp
what this patch really does.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 2199302597fa..4012cd68ac93 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> }
> }
>
> -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> +
> +/*
> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> + * is not queued on some running VCPU's LRs, because then the change to the
> + * active state can be overwritten when the VCPU's state is synced coming back
> + * from the guest.
> + *
> + * For shared interrupts as well as GICv3 private interrupts, we have to
> + * stop all the VCPUs because interrupts can be migrated while we don't hold
> + * the IRQ locks and we don't want to be chasing moving targets.
> + *
> + * For GICv2 private interrupts we don't have to do anything because
> + * userspace accesses to the VGIC state already require all VCPUs to be
> + * stopped, and only the VCPU itself can modify its private interrupts
> + * active state, which guarantees that the VCPU is not running.
> + */
> +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
I understand that this is just moved from existing code below, but
shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
"intid > VGIC_MAX_PRIVATE"?
Rest looks alright.
Cheers,
Andre
> + kvm_arm_halt_guest(vcpu->kvm);
> +}
> +
> +/* See vgic_access_active_prepare */
> +static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> +{
> + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> + intid > VGIC_NR_PRIVATE_IRQS)
> + kvm_arm_resume_guest(vcpu->kvm);
> +}
> +
> +static unsigned long __vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> {
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> u32 value = 0;
> @@ -359,6 +390,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> for (i = 0; i < len * 8; i++) {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> + /*
> + * Even for HW interrupts, don't evaluate the HW state as
> + * all the guest is interested in is the virtual state.
> + */
> if (irq->active)
> value |= (1U << i);
>
> @@ -368,6 +403,29 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 val;
> +
> + mutex_lock(&vcpu->kvm->lock);
> + vgic_access_active_prepare(vcpu, intid);
> +
> + val = __vgic_mmio_read_active(vcpu, addr, len);
> +
> + vgic_access_active_finish(vcpu, intid);
> + mutex_unlock(&vcpu->kvm->lock);
> +
> + return val;
> +}
> +
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __vgic_mmio_read_active(vcpu, addr, len);
> +}
> +
> /* Must be called with irq->irq_lock held */
> static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> bool active, bool is_uaccess)
> @@ -426,36 +484,6 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> }
>
> -/*
> - * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> - * is not queued on some running VCPU's LRs, because then the change to the
> - * active state can be overwritten when the VCPU's state is synced coming back
> - * from the guest.
> - *
> - * For shared interrupts, we have to stop all the VCPUs because interrupts can
> - * be migrated while we don't hold the IRQ locks and we don't want to be
> - * chasing moving targets.
> - *
> - * For private interrupts we don't have to do anything because userspace
> - * accesses to the VGIC state already require all VCPUs to be stopped, and
> - * only the VCPU itself can modify its private interrupts active state, which
> - * guarantees that the VCPU is not running.
> - */
> -static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_halt_guest(vcpu->kvm);
> -}
> -
> -/* See vgic_change_active_prepare */
> -static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> -{
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> - intid > VGIC_NR_PRIVATE_IRQS)
> - kvm_arm_resume_guest(vcpu->kvm);
> -}
> -
> static void __vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -477,11 +505,11 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_cactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> @@ -514,11 +542,11 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>
> mutex_lock(&vcpu->kvm->lock);
> - vgic_change_active_prepare(vcpu, intid);
> + vgic_access_active_prepare(vcpu, intid);
>
> __vgic_mmio_write_sactive(vcpu, addr, len, val);
>
> - vgic_change_active_finish(vcpu, intid);
> + vgic_access_active_finish(vcpu, intid);
> mutex_unlock(&vcpu->kvm->lock);
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 5af2aefad435..30713a44e3fa 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -152,6 +152,9 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len);
>
> +unsigned long vgic_uaccess_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
2020-04-14 11:16 ` André Przywara
(?)
@ 2020-04-14 13:43 ` Marc Zyngier
-1 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-14 13:43 UTC (permalink / raw)
To: André Przywara
Cc: linux-arm-kernel, kvmarm, kvm, Zenghui Yu, Eric Auger,
Julien Grall, James Morse, Julien Thierry, Suzuki K Poulose
On Tue, 14 Apr 2020 12:16:27 +0100
André Przywara <andre.przywara@arm.com> wrote:
> On 14/04/2020 11:35, Marc Zyngier wrote:
> > When a guest tries to read the active state of its interrupts,
> > we currently just return whatever state we have in memory. This
> > means that if such an interrupt lives in a List Register on another
> > CPU, we fail to obsertve the latest active state for this interrupt.
>
> ^^^^^^^^
>
> > In order to remedy this, stop all the other vcpus so that they exit
> > and we can observe the most recent value for the state.
>
> Maybe worth mentioning that this copies the approach we already deal
> with write accesses (split userland and guess accessors). This is in the
> cover letter, but until I found it there it took me a while to grasp
> what this patch really does.
Fair enough.
>
> >
> > Reported-by: Julien Grall <julien@xen.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> > virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> > 4 files changed, 71 insertions(+), 40 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 5945f062d749..d63881f60e1a 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index e72dcc454247..77c8ba1a2535 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> > 1, VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 2199302597fa..4012cd68ac93 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len)
> > +
> > +/*
> > + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> > + * is not queued on some running VCPU's LRs, because then the change to the
> > + * active state can be overwritten when the VCPU's state is synced coming back
> > + * from the guest.
> > + *
> > + * For shared interrupts as well as GICv3 private interrupts, we have to
> > + * stop all the VCPUs because interrupts can be migrated while we don't hold
> > + * the IRQ locks and we don't want to be chasing moving targets.
> > + *
> > + * For GICv2 private interrupts we don't have to do anything because
> > + * userspace accesses to the VGIC state already require all VCPUs to be
> > + * stopped, and only the VCPU itself can modify its private interrupts
> > + * active state, which guarantees that the VCPU is not running.
> > + */
> > +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> > +{
> > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> > + intid > VGIC_NR_PRIVATE_IRQS)
>
> I understand that this is just moved from existing code below, but
> shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
> "intid > VGIC_MAX_PRIVATE"?
Nice catch. This was introduced in abd7229626b93 ("KVM: arm/arm64:
Simplify active_change_prepare and plug race"), while we had the
opposite condition before that.
This means that on GICv2, GICD_I[CS]ACTIVER writes are unreliable for
intids 32-63 (we may fail to clear an active bit if it is set in
another vcpu's LRs, for example).
I'll add an extra patch for this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-14 13:43 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-14 13:43 UTC (permalink / raw)
To: André Przywara
Cc: Julien Grall, kvm, Suzuki K Poulose, Eric Auger, James Morse,
Julien Thierry, Zenghui Yu, kvmarm, linux-arm-kernel
On Tue, 14 Apr 2020 12:16:27 +0100
André Przywara <andre.przywara@arm.com> wrote:
> On 14/04/2020 11:35, Marc Zyngier wrote:
> > When a guest tries to read the active state of its interrupts,
> > we currently just return whatever state we have in memory. This
> > means that if such an interrupt lives in a List Register on another
> > CPU, we fail to obsertve the latest active state for this interrupt.
>
> ^^^^^^^^
>
> > In order to remedy this, stop all the other vcpus so that they exit
> > and we can observe the most recent value for the state.
>
> Maybe worth mentioning that this copies the approach we already deal
> with write accesses (split userland and guess accessors). This is in the
> cover letter, but until I found it there it took me a while to grasp
> what this patch really does.
Fair enough.
>
> >
> > Reported-by: Julien Grall <julien@xen.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> > virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> > 4 files changed, 71 insertions(+), 40 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 5945f062d749..d63881f60e1a 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index e72dcc454247..77c8ba1a2535 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> > 1, VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 2199302597fa..4012cd68ac93 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len)
> > +
> > +/*
> > + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> > + * is not queued on some running VCPU's LRs, because then the change to the
> > + * active state can be overwritten when the VCPU's state is synced coming back
> > + * from the guest.
> > + *
> > + * For shared interrupts as well as GICv3 private interrupts, we have to
> > + * stop all the VCPUs because interrupts can be migrated while we don't hold
> > + * the IRQ locks and we don't want to be chasing moving targets.
> > + *
> > + * For GICv2 private interrupts we don't have to do anything because
> > + * userspace accesses to the VGIC state already require all VCPUs to be
> > + * stopped, and only the VCPU itself can modify its private interrupts
> > + * active state, which guarantees that the VCPU is not running.
> > + */
> > +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> > +{
> > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> > + intid > VGIC_NR_PRIVATE_IRQS)
>
> I understand that this is just moved from existing code below, but
> shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
> "intid > VGIC_MAX_PRIVATE"?
Nice catch. This was introduced in abd7229626b93 ("KVM: arm/arm64:
Simplify active_change_prepare and plug race"), while we had the
opposite condition before that.
This means that on GICv2, GICD_I[CS]ACTIVER writes are unreliable for
intids 32-63 (we may fail to clear an active bit if it is set in
another vcpu's LRs, for example).
I'll add an extra patch for this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-14 13:43 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-14 13:43 UTC (permalink / raw)
To: André Przywara; +Cc: Julien Grall, kvm, kvmarm, linux-arm-kernel
On Tue, 14 Apr 2020 12:16:27 +0100
André Przywara <andre.przywara@arm.com> wrote:
> On 14/04/2020 11:35, Marc Zyngier wrote:
> > When a guest tries to read the active state of its interrupts,
> > we currently just return whatever state we have in memory. This
> > means that if such an interrupt lives in a List Register on another
> > CPU, we fail to obsertve the latest active state for this interrupt.
>
> ^^^^^^^^
>
> > In order to remedy this, stop all the other vcpus so that they exit
> > and we can observe the most recent value for the state.
>
> Maybe worth mentioning that this copies the approach we already deal
> with write accesses (split userland and guess accessors). This is in the
> cover letter, but until I found it there it took me a while to grasp
> what this patch really does.
Fair enough.
>
> >
> > Reported-by: Julien Grall <julien@xen.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> > virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> > virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> > 4 files changed, 71 insertions(+), 40 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 5945f062d749..d63881f60e1a 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index e72dcc454247..77c8ba1a2535 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_sactive,
> > - NULL, vgic_mmio_uaccess_write_sactive, 1,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> > VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> > vgic_mmio_read_active, vgic_mmio_write_cactive,
> > - NULL, vgic_mmio_uaccess_write_cactive,
> > + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> > 1, VGIC_ACCESS_32bit),
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 2199302597fa..4012cd68ac93 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -348,8 +348,39 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > -unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len)
> > +
> > +/*
> > + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> > + * is not queued on some running VCPU's LRs, because then the change to the
> > + * active state can be overwritten when the VCPU's state is synced coming back
> > + * from the guest.
> > + *
> > + * For shared interrupts as well as GICv3 private interrupts, we have to
> > + * stop all the VCPUs because interrupts can be migrated while we don't hold
> > + * the IRQ locks and we don't want to be chasing moving targets.
> > + *
> > + * For GICv2 private interrupts we don't have to do anything because
> > + * userspace accesses to the VGIC state already require all VCPUs to be
> > + * stopped, and only the VCPU itself can modify its private interrupts
> > + * active state, which guarantees that the VCPU is not running.
> > + */
> > +static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> > +{
> > + if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> > + intid > VGIC_NR_PRIVATE_IRQS)
>
> I understand that this is just moved from existing code below, but
> shouldn't that either read "intid >= VGIC_NR_PRIVATE_IRQS" or
> "intid > VGIC_MAX_PRIVATE"?
Nice catch. This was introduced in abd7229626b93 ("KVM: arm/arm64:
Simplify active_change_prepare and plug race"), while we had the
opposite condition before that.
This means that on GICv2, GICD_I[CS]ACTIVER writes are unreliable for
intids 32-63 (we may fail to clear an active bit if it is set in
another vcpu's LRs, for example).
I'll add an extra patch for this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
2020-04-14 10:35 ` Marc Zyngier
(?)
@ 2020-04-15 13:15 ` Zenghui Yu
-1 siblings, 0 replies; 24+ messages in thread
From: Zenghui Yu @ 2020-04-15 13:15 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm
Cc: Eric Auger, Andre Przywara, Julien Grall, James Morse,
Julien Thierry, Suzuki K Poulose
Hi Marc,
On 2020/4/14 18:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
>
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Thanks,
Zenghui
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-15 13:15 ` Zenghui Yu
0 siblings, 0 replies; 24+ messages in thread
From: Zenghui Yu @ 2020-04-15 13:15 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm
Cc: Julien Grall, Suzuki K Poulose, Andre Przywara, Eric Auger,
James Morse, Julien Thierry
Hi Marc,
On 2020/4/14 18:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
>
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Thanks,
Zenghui
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-15 13:15 ` Zenghui Yu
0 siblings, 0 replies; 24+ messages in thread
From: Zenghui Yu @ 2020-04-15 13:15 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm; +Cc: Julien Grall, Andre Przywara
Hi Marc,
On 2020/4/14 18:35, Marc Zyngier wrote:
> When a guest tries to read the active state of its interrupts,
> we currently just return whatever state we have in memory. This
> means that if such an interrupt lives in a List Register on another
> CPU, we fail to obsertve the latest active state for this interrupt.
>
> In order to remedy this, stop all the other vcpus so that they exit
> and we can observe the most recent value for the state.
>
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 100 ++++++++++++++++++++-----------
> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
> 4 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 5945f062d749..d63881f60e1a 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -422,11 +422,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index e72dcc454247..77c8ba1a2535 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -553,11 +553,11 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> - NULL, vgic_mmio_uaccess_write_sactive, 1,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_cactive,
> - NULL, vgic_mmio_uaccess_write_cactive,
> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
> 1, VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
2020-04-15 13:15 ` Zenghui Yu
(?)
@ 2020-04-15 13:27 ` Marc Zyngier
-1 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-15 13:27 UTC (permalink / raw)
To: Zenghui Yu
Cc: linux-arm-kernel, kvmarm, kvm, Eric Auger, Andre Przywara,
Julien Grall, James Morse, Julien Thierry, Suzuki K Poulose
Hi Zenghui,
On 2020-04-15 14:15, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/4/14 18:35, Marc Zyngier wrote:
>> When a guest tries to read the active state of its interrupts,
>> we currently just return whatever state we have in memory. This
>> means that if such an interrupt lives in a List Register on another
>> CPU, we fail to obsertve the latest active state for this interrupt.
>>
>> In order to remedy this, stop all the other vcpus so that they exit
>> and we can observe the most recent value for the state.
>>
>> Reported-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio.c | 100
>> ++++++++++++++++++++-----------
>> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
>> 4 files changed, 71 insertions(+), 40 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 5945f062d749..d63881f60e1a 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -422,11 +422,11 @@ static const struct vgic_register_region
>> vgic_v2_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index e72dcc454247..77c8ba1a2535 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -553,11 +553,11 @@ static const struct vgic_register_region
>> vgic_v3_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
>> 1, VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>
> Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Duh. Yes, of course...
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-15 13:27 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-15 13:27 UTC (permalink / raw)
To: Zenghui Yu
Cc: Julien Grall, kvm, Suzuki K Poulose, Andre Przywara, Eric Auger,
James Morse, Julien Thierry, kvmarm, linux-arm-kernel
Hi Zenghui,
On 2020-04-15 14:15, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/4/14 18:35, Marc Zyngier wrote:
>> When a guest tries to read the active state of its interrupts,
>> we currently just return whatever state we have in memory. This
>> means that if such an interrupt lives in a List Register on another
>> CPU, we fail to obsertve the latest active state for this interrupt.
>>
>> In order to remedy this, stop all the other vcpus so that they exit
>> and we can observe the most recent value for the state.
>>
>> Reported-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio.c | 100
>> ++++++++++++++++++++-----------
>> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
>> 4 files changed, 71 insertions(+), 40 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 5945f062d749..d63881f60e1a 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -422,11 +422,11 @@ static const struct vgic_register_region
>> vgic_v2_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index e72dcc454247..77c8ba1a2535 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -553,11 +553,11 @@ static const struct vgic_register_region
>> vgic_v3_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
>> 1, VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>
> Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Duh. Yes, of course...
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read
@ 2020-04-15 13:27 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2020-04-15 13:27 UTC (permalink / raw)
To: Zenghui Yu; +Cc: Julien Grall, kvm, Andre Przywara, kvmarm, linux-arm-kernel
Hi Zenghui,
On 2020-04-15 14:15, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/4/14 18:35, Marc Zyngier wrote:
>> When a guest tries to read the active state of its interrupts,
>> we currently just return whatever state we have in memory. This
>> means that if such an interrupt lives in a List Register on another
>> CPU, we fail to obsertve the latest active state for this interrupt.
>>
>> In order to remedy this, stop all the other vcpus so that they exit
>> and we can observe the most recent value for the state.
>>
>> Reported-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 4 +-
>> virt/kvm/arm/vgic/vgic-mmio.c | 100
>> ++++++++++++++++++++-----------
>> virt/kvm/arm/vgic/vgic-mmio.h | 3 +
>> 4 files changed, 71 insertions(+), 40 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 5945f062d749..d63881f60e1a 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -422,11 +422,11 @@ static const struct vgic_register_region
>> vgic_v2_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index e72dcc454247..77c8ba1a2535 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -553,11 +553,11 @@ static const struct vgic_register_region
>> vgic_v3_dist_registers[] = {
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_sactive,
>> - NULL, vgic_mmio_uaccess_write_sactive, 1,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 1,
>> VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
>> vgic_mmio_read_active, vgic_mmio_write_cactive,
>> - NULL, vgic_mmio_uaccess_write_cactive,
>> + vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive,
>> 1, VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
>> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>
> Shouldn't we also set this uaccess_read cb for GICR_I{S,C}ACTIVER0?
Duh. Yes, of course...
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 24+ messages in thread