Fix race condition and simplify vgic active handler
Subsystems (4)
v2 cover Fix race condition and simplify vgic active handler
This is a small series that reworks a problem in a previously submitted patch [1]. The previous patch version did not consider that userspace accesses already hold the KVM mutex and therefore end up in deadlock.
These patches therefore introduces uaccess read/write functions for the GICv2 MMIO register descriptor framework, splits out the distributor active register write functionality into separate guest and userspace access functions, and then finally take the KVM mutex on the guest access path and gets rid of unneeded complexity from active_change_prepare/finish.
Applied on top of v4.12-rc1.
[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2017-May/025542.html
Changes from v1:
- Separate guest and uaccess distributor active register writes.
- Only take the mutex in the guest access path.
Christoffer Dall (3):
KVM: arm/arm64: Allow GICv2 to supply a uaccess register function
KVM: arm/arm64: Separate guest and uaccess writes to dist {sc}active
KVM: arm/arm64: Simplify active_change_prepare and plug race
arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 +++--------- virt/kvm/arm/vgic/vgic-mmio-v2.c | 24 +++++++------- virt/kvm/arm/vgic/vgic-mmio-v3.c | 8 +++-- virt/kvm/arm/vgic/vgic-mmio.c | 68 ++++++++++++++++++++++++++++++--------- virt/kvm/arm/vgic/vgic-mmio.h | 12 ++++++- virt/kvm/arm/vgic/vgic.c | 11 ++++--- 8 files changed, 92 insertions(+), 55 deletions(-)
Signature · 1 line
2.9.0
PATCH v2 1/3 KVM: arm/arm64: Allow GICv2 to supply a uaccess register function
We are about to differentiate between writes from a VCPU and from userspace to the GIC's GICD_ISACTIVER and GICD_ICACTIVER registers due to different synchronization requirements.
Expand the macro to define a register description for the GIC to take uaccess functions as well.
--- virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 +++++++++++----------- virt/kvm/arm/vgic/vgic-mmio.h | 4 +++- 2 files changed, 14 insertions(+), 12 deletions(-)
virt/kvm/arm/vgic/vgic-mmio-v2.c +11 -11
@@ -296,34 +296,34 @@ 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,1 , + vgic_mmio_read_rao, 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,1 , + vgic_mmio_read_enable, vgic_mmio_write_senable,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_ENABLE_CLEAR , - vgic_mmio_read_enable, vgic_mmio_write_cenable,1 , + vgic_mmio_read_enable, vgic_mmio_write_cenable,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_PENDING_SET , - vgic_mmio_read_pending, vgic_mmio_write_spending,1 , + vgic_mmio_read_pending, vgic_mmio_write_spending,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_PENDING_CLEAR , - vgic_mmio_read_pending, vgic_mmio_write_cpending,1 , + vgic_mmio_read_pending, vgic_mmio_write_cpending,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_ACTIVE_SET , - vgic_mmio_read_active, vgic_mmio_write_sactive,1 , + vgic_mmio_read_active, vgic_mmio_write_sactive,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_ACTIVE_CLEAR , - vgic_mmio_read_active, vgic_mmio_write_cactive,1 , + vgic_mmio_read_active, vgic_mmio_write_cactive,NULL ,NULL ,1 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_PRI , - vgic_mmio_read_priority, vgic_mmio_write_priority,8 , - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), + vgic_mmio_read_priority, vgic_mmio_write_priority,NULL ,NULL , +8 , VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_TARGET , - vgic_mmio_read_target, vgic_mmio_write_target,8 , + vgic_mmio_read_target, vgic_mmio_write_target,NULL ,NULL ,8 , VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_CONFIG , - vgic_mmio_read_config, vgic_mmio_write_config,2 , + vgic_mmio_read_config, vgic_mmio_write_config,NULL ,NULL ,2 , VGIC_ACCESS_32bit), REGISTER_DESC_WITH_LENGTH(GIC_DIST_SOFTINT, vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
virt/kvm/arm/vgic/vgic-mmio.h +3 -1
@@ -75,7 +75,7 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;* The _WITH_LENGTH version instantiates registers with a fixed length * and is mutually exclusive with the _PER_IRQ version. */ -#define REGISTER_DESC_WITH_BITS_PER_IRQ (off, rd, wr, bpi, acc) \ +#define REGISTER_DESC_WITH_BITS_PER_IRQ (off, rd, wr, ur, uw, bpi, acc) \ { \ .reg_offset = off, \ .bits_per_irq = bpi, \
@@ -83,6 +83,8 @@ extern struct kvm_io_device_ops kvm_io_gic_ops; .access_flags = acc, \ .read = rd, \ .write = wr, \ + .uaccess_read = ur, \ + .uaccess_write = uw, \ }#define REGISTER_DESC_WITH_LENGTH (off, rd, wr, length, acc) \
Signature · 1 line
2.9.0
Re: [PATCH v2 1/3] KVM: arm/arm64: Allow GICv2 to supply a uaccess register function
Quoted 8 lines · Christoffer Dall
We are about to differentiate between writes from a VCPU and from userspace to the GIC's
GICD_ISACTIVERandGICD_ICACTIVERregisters due to different synchronization requirements.Expand the macro to define a register description for the GIC to take uaccess functions as well.
Signed-off-by Christoffer Dall <[email protected]>
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 2/3 KVM: arm/arm64: Separate guest and uaccess writes to dist {sc}active
In reply to Christoffer DallFactor out the core register modifier functionality from the entry points from the register description table, and only call the prepare/finish functions from the guest path, not the uaccess path.
--- virt/kvm/arm/vgic/vgic-mmio-v2.c | 6 +++-- virt/kvm/arm/vgic/vgic-mmio-v3.c | 8 ++++--- virt/kvm/arm/vgic/vgic-mmio.c | 50 ++++++++++++++++++++++++++++++++++------ virt/kvm/arm/vgic/vgic-mmio.h | 8 +++++++ 4 files changed, 60 insertions(+), 12 deletions(-)
virt/kvm/arm/vgic/vgic-mmio-v2.c +4 -2
@@ -311,10 +311,12 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, NULL, NULL, 1, VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ (GIC_DIST_ACTIVE_SET , - vgic_mmio_read_active, vgic_mmio_write_sactive,NULL ,NULL ,1 , + vgic_mmio_read_active, vgic_mmio_write_sactive, +NULL , 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 ,NULL ,1 , + vgic_mmio_read_active, vgic_mmio_write_cactive, +NULL , 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,
virt/kvm/arm/vgic/vgic-mmio-v3.c +5 -3
@@ -456,11 +456,13 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = { vgic_mmio_read_raz, vgic_mmio_write_wi, 1, VGIC_ACCESS_32bit),REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED (GICD_ISACTIVER , - vgic_mmio_read_active, vgic_mmio_write_sactive,NULL ,NULL ,1 , + vgic_mmio_read_active, vgic_mmio_write_sactive, +NULL , 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 ,NULL ,1 , - VGIC_ACCESS_32bit), + vgic_mmio_read_active, vgic_mmio_write_cactive, +NULL , 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 , 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
virt/kvm/arm/vgic/vgic-mmio.c +43 -7
81 lines not on this page · raw message
virt/kvm/arm/vgic/vgic-mmio.h +8 -0
@@ -167,6 +167,14 @@ 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); + +void 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);
Signature · 1 line
2.9.0
Re: [PATCH v2 2/3] KVM: arm/arm64: Separate guest and uaccess writes to dist {sc}active
Quoted 5 lines · Christoffer Dall
Factor out the core register modifier functionality from the entry points from the register description table, and only call the prepare/finish functions from the guest path, not the uaccess path.
Signed-off-by Christoffer Dall <[email protected]>
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 3/3 KVM: arm/arm64: Simplify active_change_prepare and plug race
In reply to Christoffer DallWe don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)
arch/arm/include/asm/kvm_host.h +0 -2
@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);
arch/arm64/include/asm/kvm_host.h +0 -2
@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
virt/kvm/arm/arm.c +4 -16
@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)
virt/kvm/arm/vgic/vgic-mmio.c +10 -8
@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }
@@ -271,11 +269,13 @@ 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_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock); } void vgic_mmio_uaccess_write_cactive(struct kvm_vcpu *vcpu,
@@ -305,11 +305,13 @@ 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_mmio_write_sactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock); } void vgic_mmio_uaccess_write_sactive(struct kvm_vcpu *vcpu,
virt/kvm/arm/vgic/vgic.c +6 -5
@@ -35,11 +35,12 @@ struct vgic_global kvm_vgic_global_state __ro_after_init = {/* * Locking order is always: -* its->cmd_lock (mutex) -* its->its_lock (mutex) -* vgic_cpu->ap_list_lock -* kvm->lpi_list_lock -* vgic_irq->irq_lock +* kvm->lock (mutex) +* its->cmd_lock (mutex) +* its->its_lock (mutex) +* vgic_cpu->ap_list_lock +* kvm->lpi_list_lock +* vgic_irq->irq_lock * * If you need to take multiple locks, always take the upper lock first, * then the lower ones, e.g. first take the its_lock, then the irq_lock.
Signature · 1 line
2.9.0
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 118 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +6
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 10 lines
virt/kvm/arm/vgic/vgic-mmio.c +2
-0@@ -271,11 +269,13 @@ 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_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);
Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);
Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
Thanks,
-Christoffer
Marc Zyngier
Reviewed-byRe: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 134 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Quoted 8 lines
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
No, that's fine, I just wanted to check whether my understanding was correct.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 141 lines · Marc Zyngier
Quoted 140 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
Quoted 8 lines
Quoted 2 lines
I don't feel strongly about it though, so I can move it if you prefer it.
No, that's fine, I just wanted to check whether my understanding was correct.
Reviewed-by Marc Zyngier <[email protected]>
Thanks.
So assuming Drew's patches will go on top of these, should we merge this as fixes to -rcX, or queue them for v4.13 ?
I'm leaning towards the latter because I don't think we've seen these races do something bad in the wild, and they're probably not going to be backportable to stable anyway. Thoughts?
-Christoffer
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 151 lines · Christoffer Dall
Quoted 141 lines · Marc Zyngier
Quoted 140 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
Quoted 8 lines
Quoted 2 lines
I don't feel strongly about it though, so I can move it if you prefer it.
No, that's fine, I just wanted to check whether my understanding was correct.
Reviewed-by Marc Zyngier <[email protected]>
Quoted 8 lines
Thanks.
So assuming Drew's patches will go on top of these, should we merge this as fixes to -rcX, or queue them for v4.13 ?
I'm leaning towards the latter because I don't think we've seen these races do something bad in the wild, and they're probably not going to be backportable to stable anyway. Thoughts?
The race would only impact a guest hammering its own registers, so I'm quite happy for this to go into 4.13.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
In reply to Christoffer DallQuoted 134 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Quoted 8 lines
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
Actually we must move the locking into the prepare/finish functions, in order to tuck them into the VGIC_NR_PRIVATE_IRQS conditions. Otherwise, with gicv3, when userspace accesses ISACTIVER0/ICACTIVER0, which are SGI_base offsets, then the vgic_v3_sgibase_registers table is used. That table doesn't provide the uaccess functions, so we try to lock twice again.
Thanks,
drew
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 144 lines · Andrew Jones
Quoted 134 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Quoted 8 lines
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
Quoted 7 lines
Actually we must move the locking into the prepare/finish functions, in order to tuck them into the
VGIC_NR_PRIVATE_IRQSconditions. Otherwise, with gicv3, when userspace accesses ISACTIVER0/ICACTIVER0, which areSGI_baseoffsets, then thevgic_v3_sgibase_registerstable is used. That table doesn't provide the uaccess functions, so we try to lock twice again.
Nice find.
How about we always use the uaccess functions or uaccesses instead? :
virt/kvm/arm/vgic/vgic-mmio-v3.c +8 -6
@@ -528,12 +528,14 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, vgic_mmio_read_raz, vgic_mmio_write_wi, 4, VGIC_ACCESS_32bit), -REGISTER_DESC_WITH_LENGTH (GICR_ISACTIVER0 , - vgic_mmio_read_active, vgic_mmio_write_sactive,4 , - VGIC_ACCESS_32bit), -REGISTER_DESC_WITH_LENGTH (GICR_ICACTIVER0 , - vgic_mmio_read_active, vgic_mmio_write_cactive,4 , - VGIC_ACCESS_32bit), +REGISTER_DESC_WITH_LENGTH_UACCESS (GICR_ISACTIVER0 , + vgic_mmio_read_active, vgic_mmio_write_sactive, +NULL , vgic_mmio_uaccess_write_sactive, +4 , VGIC_ACCESS_32bit), +REGISTER_DESC_WITH_LENGTH_UACCESS (GICR_ICACTIVER0 , + vgic_mmio_read_active, vgic_mmio_write_cactive, +NULL , vgic_mmio_uaccess_write_cactive, +4 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_LENGTH (GICR_IPRIORITYR0 , vgic_mmio_read_priority, vgic_mmio_write_priority,32 , VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
Thanks,
-Christoffer
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 153 lines · Christoffer Dall
Quoted 144 lines · Andrew Jones
Quoted 134 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Quoted 8 lines
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
Quoted 7 lines
Actually we must move the locking into the prepare/finish functions, in order to tuck them into the
VGIC_NR_PRIVATE_IRQSconditions. Otherwise, with gicv3, when userspace accesses ISACTIVER0/ICACTIVER0, which areSGI_baseoffsets, then thevgic_v3_sgibase_registerstable is used. That table doesn't provide the uaccess functions, so we try to lock twice again.
Quoted 3 lines
Nice find.
How about we always use the uaccess functions or uaccesses instead? :
Yeah, that's a better suggestion. u-access, i-access, we-all-access :-)
drew
Quoted 29 lines
virt/kvm/arm/vgic/vgic-mmio-v3.c +8
-6@@ -528,12 +528,14 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, vgic_mmio_read_raz, vgic_mmio_write_wi, 4, VGIC_ACCESS_32bit), -REGISTER_DESC_WITH_LENGTH (GICR_ISACTIVER0 , - vgic_mmio_read_active, vgic_mmio_write_sactive,4 , - VGIC_ACCESS_32bit), -REGISTER_DESC_WITH_LENGTH (GICR_ICACTIVER0 , - vgic_mmio_read_active, vgic_mmio_write_cactive,4 , - VGIC_ACCESS_32bit), +REGISTER_DESC_WITH_LENGTH_UACCESS (GICR_ISACTIVER0 , + vgic_mmio_read_active, vgic_mmio_write_sactive, +NULL , vgic_mmio_uaccess_write_sactive, +4 , VGIC_ACCESS_32bit), +REGISTER_DESC_WITH_LENGTH_UACCESS (GICR_ICACTIVER0 , + vgic_mmio_read_active, vgic_mmio_write_cactive, +NULL , vgic_mmio_uaccess_write_cactive, +4 , VGIC_ACCESS_32bit),REGISTER_DESC_WITH_LENGTH (GICR_IPRIORITYR0 , vgic_mmio_read_priority, vgic_mmio_write_priority,32 , VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),Thanks,
-Christoffer
Re: [PATCH v2 3/3] KVM: arm/arm64: Simplify active_change_prepare and plug race
Quoted 154 lines · Andrew Jones
Quoted 153 lines · Christoffer Dall
Quoted 151 lines · Andrew Jones
Quoted 134 lines · Christoffer Dall
Quoted 125 lines · Marc Zyngier
Quoted 124 lines · Christoffer Dall
We don't need to stop a specific VCPU when changing the active state, because private IRQs can only be modified by a running VCPU for the VCPU itself and it is therefore already stopped.
However, it is also possible for two VCPUs to be modifying the active state of SPIs at the same time, which can cause the thread being stuck in the loop that checks other VCPU threads for a potentially very long time, or to modify the active state of a running VCPU. Fix this by serializing all accesses to setting and clearing the active state of interrupts using the KVM mutex.
Reported-by Andrew Jones <[email protected]>Signed-off-by Christoffer Dall <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 -- arch/arm64/include/asm/kvm_host.h | 2 -- virt/kvm/arm/arm.c | 20 ++++---------------- virt/kvm/arm/vgic/vgic-mmio.c | 18 ++++++++++-------- virt/kvm/arm/vgic/vgic.c | 11 ++++++----- 5 files changed, 20 insertions(+), 33 deletions(-)arch/arm/include/asm/kvm_host.h +0
-2@@ -233,8 +233,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu __percpu **kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm64/include/asm/kvm_host.h +0
-2@@ -333,8 +333,6 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void );void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu); -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)virt/kvm/arm/arm.c +4
-16@@ -539,27 +539,15 @@ void kvm_arm_halt_guest(struct kvm *kvm)kvm_make_all_cpus_request (kvm,KVM_REQ_VCPU_EXIT ); } -void kvm_arm_halt_vcpu (struct kvm_vcpu *vcpu) -{ - vcpu->arch.pause = true; -kvm_vcpu_kick (vcpu); -} - -void kvm_arm_resume_vcpu (struct kvm_vcpu *vcpu) -{ -struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); - - vcpu->arch.pause = false; -swake_up (wq); -} -void kvm_arm_resume_guest (struct kvm *kvm) {int i;struct kvm_vcpu *vcpu; -kvm_for_each_vcpu (i, vcpu, kvm) -kvm_arm_resume_vcpu (vcpu); +kvm_for_each_vcpu (i, vcpu, kvm) { + vcpu->arch.pause = false; +swake_up (kvm_arch_vcpu_wq (vcpu)); + } }static void vcpu_sleep (struct kvm_vcpu *vcpu)virt/kvm/arm/vgic/vgic-mmio.c +7
-8@@ -231,23 +231,21 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,* be migrated while we don't hold the IRQ locks and we don't want to be * chasing moving targets. * -* For private interrupts, we only have to make sure the single and only VCPU -* that can potentially queue the IRQ is stopped. +* 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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_halt_vcpu (vcpu); -else +if (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 (intid <VGIC_NR_PRIVATE_IRQS ) -kvm_arm_resume_vcpu (vcpu); -else +if (intid >VGIC_NR_PRIVATE_IRQS )kvm_arm_resume_guest (vcpu->kvm); }@@ -271,11 +269,13 @@ 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);Quoted 7 lines
virt/kvm/arm/vgic/vgic-mmio.c void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, · line 278
Quoted 4 lines
virt/kvm/arm/vgic/vgic-mmio.c +1
-0__vgic_mmio_write_cactive (vcpu, addr, len, val);vgic_change_active_finish (vcpu, intid); +mutex_unlock (&vcpu->kvm->lock);Any reason not to move the lock/unlock calls to prepare/finish? Also, do we need to take that mutex if intid is a PPI?
Quoted 8 lines
I guess we strictly don't need to take the mutex if it's a PPI, no.
But I actually preferred this symmetry because you can easily tell we don't have a bug (famous last words) by locking and unlocking the mutex in the same function.
I don't feel strongly about it though, so I can move it if you prefer it.
Actually we must move the locking into the prepare/finish functions, in order to tuck them into the
VGIC_NR_PRIVATE_IRQSconditions. Otherwise, with gicv3, when userspace accesses ISACTIVER0/ICACTIVER0, which areSGI_baseoffsets, then thevgic_v3_sgibase_registerstable is used. That table doesn't provide the uaccess functions, so we try to lock twice again.
Quoted 6 lines
Quoted 3 lines
Nice find.
How about we always use the uaccess functions or uaccesses instead? :
Yeah, that's a better suggestion. u-access, i-access, we-all-access :-)
cool. I've put it in kvmarm/queue.
Thanks,
-Christoffer