Skip to content

Commit

Permalink
KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER…
Browse files Browse the repository at this point in the history 
… read

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. This is
similar to what we are doing for the write side of the same
registers, and results in new MMIO handlers for userspace (which
do not need to stop the guest, as it is supposed to be stopped
already).

Reported-by: Julien Grall <[email protected]>
Reviewed-by: Andre Przywara <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
  • Loading branch information
Marc Zyngier committed Apr 22, 2020
1 parent 1c32ca5 commit 9a50ebb
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 44 deletions.
4 changes: 2 additions & 2 deletions virt/kvm/arm/vgic/vgic-mmio-v2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 6 additions & 6 deletions virt/kvm/arm/vgic/vgic-mmio-v3.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -625,12 +625,12 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISACTIVER0,
vgic_mmio_read_active, vgic_mmio_write_sactive,
NULL, vgic_mmio_uaccess_write_sactive,
4, VGIC_ACCESS_32bit),
vgic_uaccess_read_active, vgic_mmio_uaccess_write_sactive, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICACTIVER0,
vgic_mmio_read_active, vgic_mmio_write_cactive,
NULL, vgic_mmio_uaccess_write_cactive,
4, VGIC_ACCESS_32bit),
vgic_uaccess_read_active, vgic_mmio_uaccess_write_cactive, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(SZ_64K + GICR_IPRIORITYR0,
vgic_mmio_read_priority, vgic_mmio_write_priority, 32,
VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
Expand Down
100 changes: 64 additions & 36 deletions virt/kvm/arm/vgic/vgic-mmio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down
3 changes: 3 additions & 0 deletions virt/kvm/arm/vgic/vgic-mmio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 9a50ebb

Please sign in to comment.