On Tue, 14 Apr 2020 12:16:27 +0100
André Przywara <andre.przywara@xxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> > 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...
