Re: [PATCH 1/3] KVM: arm: vgic: Synchronize the whole guest on GIC{D,R}_I{S,C}ACTIVER read

[Marc Zyngier <maz@kernel.org>]

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...