(sorry for the formatting)
On Wed, 7 Mar 2018, 18:23 Andre Przywara, <andre.przyw...@linaro.org> wrote:
> Hi,
>
> On 07/03/18 17:01, Julien Grall wrote:
> > Hi Andre,
> >
> > On 03/05/2018 04:03 PM, Andre Przywara wrote:
> >> As the enable register handlers are shared between the v2 and v3
> >> emulation, their implementation goes into vgic-mmio.c, to be easily
> >> referenced from the v3 emulation as well later.
> >> This introduces a vgic_sync_hardware_irq() function, which updates the
> >> physical side of a hardware mapped virtual IRQ.
> >> Because the existing locking order between vgic_irq->irq_lock and
> >> irq_desc->lock dictates so, we dropu the irq_lock and retake them in the
> >> proper order.
> >>
> >> Signed-off-by: Andre Przywara <andre.przyw...@linaro.org>
> >> ---
> >> Changelog RFC ... v1:
> >> - extend and move vgic_sync_hardware_irq()
> >> - do proper locking sequence
> >> - skip already disabled/enabled IRQs
> >>
> >> xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +-
> >> xen/arch/arm/vgic/vgic-mmio.c | 117
> >> +++++++++++++++++++++++++++++++++++++++
> >> xen/arch/arm/vgic/vgic-mmio.h | 11 ++++
> >> xen/arch/arm/vgic/vgic.c | 38 +++++++++++++
> >> xen/arch/arm/vgic/vgic.h | 3 +
> >> 5 files changed, 171 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> index 2e015ed0b1..3dd983f885 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> @@ -80,10 +80,10 @@ static const struct vgic_register_region
> >> vgic_v2_dist_registers[] = {
> >> vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> >> VGIC_ACCESS_32bit),
> >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> + vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> >> VGIC_ACCESS_32bit),
> >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> >> VGIC_ACCESS_32bit),
> >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
> >> vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c
> >> b/xen/arch/arm/vgic/vgic-mmio.c
> >> index 284a92d288..f8f0252eff 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> >> @@ -39,6 +39,123 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t
> >> addr,
> >> /* Ignore */
> >> }
> >> +/*
> >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the
> >> value
> >> + * of the enabled bit, so there is only one function for both here.
> >> + */
> >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len)
> >> +{
> >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> + uint32_t value = 0;
> >> + unsigned int i;
> >> +
> >> + /* Loop over all IRQs affected by this read */
> >> + for ( i = 0; i < len * 8; i++ )
> >> + {
> >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
> >> + i);
> >> +
> >> + if ( irq->enabled )
> >> + value |= (1U << i);
> >> +
> >> + vgic_put_irq(vcpu->domain, irq);
> >> + }
> >> +
> >> + return value;
> >> +}
> >> +
> >> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len,
> >> + unsigned long val)
> >> +{
> >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> + unsigned int i;
> >> +
> >> + for_each_set_bit( i, &val, len * 8 )
> >> + {
> >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
> >> + i);
> >> + unsigned long flags;
> >> + irq_desc_t *desc;
> >> +
> >> + spin_lock_irqsave(&irq->irq_lock, flags);
> >> +
> >> + if ( irq->enabled ) /* skip already enabled IRQs */
> >> + {
> >> + spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> + vgic_put_irq(vcpu->domain, irq);
> >> + continue;
> >> + }
> >> +
> >> + irq->enabled = true;
> >> + if ( irq->hw )
> >> + {
> >> + /*
> >> + * The irq cannot be a PPI, we only support delivery
> >> + * of SPIs to guests.
> >> + */
> >> + ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> >> +
> >> + desc = irq_to_desc(irq->hwintid);
> >> + }
> >> + else
> >> + desc = NULL;
> >
> > You could just initialize desc to NULL at the declaration time and drop
> > the else part.
>
> Can we rely on the initializer to be called on every loop iteration? I
> wasn't sure about this and what the standard has to say about this.
>
Every loop is a new scope. So everything declared within that scope is
initialized again. We do use that extensively in Xen.
> >> +
> >> + vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> >> +
> >> + if ( desc )
> >> + vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> >
> > A comment explaining why desc is done outside the locking would be
> > useful. This would avoid to loose time using git blame.
> >
> >> +
> >> + vgic_put_irq(vcpu->domain, irq);
> >> + }
> >> +}
> >> +
> >> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len,
> >> + unsigned long val)
> >> +{
> >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> + unsigned int i;
> >> +
> >> + for_each_set_bit( i, &val, len * 8 )
> >> + {
> >> + struct vgic_irq *irq;
> >> + unsigned long flags;
> >> + irq_desc_t *desc;
> >> +
> >> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> >> + spin_lock_irqsave(&irq->irq_lock, flags);
> >> +
> >> + if ( !irq->enabled ) /* skip already disabled IRQs
> */
> >> + {
> >> + spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> + vgic_put_irq(vcpu->domain, irq);
> >> + continue;
> >> + }
> >> +
> >> + irq->enabled = false;
> >> +
> >> + if ( irq->hw )
> >> + {
> >> + /*
> >> + * The irq cannot be a PPI, we only support delivery
> >> + * of SPIs to guests.
> >> + */
> >> + ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> >> +
> >> + desc = irq_to_desc(irq->hwintid);
> >> + }
> >> + else
> >> + desc = NULL;
> >> +
> >> + spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> +
> >> + if ( desc )
> >> + vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> >
> > Ditto.
> >
> >> +
> >> + vgic_put_irq(vcpu->domain, irq);
> >> + }
> >> +}
> >> +
> >> static int match_region(const void *key, const void *elt)
> >> {
> >> const unsigned int offset = (unsigned long)key;
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.h
> >> b/xen/arch/arm/vgic/vgic-mmio.h
> >> index 621b9a281c..2ddcbbf58d 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.h
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> >> @@ -96,6 +96,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
> >> void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
> >> unsigned int len, unsigned long val);
> >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len);
> >> +
> >> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len,
> >> + unsigned long val);
> >> +
> >> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> >> + paddr_t addr, unsigned int len,
> >> + unsigned long val);
> >> +
> >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
> >> #endif
> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> >> index 465a95f415..5246d7c2e7 100644
> >> --- a/xen/arch/arm/vgic/vgic.c
> >> +++ b/xen/arch/arm/vgic/vgic.c
> >> @@ -698,6 +698,44 @@ void vgic_kick_vcpus(struct domain *d)
> >> }
> >> }
> >> +static unsigned int translate_irq_type(bool is_level)
> >> +{
> >> + return is_level ? IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
> >> +}
> >> +
> >> +void vgic_sync_hardware_irq(struct domain *d,
> >> + irq_desc_t *desc, struct vgic_irq *irq)
> >> +{
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&desc->lock, flags);
> >> + spin_lock(&irq->irq_lock);
> >> +
> >> + /* Is that association actually still valid? (we entered with no
> >> locks) */
> >
> > If the association is not valid, then you need to fetch the new desc.
> > Right?
>
> I am not so sure it's that easy. If the association changed, then the
> whole reason of this call might have become invalid. So I rather bail
> out here and do nothing. The check is just to prevent doing the wrong
> thing, not necessarily to always do the right thing.
> To be honest this whole "lock drop dance" is just to cope with the
> locking order, which I consider wrong, according to my gut feeling.
>
If you don't do the dance here, you would have to do in other place. I
still think taking the desc->lock first is the right thing to do as Xen
deal with physical first then it might be a virtual (so second) or handled
by a driver.
> This function here is called from several places, so it seems a bit
> fragile to assume a way how to fix a broken association here. I can go
> back and check every existing caller in this respect, but to be honest
> I'd rather change the locking order, so we don't need to worry about
> this. But I feel like we should do this as a fixup on top later.
>
See some thought in the next patch. We might be able to simplify the whole
logic by forbidding the interrupt to be removed.
> Cheers,
> Andre.
>
>
> >
> >> + if ( desc->irq == irq->hwintid )
> >> + {
> >> + if ( irq->enabled )
> >> + {
> >> + /*
> >> + * We might end up from various callers, so check that the
> >> + * interrrupt is disabled before trying to change the
> >> config.
> >> + */
> >> + if ( irq_type_set_by_domain(d) &&
> >> + test_bit(_IRQ_DISABLED, &desc->status) )
> >> + gic_set_irq_type(desc,
> translate_irq_type(irq->config));
> >> +
> >> + if ( irq->target_vcpu )
> >> + irq_set_affinity(desc,
> >> cpumask_of(irq->target_vcpu->processor));
> >> + desc->handler->enable(desc);
> >> + }
> >> + else
> >> + desc->handler->disable(desc);
> >> + }
> >> +
> >> + spin_unlock(&irq->irq_lock);
> >> + spin_unlock_irqrestore(&desc->lock, flags);
> >> +}
> >> +
> >> /*
> >> * Local variables:
> >> * mode: C
> >> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> >> index 588bd067b7..68e205d10a 100644
> >> --- a/xen/arch/arm/vgic/vgic.h
> >> +++ b/xen/arch/arm/vgic/vgic.h
> >> @@ -50,6 +50,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq
> >> *irq)
> >> atomic_inc(&irq->refcount);
> >> }
> >> +void vgic_sync_hardware_irq(struct domain *d,
> >> + irq_desc_t *desc, struct vgic_irq *irq);
> >> +
> >> void vgic_v2_fold_lr_state(struct vcpu *vcpu);
> >> void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq,
> >> int lr);
> >> void vgic_v2_set_underflow(struct vcpu *vcpu);
> >>
> >
> > Cheers,
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
